feat(ctl): add a 'check-config' principal/agent commands to argocd-agentctl which will verify the user's configuration #631
Conversation
Rizwana777
requested review from
chetan-rns,
jannfis,
jgwest and
mikeshng
as code owners
WalkthroughAdds a new CLI command Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as U
participant CLI as "argocd-agentctl:check-config"
participant Principal as "Principal Cluster API"
participant Agent as "Agent Cluster API"
U->>CLI: check-config principal
activate CLI
CLI->>Principal: Detect Argo CD scope (CR or defaults)
CLI->>Principal: Read CA, principal TLS, proxy TLS, JWT secrets
CLI->>Principal: Parse certs/keys, verify expiry & signature, parse RSA JWT key
alt Route API present
CLI->>Principal: Fetch Route(s), verify host in TLS SANs
end
CLI-->>U: Print aggregated results & exit
deactivate CLI
U->>CLI: check-config agent
activate CLI
CLI->>Agent: Read agent TLS/CA secrets, parse cert (CN/expiry)
CLI->>Principal: Fetch principal CA and principal namespace
CLI->>Agent: Verify agent cert signed by principal CA, check principal namespace presence
CLI-->>U: Print aggregated results & exit
deactivate CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Rizwana777
force-pushed
the
issue-7958-check-config
branch
3 times, most recently
from
c1cdde9 to
67b4ce5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
- Coverage 46.25% 46.24% -0.02%
==========================================
Files 90 91 +1
Lines 10174 10525 +351
==========================================
+ Hits 4706 4867 +161
- Misses 4988 5144 +156
- Partials 480 514 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/ctl/check_config_test.go (1)
95-105: Add a regression test for wildcard Route hostsOnce the Route/SAN check handles wildcards, please add a test case here exercising a certificate with a
*.example.comSAN and a Route host likeargocd-server.example.com. That will lock in the fix and prevent the wildcard-support regression from reappearing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/ctl/check_config_test.go (5)
internal/kube/client.go (1)
KubernetesClient(37-44)internal/tlsutil/generate.go (3)
GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)cmd/ctl/check_config.go (2)
RunPrincipalChecks(128-174)RunAgentChecks(178-206)
cmd/ctl/main.go (1)
cmd/ctl/check_config.go (1)
NewCheckConfigCommand(49-61)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run unit tests
- GitHub Check: Run end-to-end tests
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/ctl/check_config.go (1)
83-111: Consider parameter type consistency.This function passes
kubernetes.InterfacetoRunAgentChecks(line 106), whileRunPrincipalChecks(line 105) accepts*kube.KubernetesClient. This inconsistency makes the API less predictable.Consider refactoring
RunAgentChecksto accept*kube.KubernetesClientfor both agent and principal parameters, matching the pattern used byRunPrincipalChecks. This would make the API more consistent and potentially allow agent checks to accessDynamicClientif needed in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/ctl/check_config_test.go (5)
internal/kube/client.go (1)
KubernetesClient(37-44)internal/tlsutil/generate.go (3)
GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)cmd/ctl/check_config.go (2)
RunPrincipalChecks(128-174)RunAgentChecks(178-206)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
cmd/ctl/main.go (1)
cmd/ctl/check_config.go (1)
NewCheckConfigCommand(49-61)
⏰ 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). (1)
- GitHub Check: Run end-to-end tests
🔇 Additional comments (9)
cmd/ctl/main.go (1)
52-52: LGTM!The integration of the new
check-configcommand follows the established pattern and correctly wires the new functionality into the CLI.cmd/ctl/check_config_test.go (3)
25-36: LGTM!The test helper correctly uses
t.Helper()and creates properly structured TLS secrets for testing.
38-111: Good happy-path coverage.The test properly validates the principal checks workflow with appropriate test fixtures and fake clients.
113-152: LGTM!The test correctly validates cross-cluster agent checks, including CA signature verification and namespace matching.
cmd/ctl/check_config.go (5)
37-61: LGTM!The
checkResulttype and command constructor follow appropriate patterns for Cobra-based CLIs.
63-81: LGTM!The command properly validates required flags and handles errors appropriately.
176-206: LGTM!The agent checks are well-structured and cover the essential cross-cluster validation scenarios.
208-234: LGTM!These validation helpers are well-focused with appropriate error handling and clear requirements (e.g., RSA-only for JWT keys).
315-395: LGTM!These helper functions implement cross-cluster validation logic correctly, including CA signature verification, certificate expiration checks, and namespace matching. The
x509FromTLSSecretutility function is well-designed and reusable.
Rizwana777
force-pushed
the
issue-7958-check-config
branch
from
67b4ce5 to
7afd305
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/ctl/check_config.go (1)
356-358: Remove or properly handle the agent namespace check.Lines 357-358 check if the agent's own namespace exists but discard the result with
_, _. This is dead code that serves no purpose.Choose one of these solutions:
Solution 1 (recommended): Remove the dead code:
} - // optional: ensure agent's own namespace exists (sanity) - _, _ = agentKube.CoreV1().Namespaces().Get(ctx, agentNS, metav1.GetOptions{}) return nil }Solution 2: If the sanity check is valuable, handle or log the result:
} - // optional: ensure agent's own namespace exists (sanity) - _, _ = agentKube.CoreV1().Namespaces().Get(ctx, agentNS, metav1.GetOptions{}) + // Verify agent's own namespace exists (sanity check) + if _, err := agentKube.CoreV1().Namespaces().Get(ctx, agentNS, metav1.GetOptions{}); err != nil { + return fmt.Errorf("agent namespace '%s' not found on agent cluster: %w", agentNS, err) + } return nil }cmd/ctl/check_config_test.go (2)
38-111: Consider expanding test coverage for robustness.The test validates the happy path comprehensively but doesn't cover failure scenarios such as:
- Expired or not-yet-valid certificates
- Missing secrets
- Invalid JWT keys (non-RSA)
- Mismatched Route hosts and certificate SANs
- ArgoCD configured in namespaced mode (the test registers the CRD but doesn't create any ArgoCD CR or ConfigMap, so it only tests the "no resources found" path of
verifyArgoCDClusterScoped)Consider adding negative test cases to improve confidence in error handling. For example:
func TestCheckConfigPrincipal_ExpiredCertificate(t *testing.T) { // Create a certificate that expired yesterday // Assert that certSecretValid returns an error } func TestCheckConfigPrincipal_NamespacedMode(t *testing.T) { // Create an ArgoCD CR with spec.applicationNamespaces set // Assert that verifyArgoCDClusterScoped returns an error }
113-152: Consider expanding test coverage for agent checks.Similar to the principal test, this validates only the happy path. Consider adding tests for failure scenarios:
- Expired agent client certificate
- Agent certificate not signed by principal CA
- Missing namespace on principal cluster (mismatched agent CN)
- Missing or invalid agent CA secret
- Invalid ca.crt field in agent CA secret
Example negative test case:
func TestCheckConfigAgent_MismatchedNamespace(t *testing.T) { // Issue client cert with CN "test-cluster" // Don't create namespace "test-cluster" on principal // Assert that namespaceMatchesAgentSubject returns an error }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctl/main.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/ctl/check_config_test.go (5)
internal/kube/client.go (1)
KubernetesClient(37-44)internal/tlsutil/generate.go (3)
GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)cmd/ctl/check_config.go (2)
RunPrincipalChecks(128-168)RunAgentChecks(172-200)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
⏰ 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). (6)
- GitHub Check: Build and push image
- GitHub Check: Run end-to-end tests
- GitHub Check: Run unit tests
- GitHub Check: Build & cache Go code
- GitHub Check: Lint Go code
- GitHub Check: Analyze (go)
cmd/ctl/check_config.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // verifyArgoCDClusterScoped checks operator CR or argocd-cm to ensure no namespaced mode is configured. |
There was a problem hiding this comment.
It appears that this logic is not checking for cluster scoped, but is instead checking if source namespaces field is non-empty. Which is a fine thing to check, but is different from whether the cluster is cluster-scoped.
There was a problem hiding this comment.
Now I have added kubectl get applications --all-namespaces logic to check whether it is cluster scoped, please let me know if this is correct or is there any other way we can check this, thank you
Rizwana777
force-pushed
the
issue-7958-check-config
branch
2 times, most recently
from
ba71141 to
30858d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cmd/ctl/check_config_test.go (2)
170-258: Update deprecated API version.Line 180 uses
v1alpha1for the applications API, which is deprecated according to a previous review comment. Usev1beta1instead to align with current API versions.Apply this diff:
dynCl := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme, map[schema.GroupVersionResource]string{ {Group: "argoproj.io", Version: "v1beta1", Resource: "argocds"}: "ArgoCDList", - {Group: "argoproj.io", Version: "v1alpha1", Resource: "applications"}: "ApplicationList", + {Group: "argoproj.io", Version: "v1beta1", Resource: "applications"}: "ApplicationList", {Group: "route.openshift.io", Version: "v1", Resource: "routes"}: "RouteList", })
260-784: Update deprecated API version across all test cases.The
v1alpha1version for the applications API appears throughout all principal test cases (lines 267, 308, 354, 408, 466, 520, 579, 621, 694). This version is deprecated. Update all occurrences to usev1beta1instead.Example for one occurrence (apply similar changes to all):
dynCl := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme, map[schema.GroupVersionResource]string{ {Group: "argoproj.io", Version: "v1beta1", Resource: "argocds"}: "ArgoCDList", - {Group: "argoproj.io", Version: "v1alpha1", Resource: "applications"}: "ApplicationList", + {Group: "argoproj.io", Version: "v1beta1", Resource: "applications"}: "ApplicationList", })
🧹 Nitpick comments (2)
cmd/ctl/check_config_test.go (2)
61-91: Consider erroring on invalid IPs in test helpers.Lines 65-69 silently skip invalid IP addresses. For test clarity, it's better to fail fast when test data is malformed so issues are caught during test setup rather than causing subtle test failures.
Apply this diff:
func createExpiredCertificate(t *testing.T, name string, signerCert *x509.Certificate, signerKey crypto.PrivateKey, ips []string, dns []string) (string, string) { t.Helper() ipAddresses := []net.IP{} for _, ip := range ips { addr := net.ParseIP(ip) - if addr != nil { - ipAddresses = append(ipAddresses, addr) - } + require.NotNil(t, addr, "invalid IP address in test: %s", ip) + ipAddresses = append(ipAddresses, addr) }
93-146: Simplify CA parsing and handle invalid IPs.Lines 100-115 use an indirect approach (creating a fake client and secret) to parse the CA certificate. This can be simplified by parsing the PEM directly. Additionally, lines 118-123 silently skip invalid IPs, which could hide test setup issues.
Apply this diff:
func createCertSignedByDifferentCA(t *testing.T, name string, ips []string, dns []string) (string, string) { t.Helper() - // Create a different CA differentCAPEM, differentCAKeyPEM, err := tlsutil.GenerateCaCertificate("different-ca") require.NoError(t, err, "generate different CA") - // Create a fake client to parse the CA - cl := fake.NewSimpleClientset() - _, err = cl.CoreV1().Secrets("default").Create(context.TODO(), &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "temp-ca", Namespace: "default"}, - Type: corev1.SecretTypeTLS, - Data: map[string][]byte{ - "tls.crt": []byte(differentCAPEM), - "tls.key": []byte(differentCAKeyPEM), - }, - }, metav1.CreateOptions{}) - require.NoError(t, err, "create temp CA secret") - - differentCA, err := tlsutil.TLSCertFromSecret(context.TODO(), cl, "default", "temp-ca") + // Parse CA cert and key directly + differentCA, err := tls.X509KeyPair([]byte(differentCAPEM), []byte(differentCAKeyPEM)) require.NoError(t, err, "read different CA") differentCASigner, err := x509.ParseCertificate(differentCA.Certificate[0]) require.NoError(t, err, "parse different CA") - // Create certificate signed by different CA ipAddresses := []net.IP{} for _, ip := range ips { addr := net.ParseIP(ip) - if addr != nil { - ipAddresses = append(ipAddresses, addr) - } + require.NotNil(t, addr, "invalid IP address in test: %s", ip) + ipAddresses = append(ipAddresses, addr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ctl/check_config.go
- cmd/ctl/main.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:27.987Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:27.987Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
cmd/ctl/check_config_test.go
🧬 Code graph analysis (1)
cmd/ctl/check_config_test.go (5)
internal/tlsutil/generate.go (4)
GenerateCertificate(148-177)GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)internal/kube/client.go (1)
KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)cmd/ctl/check_config.go (2)
RunPrincipalChecks(226-271)RunAgentChecks(275-307)
⏰ 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). (6)
- GitHub Check: Run unit tests
- GitHub Check: Run end-to-end tests
- GitHub Check: Build & cache Go code
- GitHub Check: Lint Go code
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
cmd/ctl/check_config_test.go (4)
48-59: LGTM!The helper function correctly creates TLS secrets and follows Go testing best practices with
t.Helper()and descriptive error messages.
148-168: LGTM!This helper correctly creates a certificate with an empty CommonName for testing error scenarios.
786-847: LGTM!The valid agent configuration test correctly sets up both agent and principal clusters, creates matching namespaces, and verifies cross-cluster certificate validation.
170-1195: Excellent test coverage addressing previous review feedback.These tests comprehensively cover both success and failure scenarios, directly addressing the previous review comment about checking error cases. Each test validates specific error conditions (missing secrets, expired certificates, mismatched configurations, etc.) and verifies appropriate error messages.
Based on learnings: A previous reviewer requested tests for error cases, and this implementation provides thorough coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:27.987Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:27.987Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
cmd/ctl/check_config_test.go
🧬 Code graph analysis (3)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
cmd/ctl/main.go (1)
cmd/ctl/check_config.go (1)
NewCheckConfigCommand(49-61)
cmd/ctl/check_config_test.go (5)
internal/tlsutil/generate.go (4)
GenerateCertificate(148-177)GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)internal/kube/client.go (1)
KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)cmd/ctl/check_config.go (2)
RunPrincipalChecks(226-271)RunAgentChecks(275-307)
⏰ 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 and push image
- GitHub Check: Run unit tests
- GitHub Check: Run end-to-end tests
- GitHub Check: Lint Go code
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
cmd/ctl/check_config_test.go (1)
170-784: Thorough negative-path coverage keeps regressions in checkThe matrix of principal/agent failure cases (missing secrets, bad certs, mismatched routes, etc.) is exhaustive and mirrors production misconfigurations well. Thanks for putting in this depth of testing.
cmd/ctl/main.go (1)
52-52: Good to see the new command wired inHooking
NewCheckConfigCommand()into the root CLI ensures the new validations are reachable; looks good.
Rizwana777
force-pushed
the
issue-7958-check-config
branch
from
30858d9 to
7cc945d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/ctl/check_config.go (1)
554-555: Consider removing unused optional check.These lines perform an optional check to ensure the agent's namespace exists, but the result is intentionally ignored. If the check isn't needed, remove it to avoid confusion. If it serves a purpose (e.g., logging side-effects), add a comment explaining why.
Apply this diff to remove the dead code:
} - // optional: ensure agent's own namespace exists - _, _ = agentKube.CoreV1().Namespaces().Get(ctx, agentNS, metav1.GetOptions{}) return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:27.987Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:27.987Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
cmd/ctl/check_config_test.go
🧬 Code graph analysis (3)
cmd/ctl/main.go (1)
cmd/ctl/check_config.go (1)
NewCheckConfigCommand(49-61)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
cmd/ctl/check_config_test.go (5)
internal/tlsutil/generate.go (4)
GenerateCertificate(148-177)GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)internal/kube/client.go (1)
KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)cmd/ctl/check_config.go (2)
RunPrincipalChecks(224-269)RunAgentChecks(273-305)
⏰ 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). (6)
- GitHub Check: Lint Go code
- GitHub Check: Run unit tests
- GitHub Check: Run end-to-end tests
- GitHub Check: Build & cache Go code
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
cmd/ctl/main.go (1)
52-52: LGTM!Clean integration of the new check-config command into the CLI hierarchy.
cmd/ctl/check_config_test.go (1)
170-1195: Excellent test coverage!The test suite comprehensively covers both success and failure scenarios for principal and agent validation, including edge cases like expired certificates, signature mismatches, empty CNs, and missing secrets. The test helpers effectively reduce duplication.
Rizwana777
force-pushed
the
issue-7958-check-config
branch
from
7cc945d to
87ab1c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:27.987Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:27.987Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
cmd/ctl/check_config_test.go
🧬 Code graph analysis (3)
cmd/ctl/main.go (1)
cmd/ctl/check_config.go (1)
NewCheckConfigCommand(49-61)
cmd/ctl/check_config_test.go (5)
internal/tlsutil/generate.go (4)
GenerateCertificate(148-177)GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)internal/kube/client.go (1)
KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)cmd/ctl/check_config.go (2)
RunPrincipalChecks(224-275)RunAgentChecks(279-311)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
⏰ 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). (6)
- GitHub Check: Build & cache Go code
- GitHub Check: Run end-to-end tests
- GitHub Check: Run unit tests
- GitHub Check: Lint Go code
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
Rizwana777
force-pushed
the
issue-7958-check-config
branch
3 times, most recently
from
6dcc31a to
527d8e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctl/main.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:28.008Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:28.008Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
cmd/ctl/check_config_test.go
🧬 Code graph analysis (2)
cmd/ctl/check_config_test.go (5)
internal/tlsutil/generate.go (4)
GenerateCertificate(148-177)GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)internal/kube/client.go (1)
KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)cmd/ctl/check_config.go (2)
RunPrincipalChecks(221-292)RunAgentChecks(296-345)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
⏰ 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). (6)
- GitHub Check: Build & cache Go code
- GitHub Check: Lint Go code
- GitHub Check: Run end-to-end tests
- GitHub Check: Run unit tests
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
| if len(cert.Certificate) == 0 || cert.Certificate[0] == nil { | ||
| return nil, fmt.Errorf("%s/%s: secret does not contain certificate data", ns, name) | ||
| } | ||
| if len(cert.Certificate) > 1 { | ||
| return nil, fmt.Errorf("%s/%s: secret contains %d certificates, expected exactly one", ns, name, len(cert.Certificate)) | ||
| } | ||
| parsed, err := x509.ParseCertificate(cert.Certificate[0]) |
There was a problem hiding this comment.
Remove the single-certificate restriction in x509FromTLSSecret.
Rejecting secrets that contain more than one certificate will flag perfectly valid Kubernetes TLS secrets, because tls.crt commonly bundles the full chain (leaf + intermediates). Operators routinely store chains this way (see the ingress/TLS docs and cert-manager guidance). Our CLI will therefore fail even when the secret is healthy. Please drop the len(cert.Certificate) > 1 guard and just parse the first entry, optionally keeping the rest for follow-up validation.
- if len(cert.Certificate) == 0 || cert.Certificate[0] == nil {
- return nil, fmt.Errorf("%s/%s: secret does not contain certificate data", ns, name)
- }
- if len(cert.Certificate) > 1 {
- return nil, fmt.Errorf("%s/%s: secret contains %d certificates, expected exactly one", ns, name, len(cert.Certificate))
- }
+ if len(cert.Certificate) == 0 || cert.Certificate[0] == nil {
+ return nil, fmt.Errorf("%s/%s: secret does not contain certificate data", ns, name)
+ }
parsed, err := x509.ParseCertificate(cert.Certificate[0])📝 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.
| if len(cert.Certificate) == 0 || cert.Certificate[0] == nil { | |
| return nil, fmt.Errorf("%s/%s: secret does not contain certificate data", ns, name) | |
| } | |
| if len(cert.Certificate) > 1 { | |
| return nil, fmt.Errorf("%s/%s: secret contains %d certificates, expected exactly one", ns, name, len(cert.Certificate)) | |
| } | |
| parsed, err := x509.ParseCertificate(cert.Certificate[0]) | |
| if len(cert.Certificate) == 0 || cert.Certificate[0] == nil { | |
| return nil, fmt.Errorf("%s/%s: secret does not contain certificate data", ns, name) | |
| } | |
| parsed, err := x509.ParseCertificate(cert.Certificate[0]) |
🤖 Prompt for AI Agents
In cmd/ctl/check_config.go around lines 610 to 616, the function
x509FromTLSSecret incorrectly rejects secrets with more than one certificate
(tls.crt often contains a chain). Remove the guard that returns an error when
len(cert.Certificate) > 1 so the function no longer fails for valid chained
certificates; keep parsing cert.Certificate[0] as the leaf as currently done
and, if desired, retain cert.Certificate slice for any downstream chain
validation or logging rather than rejecting the secret outright.
…entctl which will verify the user's configuration Assisted-by: Cursor Signed-off-by: Rizwana777 <[email protected]>
Rizwana777
force-pushed
the
issue-7958-check-config
branch
from
527d8e9 to
d7376bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/ctl/check_config.go (1)
603-621: Remove the single-certificate restriction to support certificate chains.The check at lines 613-615 rejects secrets containing more than one certificate, but
tls.crtcommonly bundles the full chain (leaf + intermediates). This is standard practice and explicitly supported by Kubernetes TLS secrets. The current logic will incorrectly fail validation for legitimate secrets.Apply this diff to allow certificate chains:
func x509FromTLSSecret(ctx context.Context, kubeClient kubernetes.Interface, ns, name string) (*x509.Certificate, error) { cert, err := tlsutil.TLSCertFromSecret(ctx, kubeClient, ns, name) if err != nil { return nil, err } if len(cert.Certificate) == 0 || cert.Certificate[0] == nil { return nil, fmt.Errorf("%s/%s: secret does not contain certificate data", ns, name) } - if len(cert.Certificate) > 1 { - return nil, fmt.Errorf("%s/%s: secret contains %d certificates, expected exactly one", ns, name, len(cert.Certificate)) - } parsed, err := x509.ParseCertificate(cert.Certificate[0]) if err != nil { return nil, fmt.Errorf("could not parse certificate in secret %s/%s: %w", ns, name, err) } return parsed, nil }
🧹 Nitpick comments (1)
cmd/ctl/check_config_test.go (1)
93-146: Consider simplifying the CA parsing logic.The helper creates a fake Kubernetes client just to store and retrieve the CA certificate via
TLSCertFromSecret. You could simplify by directly parsing the PEM-encoded CA:func createCertSignedByDifferentCA(t *testing.T, name string, ips []string, dns []string) (string, string) { t.Helper() - // Create a different CA differentCAPEM, differentCAKeyPEM, err := tlsutil.GenerateCaCertificate("different-ca") require.NoError(t, err, "generate different CA") - // Create a fake client to parse the CA - cl := fake.NewSimpleClientset() - _, err = cl.CoreV1().Secrets("default").Create(context.TODO(), &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "temp-ca", Namespace: "default"}, - Type: corev1.SecretTypeTLS, - Data: map[string][]byte{ - "tls.crt": []byte(differentCAPEM), - "tls.key": []byte(differentCAKeyPEM), - }, - }, metav1.CreateOptions{}) - require.NoError(t, err, "create temp CA secret") - - differentCA, err := tlsutil.TLSCertFromSecret(context.TODO(), cl, "default", "temp-ca") - require.NoError(t, err, "read different CA") - differentCASigner, err := x509.ParseCertificate(differentCA.Certificate[0]) - require.NoError(t, err, "parse different CA") + // Parse CA directly from PEM + differentCA, err := tls.X509KeyPair([]byte(differentCAPEM), []byte(differentCAKeyPEM)) + require.NoError(t, err, "parse different CA keypair") + differentCASigner, err := x509.ParseCertificate(differentCA.Certificate[0]) + require.NoError(t, err, "parse different CA cert")This requires adding
"crypto/tls"to imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctl/check_config.go(1 hunks)cmd/ctl/check_config_test.go(1 hunks)cmd/ctl/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctl/main.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:28.008Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:28.008Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
cmd/ctl/check_config_test.go
🧬 Code graph analysis (2)
cmd/ctl/check_config_test.go (3)
internal/tlsutil/generate.go (4)
GenerateCertificate(148-177)GenerateCaCertificate(42-84)GenerateServerCertificate(114-142)GenerateClientCertificate(91-107)internal/tlsutil/kubernetes.go (1)
TLSCertFromSecret(44-66)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)
cmd/ctl/check_config.go (3)
internal/kube/client.go (2)
NewKubernetesClientFromConfig(58-98)KubernetesClient(37-44)internal/config/constants.go (6)
SecretNamePrincipalCA(9-9)SecretNamePrincipalTLS(17-17)SecretNameProxyTLS(21-21)SecretNameJWT(29-29)SecretNameAgentCA(13-13)SecretNameAgentClientCert(25-25)internal/tlsutil/kubernetes.go (3)
TLSCertFromSecret(44-66)JWTSigningKeyFromSecret(167-191)X509CertPoolFromSecret(106-128)
⏰ 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). (6)
- GitHub Check: Run end-to-end tests
- GitHub Check: Build & cache Go code
- GitHub Check: Lint Go code
- GitHub Check: Run unit tests
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
🔇 Additional comments (22)
cmd/ctl/check_config_test.go (8)
48-59: LGTM!Well-structured test helper with proper use of
t.Helper()for clean stack traces.
61-91: LGTM!Correctly generates expired certificates for testing expiration validation logic.
148-168: LGTM!Clean helper for testing empty CN validation.
170-258: LGTM!Comprehensive "Valid configuration" test that properly sets up all required secrets (CA, TLS, Proxy TLS, JWT) and verifies all checks pass. Good use of fake clients and dynamic client mocking.
260-299: LGTM!Good error case coverage for missing CA secret, properly verifies the check fails with a meaningful error.
687-783: LGTM!Thorough test for Route host mismatch scenario. Correctly mocks the OpenShift Route API discovery and creates a route with a hostname that doesn't match the certificate SANs.
786-847: LGTM!Comprehensive "Valid configuration" test for agent checks. Properly sets up both agent and principal clusters with correct CA chain and client certificates, and verifies namespace matching works correctly.
1073-1133: LGTM!Important test case that verifies client certificates signed by a different CA are properly rejected. This is critical for mTLS security validation.
cmd/ctl/check_config.go (14)
38-48: LGTM!Clean struct with user-friendly output formatting using emoji indicators.
50-112: LGTM!Well-structured Cobra commands with proper flag validation. The agent command correctly runs principal checks as part of its validation, ensuring comprehensive cross-cluster verification.
128-148: LGTM!Clean implementation with proper error handling. Correctly distinguishes between "ArgoCD CR not found" (returns
nil, false, nil) and actual errors. The nil DynamicClient check addresses previous review feedback.
150-190: LGTM!Properly reads custom secret names from the ArgoCD CR spec while falling back to defaults. This addresses the previous review feedback about respecting
.spec.argoCDAgent.principal.*field customizations.
220-292: LGTM!Well-structured validation orchestration. Properly:
- Reads CR-configured secret names with fallback to defaults
- Only performs OpenShift Route validation when Route API exists AND routes are present
- Surfaces errors appropriately at each step
294-345: LGTM!Clean agent validation that properly orchestrates cross-cluster checks including CA verification, certificate expiry, namespace existence on principal, and signature chain validation.
380-453: LGTM!Thorough cluster-scoped mode verification that:
- Checks
spec.applicationNamespacesin ArgoCD CR- Falls back to
application.namespacesin argocd-cm ConfigMap- Verifies actual cluster-scoped behavior by listing Applications across all namespaces
The
Namespace(metav1.NamespaceAll)usage correctly addresses previous feedback about namespaced resource listing.
455-468: LGTM!Correctly distinguishes between "Route API not installed" (returns
false, nil) and actual discovery errors (returnsfalse, err). Addresses previous feedback about surfacing real errors rather than silently returning false.
470-481: LGTM!Clean implementation that properly surfaces errors instead of silently returning false. Addresses previous review feedback.
483-534: LGTM!Correctly uses
parsed.VerifyHostname(hostname)which handles wildcard SANs properly. Addresses previous feedback about not requiring exact SAN matches.
352-366: LGTM!Correctly validates the full certificate validity window by checking both
NotBeforeandNotAfter. Addresses previous review feedback.
553-566: LGTM!Correctly validates the full certificate validity window. Addresses previous review feedback.
568-581: LGTM!Clean signature verification using
CheckSignatureFromto validate the agent certificate is signed by the principal CA.
583-601: LGTM!Proper cross-cluster validation that verifies the namespace matching the agent certificate's CN exists on the principal cluster.
3 participants
JIRA - https://issues.redhat.com/browse/GITOPS-7958
Commands implemented via cobra
Implemented check-config with principal and agent subcommands
Principal: requires --principal-namespace; --principal-context is available and optional.
Agent: requires all four flags (--agent-context, --agent-namespace, --principal-context, --principal-namespace).
Principal checks implemented:
Verify CA secret exists and is valid: argocd-agent-ca
Verify principal gRPC TLS secret exists and is valid: argocd-agent-principal-tls
Verify resource proxy TLS secret exists and is valid: argocd-agent-resource-proxy-tls
Verify JWT signing key secret exists and parses: argocd-agent-jwt
Argo CD cluster-scoped, apps-any-namespace, OpenShift Route host matches IPS/DNS.
Agent checks implemented:
Verify agent CA secret with ca.crt exists: argocd-agent-ca
Verify agent client TLS secret exists, not expired, and signed by principal CA: argocd-agent-client-tls
Verify a namespace on principal matches agent cert CN (e.g., CN jonathans-cluster => namespace jonathans-cluster)
Also runs all principal checks (above) using provided principal context/namespace
Steps to verify this -
make setup-e2e
make cli
principal -
argocd-agentctl check-config principal --principal-namespace argocd --principal-context
agent -
argocd-agentctl check-config agent --agent-context --agent-namespace argocd --principal-context --principal-namespace argocd
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.