[GITOPS-9258]: Configurable TLS server settings for argocd and argocd-agent components#2139
Open
akhilnittala wants to merge 14 commits intoargoproj-labs:masterfrom
Open
[GITOPS-9258]: Configurable TLS server settings for argocd and argocd-agent components#2139akhilnittala wants to merge 14 commits intoargoproj-labs:masterfrom
akhilnittala wants to merge 14 commits intoargoproj-labs:masterfrom
Conversation
akhilnittala
commented
Mar 27, 2026
Contributor
Author
akhilnittala
left a comment
akhilnittala
left a comment
There was a problem hiding this comment.
Initial DRAFT PR for POC idea.
svghadi
reviewed
Apr 6, 2026
akhilnittala
force-pushed
the
usr/akhil/GITOPS-9260
branch
from
300c2d6 to
c441426
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an ArgoCDTlsConfig API and Changes
Sequence Diagram(s)sequenceDiagram
participant CR as ArgoCD_CR
participant Controller as argocd-operator
participant Argoutil as tls-utils
participant K8s as Kubernetes_API
participant Redis as redis-Deployment
participant Repo as repo-server-Deployment
participant Server as argocd-server-Deployment
CR->>Controller: Reconcile(spec.tlsConfig)
Controller->>Argoutil: BuildRedisArgs(cr.Spec.Redis.TlsConfig)
Controller->>Argoutil: BuildTLSArgs(cr.Spec.Server.TlsConfig / cr.Spec.Repo.TlsConfig)
Controller->>Argoutil: BuildArgoCDAgentTLSArgs(cr.Spec.Principal.TlsConfig)
Argoutil-->>Controller: args map / args slices or error
alt builder error
Controller->>K8s: Update ArgoCD status (ErrorOccurred)
Controller-->>CR: abort reconcile (error)
else builders succeed
Controller->>K8s: Create/Update Deployments with container Args/Env
K8s-->>Redis: apply redis args (e.g., --tls-protocols, --tls-ciphers)
K8s-->>Repo: apply repo-server args (--tlsminversion/--tlsciphers)
K8s-->>Server: apply server args (--tlsminversion/--tlsmaxversion)
K8s-->>Controller: Deployments updated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
akhilnittala
force-pushed
the
usr/akhil/GITOPS-9260
branch
3 times, most recently
from
3259470 to
7640cd5
Compare
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
akhilnittala
force-pushed
the
usr/akhil/GITOPS-9260
branch
from
7640cd5 to
d4ebec4
Compare
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/argocd/deployment_test.go (1)
2027-2036:⚠️ Potential issue | 🟡 MinorAlign the Redis TLS auth expectation with the E2E contract.
This unit test still expects
--aclfile /app/config/redis-auth/users.acl, but the updated E2E coverage intests/ginkgo/parallel/1-066_validate_redis_secure_comm_no_autotls_no_ha_test.gonow asserts--requirepass $(REDIS_PASSWORD)for the TLS startup path. Keeping both expectations different makes the Redis-over-TLS contract ambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/argocd/deployment_test.go` around lines 2027 - 2036, The test's expected Redis startup arguments in the want slice are out of sync with the E2E contract: replace the ACL-based expectation with the TLS startup expectation that requires a password. In the want slice defined in deployment_test.go (the variable named want in the test), remove the "--aclfile", "/app/config/redis-auth/users.acl" entries and instead include the "--requirepass", "$(REDIS_PASSWORD)" pair (and ensure no other conflicting flags like "--tls-auth-clients" remain that contradict this TLS-with-password path); update any assertions that compare this want slice to the generated container args so they now expect the requirepass entry.
🧹 Nitpick comments (2)
config/crd/bases/argoproj.io_argocds.yaml (1)
22299-22327: Consider adding field-level descriptions for TLS subfields.Only object-level
tlsConfigdescriptions are present here. Adding descriptions forminVersion,maxVersion, andcipherSuiteswould improve CRD discoverability (kubectl explain) and reduce ambiguity for users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/argoproj.io_argocds.yaml` around lines 22299 - 22327, Add field-level descriptions under the tlsConfig properties: add a description for minVersion (explain it is the minimum TLS protocol allowed and list accepted enum values like "1.1","1.2","1.3" and variants), a description for maxVersion (explain it is the maximum TLS protocol allowed and list accepted enum values), and a description for cipherSuites (explain it is an ordered list of cipher suite names to enable/disable and the expected string format). Place these description keys directly alongside the existing schema entries for properties.cipherSuites, properties.minVersion, and properties.maxVersion so kubectl explain surfaces clear guidance for tlsConfig.controllers/argocdagent/deployment.go (1)
111-113: Reduce duplicate error logs across helper layers.The same env/TLS error is logged multiple times. Consider returning wrapped errors from helpers and logging once in the top reconcile function for cleaner signal.
♻️ Proposed refactor
func buildPrincipalSpec(compName, saName string, cr *argoproj.ArgoCD) (appsv1.DeploymentSpec, error) { redisAuthVolume, redisAuthMount := argoutil.MountRedisAuthToArgo(cr) envParams, err := buildPrincipalContainerEnv(cr) if err != nil { - log.Error(err, "failed to build principal container env") - return appsv1.DeploymentSpec{}, err + return appsv1.DeploymentSpec{}, fmt.Errorf("build principal container env: %w", err) }func buildPrincipalContainerEnv(cr *argoproj.ArgoCD) ([]corev1.EnvVar, error) { arguments, err := getPrincipalTlsConfig(cr) if err != nil { - log.Error(err, "failed to get principal TLS config") - return nil, err + return nil, fmt.Errorf("get principal TLS config: %w", err) }Also applies to: 331-333, 292-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/argocdagent/deployment.go` around lines 111 - 113, Helpers like the ones that currently call log.Error("failed to build principal container env", ...) are logging errors multiple times; remove logging from helper functions (e.g., the function that returns the env and the TLS helper referenced by the message) and instead return wrapped errors (use fmt.Errorf or errors.Wrap to add context) up to the top-level reconcile function (e.g., Reconcile or the deployment reconcile method) where you log once; update callers that currently ignore or re-log errors to propagate the wrapped error so the single top-level log contains full context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1beta1/argocd_types.go`:
- Around line 1381-1382: The comment on PrincipalTLSSpec is incorrect mentioning
"repo server" and should be updated to accurately describe the field on the
agent principal TLS spec; change the doc comment above TlsConfig (type
*ArgoCDTlsConfig) or the PrincipalTLSSpec type to something like "TLS
configuration for the agent principal" so generated CRD/docs are correct,
ensuring you update the comment immediately above the TlsConfig field or the
PrincipalTLSSpec type declaration (PrincipalTLSSpec, TlsConfig,
ArgoCDTlsConfig).
In `@bundle/manifests/argoproj.io_argocds.yaml`:
- Around line 11923-11954: Add a CRD-level cross-field validation to tlsConfig
to ensure minVersion <= maxVersion: update the source CRD's openAPIV3Schema for
the tlsConfig object to include an "allOf" (or a set of "if"/"then") validation
rules that check combinations of minVersion and maxVersion (e.g., if minVersion
is "1.3" then maxVersion must be one of ["1.3","TLSv1.3","tls1.3"], if
minVersion is "1.2" then maxVersion must be one of ["1.2","1.3",...], etc.),
referencing the existing properties minVersion and maxVersion so invalid pairs
are rejected at schema validation; after updating the source CRD regenerate this
bundle so the changes appear in argoproj.io_argocds.yaml, and repeat the same
change for the other tlsConfig blocks noted (the other ranges referenced in the
comment).
In `@controllers/argocd/deployment.go`:
- Around line 907-910: The BuildTLSArgs call is performed unconditionally and
can cause reconciliation to fail on invalid cr.Spec.Server.TlsConfig even when
the server is disabled; change the logic so BuildTLSArgs is only invoked when
the server is enabled (check cr.Spec.Server.Enabled or the equivalent
server-enabled branch) and propagate/handle its error there, or move the call
into the enabled-component path so the controller can proceed to the
delete/no-op flow when the server is turned off; reference BuildTLSArgs,
cr.Spec.Server.TlsConfig and the local variable arguments to locate and adjust
the code.
- Around line 447-451: getArgoRedisArgs is being called and validating
spec.redis.tlsConfig even when Redis won't be reconciled, causing reconciles to
fail for disabled/remote/HA-backed Redis; change the call site in the deployment
reconcile to first check the Redis reconciliation conditions (use the existing
methods/properties IsEnabled(), IsRemote(), and HA.Enabled on the Redis spec)
and only call getArgoRedisArgs when Redis will actually be reconciled locally,
or alternatively move the tlsConfig validation into getArgoRedisArgs behind the
same checks so that tls validation is skipped for remote/disabled/HA Redis.
In `@controllers/argocd/repo_server.go`:
- Around line 217-223: The call to argoutil.BuildTLSArgs is happening before we
check whether the repo-server should be skipped/remote, causing tlsConfig
validation to block reconciliation; update controllers/argocd/repo_server.go so
that argoutil.BuildTLSArgs(cr.Spec.Repo.TlsConfig) is only invoked after
confirming the repo-server is enabled and not remote (i.e., after the
cr.Spec.Repo.IsEnabled() and cr.Spec.Repo.IsRemote() checks), or move the
BuildTLSArgs call below the branches that return/skip deletion; ensure
deploy.Spec.Template.Spec.Containers and the call to getArgoRepoCommand(cr,
useTLSForRedis) remain unchanged except for using the previously computed
arguments when appropriate.
In `@controllers/argoutil/tls.go`:
- Around line 293-305: buildRedisProtocols currently returns only the min/max
endpoints and uses a map causing non-deterministic ordering; change it to build
a deterministic, ordered slice of all TLS versions between min and max inclusive
(e.g., TLS1.0→TLS1.3 yields TLSv1 TLSv1.1 TLSv1.2 TLSv1.3) and ensure stable
ordering. Also update the Redis argument construction (the related logic at the
other block noted) to separate pre-1.3 and 1.3 cipher handling: emit
--tls-ciphers with OpenSSL-style cipher strings for TLS ≤1.2 connections and
emit --tls-ciphersuites only for TLS1.3 using the IANA/TLS 1.3 suite names; do
not pass Go crypto/tls constant names directly to Redis. Map Go cipher constants
to appropriate OpenSSL cipher-string equivalents for older TLS and to TLS 1.3
suite names for 1.3, and only include the corresponding flags when those
protocols are in the resulting protocol list.
In `@deploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yaml`:
- Around line 11931-11952: The CRD schema currently allows TLS 1.1 via multiple
enum variants in the cipher/minVersion blocks; remove all TLS 1.1 entries
("1.1", "TLSv1.1", "tls1.1") from both the enum list under the unnamed field and
from minVersion so that only 1.2 and 1.3 variants remain (e.g., "1.2", "1.3",
"TLSv1.2", "TLSv1.3", "tls1.2", "tls1.3"), ensuring the fields minVersion and
the related enum no longer accept TLS 1.1.
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Around line 613-617: The test is brittle because it asserts against
argo.Status.Conditions[0]; instead poll until the expected TLS validation
condition appears and assert by matching a condition's Reason/Message (or Type)
rather than relying on index 0. Replace the direct index access
(argo.Status.Conditions[0] in this test) with an Eventually-style loop that
fetches the ArgoCD resource (Get on types.NamespacedName{Name:
argocdInstanceName, Namespace: argocdNamespace}) and scans
argo.Status.Conditions for any entry where Reason == "ErrorOccurred" and Message
== "invalid TLS configuration: unsupported cipher suite:
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"
(or use Type if available), then assert equality once that matching condition is
found.
---
Outside diff comments:
In `@controllers/argocd/deployment_test.go`:
- Around line 2027-2036: The test's expected Redis startup arguments in the want
slice are out of sync with the E2E contract: replace the ACL-based expectation
with the TLS startup expectation that requires a password. In the want slice
defined in deployment_test.go (the variable named want in the test), remove the
"--aclfile", "/app/config/redis-auth/users.acl" entries and instead include the
"--requirepass", "$(REDIS_PASSWORD)" pair (and ensure no other conflicting flags
like "--tls-auth-clients" remain that contradict this TLS-with-password path);
update any assertions that compare this want slice to the generated container
args so they now expect the requirepass entry.
---
Nitpick comments:
In `@config/crd/bases/argoproj.io_argocds.yaml`:
- Around line 22299-22327: Add field-level descriptions under the tlsConfig
properties: add a description for minVersion (explain it is the minimum TLS
protocol allowed and list accepted enum values like "1.1","1.2","1.3" and
variants), a description for maxVersion (explain it is the maximum TLS protocol
allowed and list accepted enum values), and a description for cipherSuites
(explain it is an ordered list of cipher suite names to enable/disable and the
expected string format). Place these description keys directly alongside the
existing schema entries for properties.cipherSuites, properties.minVersion, and
properties.maxVersion so kubectl explain surfaces clear guidance for tlsConfig.
In `@controllers/argocdagent/deployment.go`:
- Around line 111-113: Helpers like the ones that currently call
log.Error("failed to build principal container env", ...) are logging errors
multiple times; remove logging from helper functions (e.g., the function that
returns the env and the TLS helper referenced by the message) and instead return
wrapped errors (use fmt.Errorf or errors.Wrap to add context) up to the
top-level reconcile function (e.g., Reconcile or the deployment reconcile
method) where you log once; update callers that currently ignore or re-log
errors to propagate the wrapped error so the single top-level log contains full
context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: baf2c10f-e06b-4b22-ad49-90f41185fe7b
📒 Files selected for processing (13)
api/v1beta1/argocd_types.goapi/v1beta1/zz_generated.deepcopy.gobundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/deployment.gocontrollers/argocd/deployment_test.gocontrollers/argocd/repo_server.gocontrollers/argocdagent/deployment.gocontrollers/argocdagent/deployment_test.gocontrollers/argoutil/tls.godeploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yamltests/ginkgo/parallel/1-066_validate_redis_secure_comm_no_autotls_no_ha_test.gotests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Outdated
Show resolved
Hide resolved
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go (3)
43-43: Consider handling the error fromGetE2ETestKubeClient.The second return value (error) is discarded. If client initialization fails, subsequent test operations will panic with nil pointer dereference rather than failing gracefully.
♻️ Suggested fix
- c, _ = utils.GetE2ETestKubeClient() + var err error + c, err = utils.GetE2ETestKubeClient() + Expect(err).ToNot(HaveOccurred(), "failed to get E2E test client")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go` at line 43, The call to GetE2ETestKubeClient discards its error return which can lead to nil-pointer panics; change the assignment to capture the error (e.g., c, err := utils.GetE2ETestKubeClient()), then immediately handle it in the test (for Ginkgo use Expect(err).NotTo(HaveOccurred()) or call Fail/FailNow with the error) so the test fails fast and provides a clear error message; update references to variable c accordingly.
254-254: Remove redundanttime.Sleepcalls.The
time.Sleep(5 * time.Second)at lines 254, 339, 440, 525, and 611 are redundant sinceEventuallyblocks immediately follow and handle the waiting with proper polling. These fixed sleeps add 25+ seconds of unnecessary test runtime.♻️ Example removal
Expect(c.Update(ctx, argo)).To(Succeed()) -time.Sleep(5 * time.Second) // --- Validate updated TLS values ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go` at line 254, Remove the redundant time.Sleep(5 * time.Second) calls that immediately precede Gomega Eventually blocks (e.g., the sleep at the top of the snippet and the ones at lines referenced in the review) — locate occurrences of time.Sleep in the test file (tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go) and delete them so the test relies on Eventually's polling; ensure no code outside the Immediately-following Eventually depends on the delay and run the test suite to confirm timing remains correct.
52-91: Consider usingstringspackage for cleaner prefix handling and extracting a Redis helper.The current implementation manually checks prefix lengths. Using
strings.HasPrefixandstrings.TrimPrefixwould be more readable. Also, Redis args parsing (lines 205-221, 299-315, etc.) is repeated 6 times—consider extracting a similar helper.♻️ Simplified example using strings package
+import "strings" + getTLSValues := func(args []string) (min string, max string, hasMin bool, hasMax bool, hasCiphers bool, ciphers string) { + const ( + minFlag = "--tlsminversion" + maxFlag = "--tlsmaxversion" + ciphersFlag = "--tlsciphers" + ) for i := 0; i < len(args); i++ { arg := args[i] - // handle --tlsminversion <value> - if arg == "--tlsminversion" { + if arg == minFlag { hasMin = true if i+1 < len(args) { min = args[i+1] } + } else if strings.HasPrefix(arg, minFlag+"=") { + hasMin = true + min = strings.TrimPrefix(arg, minFlag+"=") } - // ... similar for other flags🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go` around lines 52 - 91, The helper getTLSValues currently uses manual length checks and slicing to detect "--tlsminversion", "--tlsmaxversion" and "--tlsciphers" prefixes; replace those checks with strings.HasPrefix and strings.TrimPrefix for clarity and safety (update getTLSValues to import "strings" and use HasPrefix/TrimPrefix when handling "--tlsminversion=", "--tlsmaxversion=", and "--tlsciphers="). Also extract the repeated Redis args parsing logic (the duplicated blocks around lines referenced in the review: 205-221 and 299-315) into a reusable helper function (e.g., parseRedisArgs or getRedisValues) to avoid duplication and call that helper from the six places where the same parsing occurs. Ensure the new helpers preserve existing return values and behavior and update call sites accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Around line 104-116: The test creates temporary files (redis_crt_File,
redis_key_File, openssl_test_File) but does not remove them; update the test (in
1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go) to defer
cleanup immediately after each os.CreateTemp call (or collect names and remove
at the end) by calling os.Remove or os.RemoveAll on redis_crt_File.Name(),
redis_key_File.Name(), and openssl_test_File.Name() so the temp files are
deleted regardless of test outcome.
- Line 200: The test accesses deployment.Spec.Template.Spec.Containers[0]
unsafely and will panic if the containers slice is empty; update the checks in
this test (and the other occurrences at the same file around the references
noted) to guard against an empty slice by verifying
len(deployment.Spec.Template.Spec.Containers) > 0 before indexing, or locate the
intended container by name/selector instead of assuming index 0; ensure any
assertion that follows (e.g., reading Args) is conditional on that check and
fail the test with a clear message if no containers exist.
---
Nitpick comments:
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Line 43: The call to GetE2ETestKubeClient discards its error return which can
lead to nil-pointer panics; change the assignment to capture the error (e.g., c,
err := utils.GetE2ETestKubeClient()), then immediately handle it in the test
(for Ginkgo use Expect(err).NotTo(HaveOccurred()) or call Fail/FailNow with the
error) so the test fails fast and provides a clear error message; update
references to variable c accordingly.
- Line 254: Remove the redundant time.Sleep(5 * time.Second) calls that
immediately precede Gomega Eventually blocks (e.g., the sleep at the top of the
snippet and the ones at lines referenced in the review) — locate occurrences of
time.Sleep in the test file
(tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go)
and delete them so the test relies on Eventually's polling; ensure no code
outside the Immediately-following Eventually depends on the delay and run the
test suite to confirm timing remains correct.
- Around line 52-91: The helper getTLSValues currently uses manual length checks
and slicing to detect "--tlsminversion", "--tlsmaxversion" and "--tlsciphers"
prefixes; replace those checks with strings.HasPrefix and strings.TrimPrefix for
clarity and safety (update getTLSValues to import "strings" and use
HasPrefix/TrimPrefix when handling "--tlsminversion=", "--tlsmaxversion=", and
"--tlsciphers="). Also extract the repeated Redis args parsing logic (the
duplicated blocks around lines referenced in the review: 205-221 and 299-315)
into a reusable helper function (e.g., parseRedisArgs or getRedisValues) to
avoid duplication and call that helper from the six places where the same
parsing occurs. Ensure the new helpers preserve existing return values and
behavior and update call sites accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebfde1f6-d1f1-4296-8f63-7df8ccf6fd4a
📒 Files selected for processing (1)
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
akhilnittala
commented
Apr 13, 2026
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go (1)
259-260: LetEventuallydo the waiting instead of sleeping first.The fixed 5-second delay slows the happy path and still doesn't guarantee reconciliation has finished. Since each update is already followed by an
Eventually, dropping the sleep will make the test faster and less timing-sensitive. The same pattern repeats after the later updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go` around lines 259 - 260, Remove the fixed time.Sleep calls after controller updates and let the existing Eventually assertions wait for reconciliation; specifically, after calling c.Update(ctx, argo) (and other c.Update(...) calls), delete time.Sleep(5 * time.Second) and rely on the following Eventually(...) blocks to poll until the desired state is reached (do the same for the later updates in this test file). Ensure no new sleeps are introduced and that the Eventually timeouts/intervals are adequate for the update assertions.api/v1beta1/argocd_types.go (1)
610-617: Document the new CRD-facing TLS fields.
ArgoCDTlsConfigis now part of the public CRD surface, but the type and its fields have no doc comments. That leaves the generated CRD/OLM docs with little or no guidance forminVersion,maxVersion, andcipherSuites.📘 Suggested docs
+// ArgoCDTlsConfig defines configurable TLS settings for operator-managed components. type ArgoCDTlsConfig struct { + // MinVersion is the minimum TLS version accepted by the component. // +kubebuilder:validation:Optional // +kubebuilder:validation:Enum="1.1";"1.2";"1.3";"tls1.1";"tls1.2";"tls1.3";"TLSv1.1";"TLSv1.2";"TLSv1.3" MinVersion string `json:"minVersion,omitempty"` + // MaxVersion is the maximum TLS version accepted by the component. // +kubebuilder:validation:Optional // +kubebuilder:validation:Enum="1.1";"1.2";"1.3";"TLSv1.1";"TLSv1.2";"TLSv1.3";"tls1.1";"tls1.2";"tls1.3" MaxVersion string `json:"maxVersion,omitempty"` + // CipherSuites lists the allowed TLS cipher suites for the component. CipherSuites []string `json:"cipherSuites,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1beta1/argocd_types.go` around lines 610 - 617, Add descriptive Go doc comments to the ArgoCDTlsConfig type and each exported field (ArgoCDTlsConfig, MinVersion, MaxVersion, CipherSuites) so CRD/OLM generation includes user-facing documentation: explain that ArgoCDTlsConfig configures TLS protocol limits and cipher suites for Argo CD connections, state the allowed values for MinVersion and MaxVersion (e.g., "1.1", "1.2", "1.3" and equivalent TLSv1.x/tls1.x tokens), clarify that CipherSuites is an ordered list of OpenSSL/Go cipher suite names (give an example entry), and note default behavior when fields are omitted; attach these comments immediately above the type and each field declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Around line 41-45: The test drops the error returned by
utils.GetE2ETestKubeClient() inside the BeforeEach, so if client setup fails the
suite will error later; update the BeforeEach to capture the error (e.g., c, err
:= utils.GetE2ETestKubeClient() or assign to an existing err variable) and
assert it succeeded immediately using the Ginkgo/Gomega assertion (e.g.,
Expect(err).NotTo(HaveOccurred())) so the test fails fast and with a clear
message; modify the BeforeEach block and references to
utils.GetE2ETestKubeClient/BeforeEach accordingly.
- Around line 104-116: The temp files created via os.CreateTemp (redis_crt_File,
redis_key_File, openssl_test_File) are never closed; after creating each temp
file call Close() (or defer a Close immediately after creation) so their file
descriptors are released before you use their .Name() (e.g., call
redis_crt_File.Close(), redis_key_File.Close(), openssl_test_File.Close() right
after each CreateTemp or defer closing where appropriate) to avoid descriptor
leaks and make subsequent os.WriteFile/openssl calls robust.
- Around line 211-227: The loop that scans args only handles space-separated
flags and misses `--flag=value` forms, causing false negatives for Redis TLS
checks; update the parser inside the for loop that iterates over args (look for
variables args, hasProtocols, hasCiphers, tlsProtocols, tlsCipherSuites) to also
detect and split `--tls-protocols=` and `--tls-ciphersuites=` variants (similar
to getTLSValues' handling) so both `--flag value` and `--flag=value` are
supported; apply the same fix to the later copy-pasted Redis assertion block to
keep behavior consistent.
---
Nitpick comments:
In `@api/v1beta1/argocd_types.go`:
- Around line 610-617: Add descriptive Go doc comments to the ArgoCDTlsConfig
type and each exported field (ArgoCDTlsConfig, MinVersion, MaxVersion,
CipherSuites) so CRD/OLM generation includes user-facing documentation: explain
that ArgoCDTlsConfig configures TLS protocol limits and cipher suites for Argo
CD connections, state the allowed values for MinVersion and MaxVersion (e.g.,
"1.1", "1.2", "1.3" and equivalent TLSv1.x/tls1.x tokens), clarify that
CipherSuites is an ordered list of OpenSSL/Go cipher suite names (give an
example entry), and note default behavior when fields are omitted; attach these
comments immediately above the type and each field declaration.
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Around line 259-260: Remove the fixed time.Sleep calls after controller
updates and let the existing Eventually assertions wait for reconciliation;
specifically, after calling c.Update(ctx, argo) (and other c.Update(...) calls),
delete time.Sleep(5 * time.Second) and rely on the following Eventually(...)
blocks to poll until the desired state is reached (do the same for the later
updates in this test file). Ensure no new sleeps are introduced and that the
Eventually timeouts/intervals are adequate for the update assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63c1661c-81bc-4378-8f65-b6a433e0a021
📒 Files selected for processing (7)
api/v1beta1/argocd_types.gobundle/manifests/argocd-operator.clusterserviceversion.yamlbundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamldeploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yamltests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
✅ Files skipped from review due to trivial changes (4)
- bundle/manifests/argocd-operator.clusterserviceversion.yaml
- deploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yaml
- config/crd/bases/argoproj.io_argocds.yaml
- deploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- bundle/manifests/argoproj.io_argocds.yaml
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go (1)
104-111:⚠️ Potential issue | 🟡 MinorClose the temp files right after
CreateTemp.These handles are only used for their paths. Leaving them open leaks FDs and can make the later
WriteFile/opensslsteps more brittle on some environments.💡 Suggested fix
redis_crt_File, err := os.CreateTemp("", "redis.crt") Expect(err).ToNot(HaveOccurred()) + Expect(redis_crt_File.Close()).To(Succeed()) redis_key_File, err := os.CreateTemp("", "redis.key") Expect(err).ToNot(HaveOccurred()) + Expect(redis_key_File.Close()).To(Succeed()) openssl_test_File, err := os.CreateTemp("", "openssl_test.cnf") Expect(err).ToNot(HaveOccurred()) + Expect(openssl_test_File.Close()).To(Succeed())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go` around lines 104 - 111, The temp files created by os.CreateTemp (redis_crt_File, redis_key_File, openssl_test_File) are left open and should be closed immediately after creation; update the test to call Close() on each returned *os.File right after the CreateTemp call (checking/ignoring Close errors as appropriate) so only the paths are used for subsequent os.WriteFile/openssl steps, preventing file descriptor leaks and race issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Around line 602-632: The test should skip containers that don't contain TLS
flags before asserting values: in the loop over
deployment.Spec.Template.Spec.Containers (for coreDeployments) call
getTLSValues(container.Args) and check the same presence guards used earlier
(hasMin and hasMax) — only perform the tls min/max/ciphers assertions when those
guards are true for that container; if not, continue to the next container so
non-target containers won't cause empty-string failures. Ensure you apply the
same guard change to the second similar block around lines 718-748.
- Around line 417-476: The test currently only validates tls-ciphers and
tls-ciphersuites values if those flags are present, allowing a missing flag to
pass; update the three validation blocks that check cipher values (the ones
using hasCiphers, hasCiphersTLS2, tlsCipherSuites, tlsCiphers) to require
presence as well as the exact value (e.g., change the condition to if
!hasCiphers || tlsCipherSuites != "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384" and
similarly for hasCiphersTLS2/tlsCiphers), and apply the same presence+value
requirement to the other identical blocks in this file (the other Redis cipher
validation sections).
---
Duplicate comments:
In
`@tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go`:
- Around line 104-111: The temp files created by os.CreateTemp (redis_crt_File,
redis_key_File, openssl_test_File) are left open and should be closed
immediately after creation; update the test to call Close() on each returned
*os.File right after the CreateTemp call (checking/ignoring Close errors as
appropriate) so only the paths are used for subsequent os.WriteFile/openssl
steps, preventing file descriptor leaks and race issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1ef7ae8-4621-435f-ac28-f6ef87b1c449
📒 Files selected for processing (8)
api/v1beta1/argocd_types.gobundle/manifests/argocd-operator.clusterserviceversion.yamlbundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argoutil/tls.godeploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yamltests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
✅ Files skipped from review due to trivial changes (4)
- bundle/manifests/argocd-operator.clusterserviceversion.yaml
- deploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yaml
- config/crd/bases/argoproj.io_argocds.yaml
- deploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v1beta1/argocd_types.go
- controllers/argoutil/tls.go
- bundle/manifests/argoproj.io_argocds.yaml
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
tests/ginkgo/sequential/1-143_validate_deployment_Env_Args_For_Tls_Configuration_test.go
Show resolved
Hide resolved
…-agent components Signed-off-by: akhil nittala <nakhil@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind chore
What does this PR do / why we need it:
This PR introduces configurability for TLS server settings across all Argo CD-related components managed by the operator, aligning with OpenShift Container Platform (OCP) requirements for Post-Quantum Cryptography (PQC) readiness starting from OCP 4.22.
OCP mandates that all layered products must support streamlined TLS configuration, including control over:
Minimum and maximum TLS versions
Allowed TLS cipher suites
This change ensures that Argo CD components can comply with future cryptographic standards and evolving security policies.
This PR enables TLS configurability for all operator-managed components that expose TLS endpoints:
Argo CD
argocd-server
→ Adds support for configuring:
--tlsminversion
--tlsmaxversion
--tlsciphers
argocd-repo-server
→ Adds TLS configuration support via flags/env:
TLS versions
Cipher suites
Redis (optional TLS)
Ensures TLS-enabled Redis can be configured with:
TLS protocol versions
Cipher suites
argocd agent
→ Adds support for configuring:
--tlsminversion
--tlsmaxversion
--tlsciphers
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
https://redhat.atlassian.net/browse/GITOPS-9073
How to test changes / Special notes to the reviewer:
Default scenario:
Create a simple argocd with spec {} and see the arguments for argocd server, argocd repo server, argocd redis deployments values of min version and max version and check the connection using openssl.
create argocd agent principal pod according to documentation and see the environement variables for tls min version, max version and cipher suites
Now configure the argument and env variables via argocd CR as below
Now observe the values and verify the connections using openssl.
Summary by CodeRabbit