[ACM-35533] Depricate use of BYO CA certs for alertmanager#2568
Conversation
Signed-off-by: Daniel Buchanan <dbuchana@redhat.com>
Signed-off-by: Daniel Buchanan <dbuchana@redhat.com>
Signed-off-by: Daniel Buchanan <dbuchana@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAlertmanager Route exposure and BYO Route support were removed from configuration, reconciliation, controller watches, placement-rule wiring, and associated test fixtures. ChangesAlertmanager Route removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.go (1)
227-230: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMark
CustomAlertmanagerHubURLdeprecated inAdvancedConfig. It’s still exposed in the API/CRD, so users can keep flipping a switch that now controls nothing. Add a deprecation note inoperators/multiclusterobservability/api/v1beta2/multiclusterobservability_types.goand regenerate the CRD/bundle, or remove it once compatibility allows.🤖 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 `@operators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.go` around lines 227 - 230, Mark AdvancedConfig.CustomAlertmanagerHubURL as deprecated in multiclusterobservability_types.go with an appropriate API deprecation annotation and regenerate the corresponding CRD and bundle artifacts; preserve the field for compatibility while clearly indicating it no longer controls behavior.
🧹 Nitpick comments (1)
operators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.go (1)
252-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestNewBYOSecretno longer tests a BYO scenario.With the BYO alertmanager secrets gone, this test's fixtures/assertions are now the same shape as
TestNewSecret's default-path case — just missingnewTestIngressController()/newTestRouteCASecret(). The name is now a bit of a misnomer and the coverage looks largely duplicated. Consider renaming it to reflect what it actually verifies now, or folding it intoTestNewSecret(e.g., as a subtest) to avoid maintaining two near-identical assertions.🤖 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 `@operators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.go` around lines 252 - 280, Update TestNewBYOSecret to reflect that it now verifies the default hub info secret path rather than a BYO alertmanager scenario: either rename it to a name describing the default-path behavior or fold it into TestNewSecret as a subtest, while preserving the existing endpoint and CA assertions and avoiding duplicate test coverage.
🤖 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
`@operators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.go`:
- Around line 227-230: Mark AdvancedConfig.CustomAlertmanagerHubURL as
deprecated in multiclusterobservability_types.go with an appropriate API
deprecation annotation and regenerate the corresponding CRD and bundle
artifacts; preserve the field for compatibility while clearly indicating it no
longer controls behavior.
---
Nitpick comments:
In
`@operators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.go`:
- Around line 252-280: Update TestNewBYOSecret to reflect that it now verifies
the default hub info secret path rather than a BYO alertmanager scenario: either
rename it to a name describing the default-path behavior or fold it into
TestNewSecret as a subtest, while preserving the existing endpoint and CA
assertions and avoiding duplicate test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0295cc7-fc48-4b5a-aa1d-ec0ffb6f012f
📒 Files selected for processing (11)
operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller_integration_test.gooperators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller_test.gooperators/multiclusterobservability/controllers/multiclusterobservability/predicate_func.gooperators/multiclusterobservability/controllers/placementrule/hub_info_secret_test.gooperators/multiclusterobservability/controllers/placementrule/manifestwork_test.gooperators/multiclusterobservability/controllers/placementrule/mco_predicate_test.gooperators/multiclusterobservability/controllers/placementrule/placementrule_controller.gooperators/multiclusterobservability/controllers/placementrule/placementrule_controller_test.gooperators/multiclusterobservability/pkg/config/config.gooperators/multiclusterobservability/pkg/config/config_test.go
💤 Files with no reviewable changes (6)
- operators/multiclusterobservability/controllers/multiclusterobservability/predicate_func.go
- operators/multiclusterobservability/controllers/placementrule/manifestwork_test.go
- operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go
- operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller_integration_test.go
- operators/multiclusterobservability/controllers/placementrule/placementrule_controller.go
- operators/multiclusterobservability/pkg/config/config_test.go
Signed-off-by: Daniel Buchanan <dbuchana@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go (1)
798-859: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
context.TODO()is a placeholder that outlived its welcome.
GenerateAlertmanagerRoutedoesn't accept acontext.Contextand instead callsrunclient.Get(context.TODO(), ...), unlike its siblingsGenerateAPIGatewayRoute/GenerateGrafanaRoutewhich thread the Reconcilectxthrough. Since this function is being reintroduced in this PR, it's a good spot to wire up proper cancellation instead of importing anotherTODO. As per path instructions, "Check for proper context propagation and cancellation."Note: dropping the BYO CA/cert/key population for the route's TLS is fine — OpenShift falls back to the service-serving CA bundle when
destinationCACertificateis unset on a reencrypt route, so no TLS verification regression here.♻️ Proposed fix to thread context through
func GenerateAlertmanagerRoute( - runclient client.Client, scheme *runtime.Scheme, + ctx context.Context, runclient client.Client, scheme *runtime.Scheme, mco *mcov1beta2.MultiClusterObservability, ) (*ctrl.Result, error) { @@ found := &routev1.Route{} err := runclient.Get( - context.TODO(), + ctx, types.NamespacedName{Name: amGateway.Name, Namespace: amGateway.Namespace}, found, )And update the call site:
- result, err = GenerateAlertmanagerRoute(r.Client, r.Scheme, instance) + result, err = GenerateAlertmanagerRoute(ctx, r.Client, r.Scheme, instance)🤖 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 `@operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go` around lines 798 - 859, Update GenerateAlertmanagerRoute to accept a context.Context parameter and replace both context.TODO() usages in the client Get and Create calls with that context. Update its caller to pass the Reconcile ctx, matching the context propagation used by GenerateAPIGatewayRoute and GenerateGrafanaRoute.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go`:
- Around line 798-859: Update GenerateAlertmanagerRoute to accept a
context.Context parameter and replace both context.TODO() usages in the client
Get and Create calls with that context. Update its caller to pass the Reconcile
ctx, matching the context propagation used by GenerateAPIGatewayRoute and
GenerateGrafanaRoute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35c4a8bd-56b4-4bad-8540-35cf36be0e94
📒 Files selected for processing (2)
operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/pkg/config/config.go
Signed-off-by: Daniel Buchanan <dbuchana@redhat.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coleenquadros, dbuchanaRH 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 |
|
/test e2e-kind |
2 participants
Cleanup of BYO CA certs functionality not currently used
modified Function
GenerateAlertmanagerRoute()multiclusterobservability_controller.goremoved Functions
GetAlertManagerSecretPredicateFunc()predicate_func.goalertmanager-configdelete and backup-label secretsGetAlertmanagerRouterCA()pkg/config/config.goamRouterCertSecretPred(inline)placementrule_controller.goupdateHubInfoSecret()newTestAmRouteBYOCA()/newTestAmRouteBYOCert()hub_info_secret_test.goGetAlertmanagerURL()pkg/config/config.goController Wiring Removed
MCO reconciler (
multiclusterobservability_controller.go)Reconcile()— removed call toGenerateAlertmanagerRoute()SetupWithManager()— removedsecretPred := GetAlertManagerSecretPredicateFunc()and the associatedSecretwatchPlacementRule reconciler (
placementrule_controller.go)Secretwatch usingamRouterCertSecretPredfor AM BYO cert changesNot Removed (out of scope for this PR)
ProxyRouteBYOCAName/ProxyRouteBYOCERTName— proxy route BYO cert support remainsBehavioral Impact
alertmanager-byo-caandalertmanager-byo-certare no longer watched, backed up, or referencedcustomObservabilityHubURLref: https://docs.redhat.com/en/documentation/red_hat_advanced_cluster_management_for_kubernetes/2.16/html/observability/observing-environments-intro#custom-obervatorium-alert-urlSummary by CodeRabbit
Summary by CodeRabbit