NE-2739: Migrate operator to native metrics protection with centralized TLS - #313
NE-2739: Migrate operator to native metrics protection with centralized TLS#313bentito wants to merge 11 commits into
Conversation
|
@bentito: This pull request references NE-2739 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe manager now serves authenticated and authorized HTTPS metrics on port 8443 with TLS settings derived from the API server profile. Deployment manifests remove the Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
main.go (2)
351-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging dropped/unsupported TLS groups.
Groups without a Go
CurveIDmapping are silently skipped. A debug/info log when a configuredTLSGrouphas no equivalent would make misconfigurations easier to diagnose.🤖 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 `@main.go` around lines 351 - 361, Update the TLS group processing around TLSGroupToCurveID to log each configured group that lacks a CurveID mapping before skipping it. Use the existing logging mechanism and include the unsupported group value, while preserving the current behavior for supported groups and empty curve lists.
301-364: 🩺 Stability & Availability | 🔵 TrivialTLS profile is resolved once at startup; cluster profile changes require a pod restart to take effect.
There's no watch/reconcile loop reacting to updates on the
APIServerobject'sTLSSecurityProfile, so an admin changing the cluster TLS profile won't be picked up by the running metrics server until the operator pod restarts. This may be an acceptable initial tradeoff, but worth confirming it's the intended behavior for this release given other OpenShift components implement a watcher for exactly this scenario.🤖 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 `@main.go` around lines 301 - 364, Confirm and preserve the intended startup-only behavior for TLS profile resolution in getTLSSecurityProfile and getTLSConfigFromProfile, or add a watch/reconcile path that refreshes the metrics server’s TLS configuration when the APIServer object’s TLSSecurityProfile changes. Ensure runtime profile updates take effect without requiring an operator pod restart if dynamic behavior is required for this release.demonstrate-deregistration-gap.sh (2)
68-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
diagnose_failureis defined but never invoked.The detailed diagnostics helper (CR status, CredentialsRequest, deployments/pods, manager logs) would be most useful right when the script falls back to
SIMULATION_MODE=true, but nothing calls it.🤖 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 `@demonstrate-deregistration-gap.sh` around lines 68 - 92, Invoke diagnose_failure at the point where the script falls back to SIMULATION_MODE=true, before or alongside the fallback message. Keep the existing diagnostic collection and simulation behavior unchanged, ensuring the CR, CredentialsRequest, workload, and manager log checks run automatically when the fallback occurs.
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing project copyright header on all three new files. Contribution guidelines require new files to carry the project's copyright template at the top, updated to the current year; none of these new additions include one.
demonstrate-deregistration-gap.sh#L1-L14: add the copyright header comment block before the shebang/comment banner.OCPBUGS-97941-analysis.md#L1-L2: add the copyright header before the# Technical Analysis...title.OCPBUGS-97941-short-analysis.md#L1-L2: add the copyright header before the# OCPBUGS-97941...title.Based on learnings, "New files must include the project copyright message template at the top as a comment, with the year updated."
🤖 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 `@demonstrate-deregistration-gap.sh` around lines 1 - 14, Add the project’s standard copyright header, updated to the current year, at the top of demonstrate-deregistration-gap.sh before the shebang/comment banner, and at the top of OCPBUGS-97941-analysis.md and OCPBUGS-97941-short-analysis.md before their titles.Source: Learnings
🤖 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`:
- Line 286: Update the google.golang.org/grpc dependency in go.mod from v1.72.2
to the patched v1.79.3 or later release, then regenerate go.sum so the module
checksums match the upgraded dependency.
- Around line 258-262: Upgrade the OpenTelemetry dependency set represented by
the go.mod entries for otel, exporters, metric, and sdk to a consistent current
version that includes the fixes from otel v1.41.0 and sdk v1.40.0. Regenerate
the Go module graph so go.mod and go.sum reflect the coordinated updates.
In `@main.go`:
- Around line 128-134: Replace context.TODO() in the startup call to
getTLSSecurityProfile with a bounded context.Context, creating and canceling a
startup timeout context before the fetch. Pass that context through
getTLSSecurityProfile so its client.Get call observes the deadline, while
preserving the existing error logging and exit behavior.
---
Nitpick comments:
In `@demonstrate-deregistration-gap.sh`:
- Around line 68-92: Invoke diagnose_failure at the point where the script falls
back to SIMULATION_MODE=true, before or alongside the fallback message. Keep the
existing diagnostic collection and simulation behavior unchanged, ensuring the
CR, CredentialsRequest, workload, and manager log checks run automatically when
the fallback occurs.
- Around line 1-14: Add the project’s standard copyright header, updated to the
current year, at the top of demonstrate-deregistration-gap.sh before the
shebang/comment banner, and at the top of OCPBUGS-97941-analysis.md and
OCPBUGS-97941-short-analysis.md before their titles.
In `@main.go`:
- Around line 351-361: Update the TLS group processing around TLSGroupToCurveID
to log each configured group that lacks a CurveID mapping before skipping it.
Use the existing logging mechanism and include the unsupported group value,
while preserving the current behavior for supported groups and empty curve
lists.
- Around line 301-364: Confirm and preserve the intended startup-only behavior
for TLS profile resolution in getTLSSecurityProfile and getTLSConfigFromProfile,
or add a watch/reconcile path that refreshes the metrics server’s TLS
configuration when the APIServer object’s TLSSecurityProfile changes. Ensure
runtime profile updates take effect without requiring an operator pod restart if
dynamic behavior is required for this release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
/jira refresh |
|
@bentito: This pull request references NE-2739 which is a valid jira issue. 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. |
… to fix verify checks
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 `@config/manifests/kustomization.yaml`:
- Line 22: Update the volumeMount removal patch in the kustomization
configuration to target the cert mount rather than index 0, which still refers
to bound-sa-token; identify the mount by name where supported, or reorder the
patch operations so the cert mount is removed reliably.
In `@hack/operator-permission-policy.json`:
- Around line 5-24: Update the IAM policy entries shown so the keys use AWS’s
canonical casing: change every action, effect, and resource key to Action,
Effect, and Resource, preserving all existing values and permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 524ec681-e2d8-4faa-b402-836bc2c4c086
📒 Files selected for processing (3)
config/default/manager_webhook_patch.yamlconfig/manifests/kustomization.yamlhack/operator-permission-policy.json
💤 Files with no reviewable changes (1)
- config/default/manager_webhook_patch.yaml
|
Unsupported TLS groups logging added and accidental files removed in commit 26970fc. |
…d restore canonical IAM policy casing
|
/assign @alebedev87 |
|
/retest |
|
/retest-required |
|
/retest |
|
@bentito: GitHub didn't allow me to assign the following users: davide. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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 kubernetes-sigs/prow repository. |
|
/assign @davidesalerno |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
@davidesalerno I think the rosa test is a permafail until someone updates a role somewhere. Can you review when you get a chance please |
Right, after we lost QEs some Prow CI steps were left unmaintained. We need to come back to this job and fix it. But for the moment we'd have to ignore it. |
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
Mostly LGTM. Some minor remarks and questions. One important thing I noticed though, ALBO doesn't add servicemointor so the metrics are not actually scraped. This is gap which we can address as a separate PR.
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
We can add a log message similar to the unsupported group to leave a trace of unrecognized cipher suite.
| if err != nil { | ||
| return nil, fmt.Errorf("invalid TLS version %q: %w", profileSpec.MinTLSVersion, err) | ||
| } |
There was a problem hiding this comment.
How do we decide which of TLS settings are blocking like this minimal TLS version and which can be skipped like unrecognized ciphersuite of curves? Should we do the best effort here too and let the default (TLS1.2) be applied?
There was a problem hiding this comment.
The distinction comes down to fail-closed vs. fail-safe negotiation:
- Fail-Closed for Protocol Versions: We cannot allow the metrics server to start up with a lower TLS protocol version than the global cluster policy specifies (e.g. falling back to TLS 1.2 when a strict TLS 1.3 profile is configured), as this would violate security compliance audits. Hence, an unrecognized protocol version must be a blocking startup error (fail-closed).
- Fail-Safe for Ciphers/Curves: Unrecognized cipher suites and elliptic curves can be safely skipped (logged as warnings) because the TLS handshake protocol natively negotiates only the overlapping set of supported parameters. This is a fail-safe negotiation.
| } | ||
| } | ||
|
|
||
| return libgocrypto.SecureTLSConfig(cfg), nil |
There was a problem hiding this comment.
SecureTLSConfig will fill unset settings with defaults. This makes the defaults we set in this function (e.g. min tls version) redundant. Should we just let SecureTLSConfig handle the unfilled defaults?
There was a problem hiding this comment.
Agreed that it's technically redundant because SecureTLSConfig will populate the defaults. However, we explicitly initialize MinVersion inside the tls.Config declaration primarily to satisfy the gosec security linter (which flags any uninitialized or default TLS instantiation as G402: TLS MinVersion too low).
| // TLSGroupToCurveID converts a configv1.TLSGroup name to its crypto/tls | ||
| // CurveID. The second return value is false when the group is not supported | ||
| // by the Go runtime. | ||
| func TLSGroupToCurveID(group configv1.TLSGroup) (tls.CurveID, bool) { |
There was a problem hiding this comment.
Do we really need this function to be an exported symbol? As a matter of fact I don't see the reason for its existance, simple map lookup (tlsGroupToCurveID[group]) gives the same result.
There was a problem hiding this comment.
Agreed. Made the map unexported, removed the wrapper function in favor of a direct map lookup, and aligned the warning casing in commit 4ce1511
| if id, ok := TLSGroupToCurveID(g); ok { | ||
| curves = append(curves, id) | ||
| } else { | ||
| setupLog.Info("Skipping unsupported TLS group", "group", g) |
There was a problem hiding this comment.
To match other log messages:
| setupLog.Info("Skipping unsupported TLS group", "group", g) | |
| setupLog.Info("skipping unsupported TLS group", "group", g) |
There was a problem hiding this comment.
Agreed. Aligned the log casing to match other messages in commit 4ce1511
| EOF | ||
| ${YQ_BIN} -i -o=json "${POLICY_FILE}" | ||
| sed -i -e 's/action/Action/g' -e 's/effect/Effect/g' -e 's/resource/Resource/g' "${POLICY_FILE}" | ||
| python3 -c "import sys; p = sys.argv[1]; text = open(p).read().replace('\"action\"', '\"Action\"').replace('\"effect\"', '\"Effect\"').replace('\"resource\"', '\"Resource\"'); open(p, 'w').write(text)" "${POLICY_FILE}" |
There was a problem hiding this comment.
Is it just for a safer replacement? Or somehow related to the metrics change?
There was a problem hiding this comment.
The original sed command was platform-dependent and failed silently on macOS, which left uncommitted lowercase keys (action/effect/resource) in our local IAM permission policy compared to Linux CI. The Python replacement is cross-platform, allowing successful local schema generation and keeping the repository in sync
|
[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 |
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 `@hack/generate-iam-from-credrequest.sh`:
- Line 18: The inline Python transformation in the policy-generation command
must rename keys structurally rather than globally replacing text. Parse
POLICY_FILE as JSON, traverse each Statement object, and rename only its action,
effect, and resource fields to Action, Effect, and Resource while preserving
string values and unrelated nested objects; serialize the transformed policy
back to the same file.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 87492459-6712-4cc1-93a2-c13d81b21ff1
📒 Files selected for processing (2)
hack/generate-iam-from-credrequest.shmain.go
b17c29f to
4ce1511
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Agreed. Enabling the |
|
@bentito: 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. |
5 participants
Migrates the
aws-load-balancer-operatorfrom utilizingkube-rbac-proxysidecars to the native secure serving metrics server introduced in newer versions ofcontroller-runtime.Key Changes:
main.go: Integrated secure serving on port:8443using the native auth and authz filters. Added startup resolution of the OpenShiftAPIServerconfiguration namedclusterto parse and apply the centralizedTLSSecurityProfile(MinVersion, CipherSuites, and elliptic curves/groups, including post-quantumX25519MLKEM768). Restressed HTTP/2 by disabling it on the metrics server to match External DNS Operator (EDO) PR 502's security posture.config/: Deletedkube-rbac-proxypatches, updatedmanagerdeployment configuration to expose port8443directly, and commented out the auth proxy references in default Kustomizations.bundle/&vendor/: Regenerated the OLM bundle usingmake bundleand validated it successfully withoperator-sdk. Resolved the vendored dependencies and updated the test CRD fixtures to keep test pipelines completely green.Fully validated and verified locally using
make verifyandmake lintwith zero issues found.