feat(gitops-backend): Configure TLS MinVersion and Ciphers#1227
feat(gitops-backend): Configure TLS MinVersion and Ciphers#1227akhilnittala wants to merge 3 commits into
Conversation
…nd component Signed-off-by: akhil nittala <nakhil@redhat.com>
|
[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 |
📝 WalkthroughWalkthroughThe controller now receives cluster TLS profile settings and passes supported minimum TLS versions and cipher suites to the GitOps backend Deployment through environment variables. ChangesTLS backend configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterSecurityProfile
participant ReconcileGitopsService
participant newBackendDeployment
participant BackendDeployment
ClusterSecurityProfile->>ReconcileGitopsService: provide MinTLSVersion and Ciphers
ReconcileGitopsService->>newBackendDeployment: pass TLSMinVersion and TLSCiphers
newBackendDeployment->>newBackendDeployment: convert TLS version and build environment
newBackendDeployment->>BackendDeployment: set TLS environment variables
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
🧹 Nitpick comments (1)
controllers/gitopsservice_controller.go (1)
803-814: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse a switch statement instead of a map.
To avoid allocating a map on every function call, consider using a
switchstatement. It is more idiomatic and performs better.♻️ Proposed refactor
func TLSVersionToBackend(tlsVersion string) string { - versionMap := map[string]string{ - "VersionTLS12": "1.2", - "VersionTLS13": "1.3", - "VersionTLS11": "1.1", - "VersionTLS10": "1", - } - if v, ok := versionMap[tlsVersion]; ok { - return v - } - return "" // default fallback + switch tlsVersion { + case "VersionTLS12": + return "1.2" + case "VersionTLS13": + return "1.3" + case "VersionTLS11": + return "1.1" + case "VersionTLS10": + return "1" + default: + return "" // default fallback + } }🤖 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 `@controllers/gitopsservice_controller.go` around lines 803 - 814, Update TLSVersionToBackend to replace the per-call versionMap allocation with a switch on tlsVersion, returning the existing TLS version strings for VersionTLS10 through VersionTLS13 and preserving the empty-string fallback for unknown values.
🤖 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.
Nitpick comments:
In `@controllers/gitopsservice_controller.go`:
- Around line 803-814: Update TLSVersionToBackend to replace the per-call
versionMap allocation with a switch on tlsVersion, returning the existing TLS
version strings for VersionTLS10 through VersionTLS13 and preserving the
empty-string fallback for unknown values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 21920b77-0290-4a4e-877d-7c69f5b22609
📒 Files selected for processing (2)
cmd/main.gocontrollers/gitopsservice_controller.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
…nd component 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/gitopsservice_controller_test.go`:
- Around line 113-126: In controllers/gitopsservice_controller_test.go sites
113-126 and 133-146, update both environment-variable loops to remove the early
break statements so TLS_MIN_VERSION and TLS_CIPHER_SUITES are always collected.
Separate the assertions in each test so gotTLSMinVersion is independently
checked against "1.3" and gotTLSCiphers is independently checked against
"dummy1" at 113-126 and "dummy1:dummy2" at 133-146.
🪄 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: 68ef1871-9cc1-442b-9fea-2370e80ce23a
📒 Files selected for processing (1)
controllers/gitopsservice_controller_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
| for _, env := range deployment.Spec.Template.Spec.Containers[0].Env { | ||
| if env.Name == "TLS_MIN_VERSION" { | ||
| gotTLSMinVersion = env.Value | ||
| break | ||
| } | ||
| if env.Name == "TLS_CIPHER_SUITES" { | ||
| gotTLSCiphers = env.Value | ||
| break | ||
| } | ||
| } | ||
| if gotTLSMinVersion != "1.3" && gotTLSCiphers != "dummy1" { | ||
| t.Errorf("TLS Min Version mismatch: got %s, want %s", gotTLSMinVersion, "1.3") | ||
| t.Errorf("TLS Cipher mismatch: got %s, want %s", gotTLSCiphers, "dummy1") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix flawed test assertions due to early break and && condition.
The break statement inside the loop causes the iteration to exit immediately after finding the first environment variable (TLS_MIN_VERSION), meaning gotTLSCiphers remains empty. Furthermore, using && in the assertion condition hides this bug because the test falsely passes when gotTLSMinVersion is correct, effectively silencing the failure for the missing cipher verification.
controllers/gitopsservice_controller_test.go#L113-L126: Remove thebreakstatements from the loop and separate the assertions to checkgotTLSMinVersionandgotTLSCiphersindependently against"1.3"and"dummy1".controllers/gitopsservice_controller_test.go#L133-L146: Remove thebreakstatements from the loop and separate the assertions to checkgotTLSMinVersionandgotTLSCiphersindependently against"1.3"and"dummy1:dummy2".
📍 Affects 1 file
controllers/gitopsservice_controller_test.go#L113-L126(this comment)controllers/gitopsservice_controller_test.go#L133-L146
🤖 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 `@controllers/gitopsservice_controller_test.go` around lines 113 - 126, In
controllers/gitopsservice_controller_test.go sites 113-126 and 133-146, update
both environment-variable loops to remove the early break statements so
TLS_MIN_VERSION and TLS_CIPHER_SUITES are always collected. Separate the
assertions in each test so gotTLSMinVersion is independently checked against
"1.3" and gotTLSCiphers is independently checked against "dummy1" at 113-126 and
"dummy1:dummy2" at 133-146.
|
@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. |
1 participant
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
To make gitops backend component complaint with OCP 5.0 goals, making tls minversion and ciphers configurable.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
https://redhat.atlassian.net/browse/GITOPS-10474
Test acceptance criteria:
How to test changes / Special notes to the reviewer: