Skip to content

NE-2743: Support centralized TLS security profiles for CDO metrics#482

Open
Thealisyed wants to merge 6 commits into
openshift:masterfrom
Thealisyed:bump-to-4.22
Open

NE-2743: Support centralized TLS security profiles for CDO metrics#482
Thealisyed wants to merge 6 commits into
openshift:masterfrom
Thealisyed:bump-to-4.22

Conversation

@Thealisyed

@Thealisyed Thealisyed commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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.

  • Vendor openshift/api to pick up TLSProfileSpec.Groups (TLSGroupPreferences) and bump k8s.io dependencies to v0.35.1
  • Remove kube-rbac-proxy sidecar from the operator deployment (port 9393), serve metrics directly with TLS + auth
  • Read cluster-wide APIServer TLS security profile at startup for cipher suites, min TLS version, and curve preferences
  • Add TLSConfigFromProfile / TLSGroupToCurveID helpers with warning logs for unsupported groups and ciphers
  • Consolidate DNSNameResolver CRD from 3 feature-gated variants to 1 ungated CRD

What's deliberately kept

KubeRBACProxyImage, kubeRBACProxyArgs(), and the KUBE_RBAC_PROXY_IMAGE env 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

    • Updated the DNS operator proxy to use modern TLS cipher suites.
    • Enforced a minimum TLS version of 1.2.
  • Maintenance

    • Updated Go toolchain version.
    • Updated Kubernetes/OpenShift dependencies to newer patch releases.
    • Updated DNSNameResolver CRD handling: removed the TechPreviewNoUpgrade API variant manifest and broadened the CRD feature-set annotation to include DevPreviewNoUpgrade and TechPreviewNoUpgrade.
  • Documentation

    • Expanded CRD documentation for toleration operators to include numeric comparisons (Lt, Gt), feature-gated.

@openshift-ci-robot

openshift-ci-robot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@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.

Details

In response to this:

Summary

  • Vendor openshift/api to pick up TLSProfileSpec.Groups (TLSGroupPreferences) and bump k8s.io dependencies to v0.35.1
  • CoreDNS DaemonSet's kube-rbac-proxy sidecar now derives TLS cipher suites and minimum TLS version dynamically from the cluster-wide APIServer TLS security profile instead of hardcoded values
  • Operator watches the APIServer "cluster" resource and reconciles when the TLS policy changes
  • TLS curves/groups are not explicitly passed to kube-rbac-proxy (no --tls-curves flag); Go 1.25 crypto/tls defaults already include X25519MLKEM768 (ML-KEM post-quantum) when CurvePreferences is unset

Jira

https://redhat.atlassian.net/browse/NE-2743

Test plan

  • Unit tests pass (go test ./pkg/operator/controller/)
  • Verify kube-rbac-proxy args are dynamically set from APIServer TLS profile
  • Verify reconciliation triggers on APIServer TLS profile change
  • Verify fallback to Intermediate profile when APIServer config is absent
  • e2e validation on cluster with DEFAULT:PQ crypto policy

Assisted with Claude

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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 9, 2026
@openshift-ci openshift-ci Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 9, 2026
@openshift-ci
openshift-ci Bot requested review from davidesalerno and rikatz July 9, 2026 17:20
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR updates Go and OpenShift/Kubernetes dependencies, tightens kube-rbac-proxy TLS settings, and aligns DNSNameResolver CRD metadata, toleration documentation, and generated-manifest verification.

Changes

DNS operator updates

Layer / File(s) Summary
Dependency refresh
go.mod
The Go toolchain is set to 1.25.0, OpenShift and Kubernetes modules are updated, and github.com/gogo/protobuf is removed from indirect requirements.
Deployment TLS configuration
manifests/0000_70_dns-operator_02-deployment.yaml, manifests/0000_70_dns-operator_02-deployment-ibm-cloud-managed.yaml
The kube-rbac-proxy cipher suite lists are replaced with updated TLS 1.3 and TLS 1.2 suites; the standard deployment also sets VersionTLS12 as the minimum version.
CRD metadata and verification alignment
manifests/0000_70_dns-operator_00.crd.yaml, manifests/0000_70_dns_00_dnsnameresolvers.crd.yaml, hack/verify-generated-crd.sh
Toleration documentation includes Lt and Gt, DNSNameResolver feature-set metadata includes all three listed variants, and CRD verification targets the consolidated manifest.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo test files or title calls changed; the PR only edits CRDs and a verification shell script, so there are no dynamic test names to flag.
Test Structure And Quality ✅ Passed No Ginkgo test files were changed; the PR only modifies manifests and a verification shell script, so these test-quality checks are not applicable.
Microshift Test Compatibility ✅ Passed PR only changes manifests and a CRD verification script; no new Ginkgo tests or MicroShift-unsafe test code were added.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests were added or modified; the PR only changes manifests, go.mod, and a CRD verification script.
Topology-Aware Scheduling Compatibility ✅ Passed Actual diff only changes CRD docs and verification; no Deployment/controller scheduling constraints were added or modified.
Ote Binary Stdout Contract ✅ Passed Diff scan found no added fmt.Print/log.SetOutput/klog stdout use or process-level entrypoints in the changed files.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No Ginkgo/e2e test files were added; this commit only changes manifests and a verification script, so the IPv4/disconnected-network test check does not apply.
No-Weak-Crypto ✅ Passed The commit diff only changes CRD manifests and a verification script; no MD5/SHA1/DES/RC4/3DES/ECB or custom secret-comparison code appears in the patch.
Container-Privileges ✅ Passed The modified manifests set allowPrivilegeEscalation:false, drop ALL capabilities, run as non-root, and do not enable hostPID/hostNetwork/hostIPC or privileged.
No-Sensitive-Data-In-Logs ✅ Passed The PR only updates CRD manifests and a verification script; no logging code or sensitive values (secrets, tokens, PII, hostnames) were added.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: using centralized TLS security profiles for metrics-related TLS settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
pkg/operator/controller/controller.go (1)

