OCPBUGS-87028: Fix KAS ReconcileServiceStatus to check LoadBalancer.Hostname before LB provisioning#8663
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-important
|
@sdminonne: This pull request references Jira Issue OCPBUGS-87028, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds netutil.RouteHost(route) to compute an effective Route hostname (Spec.Host then Status.Ingress[0].Host), updates KAS and OAuth service reconciliation to prefer configured LoadBalancer hostnames and to use RouteHost for Route-based host resolution, tightens a shared-ingress special-case to Route strategies, expands unit tests for these behaviors, and adds two small structured-log improvements in the hosted control plane controller. Sequence Diagram(s)sequenceDiagram
participant HCPCtrl as HostedControlPlaneController
participant ServiceRecon as ReconcileServiceStatus (KAS/OAuth)
participant Route as routev1.Route / Service.Status.LoadBalancer
participant util as netutil.RouteHost
participant Infra as InfrastructureStatus
HCPCtrl->>ServiceRecon: trigger reconciliation
ServiceRecon->>Route: read Route or Service status
ServiceRecon->>util: RouteHost(route)
util-->>ServiceRecon: hostname or ""
ServiceRecon->>Infra: assemble host/port/message into InfrastructureStatus
HCPCtrl->>Infra: log infraStatus (structured)
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdminonne 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 |
openshift-ci
Bot
added
area/hypershift-operator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8663 +/- ##
==========================================
+ Coverage 40.68% 41.51% +0.82%
==========================================
Files 755 756 +1
Lines 93368 93673 +305
==========================================
+ Hits 37985 38884 +899
+ Misses 52649 52067 -582
+ Partials 2734 2722 -12
... and 58 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ostname resolution Add a RouteHost helper in support/netutil that resolves route hostnames consistently across service types. The helper checks RoutePublishingStrategy.Hostname first, then falls back to status.Conditions with the route.openshift.io/host key, and finally the legacy BaseDomain-based default. Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ovisioning in KAS service status - Update kas.ReconcileServiceStatus and oauth.ReconcileServiceStatus to check LoadBalancer.Hostname before calling CollectLBMessageIfNotProvisioned, so clusters with a pre-configured LB hostname skip the cloud LB provisioning wait entirely. - Update infra.ReconcileInfrastructure to use the RouteHost helper for OAuth and Ignition route hostname resolution, ensuring consistent behavior across Route and LoadBalancer service types. Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sdminonne
force-pushed
the
OCPBUGS-87028/cpo-kas-lb-hostname-check
branch
from
b2f9af1 to
efc01ec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@control-plane-operator/controllers/hostedcontrolplane/infra/infra.go`:
- Around line 55-62: InfrastructureStatus.String() currently prints raw
hostnames and message (APIHost, KonnectivityHost, InternalHCPRouterHost,
ExternalHCPRouterHost, Message, etc.), which can leak sensitive topology/runtime
details; change the formatter to avoid emitting raw values by replacing host
fields with presence flags or redacted placeholders (e.g., "set" / "<redacted>")
and truncate or redact Message (or replace with a generic summary), keeping
boolean flags like NeedInternalRouter/NeedExternalRouter and ports if
non-sensitive; update the InfrastructureStatus.String() implementation to
reference those fields (APIHost, APIPort, KonnectivityHost, KonnectivityPort,
OAuthEnabled, OAuthHost, OAuthPort, NeedInternalRouter, InternalHCPRouterHost,
NeedExternalRouter, ExternalHCPRouterHost, Message) but only emit non-sensitive
indicators instead of the verbatim hostnames/messages.
🪄 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: 5e35afe5-9ecf-4d62-9913-1b0a35a473e5
⛔ Files ignored due to path filters (2)
control-plane-operator/controllers/hostedcontrolplane/infra/testdata/zz_fixture_TestReconcileInfrastructure_When_ARO_shared_ingress_is_enabled_with_KAS_LoadBalancer_strategy__it_should_use_LoadBalancer_host_not_shared_ingress.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/infra/testdata/zz_fixture_TestReconcileInfrastructure_When_NonePlatform_cluster_has_KAS_LoadBalancer_with_configured_hostname__it_should_use_hostname_without_waiting_for_LB.yamlis excluded by!**/testdata/**
📒 Files selected for processing (9)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/infra/infra_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/service_test.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/service_test.gosupport/netutil/route.gosupport/netutil/route_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
🚧 Files skipped from review as they are similar to previous changes (7)
- control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go
- support/netutil/route.go
- control-plane-operator/controllers/hostedcontrolplane/oauth/service.go
- control-plane-operator/controllers/hostedcontrolplane/kas/service.go
- control-plane-operator/controllers/hostedcontrolplane/kas/service_test.go
- support/netutil/route_test.go
- control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go
| func (s InfrastructureStatus) String() string { | ||
| return fmt.Sprintf("apiHost=%s apiPort=%d konnectivityHost=%s konnectivityPort=%d oauthEnabled=%t oauthHost=%s oauthPort=%d needInternalRouter=%t internalRouterHost=%s needExternalRouter=%t externalRouterHost=%s message=%s", | ||
| s.APIHost, s.APIPort, s.KonnectivityHost, s.KonnectivityPort, | ||
| s.OAuthEnabled, s.OAuthHost, s.OAuthPort, | ||
| s.NeedInternalRouter, s.InternalHCPRouterHost, | ||
| s.NeedExternalRouter, s.ExternalHCPRouterHost, | ||
| s.Message) | ||
| } |
There was a problem hiding this comment.
Avoid serializing hostnames/message verbatim in InfrastructureStatus.String().
Line 56 currently includes internal/external hostnames and raw message, which can leak sensitive topology/runtime details when this struct is logged. Prefer presence flags (or redacted values) instead of raw host/message content.
Proposed safer formatter
func (s InfrastructureStatus) String() string {
- return fmt.Sprintf("apiHost=%s apiPort=%d konnectivityHost=%s konnectivityPort=%d oauthEnabled=%t oauthHost=%s oauthPort=%d needInternalRouter=%t internalRouterHost=%s needExternalRouter=%t externalRouterHost=%s message=%s",
- s.APIHost, s.APIPort, s.KonnectivityHost, s.KonnectivityPort,
- s.OAuthEnabled, s.OAuthHost, s.OAuthPort,
- s.NeedInternalRouter, s.InternalHCPRouterHost,
- s.NeedExternalRouter, s.ExternalHCPRouterHost,
- s.Message)
+ return fmt.Sprintf("apiPort=%d konnectivityPort=%d oauthEnabled=%t oauthPort=%d needInternalRouter=%t needExternalRouter=%t apiHostSet=%t konnectivityHostSet=%t oauthHostSet=%t internalRouterHostSet=%t externalRouterHostSet=%t hasMessage=%t",
+ s.APIPort, s.KonnectivityPort, s.OAuthEnabled, s.OAuthPort,
+ s.NeedInternalRouter, s.NeedExternalRouter,
+ s.APIHost != "", s.KonnectivityHost != "", s.OAuthHost != "",
+ s.InternalHCPRouterHost != "", s.ExternalHCPRouterHost != "",
+ s.Message != "")
}As per coding guidelines: "Flag logging that may expose passwords, tokens, API keys, PII ... internal hostnames, or customer data."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s InfrastructureStatus) String() string { | |
| return fmt.Sprintf("apiHost=%s apiPort=%d konnectivityHost=%s konnectivityPort=%d oauthEnabled=%t oauthHost=%s oauthPort=%d needInternalRouter=%t internalRouterHost=%s needExternalRouter=%t externalRouterHost=%s message=%s", | |
| s.APIHost, s.APIPort, s.KonnectivityHost, s.KonnectivityPort, | |
| s.OAuthEnabled, s.OAuthHost, s.OAuthPort, | |
| s.NeedInternalRouter, s.InternalHCPRouterHost, | |
| s.NeedExternalRouter, s.ExternalHCPRouterHost, | |
| s.Message) | |
| } | |
| func (s InfrastructureStatus) String() string { | |
| return fmt.Sprintf("apiPort=%d konnectivityPort=%d oauthEnabled=%t oauthPort=%d needInternalRouter=%t needExternalRouter=%t apiHostSet=%t konnectivityHostSet=%t oauthHostSet=%t internalRouterHostSet=%t externalRouterHostSet=%t hasMessage=%t", | |
| s.APIPort, s.KonnectivityPort, s.OAuthEnabled, s.OAuthPort, | |
| s.NeedInternalRouter, s.NeedExternalRouter, | |
| s.APIHost != "", s.KonnectivityHost != "", s.OAuthHost != "", | |
| s.InternalHCPRouterHost != "", s.ExternalHCPRouterHost != "", | |
| s.Message != "") | |
| } |
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/infra/infra.go` around
lines 55 - 62, InfrastructureStatus.String() currently prints raw hostnames and
message (APIHost, KonnectivityHost, InternalHCPRouterHost,
ExternalHCPRouterHost, Message, etc.), which can leak sensitive topology/runtime
details; change the formatter to avoid emitting raw values by replacing host
fields with presence flags or redacted placeholders (e.g., "set" / "<redacted>")
and truncate or redact Message (or replace with a generic summary), keeping
boolean flags like NeedInternalRouter/NeedExternalRouter and ports if
non-sensitive; update the InfrastructureStatus.String() implementation to
reference those fields (APIHost, APIPort, KonnectivityHost, KonnectivityPort,
OAuthEnabled, OAuthHost, OAuthPort, NeedInternalRouter, InternalHCPRouterHost,
NeedExternalRouter, ExternalHCPRouterHost, Message) but only emit non-sensitive
indicators instead of the verbatim hostnames/messages.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sdminonne: This pull request references Jira Issue OCPBUGS-87028, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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
Bot
removed
the
do-not-merge/work-in-progress
|
/pipeline-required |
| func ReconcileServiceStatus(svc *corev1.Service, strategy *hyperv1.ServicePublishingStrategy, apiServerPort int, messageCollector events.MessageCollector) (host string, port int32, message string, err error) { | ||
| switch strategy.Type { | ||
| case hyperv1.LoadBalancer: | ||
| if strategy.LoadBalancer != nil && strategy.LoadBalancer.Hostname != "" { |
There was a problem hiding this comment.
I think we need to think more carefully about the implications of this. Currently we block provisioning if the LoadBalancer has not provisioned. By returning the configured hostname, we're skipping that and proceeding with the control plane rollout. It's not a bad change, but it's definitely a change in behavior and we need to check whether it might negatively impact existing deployments.
There was a problem hiding this comment.
Yeah, it make sense. I've moved the LB-bypass out of ReconcileServiceStatus (now reverted to original logic) and only for NonePlatform in reconcileAPIServerServiceStatus. I think this should preserve all the other platform behaviors. At the end of the day this is only to test NonePlatform avoiding different behaviours between AWS and AKS (at least for the moment).
… only Revert ReconcileServiceStatus to the original behavior where the LB provisioning check always runs before returning a host. The LB-bypass for a configured LoadBalancer.Hostname is now handled at the call site in reconcileAPIServerServiceStatus, scoped to NonePlatform only. On NonePlatform there is no cloud provider to provision a real load balancer, so the configured hostname is authoritative and waiting for LB ingress status would block indefinitely. For all other platforms the existing behavior is preserved: the LB must be provisioned before the control plane rollout proceeds. This follows the same pattern used for shared ingress and IBMCloud platform-specific early returns in the same function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@csrwng PTAL |
|
@sdminonne: This pull request references Jira Issue OCPBUGS-87028, which is valid. 3 validation(s) were run on this bug
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. |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe
Recommendations
Evidence
|
|
@sdminonne: 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. |
3 participants
Summary
RouteHosthelper insupport/netutil/route.gothat resolves route hostnames consistently, checkingRoutePublishingStrategy.Hostnamefirst, then falling back tostatus.Conditionswith theroute.openshift.io/hostkey, and finally the legacyBaseDomain-based default.kas.ReconcileServiceStatus(andoauth.ReconcileServiceStatus) to checkLoadBalancer.Hostnamebefore callingCollectLBMessageIfNotProvisioned, so that clusters with a pre-configured LB hostname skip the cloud LB provisioning wait entirely.infra.ReconcileInfrastructureto use theRouteHosthelper for OAuth and Ignition route hostname resolution, ensuring consistent behavior across Route and LoadBalancer service types.Test plan
kas/service_test.go,oauth/service_test.go,infra/infra_test.go,netutil/route_test.gomake build && make testpass🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Tests