-
Notifications
You must be signed in to change notification settings - Fork 20
feat: webhook readiness wait for informer cache sync #367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: webhook readiness wait for informer cache sync #367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds a webhook readiness check that prevents the webhook from accepting requests until all resource informer caches are synced. This addresses an issue where webhook replicas could report ready and start serving requests before the discovery cache was fully populated, causing validation failures for valid resources.
Key Changes:
- Adds a new
ResourceInformerReadinessCheckerfunction that verifies all informer caches are synced before marking the webhook pod as ready - Implements
GetAllResources()method in the informer manager interface to retrieve all watched resources (both cluster-scoped and namespace-scoped) - Integrates the readiness check into the hub agent startup sequence after controllers are initialized
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/webhook/readiness.go | New file implementing the readiness check function that validates all informer caches are synced |
| pkg/webhook/readiness_test.go | Comprehensive unit tests for the readiness checker covering nil informer, no resources, synced/unsynced states |
| pkg/utils/informer/informermanager.go | Adds GetAllResources() method to return all present resources (cluster + namespace scoped) with proper locking |
| pkg/utils/informer/informermanager_test.go | Unit tests for GetAllResources() including edge cases and concurrency testing |
| test/utils/informer/manager.go | Implements GetAllResources() in the fake manager for testing purposes |
| cmd/hubagent/main.go | Integrates the readiness check after controller setup to ensure ResourceInformer is initialized |
5527613 to
1b9dd16
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
686f3e2 to
3048a51
Compare
a6a7d9e to
44004f9
Compare
7d474b3 to
a798337
Compare
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
a798337 to
27ebb6b
Compare
Description of your changes
I hit an issue with webhook informer cache not synced while working on another PR #366 where I enabled multiple webhook replicas and hit this issue frequently in e2e tests
The error I encountered is saying Namespace is not found to be a valid schema. This is due to some replicas are still getting api resources data from kube api server but they already report ready and start serving requests.
[FAILED] Failed to create cluster resource placement Expected success, but got an error: <*errors.StatusError | 0xc0011c80a0>: admission webhook "fleet.clusterresourceplacementv1beta1.validating" denied the request: deny create/update v1beta1 CRP has invalid fields the resource is not found in schema (please retry) or it is not a cluster scoped resource: /v1, Kind=Namespace { ErrStatus: { TypeMeta: {Kind: "", APIVersion: ""}, ListMeta: { SelfLink: "", ResourceVersion: "", Continue: "", RemainingItemCount: nil, }, Status: "Failure", Message: "admission webhook \"fleet.clusterresourceplacementv1beta1.validating\" denied the request: deny create/update v1beta1 CRP has invalid fields the resource is not found in schema (please retry) or it is not a cluster scoped resource: /v1, Kind=Namespace", Reason: "Forbidden", Details: nil, Code: 403, }, }I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer