webhook: skip Secret informer when ImagePullSecrets is disabled#7355
Open
EngHabu wants to merge 2 commits into
Open
webhook: skip Secret informer when ImagePullSecrets is disabled#7355EngHabu wants to merge 2 commits into
EngHabu wants to merge 2 commits into
Conversation
The embedded secret manager's mutating webhook issues two synchronous kube-apiserver Gets per pod admission to resolve image pull secrets (one against the reference namespace, one against the pod namespace for the mirrored copy). At admission scale this is unnecessary load on the API server. Replace the bare controller-runtime client.New with a cache-backed delegating client. The cache is started for the lifetime of the webhook and watches Secrets cluster-wide; reads are served from the informer, writes still go through the API server. Signed-off-by: Haytham Abuelfutuh <[email protected]>
The k8s client is only used by embedded_secret_manager's image-pull-secret path, which is gated on EmbeddedSecretManagerConfig.ImagePullSecrets.Enabled. When the feature is disabled the informer was generating list/watch traffic for nothing, so only build the cache when the feature is on. In the disabled path fall back to a bare direct client. Also explicitly register the Secret informer with the cache. controller- runtime caches are type-lazy (an informer is only started on first access), so the previous code relied on the embedded secret manager being the only caller. The explicit registration: - documents that this cache is intended only for Secret reads - ensures WaitForCacheSync actually blocks on the initial Secret list rather than returning immediately on an empty informer set, so the first admission isn't a guaranteed cache miss Signed-off-by: Haytham Abuelfutuh <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the embedded secret manager’s Kubernetes client initialization in the webhook to avoid running a controller-runtime Secret informer when image pull secret injection is disabled, and to make the informer scope/sync behavior explicit when it is enabled.
Changes:
- Conditionally create/start a controller-runtime cache only when
EmbeddedSecretManagerConfig.ImagePullSecrets.Enabledis true; otherwise use a direct (non-cached) controller-runtime client. - When enabled, explicitly register the
corev1.Secretinformer beforeWaitForCacheSyncto ensure startup blocks on the initial Secret list and the cache remains Secret-scoped.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pingsutw
approved these changes
May 8, 2026
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.
Tracking issue
Follow-up to #7353.
Why are the changes needed?
#7353 introduced a controller-runtime informer cache to back the embedded secret manager's k8s client, eliminating per-admission
Getround-trips against kube-apiserver. Two issues with that change as merged:findAndAddImagePullSecret/addImagePullSecretToPod(both inembedded_secret_manager.go), which run only whenEmbeddedSecretManagerConfig.ImagePullSecrets.Enabledis true. When the flag is off the informer is dead weight — it still runs a cluster-wide list/watch on Secrets that nothing in the webhook ever reads.WaitForCacheSynconly waits on currently-started informers. The previous patch added the cache to the Scheme viacorev1.AddToScheme(which registers Pods, ConfigMaps, Nodes, ... on top of Secrets) and relied on the embedded secret manager being the only caller to keep the watched set restricted to Secrets.WaitForCacheSyncreturned immediately on an empty informer set, so the first admission paid the Secret informer's cold-start latency.What changes were proposed in this pull request?
In
flyteplugins/go/tasks/pluginmachinery/secret/secrets_injector.go:ImagePullSecrets.Enabled. When the feature is disabled, fall back to a bare directclient.New(...)— same behavior as before Cache image-pull-secret reads in webhook via controller-runtime informer #7353.secretInformerCache.GetInformer(ctx, &corev1.Secret{})beforeWaitForCacheSync. This makes the cache scope explicit (Secrets only, not "whatever someone happens to read first"), and ensuresWaitForCacheSyncblocks on the initial Secret list so the first admission isn't a guaranteed cache miss.How was this patch tested?
go build ./flyteplugins/go/tasks/pluginmachinery/secret/... ./executor/...— clean.go vet ./flyteplugins/go/tasks/pluginmachinery/secret/... ./executor/...— clean.go test ./flyteplugins/go/tasks/pluginmachinery/secret/... -count=1— all pass.Labels
Check all the applicable boxes
Related PRs