feat(repo-server): Declare custom trust certs for repo-server and plugins

[olivergondza]

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:

Permit users to trust CAs on a repo-server system level

Have you updated the necessary documentation?

• Documentation update is required by this PR.
• Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #1830

How to test changes / Special notes to the reviewer:

Can be tested against:

k3d cluster create localtest \
              --k3s-arg "--kube-apiserver-arg=feature-gates=ClusterTrustBundle=true,ClusterTrustBundleProjection=true@server:*" \
              --k3s-arg "--kube-apiserver-arg=runtime-config=certificates.k8s.io/v1beta1/clustertrustbundles=true@server:*" \
              --k3s-arg "--kubelet-arg=feature-gates=ClusterTrustBundle=true,ClusterTrustBundleProjection=true@agent:*" \
              --image rancher/k3s:v1.33.0-k3s1

Summary by CodeRabbit

• New Features

  • System CA trust injection for the repo server and its plugins via ClusterTrustBundles, Secrets, and ConfigMaps; mounts, init-container CA installation, and targeted pod restarts on trust changes.

• Documentation

  • Added reference and YAML examples for system CA trust configuration, semantics, and usage guidance.

• Tests

  • Added comprehensive end-to-end tests and new test helpers/matchers to validate trust projection and dynamic updates.

• Chores

  • CI matrix expanded to cover additional k3s versions and enable feature flags for trust-related scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[olivergondza]

Compared to the proposal in #1876, it turned out 1 init container is enough. Also, this implements DropImageAnchors to suppress whatever CAs was in the image originally.

[olivergondza]

/ok-to-test

[olivergondza]

The "Code scans / Run golangci-lint and gosec (pull_request)" failure to be adressed by #1880

[olivergondza]

The test failures are related to the fact the code depends on a tech-preview features. Any advise on how to handle such functionality?

https://github.com/argoproj-labs/argocd-operator/actions/runs/17917840047/job/50944661583?pr=1876#step:11:45

[olivergondza]

@jannfis, after your review, this is the added feature of change detection: 9c3639c

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

controllers/argocd/repo_server.go (2)

620-626: Consider defensive handling for missing /tmp mount.

While the current flow ensures /tmp is always mounted, the lookup at lines 620-626 could theoretically return volumeTmp = "" if no match is found. This would create an invalid VolumeMount at line 748-750 with Name: "", causing pod admission to fail.

Consider adding a defensive check:

if volumeTmp == "" {
	log.Error(fmt.Errorf("no /tmp volume mount found"), "CA trust init container requires /tmp")
	return
}

This provides a clearer error message rather than a cryptic admission failure.

Also applies to: 748-750

---

964-969: Clarify namespace filtering for cluster-scoped resources.

client.InNamespace(o.GetNamespace()) works correctly—for cluster-scoped resources like ClusterTrustBundle, GetNamespace() returns "", which matches all namespaces—but the intent is implicit. Consider adding a comment:

// For cluster-wide resources (ClusterTrustBundle), GetNamespace() returns "" which matches all namespaces.
// For namespaced resources (Secret/ConfigMap), only ArgoCD instances in the same namespace are considered.
argoNamespace := client.InNamespace(o.GetNamespace())

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0515499 and 8c0c9da.

📒 Files selected for processing (2)

• controllers/argocd/repo_server.go (5 hunks)
• tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go

🧰 Additional context used

🧬 Code graph analysis (1)

controllers/argocd/repo_server.go (4)

controllers/argocd/argocd_controller.go (1)

• ReconcileArgoCD (73-88)

api/v1beta1/argocd_types.go (2)

• ArgoCD (61-67)
• ArgoCDList (399-403)

controllers/argoutil/resource.go (1)

• GetImagePullPolicy (270-280)

controllers/argoutil/security_context.go (1)

• DefaultSecurityContext (22-36)

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

• GitHub Check: Run end-to-end tests (v1.27.1-k3s1)
• GitHub Check: Run golangci-lint and gosec
• GitHub Check: Run end-to-end tests (v1.33.5-k3s1)
• GitHub Check: build-operator
• GitHub Check: build

🔇 Additional comments (7)

controllers/argocd/repo_server.go (7)

22-22: LGTM! Imports are appropriate for CA trust functionality.

The new imports support ClusterTrustBundle handling, label selection, volume projection, and reconciliation triggers.

Also applies to: 26-26, 29-30, 33-34, 36-36

---

360-361: CA trust injection correctly positioned in reconciliation flow.

Called after container/volume setup ensures the /tmp volume lookup succeeds.

---

378-383: Reconciler hook application is correctly logged.

Note that the hook is applied after CA trust injection, so hooks can override CA trust configuration if needed.

---

643-650: Image-based filtering correctly prevents incompatible CA trust injection.

Only injecting into containers with argoImage (Ubuntu-based) is appropriate since update-ca-certificates is distro-specific. Custom sidecar images are safely excluded.

---

661-705: LGTM! CA trust volume projection is well-implemented.

Key strengths:

• Deep copies prevent unintended CR mutations
• Automatic .crt suffix ensures update-ca-certificates compatibility
• Source tracking provides clear audit trail in logs

---

707-755: Init container implementation is robust and secure.

Highlights:

• Respects CR's image pull policy (line 717)
• Bash script has comprehensive error handling (set -eEuo pipefail + ERR trap)
• Creates empty ca-certificates.crt fallback (line 731) if update-ca-certificates produces no output
• Security context uses read-only root filesystem with selective writable mounts

---

1031-1056: ClusterTrustBundle matching logic correctly implements Kubernetes semantics.

The three-tier matching strategy is well-implemented:

1. Direct name match (precedence)
2. SignerName with nil LabelSelector → match nothing
3. SignerName with empty LabelSelector → match everything
4. SignerName with populated LabelSelector → evaluate selector

[olivergondza]

@anandf, what would you say are the next steps here?

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/reference/argocd.md (1)

1054-1150: Polish SystemCATrust docs and fix markdownlint warnings

The new SystemCATrust entry and TLS trust section read well, but there are a couple of small issues:

• Lines 1140–1142: the nested - Kustomize ... bullets are over‑indented; markdownlint (MD007) expects 2‑space indent for the sub‑list. Adjusting the indentation will clear the lint error.
• Line 1147: typo “absense” → “absence”.

Consider updating these to keep the reference clean and passing existing markdown linting.

♻️ Duplicate comments (1)

controllers/argocd/repo_server.go (1)

958-1029: Fix ClusterTrustBundle handling and avoid panicking on unknown mapper types

Two issues in systemCATrustMapper:

1. ClusterTrustBundle events never see any ArgoCDs

   • You always list ArgoCDs with client.InNamespace(o.GetNamespace()).
   • For ClusterTrustBundle (cluster-scoped), o.GetNamespace() is empty, so the list is effectively filtered to a non-existent namespace and no ArgoCDs are examined.
   • Result: updates to referenced ClusterTrustBundles will not restart repo-server pods, which breaks the intended “dynamic trust refresh” behavior.

2. panic on unknown types can crash the controller

   • The default case panics on an unexpected type. Even if unreachable today, this is brittle and contrary to controller best practices; we should just log and return.

