fix: remove duplicates from imagePullSecrets

[julienmancuso]

Overview:

remove duplicates from imagePullSecrets

Summary by CodeRabbit

• Bug Fixes

  • Prevented duplicate image pull secrets in pod configurations.

• Chores

  • Updated default Kubernetes service DNS configuration for NATS and ETCD addresses.

• Tests

  • Added test coverage for image pull secrets deduplication logic.

[coderabbitai]

Walkthrough

The pull request updates Kubernetes service discovery configuration in the Helm deployment template to append the DNS suffix .svc.cluster.local to NATS and ETCD addresses, and introduces a deduplication function for image pull secrets in the operator's dynamic configuration logic with corresponding test coverage.

Changes

Cohort / File(s)	Summary
Helm Deployment Configuration deploy/cloud/helm/platform/components/operator/templates/deployment.yaml	Updated default argument values for NATS and ETCD addresses to append Kubernetes service DNS suffix (.svc.cluster.local). Format changes from host:port to host.svc.cluster.local:port when no explicit value provided.
Image Pull Secrets Deduplication deploy/cloud/operator/internal/dynamo/graph.go	Added helper function ensureUniqueImagePullSecrets() to deduplicate image pull secrets by name when merging into PodSpec, replacing direct concatenation with deduplication logic. Returns nil if both inputs are empty.
Unit Tests deploy/cloud/operator/internal/dynamo/graph_test.go	Added Test_ensureUniqueImagePullSecrets() test function validating deduplication behavior across empty inputs, mixed secret combinations, and duplicate detection scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• graph.go: Review deduplication logic to ensure set-based uniqueness is correctly implemented and handles edge cases
• deployment.yaml: Verify DNS suffix formatting is correct and doesn't break existing explicit value overrides
• graph_test.go: Validate test coverage comprehensiveness and sorting approach for deterministic comparisons

Poem

🐰✨ A rabbit hops through DNS lands so fine,
With .svc.cluster.local in each line,
Secrets deduplicated, no more they repeat,
Clean merges and config make this change so sweet! 🎯

Pre-merge checks

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description is largely incomplete when compared to the required template. It only provides the Overview section with minimal content that merely restates the title, while the Details section (describing specific changes made), Where should the reviewer start section (identifying key files to review), and Related Issues section (linking to related GitHub issues) are all entirely missing. This leaves reviewers without essential context about the implementation details and scope of the changes.	Please expand the PR description to follow the provided template. Add a Details section explaining the implementation of the ensureUniqueImagePullSecrets function and the test coverage, include a Where should the reviewer start section pointing to deploy/cloud/operator/internal/dynamo/graph.go and graph_test.go as key files, and add a Related Issues section with the appropriate GitHub issue reference (e.g., "Closes #xxx" or "Relates to #xxx").
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "fix: remove duplicates from imagePullSecrets" is directly related to the main change in the changeset. The raw summary confirms that the primary modification is introducing a new helper function ensureUniqueImagePullSecrets to deduplicate imagePullSecrets when merging into a PodSpec, along with corresponding unit tests. The title is concise, clear, and uses conventional commit format, making it easy for teammates to understand the primary change when scanning through the git history.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

deploy/cloud/operator/internal/dynamo/graph.go (1)

862-875: Consider deterministic ordering for predictability.

The map iteration on line 871 produces non-deterministic ordering, which could cause instability in intermediate stages (e.g., debugging, logging). While CanonicalizePodSpec sorts secrets later, it's better to maintain deterministic behavior throughout.

Apply this diff to preserve existing order and append new unique secrets:

 func ensureUniqueImagePullSecrets(existing, new []corev1.LocalObjectReference) []corev1.LocalObjectReference {
 	if len(existing) == 0 && len(new) == 0 {
 		return nil
 	}
-	uniqueSecrets := make(map[string]corev1.LocalObjectReference)
-	for _, secret := range append(existing, new...) {
-		uniqueSecrets[secret.Name] = secret
+	seen := make(map[string]bool)
+	result := make([]corev1.LocalObjectReference, 0, len(existing)+len(new))
+	
+	// Add existing secrets first
+	for _, secret := range existing {
+		if !seen[secret.Name] {
+			seen[secret.Name] = true
+			result = append(result, secret)
+		}
 	}
-	uniqueSecretsList := make([]corev1.LocalObjectReference, 0, len(uniqueSecrets))
-	for _, secret := range uniqueSecrets {
-		uniqueSecretsList = append(uniqueSecretsList, secret)
+	
+	// Add new secrets that aren't duplicates
+	for _, secret := range new {
+		if !seen[secret.Name] {
+			seen[secret.Name] = true
+			result = append(result, secret)
+		}
 	}
-	return uniqueSecretsList
+	
+	return result
 }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0028cdf and 23bf664.

📒 Files selected for processing (3)

• deploy/cloud/helm/platform/components/operator/templates/deployment.yaml (1 hunks)
• deploy/cloud/operator/internal/dynamo/graph.go (1 hunks)
• deploy/cloud/operator/internal/dynamo/graph_test.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1308-1312
Timestamp: 2025-06-11T21:29:28.650Z
Learning: User julienmancuso expects replies in English; avoid switching languages unless explicitly requested.

🧬 Code graph analysis (1)

deploy/cloud/operator/internal/dynamo/graph.go (1)

deploy/cloud/operator/internal/controller_common/pod.go (1)

• CanonicalizePodSpec (12-277)

⏰ 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). (7)

• GitHub Check: operator (arm64)
• GitHub Check: vllm (amd64)
• GitHub Check: trtllm (arm64)
• GitHub Check: sglang
• GitHub Check: vllm (arm64)
• GitHub Check: operator (amd64)
• GitHub Check: Build and Test - dynamo

🔇 Additional comments (3)

deploy/cloud/helm/platform/components/operator/templates/deployment.yaml (1)

90-90: LGTM: FQDN for NATS and ETCD addresses

Appending .svc.cluster.local to the service addresses is a good practice that makes the DNS lookups explicit and more portable.

Also applies to: 95-95

deploy/cloud/operator/internal/dynamo/graph.go (1)

856-856: Good: Deduplication prevents duplicate imagePullSecrets.

Replacing direct concatenation with ensureUniqueImagePullSecrets properly addresses the PR objective to remove duplicates.

deploy/cloud/operator/internal/dynamo/graph_test.go (1)

5209-5268: LGTM: Comprehensive test coverage for deduplication.

The test covers the key scenarios (no secrets, combination, and deduplication) and properly sorts results before comparison to handle non-deterministic ordering.