feat(operator): mechanism for disabling imagePullSecrets discovery #3092
Conversation
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
Signed-off-by: tmontfort <[email protected]>
Signed-off-by: tmontfort <[email protected]>
tmonty12
requested review from
hutm,
biswapanda,
ishandhanani,
julienmancuso,
hhzhang16,
nnshah1,
mohammedabdulwahhab and
atchernych
as code owners
WalkthroughAdds a new opt-out annotation constant and gates image pull secret discovery in GenerateBasePodSpec based on that annotation. Introduces tests (including a duplicate test function) for the gating behavior and updates documentation to describe automatic discovery and the opt-out, with examples for manual configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Dyn as Dynamo Operator
participant Gen as GenerateBasePodSpec
participant SR as SecretsRetriever
participant K8s as Kubernetes API
User->>Dyn: Create/Update Component
Dyn->>Gen: Build base PodSpec
Gen->>Gen: Read annotation nvidia.com/disable-image-pull-secret-discovery
alt Annotation == "true" (opt-out)
note right of Gen: Discovery disabled
Gen-->>Dyn: PodSpec (no imagePullSecrets from discovery)
else Not "true" or absent
Gen->>SR: RetrieveImagePullSecrets(component)
SR->>K8s: List/Get Docker config secrets
K8s-->>SR: Matching secrets
SR-->>Gen: Secret refs
Gen-->>Dyn: PodSpec (imagePullSecrets set)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
deploy/cloud/operator/internal/consts/consts.go (1)
33-34: LGTM: new opt-out annotation constantName, scope, and placement look consistent with existing Kube* constants. Consider adding a short GoDoc explaining default behavior (discovery on; set "true" to disable) for searchability.
docs/guides/dynamo_deploy/create_deployment.md (1)
135-166: Clarify behavior and edge cases for imagePullSecrets discovery
- Note that discovery only runs when extraPodSpec.mainContainer.image is set.
- State that manual extraPodSpec.podSpec.imagePullSecrets are honored; if discovery is not disabled, both sets are merged.
- Specify the expected literal value "true" for the annotation (or make code accept case-insensitive—see separate comment).
- Optionally note that only dockerconfigjson-type secrets are considered.
Suggested doc tweak:
@@ -**Disabling Automatic Discovery:** +**Disabling Automatic Discovery:** +Notes: +- Discovery runs only if `extraPodSpec.mainContainer.image` is set. +- Manually specified `extraPodSpec.podSpec.imagePullSecrets` are always honored. If you do not disable discovery, discovered secrets will be merged in addition to your manual list. +- Set the annotation value to the lowercase string `"true"`. @@ annotations: nvidia.com/disable-image-pull-secret-discovery: "true"deploy/cloud/operator/internal/dynamo/graph.go (1)
768-779: Make annotation parsing tolerant + keep behavior explicit
- Use case-insensitive parsing for the boolean-like annotation to reduce footguns.
- Current merge appends discovered secrets after any user-provided ones; add de-duplication to avoid duplicate entries when users also list the same secret.
Proposed changes:
@@ - disableImagePullSecretAnnotationValue := component.Annotations[commonconsts.KubeAnnotationDisableImagePullSecretDiscovery] - shouldDisableImagePullSecret := disableImagePullSecretAnnotationValue == commonconsts.KubeLabelValueTrue + disableImagePullSecretAnnotationValue := component.Annotations[commonconsts.KubeAnnotationDisableImagePullSecretDiscovery] + shouldDisableImagePullSecret := strings.EqualFold(disableImagePullSecretAnnotationValue, commonconsts.KubeLabelValueTrue) @@ - podSpec.ImagePullSecrets = append(podSpec.ImagePullSecrets, imagePullSecrets...) + // Merge discovered imagePullSecrets without duplicates (favor user-provided) + if len(imagePullSecrets) > 0 { + seen := make(map[string]struct{}, len(podSpec.ImagePullSecrets)) + for _, s := range podSpec.ImagePullSecrets { + seen[s.Name] = struct{}{} + } + for _, s := range imagePullSecrets { + if _, ok := seen[s.Name]; !ok { + podSpec.ImagePullSecrets = append(podSpec.ImagePullSecrets, s) + } + } + }Optional: log at debug when GetSecrets errors are swallowed to aid troubleshooting.
deploy/cloud/operator/internal/dynamo/graph_test.go (1)
4501-4631: Great coverage; add cases for manual + discovered secrets and de-dup
- Add a case where ExtraPodSpec.PodSpec.ImagePullSecrets is preset and discovery is enabled to assert both are present.
- Add a case where the same secret appears manually and via discovery to assert de-dup (if you adopt the merge change).
Minimal additions:
@@ func TestGenerateBasePodSpec_DisableImagePullSecretDiscovery(t *testing.T) { tests := []struct { @@ }{ + { + name: "manual imagePullSecrets retained alongside discovery", + component: &v1alpha1.DynamoComponentDeploymentOverridesSpec{ + DynamoComponentDeploymentSharedSpec: v1alpha1.DynamoComponentDeploymentSharedSpec{ + ComponentType: commonconsts.ComponentTypeFrontend, + ExtraPodSpec: &common.ExtraPodSpec{ + PodSpec: &corev1.PodSpec{ + ImagePullSecrets: []corev1.LocalObjectReference{{Name: "manual-secret"}}, + }, + MainContainer: &corev1.Container{Image: "test-registry/test-image:latest"}, + }, + }, + }, + secretsRetriever: &mockSecretsRetrieverWithSecrets{}, + expectedImagePullSecrets: []corev1.LocalObjectReference{ + {Name: "manual-secret"}, {Name: "test-docker-secret"}, + }, + }, + { + name: "duplicate secret names are de-duplicated", + component: &v1alpha1.DynamoComponentDeploymentOverridesSpec{ + DynamoComponentDeploymentSharedSpec: v1alpha1.DynamoComponentDeploymentSharedSpec{ + ComponentType: commonconsts.ComponentTypeFrontend, + ExtraPodSpec: &common.ExtraPodSpec{ + PodSpec: &corev1.PodSpec{ + ImagePullSecrets: []corev1.LocalObjectReference{{Name: "test-docker-secret"}}, + }, + MainContainer: &corev1.Container{Image: "test-registry/test-image:latest"}, + }, + }, + }, + secretsRetriever: &mockSecretsRetrieverWithSecrets{}, + expectedImagePullSecrets: []corev1.LocalObjectReference{{Name: "test-docker-secret"}}, + },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deploy/cloud/operator/internal/consts/consts.go(1 hunks)deploy/cloud/operator/internal/dynamo/graph.go(1 hunks)deploy/cloud/operator/internal/dynamo/graph_test.go(2 hunks)docs/guides/dynamo_deploy/create_deployment.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-03T15:26:55.732Z
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1337
File: deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml:0-0
Timestamp: 2025-06-03T15:26:55.732Z
Learning: The image-builder ServiceAccount in deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml does not need imagePullSecrets, unlike the component ServiceAccount.
Applied to files:
deploy/cloud/operator/internal/dynamo/graph.godocs/guides/dynamo_deploy/create_deployment.md
🧬 Code graph analysis (2)
deploy/cloud/operator/internal/dynamo/graph.go (2)
deploy/cloud/operator/internal/consts/consts.go (2)
KubeAnnotationDisableImagePullSecretDiscovery(33-33)KubeLabelValueTrue(42-42)deploy/cloud/operator/api/dynamo/common/common.go (1)
ExtraPodSpec(58-61)
deploy/cloud/operator/internal/dynamo/graph_test.go (5)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (2)
DynamoComponentDeploymentOverridesSpec(56-58)DynamoComponentDeploymentSharedSpec(60-115)deploy/cloud/operator/internal/dynamo/graph.go (5)
SecretsRetriever(316-318)Config(64-74)GenerateBasePodSpec(699-839)BackendFrameworkNoop(1067-1067)RoleMain(572-572)deploy/cloud/operator/internal/consts/consts.go (5)
ComponentTypeFrontend(51-51)KubeAnnotationDisableImagePullSecretDiscovery(33-33)KubeLabelValueTrue(42-42)KubeLabelValueFalse(41-41)MultinodeDeploymentTypeGrove(83-83)deploy/cloud/operator/api/dynamo/common/common.go (1)
ExtraPodSpec(58-61)deploy/cloud/operator/internal/controller_common/predicate.go (1)
Config(50-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test - vllm
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
deploy/cloud/operator/internal/dynamo/graph_test.go (1)
3144-3157: LGTM: focused mock for docker secret injection testsThe mock covers both interfaces and returns a stable secret, good for table tests.
julienmancuso
approved these changes
Sep 17, 2025
…ret-auto-injection
tmonty12
deleted the
tmonty12/dep-383-mechanism-disable-image-secret-auto-injection
branch
hhzhang16
pushed a commit
that referenced
this pull request
Sep 19, 2025
…3092) Signed-off-by: tmontfort <[email protected]>
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.
Overview:
The dynamo operator automatically detects docker config secret present in the namespace where DGD (
DynamoGraphDeployment) is deployed that match the docker image that are being used and will inject into the pod spec's imagePullSecrets.There can be scenario where this mechanism should be disabled.
For example, if a publicly available image is used and there is an invalid secret present in the namespace .
Details:
This PR adds an annotation
nvidia.com/disable-image-pull-secret-discoveryfor individual DGD components to disable the injection of the imagePullSecrets.Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Documentation
Tests