fix(webhook): pre-filter BindDefinitions by RoleBindings via field index#275
Open
MaxRink wants to merge 2 commits intotelekom:mainfrom
Open
fix(webhook): pre-filter BindDefinitions by RoleBindings via field index#275MaxRink wants to merge 2 commits intotelekom:mainfrom
MaxRink wants to merge 2 commits intotelekom:mainfrom
Conversation
authorizeViaBindDefinitions listed ALL BindDefinitions cluster-wide on every namespace admission request with no server-side filter, causing O(N) in-memory scans where N is total BindDefinitions in the cluster. BindDefinitions with only ClusterRoleBindings (cluster-only) were still iterated only to be skipped inside the loop. Add a new field index BindDefinitionHasRoleBindingsField and a matching extractor BindDefinitionHasRoleBindingsFunc to pkg/indexer. Use client.MatchingFields to restrict the List to only BindDefinitions that carry at least one RoleBinding, eliminating cluster-only entries from the webhook hot path and reducing scan cost to O(K) where K <= N.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the namespace validating webhook’s BindDefinition authorization path by avoiding a full cluster-wide BindDefinition list on every namespace admission request, instead leveraging a controller-runtime cache field index to pre-filter candidates.
Changes:
- Added a BindDefinition field index (
.spec.hasRoleBindings) and extractor to identify BindDefinitions that contain namespace-scoped RoleBindings. - Registered the new index in
indexer.SetupIndexes. - Updated the namespace validating webhook to
ListBindDefinitions usingclient.MatchingFieldsso only namespace-scoped BindDefinitions are fetched from cache.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/indexer/indexer.go |
Adds and registers a new field index + extractor for “has RoleBindings” filtering. |
internal/webhook/authorization/namespace_validating_webhook.go |
Uses the new field index to restrict BindDefinition listing to namespace-scoped candidates. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
authorizeViaBindDefinitionsin the namespace validating webhook performed an unscopedclient.Listof all BindDefinitions in the cluster on every namespace CREATE/UPDATE/DELETE admission call. BindDefinitions that only haveclusterRoleBindings(no namespace-scoped bindings) were fetched and then immediately skipped inside the iteration loop — wasted work proportional to their count.Changes
pkg/indexer/indexer.go: AddBindDefinitionHasRoleBindingsFieldconstant,BindDefinitionHasRoleBindingsTruesentinel, andBindDefinitionHasRoleBindingsFuncextractor. Register the new index inSetupIndexes.internal/webhook/authorization/namespace_validating_webhook.go: Passclient.MatchingFields{indexer.BindDefinitionHasRoleBindingsField: indexer.BindDefinitionHasRoleBindingsTrue}to theListcall, restricting the result set to BindDefinitions with at least one RoleBinding.Since
client.Listis served from the controller-runtime informer cache, the field index lookup is an in-process O(1) hash lookup rather than an additional API call. Cluster-only BindDefinitions are excluded before iteration.Testing
go build ./... go test ./pkg/indexer/...All tests pass.
Risk
Low — the
SetupIndexescall must be invoked before the webhook starts (it already is incmd/main.go). If the index is not registered (e.g. standalone webhook withoutSetupIndexes), theListwill return a field-index error. This is the same pattern used by the existingWebhookAuthorizerHasNamespaceSelectorFieldindex.