feat: integrate with cluster TLS security profile - #812
Conversation
|
Hi @ugiordan. Thanks for your PR. I'm waiting for a trustyai-explainability member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR adds OpenShift APIServer-driven TLS resolution, wires the result into controller-manager metrics and webhook TLS settings, and adds the RBAC and policy updates needed to read the APIServer profile. ChangesTLS Profile Resolution Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/main.go (1)
150-154: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEvalHub webhook server discards the resolved
TLSOpts.When
serviceEvalHubis enabled,mgrOpts.WebhookServeris replaced with a new server that only setsPort: 9443and omitsTLSOpts. This silently drops the cluster TLS profile for the webhook server on EvalHub deployments, so the metrics server would honor the profile while the webhook server would not — an inconsistency that defeats the PR's goal for that configuration.🔒 Proposed fix to preserve TLSOpts
if slices.Contains(enabledServices, serviceEvalHub) { mgrOpts.WebhookServer = ctrlwebhook.NewServer(ctrlwebhook.Options{ - Port: 9443, + Port: 9443, + TLSOpts: tlsOpts, }) }Since the only apparent difference between the two
NewServercalls is this branch, consider dropping the duplicate construction entirely and keeping the single one at lines 143-146.🤖 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 `@cmd/main.go` around lines 150 - 154, The EvalHub branch in main.go recreates mgrOpts.WebhookServer with only Port set, which drops the previously resolved TLSOpts. Update the serviceEvalHub conditional so it preserves the existing TLS configuration from the earlier ctrlwebhook.NewServer setup, ideally by removing the duplicate server construction and reusing the single mgrOpts.WebhookServer initialization path.
🧹 Nitpick comments (3)
config/rbac-base/tls_profile_role.yaml (1)
6-13: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider scoping down permissions to least privilege.
Per the PR description, the operator performs a single
Geton theclustersingletonAPIServerobject at startup, not a watch/list. Grantinglist/watchon allapiserversobjects is broader than needed.🔒 Suggested tightening
rules: - apiGroups: - config.openshift.io resources: - apiservers + resourceNames: + - cluster verbs: - get - - list - - watchPlease confirm the exact API calls made against this resource in
pkg/tls/tls.go(aGet-only client vs. a cached/watched client) before applying this change, since informer-backed clients would still requirelist/watch.🤖 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 `@config/rbac-base/tls_profile_role.yaml` around lines 6 - 13, The RBAC rule for the APIServer resource is broader than the current usage and should be tightened only if pkg/tls/tls.go uses a direct Get call on the singleton cluster APIServer object. Verify whether the tls package uses a plain client Get or an informer/cached client, then update the tls_profile_role permissions accordingly: keep only get for a direct read, or retain list/watch only if the code वास्तव में relies on watching/caching that resource. Use the APIServer access in pkg/tls/tls.go and the apiservers rule in tls_profile_role.yaml to locate the change.pkg/tls/tls.go (1)
145-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModern/Custom TLS 1.3 cipher suites are silently ignored by Go.
For
TLSProfileModernTypeyou return nil ciphers (fine), but note that Go ignoresCipherSuitesentirely for TLS 1.3 connections. A Custom profile pinned toVersionTLS13with explicitCipherswill still have those ciphers accepted intoc.CipherSuitesyet have no runtime effect. This is acceptable behavior but worth a brief comment so future readers don't assume TLS 1.3 cipher selection is honored.🤖 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/tls/tls.go` around lines 145 - 148, Add a brief comment in the TLS profile/version mapping logic around TLSProfileModernType and any custom TLS 1.3 path in tls.go to note that Go ignores CipherSuites for TLS 1.3, so explicitly configured ciphers are accepted in the config but have no runtime effect. Keep the existing behavior in the version-returning code, and make the note near the relevant switch/translation logic so future readers do not assume TLS 1.3 cipher selection is honored.pkg/tls/tls_test.go (1)
26-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider aligning with the repo's Ginkgo/Gomega test convention.
These are pure unit tests over
parseProfile, so plaintestingworks fine, but the project convention is Ginkgo v2 + Gomega. AdoptingExpect(...)-style assertions here would keep the test suite consistent and reuse existing tooling. Not blocking given this is a dependency-free pure function.As per coding guidelines: "Use Ginkgo v2, Gomega, and controller-runtime envtest with K8s 1.29.0 binaries for unit tests".
🤖 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/tls/tls_test.go` around lines 26 - 173, The TestParseProfile function uses the standard Go testing package with table-driven tests, but the project convention is to use Ginkgo v2 and Gomega for unit tests. Convert this test to use Ginkgo v2 style with Describe and It blocks, and replace the manual error comparisons (t.Errorf, t.Fatal calls) with Gomega Expect assertions to align with the repository's testing standards and maintain consistency across the test suite.Source: Coding guidelines
🤖 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 121-123: The metrics server configuration in ctrl.Options is
mixing TLSOpts with plain HTTP, so either enable SecureServing for the metrics
endpoint and switch it to HTTPS, or remove TLSOpts entirely if metrics should
stay unencrypted. Update the metrics server setup in main by adjusting the
ctrl.Options/Metrics configuration so it matches the intended serving mode.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 150-154: The EvalHub branch in main.go recreates
mgrOpts.WebhookServer with only Port set, which drops the previously resolved
TLSOpts. Update the serviceEvalHub conditional so it preserves the existing TLS
configuration from the earlier ctrlwebhook.NewServer setup, ideally by removing
the duplicate server construction and reusing the single mgrOpts.WebhookServer
initialization path.
---
Nitpick comments:
In `@config/rbac-base/tls_profile_role.yaml`:
- Around line 6-13: The RBAC rule for the APIServer resource is broader than the
current usage and should be tightened only if pkg/tls/tls.go uses a direct Get
call on the singleton cluster APIServer object. Verify whether the tls package
uses a plain client Get or an informer/cached client, then update the
tls_profile_role permissions accordingly: keep only get for a direct read, or
retain list/watch only if the code वास्तव में relies on watching/caching that
resource. Use the APIServer access in pkg/tls/tls.go and the apiservers rule in
tls_profile_role.yaml to locate the change.
In `@pkg/tls/tls_test.go`:
- Around line 26-173: The TestParseProfile function uses the standard Go testing
package with table-driven tests, but the project convention is to use Ginkgo v2
and Gomega for unit tests. Convert this test to use Ginkgo v2 style with
Describe and It blocks, and replace the manual error comparisons (t.Errorf,
t.Fatal calls) with Gomega Expect assertions to align with the repository's
testing standards and maintain consistency across the test suite.
In `@pkg/tls/tls.go`:
- Around line 145-148: Add a brief comment in the TLS profile/version mapping
logic around TLSProfileModernType and any custom TLS 1.3 path in tls.go to note
that Go ignores CipherSuites for TLS 1.3, so explicitly configured ciphers are
accepted in the config but have no runtime effect. Keep the existing behavior in
the version-returning code, and make the note near the relevant
switch/translation logic so future readers do not assume TLS 1.3 cipher
selection is honored.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d0a4c3b-4ad1-4a7c-90f8-92c837fea4a5
📒 Files selected for processing (8)
cmd/main.goconfig/rbac-base/kustomization.yamlconfig/rbac-base/tls_profile_role.yamlconfig/rbac-base/tls_profile_role_binding.yamlpkg/tls/tls.gopkg/tls/tls_test.gopolicy/clusterrole.regopolicy/rbac.rego
|
/ok-to-test |
Read the cluster TLS profile from apiservers.config.openshift.io/cluster at startup via pkg/tls.Resolve(). Apply MinVersion, CipherSuites, and NextProtos to metrics server TLS config. Fail closed on unexpected errors. Use Intermediate defaults on non-OpenShift clusters. TLS code extracted into pkg/tls/ for reusability and testability. Signed-off-by: Ugo Giordano <ugiordan@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ugo Giordano <ugiordan@redhat.com>
d8cbcde to
768f504
Compare
|
@ugiordan: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ruivieira 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 |
b7f3922
into
trustyai-explainability:main
2 participants
Summary
apiservers.config.openshift.io/clusterat startuppkg/tls/for reusability and testabilitypkg/tls.Resolve()uses unstructured access (zero new dependencies, compatible with controller-runtime v0.17.0)config.openshift.io/apiserversto OPA policy allowlistconfig/rbac-base/kustomization.yamlpolicy/rbac.regoallowlist (both prefixed and un-prefixed)Files changed
cmd/main.go: Replaced inline TLS code with call topkgtls.Resolve()pkg/tls/tls.go: Reusable TLS profile resolution (unstructured APIServer read, cipher mapping, Intermediate defaults, transient error handling, 10s timeout)pkg/tls/tls_test.go: Table-driven tests for all profile typesconfig/rbac-base/tls_profile_role.yaml: RBAC ClusterRole for reading APIServerconfig/rbac-base/tls_profile_role_binding.yaml: RBAC ClusterRoleBinding for operator SAconfig/rbac-base/kustomization.yaml: Added TLS RBAC resourcespolicy/clusterrole.rego: OPA allowlist update forconfig.openshift.io/apiserverspolicy/rbac.rego: OPA allowlist update for TLS CRB (prefixed + un-prefixed)Dockerfile: AddedCOPY pkg/ pkg/for container buildsTest plan
go test ./pkg/tls/... -v)Supersedes #774 (closed due to broken shallow clone force-push).
Ref: RHOAIENG-61068
Summary by CodeRabbit