NE-2743: Support centralized TLS security profiles for CDO metrics#482
NE-2743: Support centralized TLS security profiles for CDO metrics#482Thealisyed wants to merge 6 commits into
Conversation
|
@Thealisyed: This pull request references NE-2743 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughThe PR updates Go and OpenShift/Kubernetes dependencies, tightens kube-rbac-proxy TLS settings, and aligns DNSNameResolver CRD metadata, toleration documentation, and generated-manifest verification. ChangesDNS operator updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/operator/controller/controller.go (1)
111-118: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWatch reconciles on any change to
APIServer/cluster, not just TLS profile changes.The predicate only filters by object name, so any unrelated field change on the cluster
APIServer(audit policy, encryption config, etc.) triggers a DNS reconcile. Consider comparingTLSSecurityProfilebetweenUpdateEvent.ObjectOld/ObjectNewto avoid unnecessary reconciliation.🤖 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 `@pkg/operator/controller/controller.go` around lines 111 - 118, The current APIServer watch in controller.Setup only filters by name, so any update to configv1.APIServer/cluster triggers reconciliation. Update the predicate around apiServerPredicate to inspect UpdateEvent and only return true when the TLSSecurityProfile actually changes between the old and new APIServer objects, while still allowing the initial cluster object match for create events. Keep the existing watch on source.Kind with objectToDNS, but tighten the predicate logic so unrelated APIServer edits do not enqueue DNS reconciles.
🤖 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 `@pkg/operator/controller/controller_dns_daemonset.go`:
- Around line 160-177: kubeRBACProxyArgs currently falls back for empty Ciphers
but passes MinTLSVersion through unchanged, which can produce an invalid
--tls-min-version flag if the profile is malformed. Update kubeRBACProxyArgs to
mirror the cipher fallback by defaulting tlsProfile.MinTLSVersion to the
Intermediate TLS profile’s MinTLSVersion when it is empty, then build the args
from that resolved value. Keep the change localized to kubeRBACProxyArgs and use
the existing configv1.TLSProfiles lookup for the fallback.
In `@pkg/operator/controller/tls_test.go`:
- Around line 62-68: The “Custom profile with nil Custom field defaults to
empty” test case in TestTLSProfileSpecForSecurityProfile only checks ciphers and
misses MinTLSVersion, so it won’t catch the empty-spec regression. Update this
case to set an explicit expectMinVer matching the expected default for the
Custom profile branch (the Intermediate MinTLSVersion after the fix), and ensure
the existing assertion logic in TestTLSProfileSpecForSecurityProfile validates
MinTLSVersion for this profile path.
In `@pkg/operator/controller/tls.go`:
- Around line 20-32: TLSProfileSpecForSecurityProfile currently returns an empty
TLSProfileSpec when profile.Type is TLSProfileCustomType and profile.Custom is
nil, which leads to invalid kube-rbac-proxy args. Update
TLSProfileSpecForSecurityProfile in tls.go so malformed Custom profiles fall
back to the Intermediate entry from configv1.TLSProfiles instead of returning an
empty spec, while keeping the existing behavior for valid Custom and known
built-in types.
---
Nitpick comments:
In `@pkg/operator/controller/controller.go`:
- Around line 111-118: The current APIServer watch in controller.Setup only
filters by name, so any update to configv1.APIServer/cluster triggers
reconciliation. Update the predicate around apiServerPredicate to inspect
UpdateEvent and only return true when the TLSSecurityProfile actually changes
between the old and new APIServer objects, while still allowing the initial
cluster object match for create events. Keep the existing watch on source.Kind
with objectToDNS, but tighten the predicate logic so unrelated APIServer edits
do not enqueue DNS reconciles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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 `@go.mod`:
- Around line 57-58: Update the go.opentelemetry.io/otel and
go.opentelemetry.io/otel/trace dependencies in go.mod from v1.36.0 to v1.41.0 or
newer, then refresh go.sum as needed and verify the module dependency graph
remains consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| go.opentelemetry.io/otel v1.36.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.36.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## .agents/context.md\n'
cat -n .agents/context.md
printf '\n## go.mod (relevant lines)\n'
sed -n '45,70p' go.mod | cat -n
printf '\n## otel usage in the repo\n'
rg -n --hidden --glob '!**/vendor/**' 'go\.opentelemetry\.io/otel|go\.opentelemetry\.io/otel/trace' .
printf '\n## go.sum entries for otel\n'
rg -n '^go\.opentelemetry\.io/otel|^go\.opentelemetry\.io/otel/trace' go.sumRepository: openshift/cluster-dns-operator
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## repo root\n'
pwd
printf '\n## top-level files\n'
git ls-files | sed -n '1,200p'
printf '\n## locate .agents and go.mod\n'
fd -a '^\.agents$|^go\.mod$|^go\.sum$' .
printf '\n## go.mod relevant lines\n'
sed -n '1,140p' go.mod | cat -n
printf '\n## go.sum otel entries\n'
rg -n '^go\.opentelemetry\.io/otel|^go\.opentelemetry\.io/otel/trace' go.sum || true
printf '\n## repo references to otel\n'
rg -n --hidden --glob '!**/vendor/**' 'go\.opentelemetry\.io/otel|go\.opentelemetry\.io/otel/trace' . || trueRepository: openshift/cluster-dns-operator
Length of output: 15447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## .agents/context.md\n'
sed -n '1,220p' .agents/context.md | cat -n
printf '\n## any otel/opentelemetry references outside go.mod/go.sum\n'
rg -n --hidden --glob '!**/vendor/**' 'opentelemetry|go\.opentelemetry\.io/otel' . || true
printf '\n## any otel/opentelemetry references in vendor\n'
rg -n --hidden 'opentelemetry|go\.opentelemetry\.io/otel' vendor || trueRepository: openshift/cluster-dns-operator
Length of output: 252
🌐 Web query:
OSV CVE-2026-29181 go.opentelemetry.io/otel v1.36.0 fixed version 1.41.0
💡 Result:
CVE-2026-29181 is a security vulnerability in the OpenTelemetry-Go library (go.opentelemetry.io/otel) related to improper control of resource allocation [1][2][3]. Specifically, the vulnerability exists in the handling of multi-value 'baggage' headers [2][3][4]. Versions 1.36.0 through 1.40.0 are affected because the library parses each header field-value independently and aggregates members across these values, allowing an attacker to trigger excessive CPU and memory allocations even when individual values remain within the 8192-byte parsing limit [3][4][5]. This issue was addressed and is fixed in version 1.41.0 of the OpenTelemetry-Go library [3][4][6]. Users are advised to upgrade to version 1.41.0 or higher to mitigate this risk [5][6].
Citations:
- 1: https://nvd.nist.gov/vuln/detail/CVE-2026-29181
- 2: GHSA-mh2q-q3fh-2475
- 3: https://nvd.nist.gov/vuln/detail/cve-2026-29181
- 4: https://app.opencve.io/cve/CVE-2026-29181
- 5: https://security.snyk.io/vuln/SNYK-GOLANG-GOOPENTELEMETRYIOOTELPROPAGATION-15928420
- 6: https://www.sentinelone.com/vulnerability-database/cve-2026-29181/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## hidden files at repo root\n'
ls -la
printf '\n## .agents files if present\n'
fd -a 'context.md|architecture.md|controllers.md|rules.md|testing.md|operand.md' .agents || true
printf '\n## otel references outside go.mod/go.sum\n'
rg -n --hidden --glob '!go.mod' --glob '!go.sum' --glob '!**/vendor/**' 'go\.opentelemetry\.io/otel|go\.opentelemetry\.io/otel/trace|opentelemetry' . || true
printf '\n## otel files in vendor\n'
fd -a 'otel|opentelemetry' vendor || trueRepository: openshift/cluster-dns-operator
Length of output: 1861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## repo source imports\n'
rg -n --glob '!vendor/**' '^import \(|"sigs\.k8s\.io/controller-runtime|"go\.opentelemetry\.io/otel' cmd pkg test tools | sed -n '1,220p'
printf '\n## vendor packages that import otel\n'
rg -n 'go\.opentelemetry\.io/otel' vendor | sed -n '1,220p'
printf '\n## controller-runtime dependency chain hints\n'
rg -n 'controller-runtime' go.mod go.sum vendor | sed -n '1,220p'Repository: openshift/cluster-dns-operator
Length of output: 40847
Upgrade the OpenTelemetry pins
go.mod:57-58 still pin go.opentelemetry.io/otel and go.opentelemetry.io/otel/trace to v1.36.0, which is affected by GHSA-mh2q-q3fh-2475 / CVE-2026-29181. Bump both to v1.41.0 or newer.
Suggested diff
- go.opentelemetry.io/otel v1.36.0 // indirect
- go.opentelemetry.io/otel/trace v1.36.0 // indirect
+ go.opentelemetry.io/otel v1.41.0 // indirect
+ go.opentelemetry.io/otel/trace v1.41.0 // indirect📝 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.
| go.opentelemetry.io/otel v1.36.0 // indirect | |
| go.opentelemetry.io/otel/trace v1.36.0 // indirect | |
| go.opentelemetry.io/otel v1.41.0 // indirect | |
| go.opentelemetry.io/otel/trace v1.41.0 // indirect |
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 57-57: go.opentelemetry.io/otel 1.36.0: OpenTelemetry-Go: multi-value baggage header extraction causes excessive allocations (remote dos amplification)
🤖 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 `@go.mod` around lines 57 - 58, Update the go.opentelemetry.io/otel and
go.opentelemetry.io/otel/trace dependencies in go.mod from v1.36.0 to v1.41.0 or
newer, then refresh go.sum as needed and verify the module dependency graph
remains consistent.
Sources: Path instructions, Linters/SAST tools
Vendor openshift/api to v0.0.0-20260615110019 to pick up TLSProfileSpec.Groups (TLSGroupPreferences) and bump k8s.io dependencies from v0.34.1 to v0.35.1, Go from 1.24 to 1.25. Assisted with Claude
…LS profile Update the CVO-managed dns-operator Deployment manifest's kube-rbac-proxy sidecar to use the full Intermediate TLS security profile cipher list (9 IANA ciphers) and add --tls-min-version=VersionTLS12. The operator's own deployment is CVO-managed and not reconciled by the operator itself, so these static args are what the operator pod uses. The CoreDNS DaemonSet's kube-rbac-proxy args are set dynamically at runtime by the operator based on the cluster's APIServer TLS profile (merged upstream in PR openshift#466). Assisted with Claude
The vendored openshift/api merged the three feature-gated DNSNameResolver CRDs into a single CRD with merged feature-set annotations. Update the checked-in manifests and verify script to match. Assisted with Claude
|
@coderabbitai can you review my PR please? |
|
✅ Action performedReview finished.
|
|
where is your feedback? @coderabbitai |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 482 --repo openshift/cluster-dns-operator --json reviews,comments --jq '.reviews[] | {author: .author.login, state: .state, body: .body, submittedAt: .submittedAt}'
echo "---REVIEW COMMENTS---"
gh api repos/openshift/cluster-dns-operator/pulls/482/comments --jq '.[] | {user: .user.login, path: .path, line: .line, body: .body}'
echo "---ISSUE COMMENTS---"
gh api repos/openshift/cluster-dns-operator/issues/482/comments --jq '.[] | {user: .user.login, body: .body[0:200]}'Length of output: 30678
Let me know if you'd like me to draft the fixes for any of these, and I'm happy to re-review once you've pushed changes. ✅ Action performedReview finished.
|
Does this make sense to you @coderabbitai (in response to your latest review commet) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd . -e go | xargs rg -l "kubeRBACProxyArgs|tlsProfileSpecForProfile" || true
echo "---"
rg -n "tlsProfileSpecForProfile|kubeRBACProxyArgs|MinTLSVersion|TLSProfileSpecForSecurityProfile" pkg/operator/controller/controller_dns_daemonset.go
echo "---FILE EXIST CHECK---"
fd tls.go pkg/operator/controller || echo "no tls.go"
echo "---otel usage---"
rg -n "go.opentelemetry.io/otel" go.mod go.sum | head -30
rg -n "baggage" --type=go -g '!vendor/**' || true
rg -rn "baggage" vendor/go.opentelemetry.io 2>/dev/null | head -5 || true
echo "---controller.go watch predicate---"
rg -n "APIServer" pkg/operator/controller/controller.go -A5 -B5Length of output: 2428 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "APIServer" pkg/operator/controller/controller.go -A8 -B8
echo "---predicate---"
rg -n "predicate\.|GenerationChangedPredicate|apiServerPredicate|func.*[Pp]redicate" pkg/operator/controller/controller.go -A10
echo "---tls fallback func full---"
sed -n '160,230p' pkg/operator/controller/controller_dns_daemonset.go
echo "---baggage indirect chain---"
rg -n "go.opentelemetry.io/otel" go.mod
grep -n "baggage" go.sum || echo "no baggage in go.sum"Length of output: 8729
Thanks for pushing back with the specifics — I've corrected my understanding. Nothing further blocking from my side on these four items. ✏️ Learnings added
|
|
@Thealisyed please do your initial reviews from the command line instead of within github. Also, this PR is incomplete, so mark it WIP or draft. |
… metrics Drop the kube-rbac-proxy sidecar from the operator deployment and serve metrics directly via controller-runtime with in-process authentication and TLS from the cluster security profile. The CoreDNS daemonset operand retains kube-rbac-proxy with a TODO for --tls-curve-preferences when upstream supports it. Assisted-By: Claude Code <noreply@anthropic.com>
| if len(spec.Groups) > 0 { | ||
| var curves []tls.CurveID | ||
| for _, g := range spec.Groups { | ||
| if id, ok := TLSGroupToCurveID(g); ok { |
There was a problem hiding this comment.
The apiserver is the authority here, and we may need to accept curves that we haven't planned to see (such as in a custom profile), so this isn't going to work.
There was a problem hiding this comment.
Unfortunately Go doesn't give us a way to convert a curve name string to a tls.CurveID. There's an open proposal for tls.SupportedCurves() but it hasn't landed so IIUC every OpenShift operator doing this mapping hits the same problem.
That said, the common case already works well here: when the profile doesn't specify groups, CurvePreferences stays nil and Go 1.24+ automatically includes PQC curves. The map only comes into play for Custom profiles that explicitly list groups. I'll add the two missing entries and put in a log warning for any unknown group so we're not silently dropping anything.
|
Hi @Thealisyed so more of a top level comment here: Upstream Kubernetes (sig-auth) has deprecated the gcr.io/kubebuilder/kube-rbac-proxy image due to security concerns, and it's no longer being published. Across OpenShift, there is a major ongoing migration (tracked under OCPSTRAT-1835) to completely remove kube-rbac-proxy sidecars from all components. Instead of investing effort into dynamically configuring the deprecated proxy, we should lift out kube-rbac-proxy entirely. Since cluster-dns-operator is a controller-runtime based operator, we can replace the sidecar with native supporting code from controller-runtime. Starting in v0.16.0 (and we are on v0.23.1), controller-runtime provides It will be a bigger change but I think we should pivot this PR (or open a new one) to implement this migration for the operator. Implementation StepsTo adopt this pattern for the operator's metrics:
import "sigs.k8s.io/controller-runtime/pkg/metrics/filters"
// ...
metricsOpts := metricsserver.Options{
BindAddress: ":8443", // Or your preferred port
SecureServing: true,
FilterProvider: filters.WithAuthenticationAndAuthorization,
}
ExamplesSeveral operators have already completed this migration. Here are a couple of excellent references you can use as a blueprint:
|
There was a problem hiding this comment.
Please provide an update on why the generator script was changed.
There was a problem hiding this comment.
The openshift/api v0.35.1 vendor bump consolidated the DNSNameResolver CRD, it went from three separate per-feature-gate manifests (TPNoUpgrade, CustomNoUpgrade, DevPreviewNoUpgrade) down to a single ungated CRD. So the script change just follows suit, we verify one file instead of three now.
| // KubeRBACProxyImage is the kube-rbac-proxy image used by the | ||
| // CoreDNS DaemonSet operand to secure its metrics endpoint. | ||
| KubeRBACProxyImage string |
There was a problem hiding this comment.
The design goal is to replace the kube-rbac-proxy. Why is this still here?
There was a problem hiding this comment.
This PR removes kube-rbac-proxy from the operator's own deployment (port 9393) but KubeRBACProxyImage is still referenced because the CoreDNS DaemonSet (port 9154) still uses it.
Removing it from the operand is a separate step that's blocked on the CoreDNS rebase to v1.14.4 (PR #195), which brings the exporter-toolkit TLS support we need.
| // TODO(NE-2743): Pass --tls-curve-preferences once kube-rbac-proxy | ||
| // supports it (kube-rbac-proxy#414), using profileSpec.Groups. |
There was a problem hiding this comment.
Same story as the image reference, kubeRBACProxyArgs() and that TODO are both for the CoreDNS DaemonSet's kube-rbac-proxy, which we're intentionally keeping in this PR.
The whole function goes away in the follow-up.
…kipping Groups that cannot be mapped to a Go tls.CurveID (e.g. SecP256r1MLKEM768 and SecP384r1MLKEM1024 which require Go 1.26+) now emit a warning log instead of being silently dropped. Adds a test covering the mixed supported/unsupported groups case. Assisted-by: Claude <noreply@anthropic.com>
|
/assign |
|
@Thealisyed I think you can remove WIP label and remove Draft status now, no? |
davidesalerno
left a comment
There was a problem hiding this comment.
I shared some minor feedback and suggestion, let me know what you think.
- Add TODO to replace tlsGroupToCurveID with library-go CurveIDsForTLSGroups once openshift/library-go#2347 merges - Add warning log for unsupported cipher suite names - Document that TLS profile is read once at startup - Remove unused copyTLSSpec function and its tests Assisted with Claude
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidesalerno The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| // Build TLS options for the operator metrics server from the | ||
| // cluster-wide TLS security profile when secure serving is enabled. | ||
| // NOTE: the TLS profile is read once at startup; runtime changes to |
There was a problem hiding this comment.
While this static loading behavior is historically common in controller-runtime operators, it does introduce a subtle asymmetry in our design:
- When the cluster TLS profile changes, the operator's reconciler dynamically updates the CoreDNS DaemonSet (operand), immediately rolling it out to match the new profile.
- The operator itself, however, remains non-compliant on port
9393until a manual restart or node reschedule occurs.
Across the OpenShift ecosystem, we have a standardized pattern to solve this exact dynamic update limitation. Instead of attempting to hot-reload TLS parameters (which can lead to inconsistent states on long-lived connections), operators use a SecurityProfileWatcher from the openshift/controller-runtime-common library to trigger a graceful self-shutdown when the TLS profile changes. Because the operator's Deployment has restartPolicy: Always, Kubernetes instantly spawns a new pod, which re-reads the fresh TLS config on boot.
You can see an example of this in the ptp-operator main.go.
This is definitely a separate, well-defined enhancement that doesn't need to block this PR. Let's merge this work as-is to unblock the kube-rbac-proxy removal, and we can track implementing the SecurityProfileWatcher in a follow-up Jira story. What do you think?
I think we should add a Jira to track this and do this as a follow-up PR.
|
I had one comment (https://github.com/openshift/cluster-dns-operator/pull/482/changes#r3615162144) about TLS config changing that we can work on in a follow up story/PR. I think you've addressed all the comments and the PR looks solid! /lgtm |
|
/retest |
|
@Thealisyed: all tests passed! 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. |
Summary
Removes kube-rbac-proxy from the operator deployment and replaces it with controller-runtime native TLS serving via
filters.WithAuthenticationAndAuthorization. This is Step 1 of the two-step kube-rbac-proxy removal agreed by the team.openshift/apito pick upTLSProfileSpec.Groups(TLSGroupPreferences) and bumpk8s.iodependencies to v0.35.1TLSConfigFromProfile/TLSGroupToCurveIDhelpers with warning logs for unsupported groups and ciphersWhat's deliberately kept
KubeRBACProxyImage,kubeRBACProxyArgs(), and theKUBE_RBAC_PROXY_IMAGEenv var remain because the CoreDNS DaemonSet still uses kube-rbac-proxy in this release.Follow-up
NE-2814 — Step 2: Remove kube-rbac-proxy from the CoreDNS operand using exporter-toolkit mTLS (target: 5.1). Blocked on upstream prometheus/exporter-toolkit#417 and openshift/coredns#195.
Jira
Summary by CodeRabbit
Security
Maintenance
Documentation
Lt,Gt), feature-gated.