feat(gitops-console-plugin): Configure SSL protocol and ciphersuites for plugin component#1224
feat(gitops-console-plugin): Configure SSL protocol and ciphersuites for plugin component#1224akhilnittala wants to merge 2 commits into
Conversation
…for plugin 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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughTLS settings now flow from the cluster TLS profile into the GitOps service controller, which generates a TLS-aware plugin ConfigMap. ConfigMap content is hashed into Deployment pod-template annotations to trigger updates when plugin configuration changes. ChangesTLS-aware console plugin configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterTLSProfile
participant ReconcileGitopsService
participant PluginConfigMap
participant Deployment
ClusterTLSProfile->>ReconcileGitopsService: Provide TLSMinVersion and TLSCiphers
ReconcileGitopsService->>PluginConfigMap: Generate TLS-aware httpd.conf
PluginConfigMap-->>ReconcileGitopsService: Return generated ConfigMap
ReconcileGitopsService->>Deployment: Set httpd-cfg-hash on pod template
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🤖 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/consoleplugin.go`:
- Around line 539-540: In ReconcileGitopsService.reconcileConfigMap, update the
existing ConfigMap after copying Data and Labels by passing
existingPluginConfigMap to r.Client.Update instead of the newly generated
newPluginConfigMap, preserving its ResourceVersion.
- Line 437: Initialize existingSpecTemplate.ObjectMeta.Annotations as a writable
map when it is nil before assigning the "httpd-cfg-hash" entry, while preserving
the existing annotation update behavior.
- Around line 529-540: Replace the crypto/md5 import with crypto/sha256 in
controllers/consoleplugin.go at lines 5-5. Update getConfigMapHash at lines
529-540 to create a SHA-256 hash, collect and lexicographically sort cm.Data
keys before writing each key and value, and preserve the existing hexadecimal
string return format.
🪄 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: 23fb09a2-e4fc-4318-898d-224970c23815
📒 Files selected for processing (3)
cmd/main.gocontrollers/consoleplugin.gocontrollers/gitopsservice_controller.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
| existingPluginDeployment.Spec.Replicas = newPluginDeployment.Spec.Replicas | ||
| existingPluginDeployment.Spec.Selector = newPluginDeployment.Spec.Selector | ||
| existingSpecTemplate.Labels = newSpecTemplate.Labels | ||
| existingSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] = newSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent nil-map panic when updating annotations.
If the existing Deployment's pod template does not have any annotations, existingSpecTemplate.ObjectMeta.Annotations will be nil. Attempting to write to a nil map will cause a runtime panic that crashes the reconciliation loop.
🛡️ Proposed fix
+ if existingSpecTemplate.ObjectMeta.Annotations == nil {
+ existingSpecTemplate.ObjectMeta.Annotations = make(map[string]string)
+ }
existingSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] = newSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"]📝 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.
| existingSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] = newSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] | |
| if existingSpecTemplate.ObjectMeta.Annotations == nil { | |
| existingSpecTemplate.ObjectMeta.Annotations = make(map[string]string) | |
| } | |
| existingSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] = newSpecTemplate.ObjectMeta.Annotations["httpd-cfg-hash"] |
🤖 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/consoleplugin.go` at line 437, Initialize
existingSpecTemplate.ObjectMeta.Annotations as a writable map when it is nil
before assigning the "httpd-cfg-hash" entry, while preserving the existing
annotation update behavior.
| // getConfigMapHash returns MD5 hash of ConfigMap data for change detection | ||
| func getConfigMapHash(cm *corev1.ConfigMap) string { | ||
| hash := md5.New() | ||
| for key, value := range cm.Data { | ||
| io.WriteString(hash, key) | ||
| io.WriteString(hash, value) | ||
| } | ||
| return fmt.Sprintf("%x", hash.Sum(nil)) | ||
| } | ||
|
|
||
| func (r *ReconcileGitopsService) reconcileConfigMap(instance *pipelinesv1alpha1.GitopsService, request reconcile.Request, newPluginConfigMap *corev1.ConfigMap) (reconcile.Result, error) { | ||
| reqLogger := logs.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace MD5 with SHA256 to ensure FIPS compliance and fix non-deterministic hashing.
The crypto/md5 package is not FIPS-compliant and can cause panics or be blocked in FIPS-enabled OpenShift clusters. Additionally, Go map iteration order is random; if cm.Data ever contains more than one key, the hash will change randomly on every reconciliation and cause continuous unwanted Deployment rollouts.
controllers/consoleplugin.go#L529-L540: updategetConfigMapHashto usesha256.New()and sort the map keys before iterating to guarantee a deterministic hash.controllers/consoleplugin.go#L5-L5: replace the"crypto/md5"import with"crypto/sha256".
🛠️ Proposed fixes
For the import (controllers/consoleplugin.go#L5-L5):
- "crypto/md5"
+ "crypto/sha256"For the function (controllers/consoleplugin.go#L529-L540):
-func getConfigMapHash(cm *corev1.ConfigMap) string {
- hash := md5.New()
- for key, value := range cm.Data {
- io.WriteString(hash, key)
- io.WriteString(hash, value)
- }
- return fmt.Sprintf("%x", hash.Sum(nil))
-}
+func getConfigMapHash(cm *corev1.ConfigMap) string {
+ hash := sha256.New()
+ keys := make([]string, 0, len(cm.Data))
+ for k := range cm.Data {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
+ for _, k := range keys {
+ io.WriteString(hash, k)
+ io.WriteString(hash, cm.Data[k])
+ }
+ return fmt.Sprintf("%x", hash.Sum(nil))
+}🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 530-530: Detected use of the 'crypto/md5' package (md5.New / md5.Sum). MD5 is a cryptographically broken, collision-prone hash and must not be used for security purposes such as integrity checks, signatures, or password hashing. Use a strong hash from 'crypto/sha256' (sha256.New / sha256.Sum256) or 'crypto/sha512' instead; for passwords use a dedicated KDF such as golang.org/x/crypto/bcrypt or argon2.
Context: md5.New
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(weak-hash-md5-go)
📍 Affects 1 file
controllers/consoleplugin.go#L529-L540(this comment)controllers/consoleplugin.go#L5-L5
🤖 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/consoleplugin.go` around lines 529 - 540, Replace the crypto/md5
import with crypto/sha256 in controllers/consoleplugin.go at lines 5-5. Update
getConfigMapHash at lines 529-540 to create a SHA-256 hash, collect and
lexicographically sort cm.Data keys before writing each key and value, and
preserve the existing hexadecimal string return format.
Source: Linters/SAST tools
| func (r *ReconcileGitopsService) reconcileConfigMap(instance *pipelinesv1alpha1.GitopsService, request reconcile.Request, newPluginConfigMap *corev1.ConfigMap) (reconcile.Result, error) { | ||
| reqLogger := logs.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix r.Client.Update target to avoid missing ResourceVersion error.
Although not directly modified in this PR, the unchanged block at the end of this function incorrectly updates newPluginConfigMap instead of existingPluginConfigMap. Because newPluginConfigMap is freshly generated and lacks a ResourceVersion, Kubernetes will reject the update.
To ensure ConfigMap updates succeed, please fix the target in the existing code block:
// ... in the unchanged code below ...
existingPluginConfigMap.Data = newPluginConfigMap.Data
existingPluginConfigMap.Labels = newPluginConfigMap.Labels
- return reconcile.Result{}, r.Client.Update(context.TODO(), newPluginConfigMap)
+ return reconcile.Result{}, r.Client.Update(context.TODO(), existingPluginConfigMap)🤖 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/consoleplugin.go` around lines 539 - 540, In
ReconcileGitopsService.reconcileConfigMap, update the existing ConfigMap after
copying Data and Labels by passing existingPluginConfigMap to r.Client.Update
instead of the newly generated newPluginConfigMap, preserving its
ResourceVersion.
…for plugin component Signed-off-by: akhil nittala <nakhil@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/consoleplugin_test.go`:
- Around line 2000-2020: Update getConfigMapHash to sort the keys of cm.Data
before writing key-value pairs into the hash, ensuring identical ConfigMaps
always produce the same hash and avoiding unnecessary rollouts.
- Around line 1931-1938: Update the TLS configuration logic exercised by the
“TLS 1.2 with cipher suites” test so TLSMinVersion VersionTLS12 emits an
additive protocol range including TLSv1.2 and TLSv1.3, rather than selecting
TLSv1.2 exclusively. Preserve the cipher-suite configuration and existing
behavior for other minimum versions.
🪄 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: 0729c3de-2de9-4c16-8c30-ff1b5044daa6
📒 Files selected for processing (1)
controllers/consoleplugin_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
| { | ||
| name: "TLS 1.2 with cipher suites", | ||
| minVersion: "VersionTLS12", | ||
| ciphers: []string{"ECDHE-RSA-AES256-GCM-SHA384", "ECDHE-RSA-AES128-GCM-SHA256"}, | ||
| wantProtocol: true, | ||
| wantCipherSuite: true, | ||
| protocol: "TLSv1.2", | ||
| cipherSuite: "ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline controllers/consoleplugin.go --match TLSVersionToPlugin --view expanded
rg -n -C4 'SSLProtocol|TLSVersionToPlugin' controllers/consoleplugin.go controllers/consoleplugin_test.go
curl -fsSL https://httpd.apache.org/docs/2.4/mod/mod_ssl.html |
grep -A40 -B5 'id="sslprotocol"' | head -n 80Repository: redhat-developer/gitops-operator
Length of output: 6411
Treat VersionTLS12 as a floor, not a fixed protocol.
SSLProtocol TLSv1.2 only enables TLS 1.2 here, so TLSMinVersion still behaves like an exact version and can exclude TLS 1.3 clients. Use an additive range such as SSLProtocol -all +TLSv1.2 +TLSv1.3.
🤖 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/consoleplugin_test.go` around lines 1931 - 1938, Update the TLS
configuration logic exercised by the “TLS 1.2 with cipher suites” test so
TLSMinVersion VersionTLS12 emits an additive protocol range including TLSv1.2
and TLSv1.3, rather than selecting TLSv1.2 exclusively. Preserve the
cipher-suite configuration and existing behavior for other minimum versions.
| func TestGetConfigMapHash(t *testing.T) { | ||
| t.Run("same configmap returns same hash", func(t *testing.T) { | ||
| cm1 := &corev1.ConfigMap{ | ||
| Data: map[string]string{ | ||
| "config": "value", | ||
| "foo": "bar", | ||
| }, | ||
| } | ||
|
|
||
| cm2 := &corev1.ConfigMap{ | ||
| Data: map[string]string{ | ||
| "config": "value", | ||
| "foo": "bar", | ||
| }, | ||
| } | ||
|
|
||
| hash1 := getConfigMapHash(cm1) | ||
| hash2 := getConfigMapHash(cm2) | ||
|
|
||
| assert.Equal(t, hash1, hash2) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline controllers/consoleplugin.go --match getConfigMapHash --view expanded
rg -n -A15 -B3 '^func getConfigMapHash' controllers/consoleplugin.goRepository: redhat-developer/gitops-operator
Length of output: 1053
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat > /tmp/hash_order_check.go <<'EOF'
package main
import (
"crypto/md5"
"fmt"
"io"
"sort"
)
func hashUnsorted(m map[string]string) string {
h := md5.New()
for k, v := range m {
io.WriteString(h, k)
io.WriteString(h, v)
}
return fmt.Sprintf("%x", h.Sum(nil))
}
func hashSorted(m map[string]string) string {
h := md5.New()
keys := make([]string, 0, len(m))
for k := range m {
keys = append(keys, k)
}
sort.Strings(keys)
for _, k := range keys {
io.WriteString(h, k)
io.WriteString(h, m[k])
}
return fmt.Sprintf("%x", h.Sum(nil))
}
func main() {
m := map[string]string{"config": "value", "foo": "bar"}
seen := map[string]int{}
for i := 0; i < 1000; i++ {
seen[hashUnsorted(m)]++
}
fmt.Println("unsorted distinct hashes:", len(seen))
for h, n := range seen {
fmt.Printf("%s %d\n", h, n)
}
fmt.Println("sorted hash:", hashSorted(m))
}
EOF
go run /tmp/hash_order_check.goRepository: redhat-developer/gitops-operator
Length of output: 318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A5 -B5 'getConfigMapHash\(' controllers/consoleplugin.goRepository: redhat-developer/gitops-operator
Length of output: 1032
Make ConfigMap hashing deterministic.
Iterating over cm.Data directly can produce different hashes for identical ConfigMaps, which will churn the pod-template annotation and trigger unnecessary rollouts. Sort the keys before writing them into the hash.
🤖 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/consoleplugin_test.go` around lines 2000 - 2020, Update
getConfigMapHash to sort the keys of cm.Data before writing key-value pairs into
the hash, ensuring identical ConfigMaps always produce the same hash and
avoiding unnecessary rollouts.
|
@akhilnittala: The following test 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:
Configure SSL Protocol and cipher suites based on apiserver tls minversion and ciphersuites
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
https://redhat.atlassian.net/browse/GITOPS-10473
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
deploy gitops operator and verify the ssl connection with apiserver tls minversion and ciphers