feat(repo-server): Declare custom trust certs for repo-server and plugins #1876
Conversation
olivergondza
changed the title
|
Compared to the proposal in #1876, it turned out 1 init container is enough. Also, this implements |
|
/ok-to-test |
olivergondza
force-pushed
the
ClusterTrustBundle
branch
from
9e0a19d to
4653fab
Compare
olivergondza
changed the title
|
The "Code scans / Run golangci-lint and gosec (pull_request)" failure to be adressed by #1880 |
|
The test failures are related to the fact the code depends on a tech-preview features. Any advise on how to handle such functionality? |
olivergondza
force-pushed
the
ClusterTrustBundle
branch
6 times, most recently
from
93a8f75 to
2e5afb6
Compare
olivergondza
force-pushed
the
ClusterTrustBundle
branch
2 times, most recently
from
de515ac to
448e641
Compare
olivergondza
force-pushed
the
ClusterTrustBundle
branch
6 times, most recently
from
c0a9ad2 to
448e641
Compare
… or plugins Signed-off-by: Oliver Gondža <ogondza@gmail.com>
…plugins from Secrets and CMs Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
… CTB support Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
olivergondza
force-pushed
the
ClusterTrustBundle
branch
2 times, most recently
from
dfe7ca7 to
198d41d
Compare
…rustBundles to repo-server automatically Signed-off-by: Oliver Gondža <ogondza@gmail.com>
olivergondza
force-pushed
the
ClusterTrustBundle
branch
from
198d41d to
9c3639c
Compare
controllers/argocd/repo_server.go
Outdated
| MountPath: "/etc/ssl/certs/", | ||
| }, | ||
| }, | ||
| Resources: getArgoRepoResources(cr), |
There was a problem hiding this comment.
Repo server resources are generally quite high. For init container, which is just updating the certs, this can be reduced IMO.
There was a problem hiding this comment.
Definitely. Though I see copyutil init container is using the same.
I am thinking if this is not actually desirable to occupy the resources early in the container start because a) the prod container is going to occupy the resources anyway, and b) init container are executed serially.
…rest - Use default image pull policy - Use default security context - Propagate proxy env vars Signed-off-by: Oliver Gondža <ogondza@gmail.com>
|
@anandf, thanks for the review. I have addressed all expect for the resources, as the discussion there is ongoing... |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds system CA trust support to Argo CD: API/schema extensions, controller changes to project CA sources into repo-server pods (volumes + init container), watchers for dynamic updates (Secrets/ConfigMaps/ClusterTrustBundles), generated deepcopy updates, CI tweaks, E2E tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Administrator
participant K8s as Kubernetes API
participant Ctrl as Argo CD Controller
participant CTB as ClusterTrustBundle
participant CM as ConfigMap/Secret
participant Pod as Repo-Server Pod
Admin->>K8s: Create/Update ArgoCD with spec.repo.systemCATrust
K8s->>Ctrl: Inform via watch (systemCATrustMapper)
Ctrl->>CTB: Resolve matching ClusterTrustBundles (if API present)
Ctrl->>CM: Resolve referenced ConfigMaps/Secrets
Ctrl->>Ctrl: Compose projected volume sources and init container spec
Ctrl->>Pod: Reconcile Deployment: add projected volumes, init container, mounts
rect rgb(235,245,255)
note over Pod: Init container runs update-ca-certificates\nmerges projected sources -> ca-trust-target
end
note over Pod: Prod container mounts ca-trust-target (/etc/ssl/certs)\nand ca-trust-source (/usr/local/share/ca-certificates)
Admin->>K8s: Update Secret/ConfigMap/CTB
K8s->>Ctrl: Trigger mapper -> Ctrl restarts pod to pick up changes
sequenceDiagram
participant Proj as Projection (spec)
participant CTB as ClusterTrustBundle
Proj->>CTB: If projection.name present -> compare names
alt Name-based
Proj->>Proj: match by name -> relevant
else Signer/Selector-based
Proj->>CTB: check spec.signerName
Proj->>CTB: evaluate labelSelector against CTB.labels
alt selector matches
Proj->>Proj: relevant = true
else
Proj->>Proj: relevant = false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
tests/ginkgo/fixture/application/fixture.go (1)
109-120: Close the backtick in the diagnostic string.
fmt.Sprintf(" -%s/%s", …)` prints an unmatched backtick; adding the closing backtick keeps the debug output balanced and a bit clearer.- found = append(found, fmt.Sprintf(" - `%s/%s", condition.Type, condition.Message)) + found = append(found, fmt.Sprintf(" - `%s/%s`", condition.Type, condition.Message))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
🧰 Additional context used
🪛 LanguageTool
docs/reference/argocd.md
[uncategorized] ~1137-~1137: Possible missing article found.
Context: ... - The certificates are not pinned for individual host, so CA/wildcard certificates can b...
(AI_HYDRA_LEO_MISSING_AN)
🪛 markdownlint-cli2 (0.18.1)
docs/reference/argocd.md
1139-1139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
1140-1140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
1141-1141: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (11)
.github/workflows/ci-build.yaml (3)
51-53: Matrix strategy correctly balances version coverage for ClusterTrustBundle support.Introducing a two-version matrix (v1.27.1-k3s1 and v1.33.5-k3s1) appropriately addresses the dual requirement: maintaining Kubernetes 1.27 compatibility for downstream users while enabling tech-preview features for newer versions.
74-86: Verify version regex pattern captures all intended versions for ClusterTrustBundle support.The conditional feature-flag logic looks correct, but the regex pattern
v1.3[3456789]*matches only v1.33 and later (v1.34–v1.39+), excluding v1.30, v1.31, and v1.32. If ClusterTrustBundle is available and supported in those earlier 1.3x versions, the pattern should be expanded tov1.3[0-9]*to avoid gaps in coverage.Please confirm whether:
- ClusterTrustBundle is only available/tested in k3s 1.33+, making the current pattern intentional?
- Or should the pattern be
v1.3[0-9]*to capture 1.30–1.39+ uniformly?If the latter, apply this diff:
- v1.3[3456789]*) + v1.3[0-9]*)
74-86: Feature flag configuration is syntactically correct and well-documented.The use of a bash array with proper expansion
"${feature_flags[@]}"and clear inline comments explaining why the flags are needed is idiomatic and maintainable. The flags correctly enable both ClusterTrustBundle and ClusterTrustBundleProjection, plus the required runtime-config, aligning with the PR objectives.tests/ginkgo/fixture/utils/fixtureUtils.go (1)
126-128: Good catch registering the certificates API.Adding
certificatesv1beta1to the scheme prevents runtime decoding errors once the tests start creating or readingClusterTrustBundleresources.tests/ginkgo/fixture/pod/fixture.go (1)
17-31: Nice helper for regex pod lookup.The focused matcher keeps the tests tidy and the
HaveLen(1)assertion guards the single-match invariant up front.deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (3)
20259-20263: Clarify dropImageCertificates semantics and defaults.The
dropImageCertificatesfield (line 20259-20263) lacks a default value specification. Clarify in documentation:
- What is the default behavior if omitted? (Presumably false, preserving image certs)
- Should this default be explicitly set to
falsein the schema for clarity?- How does this interact with the merging of system CAs?
This is a breaking behavior change if not properly documented. Consider adding:
dropImageCertificates: description: DropImageCertificates will remove all certs that are present in the image, leaving only those explicitly configured here. + default: false type: boolean
20101-20331: Based on my verification attempts, the external documentation and release notes do not provide specific information about thesystemCATrustfeature. The previous script execution was truncated due to buffer limitations. Given this, I'll provide the rewritten review comment based on the evidence gathered:Verify CRD schema is properly synchronized with corresponding Go type definitions in the codebase.
The systemCATrust schema extension for repo server certificate management appears to follow Kubernetes conventions correctly. The YAML defines a well-formed structure with proper Kubernetes markers (x-kubernetes-list-type, x-kubernetes-map-type) and includes four sub-properties: clusterTrustBundles, configMaps, dropImageCertificates, and secrets. However, direct verification with the Go type definitions (
ArgoCDSystemCATrustSpecand related types) is needed to confirm this CRD manifest was properly code-generated and remains synchronized. This is particularly important given that auto-generation from Go types can diverge if the operator's Go source was not properly rebuilt after recent changes.
20177-20190: No verification needed; path field semantics are correctly implemented.The ClusterTrustBundleProjection.path field must be a relative path (relative to the volume mount), may not be absolute, and may not contain or start with ".."—these constraints are enforced by the Kubernetes API server. The YAML at lines 20177-20190 correctly marks
pathas required for ClusterTrustBundleProjection items, reflecting the standard Kubernetes type definition. The operator's code additionally callsensureValidPath()incontrollers/argocd/repo_server.go(line 675) for supplementary validation. All path requirements are already enforced and no changes are required.bundle/manifests/argoproj.io_argocds.yaml (1)
20101-20331: Schema generation verified—all fields correctly match Go type definitions.The CRD schema is properly auto-generated from the Go types:
- All 4 fields from
ArgoCDSystemCATrustSpecare present in the manifest- Field names and types align exactly (ClusterTrustBundles, ConfigMaps, Secrets, DropImageCertificates)
- Uses standard Kubernetes projection types (corev1.ClusterTrustBundleProjection, corev1.SecretProjection, corev1.ConfigMapProjection)
- All fields correctly marked as optional (matching
omitemptyin Go struct tags)- No schema errors or discrepancies detected
config/crd/bases/argoproj.io_argocds.yaml (2)
20090-20320: Schema structure looks sound; verify mutual-exclusivity validation is enforced in code.The systemCATrust schema properly defines volume projections for injecting CA certificates into repo-server. The structure, types, and Kubernetes-specific annotations (x-kubernetes-list-type, x-kubernetes-map-type) are correct. However, the ClusterTrustBundleProjection's mutual-exclusivity constraints (lines 20154-20156 and 20172-20174: name vs. signerName/labelSelector) are documented in descriptions but cannot be enforced at the CRD schema level. These constraints must be validated in the Go controller code.
To confirm mutual-exclusivity and other validation constraints are properly handled, verify that the controller code includes validation logic for these fields.
20248-20252: Clarify field name: dropImageCertificates vs. dropImageAnchors.Per the PR comments, the feature was referred to as "DropImageAnchors," but the schema field is named "dropImageCertificates." Confirm this naming is intentional and consistent across the codebase (API types, controller, tests, documentation).
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
There was a problem hiding this comment.
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 withName: "", 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
📒 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 sinceupdate-ca-certificatesis 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-certificatescompatibility- 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:
- Direct name match (precedence)
- SignerName with nil LabelSelector → match nothing
- SignerName with empty LabelSelector → match everything
- SignerName with populated LabelSelector → evaluate selector
| panic(fmt.Errorf("systemCATrustMapper called for unknown type %t", o)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace panic with error logging.
Panicking on unknown types is too harsh and could crash the controller. Log an error and return an empty slice instead:
default:
- panic(fmt.Errorf("systemCATrustMapper called for unknown type %t", o))
+ log.Error(fmt.Errorf("systemCATrustMapper called for unknown type %T", o), "unexpected object type", "type", fmt.Sprintf("%T", o))
+ return []reconcile.Request{}
}Also note: use %T instead of %t to format types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panic(fmt.Errorf("systemCATrustMapper called for unknown type %t", o)) | |
| } | |
| log.Error(fmt.Errorf("systemCATrustMapper called for unknown type %T", o), "unexpected object type", "type", fmt.Sprintf("%T", o)) | |
| return []reconcile.Request{} | |
| } |
🤖 Prompt for AI Agents
In controllers/argocd/repo_server.go around lines 999-1000, replace the
panic(fmt.Errorf(...)) for unknown types with an error log using the existing
logger in this file (e.g., log, r.log, or similar) and return an empty slice;
also change the format verb from %t to %T to print the type. Specifically,
remove the panic call, call the logger's Error/.Errorf with a message like
"systemCATrustMapper called for unknown type %T" including the offending value,
and then return an empty slice (nil or []Type{} as appropriate) so the
controller does not crash. Ensure imports and logger names remain consistent
with the file.
3 participants
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?
Which issue(s) this PR fixes:
Fixes #1830
How to test changes / Special notes to the reviewer:
Can be tested against:
Summary by CodeRabbit
New Features
Documentation
Tests