You can address both and also avoid context.TODO() by:

 func (r *ReconcileArgoCD) systemCATrustMapper(ctx context.Context, o client.Object) []reconcile.Request {
-	// Track Argo CDs whose repo-servers need a rollout, and id of the resource that changed
-	rolloutBecause := make(map[*argocdoperatorv1beta1.ArgoCD]string)
-
-	// For cluster-wide resources, it is needed to consult all argos. For cluster-scoped ones, only the argos in the same NS.
-	argoNamespace := client.InNamespace(o.GetNamespace())
-	var argoCDs argocdoperatorv1beta1.ArgoCDList
-	if err := r.List(ctx, &argoCDs, argoNamespace); err != nil {
+	// Track ArgoCDs whose repo-servers need a rollout, and id of the resource that changed.
+	rolloutBecause := make(map[*argocdoperatorv1beta1.ArgoCD]string)
+
+	// For cluster-scoped resources (Secrets/ConfigMaps) only consider ArgoCDs in the same namespace.
+	// For cluster-wide resources (ClusterTrustBundles) we must consider all ArgoCDs.
+	var listOpts []client.ListOption
+	if _, isCTB := o.(*v1beta1.ClusterTrustBundle); !isCTB {
+		listOpts = append(listOpts, client.InNamespace(o.GetNamespace()))
+	}
+	var argoCDs argocdoperatorv1beta1.ArgoCDList
+	if err := r.List(ctx, &argoCDs, listOpts...); err != nil {
 		log.Error(err, "unable to list ArgoCD instances")
 		return []reconcile.Request{}
 	}
@@
-		switch obj := o.(type) {
+		switch obj := o.(type) {
@@
-		case *v1beta1.ClusterTrustBundle:
+		case *v1beta1.ClusterTrustBundle:
@@
-		default:
-			panic(fmt.Errorf("systemCATrustMapper called for unknown type %t", o))
-		}
-	}
+		default:
+			err := fmt.Errorf("systemCATrustMapper called for unknown type %T", o)
+			log.Error(err, "unexpected object type in systemCATrustMapper")
+			return []reconcile.Request{}
+		}
+	}
@@
-		pods := &corev1.PodList{}
-		err := r.List(context.TODO(), pods,
+		pods := &corev1.PodList{}
+		err := r.List(ctx, pods,
@@
-			if err := r.Delete(context.TODO(), &pod); err != nil {
+			if err := r.Delete(ctx, &pod); err != nil {

This ensures:

• Cluster-scoped ClusterTrustBundle changes cause restarts for all ArgoCD instances that reference them.
• The controller doesn’t crash on unexpected inputs.
• You consistently honor the mapper’s ctx for list/delete calls.

🧹 Nitpick comments (6)

tests/ginkgo/fixture/pod/fixture.go (1)

17-31: Consider refactoring to avoid intermediate slice and copying.

The current implementation copies matching pods into an intermediate slice and then returns a pointer to an element of that slice. While this works correctly, it involves unnecessary copying. A more efficient approach would iterate with an index and return a pointer directly from list.Items.

Apply this diff to eliminate the intermediate slice:

 func GetPodByNameRegexp(k8sClient client.Client, nameRegexp *regexp.Regexp, options ...client.ListOption) *corev1.Pod {
-	var pods []corev1.Pod
 	list := &corev1.PodList{}
 	err := k8sClient.List(context.Background(), list, options...)
 	Expect(err).ToNot(HaveOccurred())
-	for _, pod := range list.Items {
-		if nameRegexp.MatchString(pod.Name) {
-			pods = append(pods, pod)
+	
+	var matchedPod *corev1.Pod
+	matchCount := 0
+	for idx := range list.Items {
+		if nameRegexp.MatchString(list.Items[idx].Name) {
+			matchedPod = &list.Items[idx]
+			matchCount++
 		}
 	}
 
-	Expect(pods).Should(HaveLen(1), "expected a single pod matching "+nameRegexp.String())
+	Expect(matchCount).Should(Equal(1), "expected a single pod matching "+nameRegexp.String())
 
-	return &pods[0]
+	return matchedPod
 }

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1)

672-683: Tests depend on live external TLS endpoints

getCACert dials public hosts (e.g., github.com, google.com) over TLS at test time and derives CA bundles from the live connection. This is fine for functional coverage, but it does make the suite sensitive to:

• lack of outbound internet access in CI,
• transient DNS/TLS issues, or
• upstream certificate / CA rotation.

If you start seeing flakes, consider replacing these with in‑cluster HTTPS test fixtures (self‑signed CA + Pod/Service) so the trust behavior is exercised without relying on external infrastructure.

controllers/argocd/repo_server.go (1)

707-755: Consider lighter or dedicated resources for the CA trust init container

caTrustInitContainer currently reuses getArgoRepoResources(cr), which may significantly over-allocate CPU/memory for a short-lived update-ca-certificates job.

Consider introducing a smaller default for this init container (or a dedicated spec.repo.systemCATrust.resources override) so that clusters don’t have to reserve full repo-server resources just to rebuild CA bundles.

deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (3)

20624-20648: Consider adding validation rules for ClusterTrustBundleProjection constraints.

The schema documents mutually-exclusive constraints (lines 20625-20627: name is mutually-exclusive with signerName and labelSelector), but these are not enforced via OpenAPI schema validation rules. Kubernetes allows expressing these constraints with oneOf, anyOf, or not schemas.

Consider adding CEL validation rules or OpenAPI constraints to enforce the documented mutually-exclusive relationships:

# Example structure (not a complete diff)
properties:
  name: ...
  signerName: ...
  labelSelector: ...
# Add validation:
oneOf:
  - required: [name]
  - required: [signerName]
  - required: [labelSelector, signerName]

---

20637-20640: Add validation patterns for path fields to enforce documented constraints.

Path fields (under clusterTrustBundles, configMaps items, and secrets items) have descriptions stating they must be relative and cannot contain .. or start with .., but lack OpenAPI validation rules to enforce these constraints. Add pattern validation to the schema:

path:
  type: string
  description: "Relative path from the volume root..."
  pattern: '^[^./][^/]*(/[^/]*)*$'  # Simplified; adjust as needed
  # Enforces: no leading dot, no absolute paths, no .. references

Also applies to: 20690-20696, 20762-20768

---

20719-20723: Clarify dropImageCertificates behavior and interaction.

Line 20720-20722 describes dropImageCertificates, but the behavior when combined with clusterTrustBundles/configMaps/secrets could be clearer. Does the operator:

1. Drop all image certs first, then apply custom certs from these sources?
2. Or is this only relevant when no custom certs are configured?

Enhance the description to clarify the precedence and behavior, for example:

dropImageCertificates:
  description: >-
    DropImageCertificates removes all CA certificates bundled in the container image.
    When enabled, the system trust store will contain only certificates explicitly
    configured via clusterTrustBundles, configMaps, or secrets. If enabled but no
    custom certificates are provided, the repo-server may fail TLS verification.
  type: boolean

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c0c9da and 4130688.

📒 Files selected for processing (15)

• .github/workflows/ci-build.yaml (2 hunks)
• api/v1beta1/argocd_types.go (2 hunks)
• api/v1beta1/zz_generated.deepcopy.go (2 hunks)
• bundle/manifests/argoproj.io_argocds.yaml (1 hunks)
• config/crd/bases/argoproj.io_argocds.yaml (1 hunks)
• controllers/argocd/argocd_controller.go (1 hunks)
• controllers/argocd/repo_server.go (5 hunks)
• controllers/argocd/util.go (8 hunks)
• deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (1 hunks)
• docs/reference/argocd.md (2 hunks)
• tests/ginkgo/fixture/application/fixture.go (2 hunks)
• tests/ginkgo/fixture/argocd/fixture.go (1 hunks)
• tests/ginkgo/fixture/pod/fixture.go (2 hunks)
• tests/ginkgo/fixture/utils/fixtureUtils.go (2 hunks)
• tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• tests/ginkgo/fixture/argocd/fixture.go
• tests/ginkgo/fixture/utils/fixtureUtils.go
• controllers/argocd/argocd_controller.go

🧰 Additional context used

🧬 Code graph analysis (4)

api/v1beta1/zz_generated.deepcopy.go (1)

api/v1beta1/argocd_types.go (1)

• ArgoCDSystemCATrustSpec (608-617)

controllers/argocd/repo_server.go (4)

controllers/argocd/argocd_controller.go (1)

• ReconcileArgoCD (73-92)

api/v1beta1/argocd_types.go (2)

• ArgoCD (61-67)
• ArgoCDList (399-403)

controllers/argoutil/resource.go (1)

• GetImagePullPolicy (270-288)

controllers/argoutil/security_context.go (1)

• DefaultSecurityContext (22-36)

controllers/argocd/util.go (5)

controllers/argoutil/api.go (1)

• VerifyAPI (32-58)

api/v1alpha1/notificationsconfiguration_types.go (1)

• NotificationsConfiguration (32-37)

api/v1beta1/namespacemanagement_types.go (1)

• NamespaceManagement (41-47)

common/values.go (1)

• ArgoCDAppSetGitlabSCMTLSCertsConfigMapName (75-75)

common/keys.go (1)

• ArgoCDManagedByClusterArgoCDLabel (215-215)

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (6)

tests/ginkgo/fixture/fixture.go (2)

• EnsureSequentialCleanSlate (62-64)
• CreateRandomE2ETestNamespaceWithCleanupFunc (118-122)

tests/ginkgo/fixture/utils/fixtureUtils.go (1)

• GetE2ETestKubeClient (36-44)

api/v1beta1/argocd_types.go (1)

• ArgoCDSystemCATrustSpec (608-617)

tests/ginkgo/fixture/argocd/fixture.go (3)

• HaveServerStatus (92-97)
• HaveRepoStatus (85-90)
• BeAvailable (47-49)

tests/ginkgo/fixture/application/fixture.go (3)

• HaveConditionMatching (103-123)
• HaveSyncStatusCode (79-89)
• HaveNoConditions (91-101)

tests/ginkgo/fixture/os/fixture.go (1)

• ExecCommandWithOutputParam (15-33)

🪛 markdownlint-cli2 (0.18.1)

docs/reference/argocd.md

1140-1140: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

---

1141-1141: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

---

1142-1142: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

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

• GitHub Check: Run golangci-lint and gosec
• GitHub Check: Run end-to-end tests (v1.27.1-k3s1)
• GitHub Check: build-operator
• GitHub Check: Run end-to-end tests (v1.33.5-k3s1)
• GitHub Check: build

🔇 Additional comments (20)

tests/ginkgo/fixture/pod/fixture.go (1)

5-5: LGTM!

The regexp import is necessary for the new GetPodByNameRegexp function and is correctly added.

tests/ginkgo/fixture/application/fixture.go (2)

3-6: New fmt/regexp imports are correctly scoped and used

The added fmt and regexp imports are required by the new helper matchers and are used appropriately; no issues here.

---

91-101: HaveNoConditions helper is clear and correctly leverages expectedCondition

The matcher cleanly reuses expectedCondition, handles the empty/non-empty cases correctly, and logs current conditions on failure, which should be helpful for debugging. No changes needed.

.github/workflows/ci-build.yaml (1)

51-53: K3s matrix and feature flag wiring looks sound

The two‑version matrix plus conditional feature_flags array expansion into k3d cluster create is correct and safely no‑ops when the array is empty. No changes needed here.

Also applies to: 74-87

api/v1beta1/zz_generated.deepcopy.go (1)

853-939: DeepCopy support for SystemCATrust is correctly wired

The new SystemCATrust handling in ArgoCDRepoSpec.DeepCopyInto and the ArgoCDSystemCATrustSpec DeepCopy helpers follow the existing patterns (allocate new struct, deep‑copy each projection slice) and avoid slice aliasing. Looks good as‑is.

Also applies to: 1286-1320

api/v1beta1/argocd_types.go (1)

595-597: SystemCATrust spec shape looks appropriate

The new SystemCATrust field on ArgoCDRepoSpec and the ArgoCDSystemCATrustSpec struct (drop‑image flag plus CTB/Secret/ConfigMap projections) cleanly model the intended feature and align with the documented behavior and tests. No issues from a type/API perspective.

Also applies to: 607-617

controllers/argocd/repo_server.go (4)

17-37: CA trust–related imports are appropriate and used

The newly added imports (strings, certificates v1beta1, metav1, labels, ptr, client, reconcile) are all exercised by the CA trust injection and mapper logic; no unused or layering‑problematic imports here.

---

360-382: Placement of CA trust injection and reconciler hook looks correct

Injecting CA trust into the Deployment (injectCATrustToContainers) before applying the repo-server reconciler hook gives hooks full visibility of the added volumes/init container while still allowing them to override as needed. The additional logging around applyReconcilerHook is fine and should aid debugging.

---

576-705: CA trust volumes and injection into containers are wired coherently

The new injectCATrustToContainers and caTrustVolumes helpers cleanly encapsulate:

• projecting ClusterTrustBundles/Secrets/ConfigMaps into a dedicated source volume with .crt suffix normalization; and
• mounting both source and synthesized target trust volumes into all containers using the Argo image.

Reusing the existing /tmp volume name from the main container when constructing the init container addresses the earlier /tmp-volume-name mismatch and respects user overrides. Just be mindful that future refactors to repo-server /tmp volumeMount setup must keep this lookup in sync.

---

1031-1056: ClusterTrustBundle selector logic matches API semantics

isRelevantCtb correctly:

• matches directly on .Name when provided;
• otherwise uses .SignerName plus LabelSelector, treating nil selector as “match nothing” and an empty selector as “match everything”; and
• uses LabelSelectorAsSelector + selector.Matches to evaluate match expressions.

This lines up with the documented ClusterTrustBundle projection matching rules.

bundle/manifests/argoproj.io_argocds.yaml (1)

20561-20791: Schema structure looks solid and follows Kubernetes conventions.

The new systemCATrust property is well-designed with clear documentation and proper schema constraints. The projection sources (clusterTrustBundles, configMaps, secrets) mirror standard Kubernetes APIs, and the dropImageCertificates boolean flag provides flexibility for CA management.

Minor clarification suggestion: The description for dropImageCertificates refers to "certificates...present in the image" — consider explicitly noting this means "the base container image" to prevent any ambiguity about which image is referenced.

config/crd/bases/argoproj.io_argocds.yaml (1)

20550-20780: CRD schema correctly auto-generated from Go types—no issues found.

Verification confirms the systemCATrust field extension in the CRD (lines 20550-20780) is properly auto-generated from ArgoCDSystemCATrustSpec (api/v1beta1/argocd_types.go) via controller-gen v0.18.0. Field names, types, and descriptions are consistent. The schema correctly uses standard Kubernetes projection types (corev1.ClusterTrustBundleProjection, corev1.ConfigMapProjection, corev1.SecretProjection) with all fields properly marked as optional. No manual drift or inconsistencies detected.

deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (1)

20561-20791: File is auto-generated via controller-gen; edits must target Go type definitions, not this YAML.

The CRD file at deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml is confirmed to be auto-generated from Go type definitions (evidenced by the controller-gen.kubebuilder.io/version: v0.18.0 annotation). The systemCATrust schema correctly represents the ArgoCDSystemCATrustSpec type defined in api/v1beta1/argocd_types.go, with all four properties (dropImageCertificates, clusterTrustBundles, configMaps, secrets) properly aligned.

To ensure the schema remains consistent:

• Make any schema changes to the source Go type at api/v1beta1/argocd_types.go
• Regenerate the base CRD at config/crd/bases/argoproj.io_argocds.yaml using controller-gen
• OLM catalog versions are distribution copies and should be regenerated during the release process

Do not edit OLM catalog files directly, as changes will be overwritten on regeneration.

controllers/argocd/util.go (7)

80-82: LGTM!

The flag declaration follows the established pattern for other API availability flags in the file.

---

95-97: LGTM!

The accessor function follows the established pattern and naming convention.

---

1026-1026: LGTM!

The signature update to include systemCATrustMapper follows the established pattern for other mapper functions.

---

1060-1063: LGTM!

Good catch adding explicit ownership for StatefulSet and NotificationsConfiguration resources. These were likely missing from the ownership declarations.

---

1065-1083: LGTM!

The refactoring to use inline handler.EnqueueRequestsFromMapFunc invocations is consistent with the updated code style and doesn't change functionality.

---

39-39: No action required—ClusterTrustBundle v1beta1 is correct.

The codebase targets Kubernetes v0.33.1, and ClusterTrustBundle is available as v1beta1 in Kubernetes v1.33 (beta feature state). The import statement is correct and compatible with your minimum supported version.

---

1104-1109: The systemCATrustMapper implementation properly filters resources as expected.

Verification confirms the mapper function correctly handles the filtering concern raised in the review:

• The mapper checks the resource namespace using client.InNamespace(o.GetNamespace())
• For Secrets and ConfigMaps, it iterates through configured resources in Spec.Repo.SystemCATrust.Secrets and Spec.Repo.SystemCATrust.ConfigMaps respectively, matching only by name
• Only ArgoCD instances with matching resources are returned as reconcile requests; non-matching resources result in an empty response

The filtering is implemented in the mapper function itself rather than through predicates, which is an acceptable pattern. The conditional ClusterTrustBundle watcher correctly uses IsClusterTrustBundleAPIFound() to guard optional API support.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

controllers/argocd/util.go (1)

1104-1109: Consider filtering watched Secrets and ConfigMaps to reduce reconciliation overhead.

The current watches for Secret and ConfigMap on lines 1105-1106 will trigger for every Secret and ConfigMap in the watched namespace(s). While systemCATrustMapper filters to only relevant resources, the watch predicate still evaluates every change, potentially causing unnecessary overhead in large clusters.

Consider adding a predicate that filters by name or label to match only the Secrets/ConfigMaps referenced in spec.repo.systemCATrust, similar to how lines 1071-1073 and 1079-1083 watch specific resources. However, this would require dynamic predicate construction based on the ArgoCD CR specs.

If the current approach is acceptable for your use case (dynamic references make filtering difficult), consider documenting this trade-off.

controllers/argocd/repo_server.go (1)

661-705: LGTM with a minor suggestion.

The function correctly handles path normalization and uses DeepCopy() to avoid mutating the original CR. The .crt suffix enforcement ensures compatibility with update-ca-certificates.

One optional improvement: Consider logging a warning when a path is modified to add the .crt suffix, as this might be unexpected behavior for users who specified explicit paths. This would aid debugging configuration issues.

Example enhancement:

 	ensureValidPath := func(path string) string {
 		if strings.HasSuffix(path, ".crt") {
 			return path
 		}
+		log.Info("Adding .crt suffix to CA trust source path", "originalPath", path, "updatedPath", path+".crt")
 		return path + ".crt"
 	}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4130688 and 269f06c.

📒 Files selected for processing (3)

• controllers/argocd/repo_server.go (5 hunks)
• controllers/argocd/util.go (8 hunks)
• tests/ginkgo/fixture/application/fixture.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/ginkgo/fixture/application/fixture.go

🧰 Additional context used

🧬 Code graph analysis (2)

controllers/argocd/util.go (5)

controllers/argoutil/api.go (1)

• VerifyAPI (32-58)

api/v1alpha1/notificationsconfiguration_types.go (1)

• NotificationsConfiguration (32-37)

api/v1beta1/namespacemanagement_types.go (1)

• NamespaceManagement (41-47)

common/values.go (1)

• ArgoCDAppSetGitlabSCMTLSCertsConfigMapName (75-75)

common/keys.go (1)

• ArgoCDManagedByClusterArgoCDLabel (215-215)

controllers/argocd/repo_server.go (4)

controllers/argocd/argocd_controller.go (1)

• ReconcileArgoCD (73-92)

api/v1beta1/argocd_types.go (2)

• ArgoCD (61-67)
• ArgoCDList (399-403)

controllers/argoutil/resource.go (1)

• GetImagePullPolicy (270-288)

controllers/argoutil/security_context.go (1)

• DefaultSecurityContext (22-36)

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

• GitHub Check: Run end-to-end tests (v1.27.1-k3s1)
• GitHub Check: Run end-to-end tests (v1.33.5-k3s1)
• GitHub Check: build-operator
• GitHub Check: Run golangci-lint and gosec
• GitHub Check: build

🔇 Additional comments (12)

controllers/argocd/util.go (6)

80-82: LGTM!

Variable declaration follows the established pattern for API availability flags.

---

95-97: LGTM!

The accessor function follows the established pattern for exposing API availability flags and is correctly implemented.

---

707-709: LGTM!

Based on the resolved past review discussion, the error handling here is correct. The function returns an error only when the API check itself fails (not when the API is simply unavailable), which is the intended behavior.

---

39-39: Critical: Wrong API version for ClusterTrustBundle.

ClusterTrustBundle is defined in k8s.io/api/certificates/v1alpha1, not v1beta1. The v1beta1 certificates API contains CertificateSigningRequest but not ClusterTrustBundle. This import will cause compilation or runtime failures when attempting to use ClusterTrustBundle resources.

Apply this diff:

-	certificates "k8s.io/api/certificates/v1beta1"
+	certificates "k8s.io/api/certificates/v1alpha1"

Likely an incorrect or invalid review comment.

---

1026-1026: Call site verification complete - systemCATrustMapper parameter is being passed.

The call site in controllers/argocd/argocd_controller.go:363 correctly passes r.systemCATrustMapper as the last argument, matching the updated function signature. The change has been properly integrated.

---

119-127: Review comment is incorrect; no code changes needed.

The import k8s.io/api/certificates/v1beta1 at line 39 is correct for the project's Kubernetes version (1.33 per go.mod). ClusterTrustBundle is available in both v1alpha1 (initial version) and v1beta1 (in newer K8s releases), with v1beta1 being the appropriate choice for Kubernetes 1.31+. The function verifyClusterTrustBundleAPI() correctly queries the cluster at runtime to detect which version is available, so the compile-time version selection does not restrict runtime compatibility. No code fix is required.

Likely an incorrect or invalid review comment.

controllers/argocd/repo_server.go (6)

360-360: LGTM!

The placement of injectCATrustToContainers is appropriate—called after base volumes are configured and before the deployment is finalized. The in-place modification pattern is consistent with the rest of the function.

---

378-382: LGTM!

The reconciler hook logging and error handling are appropriate and follow established patterns in the codebase.

---

576-659: LGTM!

The function is well-structured with several good design decisions:

1. Early return when feature is not enabled (line 593-595)
2. Correctly handles user-provided /tmp volumes by looking up the actual volume name (lines 619-626) - this addresses the past review concern
3. Only injects CA trust into containers with the matching argoImage (line 645), avoiding potential compatibility issues with sidecars using different base images
4. Clear logging of what was injected and where (lines 652-656)

The implementation correctly addresses the constraints documented in the function comment regarding Ubuntu-specific certificate handling.

---

707-755: LGTM!

The init container implementation correctly addresses previous review concerns:

1. ReadOnlyRootFilesystem: Now works correctly with argoutil.DefaultSecurityContext() (which sets ReadOnlyRootFilesystem: true) because /tmp is explicitly mounted as a writable volume (line 748-750)
2. Resources: Uses getArgoRepoResources(cr) as discussed in past reviews
3. Error handling: Excellent use of set -eEuo pipefail and error trap (lines 724-725)
4. Debugging: Verbose output helps troubleshoot certificate issues (lines 727-736)
5. DropImageCertificates: Correctly handles the option to exclude image-bundled CAs (lines 709-712)

The bash script is well-written with appropriate error handling and diagnostic output.

---

958-1029: LGTM with an observation about the restart mechanism.

The function correctly identifies affected ArgoCD instances and restarts their repo-server pods. The approach of deleting pods (rather than triggering a deployment rollout) ensures immediate trust recalculation on restart.

Design note: This "delete pod to restart" approach works well because:

1. The init container will re-run with updated trust sources
2. It's faster than a full deployment rollout
3. The deployment controller will recreate the pod immediately

The error handling and logging are appropriate. The panic at line 999 is correctly retained from the resolved review discussion—it catches programming errors in watch setup.

---

1031-1056: LGTM!

The function correctly implements ClusterTrustBundle matching logic according to Kubernetes conventions:

1. Explicit name matching (lines 1033-1035)
2. SignerName + LabelSelector matching (lines 1037-1053)
3. Proper handling of nil vs. empty LabelSelector:
   • nil → match nothing (line 1039-1041)
   • Empty → match everything (line 1042-1044)
   • Populated → evaluate selector (lines 1046-1052)

The error handling for invalid label selectors is appropriate—logs and returns false as a safe default.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

controllers/argocd/repo_server.go (1)

26-26: Critical: Wrong API version for ClusterTrustBundle.

ClusterTrustBundle is defined in k8s.io/api/certificates/v1alpha1, not v1beta1. This import is incorrect and will cause compilation or runtime failures.

Apply this diff:

-	certificatesv1beta1 "k8s.io/api/certificates/v1beta1"
+	certificatesv1alpha1 "k8s.io/api/certificates/v1alpha1"

Also update line 991 to use the correct package alias:

-	case *certificatesv1beta1.ClusterTrustBundle:
+	case *certificatesv1alpha1.ClusterTrustBundle:

🧹 Nitpick comments (2)

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (2)

84-460: Excellent test coverage across all CA trust scenarios.

The test suite comprehensively validates:

• Startup behavior with missing/optional resources
• Trust propagation via ClusterTrustBundles, ConfigMaps, and Secrets
• Reconciliation on resource create/update/delete
• Selective change detection to avoid unnecessary pod restarts
• Both positive (trusted hosts succeed) and negative (untrusted hosts fail) cases

The different timeout values (6m for CTB operations at lines 379, 384 vs 30s for ConfigMap/Secret operations) appear intentional given that ClusterTrustBundles are cluster-wide resources with potentially longer propagation times.

Consider replacing fixed sleeps with condition-based polling.

Lines 240 and 835 use time.Sleep(20 * time.Second) to ensure applications have synced. While acceptable for E2E tests, this could be made more robust and potentially faster by polling for specific application conditions using Eventually():

-	// Sleep to make sure the apps sync took place - otherwise there might be no conditions _yet_
-	time.Sleep(20 * time.Second)
+	// Wait for apps to attempt sync and establish conditions
+	Eventually(func() bool {
+		_ = k8sClient.Get(ctx, client.ObjectKeyFromObject(trustedHelmApp), trustedHelmApp)
+		return len(trustedHelmApp.Status.Conditions) > 0
+	}, "30s", "2s").Should(BeTrue())

This change would make tests more resilient and potentially faster when conditions are established quickly.

---

693-860: LGTM! Diagnostic and verification helpers are comprehensive.

The diagnostic infrastructure is well-designed:

• The podTrust struct (lines 693-723) and custom trustCerts matcher (lines 725-751) provide excellent diagnostic context for debugging test failures
• getTrustedCertCount (lines 753-785) correctly parses and deduplicates certificates from the CA bundle
• verifyCorrectlyConfiguredTrust (lines 820-852) validates trust configuration with both positive (trusted hosts succeed) and negative (untrusted hosts fail) test cases
• purgeCtbs (lines 854-860) ensures proper cleanup of cluster-wide resources using label selectors

The use of strings.Lines at line 774 is correct for Go 1.24, as confirmed in the previous review discussion.

Consider replacing manual retry loop with Eventually().

Lines 757-769 implement manual retry logic for handling pod termination during trust source updates. This could be made more idiomatic using Gomega's Eventually():

-	// retry a few times, because pod can be restarting during trust source update, get Terminating between check and use.
-	for i := 0; i < 3; i++ {
-		command := []string{
-			"kubectl", "-n", rsPod.Namespace, "exec",
-			"-c", "argocd-repo-server", rsPod.Name, "--",
-			"cat", caBundlePath,
-		}
-
-		out, err = osFixture.ExecCommandWithOutputParam(false, command...)
-		if err == nil {
-			break
-		}
-		time.Sleep(1 * time.Second)
-	}
-	Expect(err).ToNot(HaveOccurred(), out)
+	command := []string{
+		"kubectl", "-n", rsPod.Namespace, "exec",
+		"-c", "argocd-repo-server", rsPod.Name, "--",
+		"cat", caBundlePath,
+	}
+	Eventually(func() error {
+		out, err = osFixture.ExecCommandWithOutputParam(false, command...)
+		return err
+	}, "5s", "1s").Should(Succeed())

This approach is more consistent with the rest of the test suite's polling patterns.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 269f06c and ab0c9e8.

📒 Files selected for processing (2)

• controllers/argocd/repo_server.go (5 hunks)
• tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1 hunks)

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

• GitHub Check: Run golangci-lint and gosec
• GitHub Check: build
• GitHub Check: Run end-to-end tests (v1.33.5-k3s1)
• GitHub Check: Run end-to-end tests (v1.27.1-k3s1)
• GitHub Check: build-operator

🔇 Additional comments (9)

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (2)

1-82: LGTM! Test setup and imports are well-structured.

The imports, global variables, and test fixtures are appropriately organized for comprehensive E2E testing. The ClusterTrustBundle API version alignment issue from the previous review has been correctly resolved—the import at line 45 (k8s.io/api/certificates/v1beta1) now matches the APIVersion used in createCtbFromCerts at line 633.

---

462-691: LGTM! Helper functions are well-designed and correct.

The helper functions demonstrate good practices:

• ctbUpdate (lines 462-476) properly handles update conflicts with retry logic
• detectClusterTrustBundleSupport (lines 489-496) gracefully handles clusters without CTB support
• getCACert (lines 672-683) correctly extracts the root CA certificate (last in the chain) using TLS 1.3
• Resource creation helpers follow Kubernetes conventions and provide good test fixtures

The ClusterTrustBundle APIVersion at line 633 correctly matches the v1beta1 import, resolving the concern from the previous review.

controllers/argocd/repo_server.go (7)

360-360: LGTM!

The CA trust injection is correctly placed after base volume configuration and before replica/annotation handling.

---

378-382: LGTM!

The reconciler hook integration follows proper error handling patterns and includes appropriate logging.

---

576-659: LGTM!

The CA trust injection logic is well-structured and correctly:

• Documents Ubuntu-specific constraints and the two-volume approach
• Resolves the actual /tmp volume name to respect user overrides (addressing previous review feedback)
• Restricts injection to compatible containers (those using argoImage)
• Provides clear logging of injection targets

---

661-705: LGTM!

The volume projection logic correctly:

• Normalizes certificate paths to .crt suffix (required by update-ca-certificates)
• Uses DeepCopy() to avoid mutating the original CR spec
• Tracks source metadata for observability
• Handles all three source types consistently

---

707-755: LGTM!

The init container implementation correctly:

• Accepts tmpVolume parameter to respect user-provided /tmp mounts (addressing previous review feedback)
• Uses strict bash error handling (set -eEuo pipefail)
• Implements DropImageCertificates via configurable IMAGE_CERT_PATH
• Creates defensive empty ca-certificates.crt before running update-ca-certificates

Note: Resource allocation (line 752) reuses repo server resources. Per past review comments, this remains under discussion but is not blocking.

---

958-1029: LGTM!

The CA trust mapper correctly:

• Scopes ArgoCD listing based on resource scope (cluster-wide for ClusterTrustBundle, namespace-scoped for Secret/ConfigMap)
• Uses isRelevantCtb for complex ClusterTrustBundle matching
• Deletes pods to force re-initialization (appropriate for init-container changes)
• Returns empty request list since pod deletion triggers reconciliation

The panic on unknown types is intentional per previous discussion.

---

1031-1056: LGTM!

The ClusterTrustBundle matching logic correctly implements Kubernetes selection semantics:

• Name-based selection (direct name match)
• SignerName-based selection with optional label selector
• Properly distinguishes between nil LabelSelector (match nothing) and empty LabelSelector (match everything)
• Includes error handling for invalid label selectors

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (5)

tests/ginkgo/fixture/argocd/fixture.go (1)

152-170: Consider aligning diagnostic format with other matchers.

The Printf format with explicit field labels is clear and informative. However, other matchers in this file (HavePhase, HaveRedisStatus, etc.) use a simpler Println pattern. For consistency, consider either:

• Updating this function to match the existing pattern, or
• Updating all matchers to use the more detailed Printf format

Both approaches work; consistency across the file aids maintenance.

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (2)

236-246: Consider using Eventually instead of time.Sleep for condition checks.

The time.Sleep(20 * time.Second) followed by assertions could be replaced with Eventually for a more resilient and idiomatic Ginkgo test pattern. The same applies to line 835.

-	// Sleep to make sure the apps sync took place - otherwise there might be no conditions _yet_
-	time.Sleep(20 * time.Second)
-
-	Expect(trustedHelmApp).Should(appFixture.HaveConditionMatching(
+	Eventually(trustedHelmApp, "30s", "5s").Should(appFixture.HaveConditionMatching(
 		"ComparisonError",
 		".*tls: failed to verify certificate: x509: certificate signed by unknown authority.*",
 	))

---

796-818: Consider returning error instead of panic for better test diagnostics.

Panicking on failure provides less helpful diagnostics than Ginkgo's assertion failures. Consider returning (*corev1.Pod, error) and letting callers handle the error with Expect.

Alternatively, if this function is always called within Eventually, the panic behavior is acceptable as a circuit breaker. Current implementation is functional.

bundle/manifests/argoproj.io_argocds.yaml (1)

20561-20791: Schema structure for systemCATrust is well-designed and follows Kubernetes conventions.

The new systemCATrust field correctly models CA trust injection with proper use of OpenAPI schema patterns:

• Kubernetes metadata markers (x-kubernetes-map-type, x-kubernetes-list-type) correctly applied
• Projection types align with native Kubernetes patterns (ClusterTrustBundleProjection, ConfigMapProjection, SecretProjection)
• Descriptions are comprehensive and guide users appropriately
• Required constraints properly specify mandatory fields (e.g., path for clusterTrustBundles)

Optional consideration: Consider adding a schema constraint to require at least one source (clusterTrustBundles, configMaps, or secrets) when systemCATrust is specified. Currently, an empty systemCATrust: {} would be schema-valid, which could confuse users unless dropImageCertificates: true is the intent. This could be enforced via validation rules if your CRD validation framework supports it (CEL/OpenAPI 3.1 validation rules).

api/v1beta1/argocd_types.go (1)

607-617: Consider adding validation tags and OLM annotations for production readiness.

The type definition is well-structured and uses appropriate Kubernetes native projection types. The API design aligns with the goal of merging multiple CA sources into a single volume.

Optional improvements to consider:

1. Kubebuilder validation tags: Add validation constraints if needed, e.g., +kubebuilder:validation:MinItems=1 on slice fields if at least one source should be required, or size limits to prevent excessively large configurations.

2. OLM annotations: Add +operator-sdk:csv:customresourcedefinitions annotations to the fields for better integration with Operator Lifecycle Manager UI (consistent with other fields in this file).

3. Documentation: Consider adding more detailed comments about:

   • Expected certificate file formats within the projections (e.g., PEM-encoded)
   • How DropImageCertificates interacts with the source fields
   • Whether it's valid to set SystemCATrust with all slice fields empty

4. Naming consistency: Minor note - DropImageCertificates uses "Certificates" while the type uses "CA" (Certificate Authority). Consider DropImageCAs for consistency, though current naming is acceptable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab0c9e8 and ef8a2b9.

📒 Files selected for processing (15)

• .github/workflows/ci-build.yaml (2 hunks)
• api/v1beta1/argocd_types.go (2 hunks)
• api/v1beta1/zz_generated.deepcopy.go (2 hunks)
• bundle/manifests/argoproj.io_argocds.yaml (1 hunks)
• config/crd/bases/argoproj.io_argocds.yaml (1 hunks)
• controllers/argocd/argocd_controller.go (1 hunks)
• controllers/argocd/repo_server.go (5 hunks)
• controllers/argocd/util.go (8 hunks)
• deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (1 hunks)
• docs/reference/argocd.md (2 hunks)
• tests/ginkgo/fixture/application/fixture.go (2 hunks)
• tests/ginkgo/fixture/argocd/fixture.go (1 hunks)
• tests/ginkgo/fixture/pod/fixture.go (2 hunks)
• tests/ginkgo/fixture/utils/fixtureUtils.go (2 hunks)
• tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• .github/workflows/ci-build.yaml
• tests/ginkgo/fixture/pod/fixture.go
• tests/ginkgo/fixture/application/fixture.go
• tests/ginkgo/fixture/utils/fixtureUtils.go

🧰 Additional context used

🧬 Code graph analysis (3)

controllers/argocd/util.go (1)

controllers/argoutil/api.go (1)

• VerifyAPI (32-58)

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (6)

tests/ginkgo/fixture/fixture.go (2)

• EnsureSequentialCleanSlate (126-128)
• CreateRandomE2ETestNamespaceWithCleanupFunc (184-188)

tests/ginkgo/fixture/utils/fixtureUtils.go (1)

• GetE2ETestKubeClient (36-44)

tests/ginkgo/fixture/argocd/fixture.go (2)

• HaveServerStatus (92-97)
• HaveRepoStatus (85-90)

tests/ginkgo/fixture/application/fixture.go (3)

• HaveConditionMatching (103-123)
• HaveSyncStatusCode (79-89)
• HaveNoConditions (91-101)

version/version.go (1)

• Version (17-17)

tests/ginkgo/fixture/os/fixture.go (1)

• ExecCommandWithOutputParam (15-33)

api/v1beta1/zz_generated.deepcopy.go (1)

api/v1beta1/argocd_types.go (1)

• ArgoCDSystemCATrustSpec (608-617)

🪛 markdownlint-cli2 (0.18.1)

docs/reference/argocd.md

1140-1140: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

---

1141-1141: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

---

1142-1142: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

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

• GitHub Check: build-operator
• GitHub Check: Run end-to-end tests (v1.27.1-k3s1)
• GitHub Check: Run end-to-end tests (v1.33.5-k3s1)
• GitHub Check: Run golangci-lint and gosec
• GitHub Check: build

🔇 Additional comments (20)

tests/ginkgo/fixture/argocd/fixture.go (1)

144-146: Improved diagnostic output.

Storing the length in a variable and using Printf to display the actual value makes test failures easier to debug compared to a generic message.

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1)

84-98: LGTM! Well-structured test setup with proper cleanup.

The test setup properly initializes the k8s client and context, detects cluster CTB support, and ensures cleanup of cluster-wide resources in AfterEach. Good use of fixture.EnsureSequentialCleanSlate() for test isolation.

docs/reference/argocd.md (1)

1105-1150: LGTM! Clear and comprehensive documentation for the new SystemCATrust feature.

The documentation effectively explains:

• Configuration structure with a practical YAML example
• Orthogonality to per-host TLS certificates in argocd-tls-certs-cm
• Benefits of CA/wildcard certificates and plugin-level trust
• Namespace requirements and optional/items mapping behavior
• DropImageCertificates merge behavior

The nested list indentation at lines 1139-1142 is correct for the markdown structure—the static analysis hint is a false positive in this context.

api/v1beta1/zz_generated.deepcopy.go (1)

1286-1320: LGTM! Auto-generated deepcopy functions are correct.

The generated DeepCopyInto and DeepCopy functions for ArgoCDSystemCATrustSpec correctly handle:

• Nil-safe slice copying for ClusterTrustBundles, Secrets, and ConfigMaps
• Element-wise deep copying via DeepCopyInto for each projection type
• The boolean DropImageCertificates field is copied by value assignment

This aligns with the struct definition in api/v1beta1/argocd_types.go.

controllers/argocd/argocd_controller.go (1)

365-369: LGTM. Properly wires the systemCATrustMapper into controller setup.

The addition of r.systemCATrustMapper to setResourceWatches correctly integrates the CA trust source watching capability into the controller's resource watch setup.

api/v1beta1/argocd_types.go (1)

594-596: Verify feature gate requirements and consider adding OLM annotations.

ClusterTrustBundle is currently beta in Kubernetes v1.33+ and remains disabled by default; feature gates must be explicitly enabled (especially in v1.34+). Ensure your operator documentation clearly documents the minimum Kubernetes version and required feature gate enablement for users deploying this feature.

Consider adding +operator-sdk:csv:customresourcedefinitions annotations similar to other fields in this file for consistent OLM integration.

deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (1)

20561-20791: Schema structure is well-formed and follows Kubernetes conventions.

The systemCATrust property and its sub-components (clusterTrustBundles, configMaps, secrets, dropImageCertificates) correctly model the feature design. Field types, requirements, and descriptions align with Kubernetes projected volume APIs. The schema is complete and properly documented.

Minor note: The mutual exclusivity constraints described for ClusterTrustBundleProjection (name vs. signerName vs. labelSelector) are documented but not enforced via schema (e.g., oneOf). Verify that controller-level validation enforces these constraints if necessary for correctness.

config/crd/bases/argoproj.io_argocds.yaml (1)

20550-20780: Schema structure follows Kubernetes conventions and is well-documented.

The systemCATrust addition properly extends the CRD with three projection source types (ClusterTrustBundles, ConfigMaps, Secrets) and a control flag, using standard Kubernetes schema patterns (label selectors, key-to-path mappings, optional markers, atomic list/map types).

Field constraints are appropriate:

• clusterTrustBundles[*].path is required, matching standard API semantics.
• configMaps and secrets reuse Kubernetes' ConfigMapProjection and SecretProjection structures (with optional item-level key-to-path mappings).
• dropImageCertificates boolean cleanly controls CA anchor stripping behavior.

The descriptions are clear and helpful for end users. Positioning after existing cert fields maintains logical coherence.

However, verify that the corresponding Go struct (ArgoCDSystemCATrustSpec or similar) exists in api/v1beta1/argocd_types.go and that controller-gen will regenerate this CRD schema consistently. Web search did not locate a public SystemCATrust type in the argocd-operator api/v1beta1 package, so manual inspection of the Go types is necessary to confirm schema alignment and that this is generated code rather than hand-written.

controllers/argocd/util.go (5)

39-39: LGTM! Consistent API tracking implementation.

The import and API presence tracking follow the established pattern in this file. The accessor function naming and implementation are consistent with existing IsVersionAPIAvailable() and IsImageUpdaterAPIAvailable() functions.

Also applies to: 80-97

---

119-127: LGTM! Correct API verification implementation.

The function correctly distinguishes between the API not being present (returns nil with flag set to false) and an actual error checking for the API (returns the error). This matches the pattern established by verifyVersionAPI() and verifyImageUpdaterAPI().

---

710-712: LGTM! Correct failure handling for ClusterTrustBundle API verification.

The error handling here is appropriate: the function returns an error only when the check itself fails, not when the ClusterTrustBundle API is simply unavailable in the cluster. When the API is not present, verifyClusterTrustBundleAPI() returns nil and sets the flag to false, allowing the operator to start normally with the feature disabled.

---

1033-1033: LGTM! Appropriate function signature extension.

Adding the systemCATrustMapper parameter to setResourceWatches is necessary to enable watches on CA trust resources (Secrets, ConfigMaps, and ClusterTrustBundles) that trigger repo-server reconciliation when trust sources change. The naming and type are consistent with existing mapper parameters.

---

1111-1116: LGTM! Proper conditional watch registration for ClusterTrustBundle.

The watch registrations correctly handle the optional ClusterTrustBundle API by checking IsClusterTrustBundleAPIFound() before registering the watch. This pattern matches how other optional APIs (Prometheus, Routes) are handled in this file and ensures the operator doesn't crash when the ClusterTrustBundle API is unavailable.

controllers/argocd/repo_server.go (7)

17-41: LGTM! Appropriate imports for CA trust integration.

The new imports support the CA trust functionality: certificatesv1beta1 for ClusterTrustBundle API types, metav1/labels for label selector matching, ptr for pointer helpers, and reconcile types for the mapper function. All are used in the new code.

---

360-360: LGTM! Proper integration point for CA trust injection.

The CA trust injection is correctly placed after volume setup but before the deployment is reconciled. The reconciler hook application follows the established pattern with appropriate error handling and logging.

Also applies to: 378-382

---

576-659: LGTM! Well-structured CA trust injection with proper safety checks.

The implementation correctly:

• Returns early when SystemCATrust is not configured
• Resolves the /tmp volume name to respect user-provided mounts
• Injects CA trust only into containers using argoImage (repo-server and compatible plugin sidecars), avoiding potentially incompatible custom sidecar images
• Provides clear logging of trust sources and affected containers

The projected volume setup and init container integration are sound.

---

661-705: LGTM! Thorough volume projection setup with good defensive practices.

The function correctly:

• Ensures all projected files have the .crt suffix required by update-ca-certificates
• Uses DeepCopy() to avoid mutating the original CR spec
• Tracks sources for informative logging including optional status
• Handles ClusterTrustBundles, Secrets, and ConfigMaps consistently

The implementation is defensive and well-structured.

---

707-755: LGTM! Robust init container implementation with proper error handling.

The init container correctly:

• Respects the DropImageCertificates flag to optionally exclude image-bundled CAs
• Uses strict bash error handling (set -eEuo pipefail with ERR trap)
• Creates a fallback empty ca-certificates.crt to handle edge cases
• Mounts all necessary volumes (source CAs, target bundle directory, and /tmp for scratch space)
• Uses the same resource configuration as the copyutil init container for consistency

The /tmp volume name resolution in the caller ensures compatibility with user-provided volume mounts.

---

958-1029: LGTM! Effective change detection and pod restart mechanism.

The mapper function correctly:

• Identifies ArgoCDs affected by trust source changes via type-specific matching logic
• Deletes repo-server pods to force immediate trust reconfiguration (cleaner than annotation-based rollout for this use case)
• Handles errors gracefully by logging and continuing
• Uses intentional fail-fast panic for unknown types (programming error detection per past discussion)
• Returns empty reconcile requests since pod deletion handles the restart

The approach of deleting pods directly ensures trust changes take effect immediately without waiting for a reconciliation cycle.

---

1031-1056: LGTM! Correct implementation of ClusterTrustBundle selection semantics.

The function properly implements both selection methods:

• Name-based: Direct name match when .Name is specified
• SignerName + LabelSelector: Matches on signer with label filtering, correctly distinguishing between:
  • nil LabelSelector → match nothing
  • Empty LabelSelector (no matchers) → match everything
  • LabelSelector with matchers → evaluate selector

Error handling for invalid selectors is appropriate with fail-safe behavior (returns false).

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

docs/reference/argocd.md (1)

1138-1143: Fix nested list indentation to satisfy markdownlint (MD007)

The nested bullet list under the TLS trust section is indented with 4 spaces, while markdownlint (MD007) expects 2. You can fix this without changing the rendered output by reducing the indentation:

- - The certificates are properly configured inside the container, permitting more sophisticated (yet secure) plugin logic. For example:
-    - Kustomize can invoke Helm reaching to other hosts than the source repo
-    - Kustomize can pull resources from other repositories/sources over HTTPS
-    - In general, Config Management Plugin can invoke any TLS-enabled tool present in the image with TLS verification on.
+ - The certificates are properly configured inside the container, permitting more sophisticated (yet secure) plugin logic. For example:
+   - Kustomize can invoke Helm reaching to other hosts than the source repo
+   - Kustomize can pull resources from other repositories/sources over HTTPS
+   - In general, Config Management Plugin can invoke any TLS-enabled tool present in the image with TLS verification on.

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (3)

239-241: Avoid fixed sleeps; prefer Eventually on application status for robustness

Both in the “trusts nothing” test and verifyCorrectlyConfiguredTrust, a fixed time.Sleep(20 * time.Second) is used before asserting application conditions. On slower clusters, syncs may legitimately take longer and cause flakes.

Consider switching to an Eventually on the relevant Application object/conditions instead of a hard sleep, e.g.:

- // Sleep to make sure the apps sync took place - otherwise there might be no conditions _yet_
- time.Sleep(20 * time.Second)
+ Eventually(trustedHelmApp, "2m", "5s").Should(appFixture.HaveSyncStatusCode(appv1alpha1.SyncStatusCodeSynced))

(Similarly for the other call site.)

Also applies to: 834-835

---

672-683: Relax TLS minimum version or document network assumptions in getCACert

getCACert dials real internet endpoints with MinVersion: tls.VersionTLS13. This makes the tests sensitive to:

• External network availability from the CI environment.
• Remote hosts dropping or misconfiguring TLS 1.3 support in the future.

For a test helper whose only purpose is to fetch a CA certificate, you could make this more robust by either:

• Allowing the default TLS version negotiation (omit MinVersion), or
• Using MinVersion: tls.VersionTLS12, which is widely supported, while still being secure enough for this use.

Example:

-func getCACert(host string) string {
-	config := &tls.Config{MinVersion: tls.VersionTLS13}
+func getCACert(host string) string {
+	config := &tls.Config{MinVersion: tls.VersionTLS12}

or:

-	config := &tls.Config{MinVersion: tls.VersionTLS13}
+	config := &tls.Config{}

This keeps the test intent while reducing environmental fragility.

---

796-818: findRunningRepoServerPod assumes a single Running repo-server pod

The helper panics unless there is exactly one Running pod whose name matches .*-repo-server.*. This is fine for the current test setup but will break if the repo-server is ever scaled (e.g., HPA or manual replicas > 1) in the E2E environment.

If you expect scaling to be a possibility, consider either:

• Allowing len(pods) >= 1 and picking a deterministic pod (e.g., first sorted by name), or
• Asserting explicitly in the tests that the repo-server is configured with a single replica before calling this helper.

That would make the helper less brittle against future changes in the operator’s default replicas.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef8a2b9 and 91d6b81.

📒 Files selected for processing (2)

• docs/reference/argocd.md (2 hunks)
• tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (4)

api/v1beta1/argocd_types.go (1)

• ArgoCDSystemCATrustSpec (608-617)

tests/ginkgo/fixture/argocd/fixture.go (2)

• HaveServerStatus (92-97)
• HaveRepoStatus (85-90)

tests/ginkgo/fixture/application/fixture.go (3)

• HaveConditionMatching (103-123)
• HaveSyncStatusCode (79-89)
• HaveNoConditions (91-101)

tests/ginkgo/fixture/os/fixture.go (1)

• ExecCommandWithOutputParam (15-33)

🪛 markdownlint-cli2 (0.18.1)

docs/reference/argocd.md

1140-1140: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

---

1141-1141: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

---

1142-1142: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

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

• GitHub Check: Run end-to-end tests (v1.33.5-k3s1)
• GitHub Check: build-operator
• GitHub Check: Run end-to-end tests (v1.27.1-k3s1)
• GitHub Check: Run golangci-lint and gosec
• GitHub Check: build

🔇 Additional comments (1)

docs/reference/argocd.md (1)

1054-1054: SystemCATrust row wiring and description look good

The new SystemCATrust entry is consistent with the underlying ArgoCDSystemCATrustSpec type and correctly links to the detailed section below. No changes needed here.

[olivergondza]

@anandf, @svghadi, can you please take a look at reviewing/integrating this? Thanks