feat: Configuring TLS options from Central TLS Profile#1184
feat: Configuring TLS options from Central TLS Profile#1184akhilnittala wants to merge 12 commits into
Conversation
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
|
@akhilnittala: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe PR integrates OpenShift cluster TLS security profile support into the gitops-operator. It adds ChangesCluster TLS Profile Integration
Sequence DiagramsequenceDiagram
participant Operator as main
participant EnvFlag as env / CLI flag
participant APIServer as config.openshift.io/apiservers
participant tlspkg as tlspkg helpers
participant Servers as Webhook + Metrics Servers
participant Watcher as SecurityProfileWatcher
participant Controller as ReconcileArgoCD
Operator->>EnvFlag: read DISABLE_CLUSTER_TLS_PROFILE
EnvFlag-->>Operator: disableClusterTLSProfile bool
Operator->>APIServer: fetch TLSProfileSpec
APIServer-->>Operator: MinTLSVersion, Ciphers
Operator->>tlspkg: build TLS option functions
tlspkg-->>Operator: tlsOpts
Operator->>Servers: apply tlsOpts
Operator->>Watcher: start watching apiservers
Watcher-->>Operator: profile changed → cancel ctx → manager exits
Operator->>Controller: pass CentralTLSConfigProfile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/main.go`:
- Around line 156-160: The code creates a bootstrapClient using crclient.New()
and assigns any error to err, but immediately calls
tlspkg.FetchAPIServerTLSProfile() with that bootstrapClient on the next line
without checking if client creation failed. Add an error check immediately after
the crclient.New() call to verify err is nil before proceeding to use
bootstrapClient in the FetchAPIServerTLSProfile call. If err is not nil, handle
it appropriately by returning or exiting early to prevent using an invalid
client.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 719b3612-683d-49e6-8b68-a89bdc3cc706
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
bundle/manifests/gitops-operator.clusterserviceversion.yamlcmd/main.goconfig/manager/manager.yamlconfig/rbac/role.yamlcontrollers/gitopsservice_controller.gogo.mod
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
Signed-off-by: akhil nittala <nakhil@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/argocd_metrics_controller.go`:
- Around line 400-401: The comparison at line 400 in the
argocd_metrics_controller.go file compares pointer addresses instead of string
values. The condition
`existingServiceMonitor.Spec.Endpoints[0].TLSConfig.ServerName !=
&desiredMetricsServerName` compares pointer addresses, which are different each
reconciliation cycle even when the string content is identical. Fix this by
dereferencing the pointer from
existingServiceMonitor.Spec.Endpoints[0].TLSConfig.ServerName and comparing it
to the string value desiredMetricsServerName instead, so the comparison checks
actual string equality rather than pointer address equality.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e2f178de-0bcf-4924-afb8-7ee735285c8c
📒 Files selected for processing (3)
bundle/manifests/gitops-operator.clusterserviceversion.yamlcontrollers/argocd_metrics_controller.gotest/openshift/e2e/ginkgo/parallel/1-104_validate_prometheus_alert_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundle/manifests/gitops-operator.clusterserviceversion.yaml
Signed-off-by: akhil nittala <nakhil@redhat.com>
Signed-off-by: akhil nittala <nakhil@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@test/openshift/e2e/ginkgo/parallel/1-066_validate_redis_secure_comm_no_autotls_no_ha_test.go`:
- Line 146: The expectedString variable contains hard-coded TLS cipher and
protocol values that lock the test to one specific TLS profile, making it fail
on clusters with different APIServer TLS profiles even when the operator is
working correctly. Instead of using the hard-coded expectedString, dynamically
read the cluster's live TLSProfileSpec from the APIServer and build the expected
Redis TLS flags (ciphers, protocols, etc.) based on the actual cluster profile
in the test before asserting against the deployment args. This can be done
either inline in the test or via a shared helper function to construct the
expected flags from the cluster's TLS profile.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4f2b652b-bc70-439f-8490-a32ba9ba55a5
📒 Files selected for processing (2)
test/openshift/e2e/ginkgo/parallel/1-066_validate_redis_secure_comm_no_autotls_no_ha_test.gotest/openshift/e2e/ginkgo/parallel/1-068_validate_redis_secure_comm_autols_no_ha_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
| By("expecting redis-server to have desired container process command/arguments") | ||
|
|
||
| expectedString := "--save \"\" --appendonly no --aclfile /app/config/redis-auth/users.acl --tls-port 6379 --port 0 --tls-cert-file /app/config/redis/tls/tls.crt --tls-key-file /app/config/redis/tls/tls.key --tls-auth-clients no" | ||
| expectedString := "--save \"\" --appendonly no --aclfile /app/config/redis-auth/users.acl --tls-protocols TLSv1.2 --tls-ciphers TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256:TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256:TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384:TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384:TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256:TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 --tls-ciphersuites TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256:TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256:TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384:TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384:TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256:TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 --tls-port 6379 --port 0 --tls-cert-file /app/config/redis/tls/tls.crt --tls-key-file /app/config/redis/tls/tls.key --tls-auth-clients no" |
There was a problem hiding this comment.
Make the TLS expectation profile-aware, not hard-coded.
Line 146 locks the test to one cipher/protocol set, so it can fail on clusters with a different APIServer TLS profile even when operator behavior is correct. That also weakens validation of the PR goal (inherit cluster profile dynamically). Build expected Redis TLS flags from the live cluster TLSProfileSpec in-test (or via a shared helper) before asserting deployment args.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@test/openshift/e2e/ginkgo/parallel/1-066_validate_redis_secure_comm_no_autotls_no_ha_test.go`
at line 146, The expectedString variable contains hard-coded TLS cipher and
protocol values that lock the test to one specific TLS profile, making it fail
on clusters with different APIServer TLS profiles even when the operator is
working correctly. Instead of using the hard-coded expectedString, dynamically
read the cluster's live TLSProfileSpec from the APIServer and build the expected
Redis TLS flags (ciphers, protocols, etc.) based on the actual cluster profile
in the test before asserting against the deployment args. This can be done
either inline in the test or via a shared helper function to construct the
expected flags from the cluster's TLS profile.
|
@akhilnittala: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| setupLog.Info("starting manager") | ||
| if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { | ||
| if err := mgr.Start(ctx); err != nil { |
There was a problem hiding this comment.
this change is required for controlling the controller runtime. example reference from https://docs.google.com/document/d/1cMc9E8psHfnoK06ntR8kHSWB8d3rMtmldhnmM4nImjs/edit?tab=t.4cxmujrb3zyn#heading=h.111788dqn5nc
In main: get initial profile, create manager (with TLSOpts if you have a TLS metrics server), create context with cancel, call setupTLSProfileWatcher(ctx, mgr, cancel), then mgr.Start(ctx). When the cluster profile changes, OnProfileChange runs; typically you cancel the context so the process exits and restarts with the new profile (e.g. under a pod restart).
|
/retest |
2 participants
What type of PR is this?
/kind enhancement
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. This change ensures that Argo CD components can comply with future cryptographic standards and evolving security policies.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
https://redhat.atlassian.net/browse/GITOPS-9073
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
Deploy Redis with tls enabled. checks arguments with underlying apiServer configuration.