NE-2739: Migrate operator to native metrics protection with centralized TLS - #313
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
|
Caution Review failedAn error occurred during the review process. Please try again later. 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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe operator metrics endpoint now uses TLS on port 8443 with authentication and authorization filtering. Deployment manifests remove the Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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. |
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. |
|
/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.
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 |
openshift-ci
Bot
added
the
needs-rebase
a72d83b to
60db6e1
Compare
openshift-ci
Bot
removed
the
needs-rebase
…ed TLS Migrates the aws-load-balancer-operator from utilizing kube-rbac-proxy sidecars to the native secure serving metrics server introduced in controller-runtime. Key Changes: - main.go: Integrated secure serving on port :8443 using native auth and authz filters. Added startup resolution of the OpenShift APIServer configuration named 'cluster' to parse and apply the centralized TLSSecurityProfile (MinVersion, CipherSuites, and elliptic curves/groups, including post-quantum X25519MLKEM768). Restressed HTTP/2 by disabling it on the metrics server to match External DNS Operator (EDO) PR 502's security posture. - config/: Deleted kube-rbac-proxy patches, updated manager deployment configuration to expose port 8443 directly, and commented out auth proxy references in default Kustomizations. - bundle/ & vendor/: Regenerated OLM bundle using 'make bundle' and validated it successfully with operator-sdk. Resolved vendored dependencies and updated test CRD fixtures. - hack/generate-iam-from-credrequest.sh: Refactored to translate JSON keys structurally using Python and ensure trailing newlines are preserved in generated IAM policy files.
60db6e1 to
41fe2dc
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/override ci/prow/e2e-aws-rosa-operator |
|
@alebedev87: Overrode contexts on behalf of alebedev87: ci/prow/e2e-aws-rosa-operator 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. |
|
/retest-required |
|
@bentito: all tests passed! 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.