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 8 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
|
[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 |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change moves controller metrics to HTTPS on port 8443 using the cluster TLS security profile, removes the metrics authentication proxy, and updates manifests. It adds ALB deregistration documentation and a demonstration script, refreshes Go dependencies, and expands Infrastructure schema validation and cloud load balancer configuration for AWS, Azure, and GCP. 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)
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 |
|
@bentito: 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. |
4 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.