111-118: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Watch 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 comparing TLSSecurityProfile between UpdateEvent.ObjectOld/ObjectNew to 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

Comment thread pkg/operator/controller/controller_dns_daemonset.go Outdated
Comment thread pkg/operator/controller/tls_test.go Outdated
Comment thread pkg/operator/controller/tls.go Outdated
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
Validation Failed: {"resource":"IssueComment","code":"custom","field":"body","message":"body is too long (maximum is 65536 characters)"} - https://docs.github.com/rest/issues/comments#update-an-issue-comment

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread go.mod
Comment on lines +57 to +58
go.opentelemetry.io/otel v1.36.0 // indirect
go.opentelemetry.io/otel/trace v1.36.0 // indirect

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.sum

Repository: 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' . || true

Repository: 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 || true

Repository: 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:


🏁 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 || true

Repository: 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.

Suggested change
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)

(GHSA-mh2q-q3fh-2475)

🤖 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
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2026
…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
@Thealisyed Thealisyed changed the title [WIP] NE-2743: Support centralized TLS security profiles for CDO metrics NE-2743: Support centralized TLS security profiles for CDO metrics Jul 10, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2026
@Thealisyed

Copy link
Copy Markdown
Contributor Author

@coderabbitai can you review my PR please?

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

@Thealisyed Sure, I'll review the PR now and share my feedback shortly.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Thealisyed

Copy link
Copy Markdown
Contributor Author

where is your feedback? @coderabbitai

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
🧩 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


@Thealisyed, my feedback is there — it was posted as inline review comments on the "Files changed" tab rather than as a single top-level comment, which is likely why it wasn't obvious. Here's a summary of the actionable items I flagged:

  1. pkg/operator/controller/tls.go (Lines 20‑32)TLSProfileSpecForSecurityProfile returns an empty TLSProfileSpec when Type=Custom and Custom is nil. This should fall back to the Intermediate profile from configv1.TLSProfiles, otherwise downstream code emits an invalid --tls-min-version= flag.

  2. pkg/operator/controller/controller_dns_daemonset.go (Lines 160‑177)kubeRBACProxyArgs has a fallback for empty Ciphers but not for MinTLSVersion. As defense-in-depth, add the same fallback there so this function is robust even if the caller passes a malformed profile.

  3. pkg/operator/controller/tls_test.go (Lines 62‑68) — The "Custom profile with nil Custom field defaults to empty" test case doesn't assert MinTLSVersion, so it masks the bug above. Add an explicit expectMinVer assertion.

  4. go.mod (Line 58)go.opentelemetry.io/otel and go.opentelemetry.io/otel/trace are still pinned to v1.36.0, which is affected by GHSA-mh2q-q3fh-2475 (CVE-2026-29181, a DoS via multi-value baggage header parsing). Please bump both to v1.41.0 or newer.

  5. Nitpick — pkg/operator/controller/controller.go (Lines 111‑118) — The APIServer watch predicate only filters by object name, so any unrelated field change on APIServer/cluster triggers a DNS reconcile. Consider comparing TLSSecurityProfile between old/new objects to avoid unnecessary reconciliation.

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 performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Thealisyed

Copy link
Copy Markdown
Contributor Author
  1. tls.go doesn't exist: The function lives in controller_dns_daemonset.go and already falls back to Intermediate when Custom is nil.
  2. MinTLSVersion fallback: Not needed because tlsProfileSpecForProfile already guarantees a valid spec with MinTLSVersion populated before kubeRBACProxyArgs ever runs.
  3. CVE-2026-29181: valid CVE but it's an indirect dep as we don't use baggage propagation and out of scope for this PR.
  4. APIServer watch predicate: This is standard OCP operator practice as reconciliation is a no-op when the DaemonSet hasn't changed, so filtering adds complexity for no real gain.

Does this make sense to you @coderabbitai (in response to your latest review commet)

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

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 -B5

Length 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


