Conversation
|
[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 |
|
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. |
|
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:
📝 WalkthroughWalkthroughIntroduces a new ChangesOpenShift TLS Profile Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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 @.github/workflows/sync-branch-incubation.yaml:
- Around line 15-21: Update the GitHub Actions workflow by replacing
`actions/github-script@v7` with `actions/github-script@v9.0.0` on line 24 to get
the latest bug fixes and improvements. For the `tretuna/sync-branches@1.4.0`
action used in the Opening pull request step, either pin it to a specific commit
SHA (e.g., `tretuna/sync-branches@ea58ab6e...`) to mitigate supply chain risks,
or replace it with an actively maintained alternative like `jdtx0/branch-sync`.
Additionally, review the github-script action block (around lines 22-32) and
ensure any untrusted data such as pull request titles, commit messages, or user
comments are passed through environment variables via the `env` input rather
than being directly interpolated in the script block to prevent script injection
vulnerabilities.
In `@cmd/tls_test.go`:
- Around line 40-203: Convert the test functions TestParseTLSProfile and
TestTLSVersionMap from using the standard testing package to Ginkgo v2 and
Gomega. Replace the testing.T parameter and t.Run calls with Ginkgo Describe and
It blocks, convert all t.Errorf assertions to Gomega Expect statements, replace
t.Fatal with appropriate Gomega assertions, and set up controller-runtime
envtest with K8s 1.29.0 binaries in the test initialization. Ensure the
makeAPIServer helper function calls and parseTLSProfile function calls are
maintained but used within the new Ginkgo/Gomega test structure.
🪄 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: bbd545ed-befc-4821-a689-54f0449485b6
📒 Files selected for processing (10)
.github/pull.yml.github/workflows/auto-merge-upstream-sync.yaml.github/workflows/sync-branch-incubation.yaml.github/workflows/sync-branch-stable.yamlcmd/main.gocmd/tls_test.goconfig/rbac-base/kustomization.yamlconfig/rbac-base/tls_profile_role.yamlcontrollers/lmes/config.gopolicy/clusterrole.rego
f08ee8e to
9c6e240
Compare
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)
152-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEVALHUB webhook server does not use resolved TLS options.
When
EVALHUBis enabled, lines 152-156 overwrite the webhook server configured at lines 145-148, discarding thetlsOptsderived from the cluster TLS profile. This means the EVALHUB path bypasses the TLS hardening entirely, potentially violating cluster security policy.Proposed fix
if slices.Contains(enabledServices, serviceEvalHub) { mgrOpts.WebhookServer = ctrlwebhook.NewServer(ctrlwebhook.Options{ - Port: 9443, + Port: 9443, + TLSOpts: tlsOpts, }) }🤖 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 152 - 156, The webhook server created for the serviceEvalHub service at lines 152-156 overwrites the previously configured webhook server without preserving the TLS options (tlsOpts) that were derived from the cluster TLS profile. Modify the ctrlwebhook.NewServer call in the serviceEvalHub conditional block to include the tlsOpts in the Options struct alongside the Port configuration, ensuring that the TLS hardening settings derived from the cluster profile are applied consistently to the EVALHUB webhook server.
🧹 Nitpick comments (1)
pkg/tls/tls_test.go (1)
17-25: 💤 Low valueConsider using Ginkgo/Gomega per project conventions (optional).
The coding guidelines specify using Ginkgo v2 and Gomega for unit tests. While this file uses standard Go
testing, the current implementation is clean and functional for testing pure functions. Converting to Ginkgo would align with project conventions but isn't strictly necessary for this non-controller test.🤖 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 17 - 25, The test file currently uses the standard Go testing package instead of Ginkgo v2 and Gomega as specified in project conventions. To align with project standards, replace the testing import with Ginkgo v2 testing imports (ginkgo v2 and gomega), and refactor the test functions in this file to use Ginkgo's Describe/It blocks and Gomega's assertion helpers instead of the standard Go testing.T approach and manual assertions.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 `@pkg/tls/tls.go`:
- Around line 80-85: The static analysis tool is flagging the error handling in
the bootstrap client creation block where an error is logged but not propagated.
To address this linting warning while maintaining the intentional fail-open
behavior, add a nolint directive above the error check in the client.New call.
Specifically, place a comment like `//nolint:errcheck` or `//nolint:errorlint`
(depending on which linter is complaining) directly above the line where
client.New is called to suppress the warning and document that the error is
intentionally handled by logging and returning safe defaults rather than
propagating the error up the call stack.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 152-156: The webhook server created for the serviceEvalHub service
at lines 152-156 overwrites the previously configured webhook server without
preserving the TLS options (tlsOpts) that were derived from the cluster TLS
profile. Modify the ctrlwebhook.NewServer call in the serviceEvalHub conditional
block to include the tlsOpts in the Options struct alongside the Port
configuration, ensuring that the TLS hardening settings derived from the cluster
profile are applied consistently to the EVALHUB webhook server.
---
Nitpick comments:
In `@pkg/tls/tls_test.go`:
- Around line 17-25: The test file currently uses the standard Go testing
package instead of Ginkgo v2 and Gomega as specified in project conventions. To
align with project standards, replace the testing import with Ginkgo v2 testing
imports (ginkgo v2 and gomega), and refactor the test functions in this file to
use Ginkgo's Describe/It blocks and Gomega's assertion helpers instead of the
standard Go testing.T approach and manual assertions.
🪄 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: 68e73947-223d-41f5-b5ad-749bda830143
📒 Files selected for processing (6)
Dockerfilecmd/main.goconfig/rbac-base/tls_profile_role.yamlpkg/tls/tls.gopkg/tls/tls_test.gopolicy/clusterrole.rego
🚧 Files skipped from review as they are similar to previous changes (2)
- policy/clusterrole.rego
- config/rbac-base/tls_profile_role.yaml
caea238 to
c7c7824
Compare
There was a problem hiding this comment.
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:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEVALHUB webhook server missing TLS options.
When
serviceEvalHubis enabled, the webhook server is re-created without the resolvedtlsOpts, discarding the cluster TLS profile settings. This creates inconsistent TLS security posture: the metrics server uses the hardened OpenShift profile while the webhook server uses default settings.🔒 Proposed fix to apply TLS options consistently
if slices.Contains(enabledServices, serviceEvalHub) { mgrOpts.WebhookServer = ctrlwebhook.NewServer(ctrlwebhook.Options{ - Port: 9443, + Port: 9443, + TLSOpts: tlsOpts, }) }🤖 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 webhook server creation in the serviceEvalHub conditional block is missing TLS options when calling ctrlwebhook.NewServer with ctrlwebhook.Options. The Options struct only sets the Port field to 9443 but should also include the resolved tlsOpts (similar to how they are applied to other servers in the configuration) to ensure consistent TLS security settings across all services. Add the tlsOpts to the ctrlwebhook.Options struct initialization alongside the Port field.
🧹 Nitpick comments (1)
pkg/tls/tls_test.go (1)
26-173: 💤 Low valueConsider using Ginkgo/Gomega for test framework consistency.
The coding guidelines specify using Ginkgo v2 and Gomega for unit tests. While standard Go testing works correctly here, consider migrating to Ginkgo/Gomega for consistency with other tests in the project.
That said, for a pure function test without envtest needs, standard Go testing is acceptable. This is a low-priority suggestion.
🤖 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 currently uses standard Go testing with *testing.T. To align with project guidelines, consider migrating this test to use Ginkgo v2 and Gomega framework. Convert the test table structure into a Ginkgo Describe block with nested Context blocks for each test case, replace t.Run() with Gomega's It() assertions, convert all t.Errorf() and t.Fatal() calls to use Gomega matchers like Expect(), and use Gomega's assertion syntax to validate the gotMinVersion and gotCiphers return values from parseProfile().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.
Outside diff comments:
In `@cmd/main.go`:
- Around line 150-154: The webhook server creation in the serviceEvalHub
conditional block is missing TLS options when calling ctrlwebhook.NewServer with
ctrlwebhook.Options. The Options struct only sets the Port field to 9443 but
should also include the resolved tlsOpts (similar to how they are applied to
other servers in the configuration) to ensure consistent TLS security settings
across all services. Add the tlsOpts to the ctrlwebhook.Options struct
initialization alongside the Port field.
---
Nitpick comments:
In `@pkg/tls/tls_test.go`:
- Around line 26-173: The TestParseProfile function currently uses standard Go
testing with *testing.T. To align with project guidelines, consider migrating
this test to use Ginkgo v2 and Gomega framework. Convert the test table
structure into a Ginkgo Describe block with nested Context blocks for each test
case, replace t.Run() with Gomega's It() assertions, convert all t.Errorf() and
t.Fatal() calls to use Gomega matchers like Expect(), and use Gomega's assertion
syntax to validate the gotMinVersion and gotCiphers return values from
parseProfile().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0001fed-b1c7-40b4-92b5-e4b22374baba
📒 Files selected for processing (6)
Dockerfilecmd/main.goconfig/rbac-base/tls_profile_role.yamlpkg/tls/tls.gopkg/tls/tls_test.gopolicy/clusterrole.rego
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- config/rbac-base/tls_profile_role.yaml
- policy/clusterrole.rego
|
@ruivieira, @RobGeada, could you please review this PR? |
e3e1746 to
89a2aef
Compare
ruivieira
left a comment
ruivieira
left a comment
There was a problem hiding this comment.
@ugiordan thanks for the PR.
On top of the inlined comments, I want to ask if it's possible to add some retry mechanism.
Resolve is called once at process start before the manager is initialised, so there is no controller-runtime retry loop or leader-election backoff for transient API server errors. A single network timeout at pod start propagates as a non-nil error and causes os.Exit(1) in main.go. So what's a minor temporary network issue can turn into a CrashLoopBackOff.
Since the fallback to Intermediate defaults is already implemented for the permanent cases (NoMatch, NotFound, Forbidden), transient errors should also reach that fallback rather than crash the process.
If a short exponential backoff (e.g. 5 steps, 0.5s base, factor 2and ~15s ceiling) on IsServiceUnavailable, IsTimeout, and IsTooManyRequests before falling back covers this window without blocking normal startup.
wdyt?
@ruivieira makes sense. Added I went with graceful fallback over exponential backoff to keep startup simple. The |
1c700ed to
1199075
Compare
b6644a7 to
8e93b07
Compare
e3fe7ef to
f6619c9
Compare
Read the cluster TLS profile from apiservers.config.openshift.io/cluster at startup. Apply MinVersion, CipherSuites, and NextProtos to webhook and metrics server TLS configs. Fail closed on unexpected errors, graceful fallback to Intermediate defaults on non-OpenShift clusters and transient API errors. - Add pkg/tls package with Resolve() that reads the APIServer CR - Apply TLSOpts to both the default and EvalHub webhook servers - Add ClusterRole and ClusterRoleBinding for apiservers RBAC - Handle IsNoMatchError, IsNotFound, IsForbidden, IsServiceUnavailable, IsTimeout, IsTooManyRequests as graceful fallback cases - Add CRB entries to policy/rbac.rego allowlist - 7 unit tests covering Intermediate, Modern, Custom, non-OpenShift, APIServer not found, unsupported ciphers, and ALPN Signed-off-by: Ugo Giordano <ugiordan@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f6619c9 to
6a258fe
Compare
|
@ugiordan: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Hey @ruivieira, unfortunately, this PR got into a broken state (disconnected git history from a shallow clone mishap on my side), so I had to open a replacement: #812 |
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)Ref: RHOAIENG-61068