@Thealisyed Yes, that all makes sense — I verified each point against the current code and my original comments don't hold up:

  1. Confirmed — there's no tls.go; tlsProfileSpecForProfile and kubeRBACProxyArgs both live in controller_dns_daemonset.go, and the Custom/nil case already falls back to Intermediate.
  2. Confirmed — tlsProfileSpecForProfile always returns a non-nil spec (Intermediate default for nil profile, unset type, or Custom with nil .Custom), so MinTLSVersion is guaranteed to be populated by the time kubeRBACProxyArgs runs. No fallback needed there.
  3. Fair — go.opentelemetry.io/otel is an indirect dependency here, baggage isn't referenced anywhere in go.sum/vendor, and bumping unrelated transitive deps is reasonable to keep out of scope for this PR.
  4. Fair — filtering the APIServer watch predicate would add complexity for marginal benefit given reconciliation is a no-op when the DaemonSet spec is unchanged; this matches common practice elsewhere in this operator (e.g., the ConfigMap/Node watches use predicates only where the event volume actually matters).

Thanks for pushing back with the specifics — I've corrected my understanding. Nothing further blocking from my side on these four items.


✏️ Learnings added
Learnt from: Thealisyed
URL: https://github.com/openshift/cluster-dns-operator/pull/482

Timestamp: 2026-07-13T14:29:40.713Z
Learning: In openshift/cluster-dns-operator, `pkg/operator/controller/controller_dns_daemonset.go` contains `tlsProfileSpecForProfile(profile *configv1.TLSSecurityProfile) *configv1.TLSProfileSpec`, which always returns a non-nil TLSProfileSpec: it falls back to `configv1.TLSProfiles[configv1.TLSProfileIntermediateType]` when the profile is nil, when the type is unset/unrecognized, and when Type is Custom but `profile.Custom` is nil. Consumers like `kubeRBACProxyArgs` in the same file can therefore rely on `MinTLSVersion` always being populated and do not need an additional fallback for `MinTLSVersion`. There is no `tls.go` file in this package; TLS profile resolution logic lives in `controller_dns_daemonset.go`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@candita

candita commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@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>
@Thealisyed Thealisyed changed the title NE-2743: Support centralized TLS security profiles for CDO metrics [WIP] NE-2743: Support centralized TLS security profiles for CDO metrics Jul 14, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2026
if len(spec.Groups) > 0 {
var curves []tls.CurveID
for _, g := range spec.Groups {
if id, ok := TLSGroupToCurveID(g); ok {

@candita candita Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Thealisyed Thealisyed Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bentito

bentito commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Hi @Thealisyed so more of a top level comment here:
The approach here of plumbing the TLS profiles through to the kube-rbac-proxy sidecar is a lot of work for a component that is actually being phased out.

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 filters.WithAuthenticationAndAuthorization, which natively embeds authn/authz into the operator's metrics server using TokenReview and SubjectAccessReview.

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 Steps

To adopt this pattern for the operator's metrics:

  • Update main.go: Configure the metrics server to use the filter.
import "sigs.k8s.io/controller-runtime/pkg/metrics/filters"
// ...
metricsOpts := metricsserver.Options{
  BindAddress:   ":8443", // Or your preferred port
  SecureServing: true,
  FilterProvider: filters.WithAuthenticationAndAuthorization,
}
  • Remove the Sidecar: Drop the kube-rbac-proxy container from the dns-operator Deployment manifest (manifests/0000_70_dns-operator_02-deployment.yaml).
  • Update RBAC: Ensure the metrics endpoint has the proper RBAC so the OpenShift Prometheus service account can scrape it via TokenReview/SubjectAccessReview.

Examples

Several operators have already completed this migration. Here are a couple of excellent references you can use as a blueprint:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide an update on why the generator script was changed.

@Thealisyed Thealisyed Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +20 to 22
// KubeRBACProxyImage is the kube-rbac-proxy image used by the
// CoreDNS DaemonSet operand to secure its metrics endpoint.
KubeRBACProxyImage string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design goal is to replace the kube-rbac-proxy. Why is this still here?

@Thealisyed Thealisyed Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +199 to +200
// TODO(NE-2743): Pass --tls-curve-preferences once kube-rbac-proxy
// supports it (kube-rbac-proxy#414), using profileSpec.Groups.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this still here?

@Thealisyed Thealisyed Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@bentito

bentito commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

/assign

@bentito

bentito commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@Thealisyed I think you can remove WIP label and remove Draft status now, no?

@Thealisyed Thealisyed changed the title [WIP] NE-2743: Support centralized TLS security profiles for CDO metrics NE-2743: Support centralized TLS security profiles for CDO metrics Jul 17, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2026

@davidesalerno davidesalerno left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shared some minor feedback and suggestion, let me know what you think.

Comment thread pkg/operator/controller/tls.go
Comment thread pkg/operator/controller/tls.go Outdated
Comment thread pkg/operator/controller/tls.go
Comment thread cmd/dns-operator/main.go
- 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
@davidesalerno

Copy link
Copy Markdown
Contributor

/approve

@openshift-ci

openshift-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2026
Comment thread cmd/dns-operator/main.go

// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this static loading behavior is historically common in controller-runtime operators, it does introduce a subtle asymmetry in our design:

  1. 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.
  2. The operator itself, however, remains non-compliant on port 9393 until 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.

@bentito

bentito commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2026
@bentito

bentito commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

/retest

@openshift-ci

openshift-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@Thealisyed: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants