From fa9b6082b1c0c245041d182d9f56349d42c506b7 Mon Sep 17 00:00:00 2001 From: Cesar Wong Date: Wed, 28 Jan 2026 23:05:46 -0500 Subject: [PATCH] fix(cpo): Correct route labeling logic for HCP router infrastructure During upgrades from 4.18 to 4.19, an unexpected LoadBalancer service named "router" was created, blocking upgrades on platforms with limited LoadBalancer IPs. This occurred because the previous fix (f0f8b085c) used incorrect logic to determine when to create the HCP router infrastructure. Root Cause: Previous fixes (f0f8b085c and ee45d1bf4) treated route labeling as a per-service decision, checking if individual services had DNS hostnames. This created router infrastructure even when routes should use the management cluster router. For PublicAndPrivate clusters with KAS LoadBalancer + OAuth Route with hostname: - Old logic: IsPublicWithDNS() returned true (OAuth has DNS) - Result: Created router LB when not needed -> upgrade blocked - Routes got labeled inconsistently based on per-service DNS config Why the previous approach was wrong: 1. Route labeling should be a cluster-level infrastructure decision, not per-service 2. Router infrastructure availability is determined by KAS publishing strategy 3. Checking if ANY service has DNS doesn't indicate if HCP router exists 4. Could label routes for HCP router even when no HCP router infrastructure exists Correct Solution: Routes should be labeled for HCP router based on HCP router infrastructure availability, which is determined by the KAS publishing strategy: - Label routes for HCP router when: 1. Cluster uses PrivateLink (AWS PublicAndPrivate or Private), OR 2. Cluster is public with dedicated DNS for KAS (KAS uses Route with hostname) - For PrivateLink with KAS LoadBalancer: - External route (OAuth) uses management cluster router - Internal routes (Konnectivity and Ignition) are handled by HCP router Implementation: - Added util.LabelHCPRoutes() as single source of truth for labeling decisions - Updated all route reconciliation (OAuth, Konnectivity, Ignition) to use unified logic - Fixed router service creation to align with labeling: only create when routes need it - Removed incorrect per-service DNS functions (UseDedicatedDNSForOAuth, etc.) - Removed IsPublicWithDNS() functions that checked if ANY service had DNS - Removed validation that relied on IsPublicWithDNS() Changes: - support/util/visibility.go: Added LabelHCPRoutes(), removed IsPublicWithDNS functions - support/util/expose.go: Removed per-service DNS helper functions - hostedcontrolplane_controller.go: Use LabelHCPRoutes() for all route labeling - v2/ignitionserver/route.go: Use LabelHCPRoutes() - v2/router/component.go: Use LabelHCPRoutes() - hostedcluster_controller.go: Removed incorrect validation Result: For PublicAndPrivate + KAS LoadBalancer + OAuth with hostname: - OAuth route NOT labeled -> uses management cluster router - Internal Router LB created -> only used for internal routes (Konnectivity and Ignition) Co-Authored-By: Claude Sonnet 4.5 --- .../hostedcontrolplane_controller.go | 116 +++- .../hostedcontrolplane_controller_test.go | 10 +- ...PlaneComponents_ignition_server_route.yaml | 26 + ..._TestControlPlaneComponents_component.yaml | 3 + ...onents_component_TechPreviewNoUpgrade.yaml | 3 + ...PlaneComponents_ignition_server_route.yaml | 26 + .../v2/ignitionserver/route.go | 15 +- .../v2/ignitionserver_proxy/route.go | 2 +- .../hostedcontrolplane/v2/router/component.go | 31 +- .../v2/router/component_test.go | 228 ++++++++ .../hostedcluster/hostedcluster_controller.go | 4 - support/util/expose.go | 12 - support/util/route.go | 10 + support/util/route_test.go | 364 +++++++++++++ support/util/visibility.go | 101 +++- support/util/visibility_test.go | 504 ++++++++++++++++++ 16 files changed, 1381 insertions(+), 74 deletions(-) create mode 100644 control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml create mode 100644 control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml create mode 100644 control-plane-operator/controllers/hostedcontrolplane/v2/router/component_test.go diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index 688aedeb9d9..fd518bd8b56 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go @@ -1044,6 +1044,73 @@ func (r *HostedControlPlaneReconciler) reconcileCPOV2(ctx context.Context, hcp * } } + createOrUpdate := r.createOrUpdate(hcp) + // Reconcile default service account + r.Log.Info("Reconciling default service account") + if err := r.reconcileDefaultServiceAccount(ctx, hcp, createOrUpdate); err != nil { + return fmt.Errorf("failed to reconcile default service account: %w", err) + } + + // Reconcile PKI + if _, exists := hcp.Annotations[hyperv1.DisablePKIReconciliationAnnotation]; !exists { + r.Log.Info("Reconciling PKI") + if err := r.reconcilePKI(ctx, hcp, infraStatus, createOrUpdate); err != nil { + return fmt.Errorf("failed to reconcile PKI: %w", err) + } + } + + // Reconcile unmanaged etcd + if hcp.Spec.Etcd.ManagementType == hyperv1.Unmanaged { + r.Log.Info("Reconciling unmanaged Etcd") + if err := r.reconcileUnmanagedEtcd(ctx, hcp, createOrUpdate); err != nil { + return fmt.Errorf("failed to reconcile etcd: %w", err) + } + } + + if err := r.reconcileSREMetricsConfig(ctx, createOrUpdate, hcp.Namespace); err != nil { + return fmt.Errorf("failed to reconcile metrics config: %w", err) + } + + if routerv2.UseHCPRouter(hcp) { + if err := r.admitHCPManagedRoutes(ctx, hcp, infraStatus.InternalHCPRouterHost, infraStatus.ExternalHCPRouterHost); err != nil { + return fmt.Errorf("failed to admit HCP managed routes: %w", err) + } + if err := r.cleanupOldRouterResources(ctx, hcp); err != nil { + return fmt.Errorf("failed to cleanup old router resources: %w", err) + } + } + + if _, exists := hcp.Annotations[hyperv1.DisableIgnitionServerAnnotation]; !exists { + // Reconcile Ignition-server configs + r.Log.Info("Reconciling ignition-server configs") + if err := r.reconcileIgnitionServerConfigs(ctx, hcp, createOrUpdate); err != nil { + return fmt.Errorf("failed to reconcile ignition-server configs: %w", err) + } + } + + if util.HCPOAuthEnabled(hcp) { + // Reconcile kubeadmin password + r.Log.Info("Reconciling kubeadmin password secret") + explicitOauthConfig := hcp.Spec.Configuration != nil && hcp.Spec.Configuration.OAuth != nil + if err := r.reconcileKubeadminPassword(ctx, hcp, explicitOauthConfig, createOrUpdate); err != nil { + return fmt.Errorf("failed to ensure control plane: %w", err) + } + + // TODO: move this up with the rest of conditions reconciliation logic? + if err := r.reconcileValidIDPConfigurationCondition(ctx, hcp, releaseImageProvider, infraStatus.OAuthHost, infraStatus.OAuthPort); err != nil { + return fmt.Errorf("failed to reconcile ValidIDPConfiguration condition: %w", err) + } + } + + if err := r.cleanupClusterNetworkOperatorResources(ctx, hcp, r.ManagementClusterCapabilities.Has(capabilities.CapabilityRoute)); err != nil { + return fmt.Errorf("failed to reconcile cluster network operator operands: %w", err) + } + + r.Log.Info("Reconciling default security group") + if err := r.reconcileDefaultSecurityGroup(ctx, hcp); err != nil { + return fmt.Errorf("failed to reconcile default security group: %w", err) + } + cpContext := component.ControlPlaneContext{ Context: ctx, Client: r.Client, @@ -1069,19 +1136,6 @@ func (r *HostedControlPlaneReconciler) reconcileCPOV2(ctx context.Context, hcp * return utilerrors.NewAggregate(errs) } -// useHCPRouter returns true if a dedicated common router is created for a HCP to handle ingress for the managed endpoints. -// This is true when the API input specifies intent for the following: -// 1 - AWS endpointAccess is private somehow (i.e. publicAndPrivate or private) or is public and configured with external DNS. -// 2 - When 1 is true, we recommend (and automate via CLI) ServicePublishingStrategy to be "Route" for all endpoints but the KAS -// which needs a dedicated Service type LB external to be exposed if no external DNS is supported. -// Otherwise, the Routes use the management cluster Domain and resolve through the default ingress controller. -func useHCPRouter(hostedControlPlane *hyperv1.HostedControlPlane) bool { - if sharedingress.UseSharedIngress() { - return false - } - return util.IsPrivateHCP(hostedControlPlane) || util.IsPublicWithDNS(hostedControlPlane) -} - func IsStorageAndCSIManaged(hostedControlPlane *hyperv1.HostedControlPlane) bool { if hostedControlPlane.Spec.Platform.Type == hyperv1.IBMCloudPlatform || hostedControlPlane.Spec.Platform.Type == hyperv1.PowerVSPlatform { return false @@ -1229,7 +1283,7 @@ func (r *HostedControlPlaneReconciler) reconcile(ctx context.Context, hostedCont return nil } - if useHCPRouter(hostedControlPlane) { + if routerv2.UseHCPRouter(hostedControlPlane) { r.Log.Info("Reconciling router") if err := r.reconcileRouter(ctx, hostedControlPlane, releaseImageProvider, createOrUpdate); err != nil { return fmt.Errorf("failed to reconcile router: %w", err) @@ -1237,7 +1291,7 @@ func (r *HostedControlPlaneReconciler) reconcile(ctx context.Context, hostedCont } } - if useHCPRouter(hostedControlPlane) { + if routerv2.UseHCPRouter(hostedControlPlane) { if err := r.admitHCPManagedRoutes(ctx, hostedControlPlane, infraStatus.InternalHCPRouterHost, infraStatus.ExternalHCPRouterHost); err != nil { return fmt.Errorf("failed to admit HCP managed routes: %w", err) } @@ -1266,7 +1320,7 @@ func (r *HostedControlPlaneReconciler) reconcile(ctx context.Context, hostedCont config.OwnerRefFrom(hostedControlPlane), openShiftTrustedCABundleConfigMapForCPOExists, r.ReleaseProvider.GetMirroredReleaseImage(), - util.UseDedicatedDNSForIgnition(hostedControlPlane), + util.LabelHCPRoutes(hostedControlPlane), ); err != nil { return fmt.Errorf("failed to reconcile ignition server: %w", err) } @@ -1833,7 +1887,7 @@ func (r *HostedControlPlaneReconciler) reconcileKonnectivityServerService(ctx co if serviceStrategy.Route != nil { hostname = serviceStrategy.Route.Hostname } - return kas.ReconcileKonnectivityExternalRoute(konnectivityRoute, p.OwnerRef, hostname, r.DefaultIngressDomain, util.UseDedicatedDNSForKonnectivity(hcp)) + return kas.ReconcileKonnectivityExternalRoute(konnectivityRoute, p.OwnerRef, hostname, r.DefaultIngressDomain, util.LabelHCPRoutes(hcp)) }); err != nil { return fmt.Errorf("failed to reconcile Konnectivity server external route: %w", err) } @@ -1871,7 +1925,7 @@ func (r *HostedControlPlaneReconciler) reconcileOAuthServerService(ctx context.C if serviceStrategy.Route != nil { hostname = serviceStrategy.Route.Hostname } - return oauth.ReconcileExternalPublicRoute(oauthExternalPublicRoute, p.OwnerRef, hostname, r.DefaultIngressDomain, util.UseDedicatedDNSForOAuth(hcp)) + return oauth.ReconcileExternalPublicRoute(oauthExternalPublicRoute, p.OwnerRef, hostname, r.DefaultIngressDomain, util.LabelHCPRoutes(hcp)) }); err != nil { return fmt.Errorf("failed to reconcile OAuth external public route: %w", err) } @@ -1885,7 +1939,7 @@ func (r *HostedControlPlaneReconciler) reconcileOAuthServerService(ctx context.C // Reconcile the external private route if a hostname is specified if serviceStrategy.Route != nil && serviceStrategy.Route.Hostname != "" { if _, err := createOrUpdate(ctx, r.Client, oauthExternalPrivateRoute, func() error { - return oauth.ReconcileExternalPrivateRoute(oauthExternalPrivateRoute, p.OwnerRef, serviceStrategy.Route.Hostname, r.DefaultIngressDomain, util.UseDedicatedDNSForOAuth(hcp)) + return oauth.ReconcileExternalPrivateRoute(oauthExternalPrivateRoute, p.OwnerRef, serviceStrategy.Route.Hostname, r.DefaultIngressDomain, util.LabelHCPRoutes(hcp)) }); err != nil { return fmt.Errorf("failed to reconcile OAuth external private route: %w", err) } @@ -1948,10 +2002,10 @@ func (r *HostedControlPlaneReconciler) reconcileHCPRouterServices(ctx context.Co } // Create the Service type LB internal for private endpoints. pubSvc := manifests.RouterPublicService(hcp.Namespace) + privSvc := manifests.PrivateRouterService(hcp.Namespace) if util.IsPrivateHCP(hcp) { - svc := manifests.PrivateRouterService(hcp.Namespace) - if _, err := createOrUpdate(ctx, r.Client, svc, func() error { - return ingress.ReconcileRouterService(svc, true, true, hcp) + if _, err := createOrUpdate(ctx, r.Client, privSvc, func() error { + return ingress.ReconcileRouterService(privSvc, true, true, hcp) }); err != nil { return fmt.Errorf("failed to reconcile private router service: %w", err) } @@ -1968,15 +2022,27 @@ func (r *HostedControlPlaneReconciler) reconcileHCPRouterServices(ctx context.Co } } } + } else { + // Clean up stale private router Service when upgrading from buggy behavior + if _, err := util.DeleteIfNeeded(ctx, r.Client, privSvc); err != nil { + return fmt.Errorf("failed to delete private router service: %w", err) + } } - // When Public access endpoint we need to create a Service type LB external. - if util.IsPublicWithDNS(hcp) { + // When Public access endpoint AND routes use HCP router, create a Service type LB external. + // This ensures we only create public router infrastructure when routes are labeled for it. + if util.IsPublicHCP(hcp) && util.LabelHCPRoutes(hcp) { if _, err := createOrUpdate(ctx, r.Client, pubSvc, func() error { return ingress.ReconcileRouterService(pubSvc, false, util.IsPrivateHCP(hcp), hcp) }); err != nil { return fmt.Errorf("failed to reconcile router service: %w", err) } + } else if util.IsPublicHCP(hcp) { + // Clean up stale public router Service when LabelHCPRoutes becomes false + // (e.g., when switching from KAS Route to KAS LoadBalancer or upgrading from buggy behavior) + if _, err := util.DeleteIfNeeded(ctx, r.Client, pubSvc); err != nil { + return fmt.Errorf("failed to delete public router service: %w", err) + } } return nil } @@ -2082,7 +2148,7 @@ func (r *HostedControlPlaneReconciler) reconcileInternalRouterServiceStatus(ctx } func (r *HostedControlPlaneReconciler) reconcileExternalRouterServiceStatus(ctx context.Context, hcp *hyperv1.HostedControlPlane) (host string, needed bool, message string, err error) { - if !util.IsPublicWithDNS(hcp) || sharedingress.UseSharedIngress() || hcp.Spec.Platform.Type == hyperv1.IBMCloudPlatform { + if !util.IsPublicHCP(hcp) || !util.LabelHCPRoutes(hcp) || sharedingress.UseSharedIngress() || hcp.Spec.Platform.Type == hyperv1.IBMCloudPlatform { return } return r.reconcileRouterServiceStatus(ctx, manifests.RouterPublicService(hcp.Namespace), events.NewMessageCollector(ctx, r.Client)) diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go index 3642d3fb216..7bee46648ad 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go @@ -24,6 +24,7 @@ import ( ignitionproxyv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy" kasv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/kas" oapiv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/oapi" + routerv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/router" "github.com/openshift/hypershift/support/api" autoscalercommon "github.com/openshift/hypershift/support/autoscaler" fakecapabilities "github.com/openshift/hypershift/support/capabilities/fake" @@ -282,11 +283,8 @@ func TestReconcileOAuthService(t *testing.T) { oauthExternalPublicRoute := func(m ...func(*routev1.Route)) routev1.Route { route := routev1.Route{ ObjectMeta: metav1.ObjectMeta{ - Namespace: targetNamespace, - Name: "oauth", - Labels: map[string]string{ - "hypershift.openshift.io/hosted-control-plane": targetNamespace, - }, + Namespace: targetNamespace, + Name: "oauth", OwnerReferences: []metav1.OwnerReference{ownerRef}, }, Spec: routev1.RouteSpec{ @@ -1577,7 +1575,7 @@ func TestReconcileRouter(t *testing.T) { } releaseInfo := &releaseinfo.ReleaseImage{ImageStream: &imagev1.ImageStream{}} - if useHCPRouter(hcp) { + if routerv2.UseHCPRouter(hcp) { if err := r.reconcileRouter(ctx, hcp, imageprovider.New(releaseInfo), controllerutil.CreateOrUpdate); err != nil { t.Fatalf("reconcileRouter failed: %v", err) } diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml new file mode 100644 index 00000000000..0de7a876608 --- /dev/null +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml @@ -0,0 +1,26 @@ +apiVersion: route.openshift.io/v1 +kind: Route +metadata: + creationTimestamp: null + labels: + hypershift.openshift.io/hosted-control-plane: hcp-namespace + name: ignition-server + namespace: hcp-namespace + ownerReferences: + - apiVersion: hypershift.openshift.io/v1beta1 + blockOwnerDeletion: true + controller: true + kind: HostedControlPlane + name: hcp + uid: "" + resourceVersion: "1" +spec: + tls: + insecureEdgeTerminationPolicy: None + termination: passthrough + to: + kind: Service + name: ignition-server-proxy + weight: null + wildcardPolicy: None +status: {} diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component.yaml index 86168a59189..230ce7c9b80 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component.yaml @@ -31,6 +31,9 @@ status: - group: rbac.authorization.k8s.io kind: RoleBinding name: ignition-server + - group: route.openshift.io + kind: Route + name: ignition-server - group: "" kind: Service name: ignition-server diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component_TechPreviewNoUpgrade.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component_TechPreviewNoUpgrade.yaml index 86168a59189..230ce7c9b80 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component_TechPreviewNoUpgrade.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component_TechPreviewNoUpgrade.yaml @@ -31,6 +31,9 @@ status: - group: rbac.authorization.k8s.io kind: RoleBinding name: ignition-server + - group: route.openshift.io + kind: Route + name: ignition-server - group: "" kind: Service name: ignition-server diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml new file mode 100644 index 00000000000..0de7a876608 --- /dev/null +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml @@ -0,0 +1,26 @@ +apiVersion: route.openshift.io/v1 +kind: Route +metadata: + creationTimestamp: null + labels: + hypershift.openshift.io/hosted-control-plane: hcp-namespace + name: ignition-server + namespace: hcp-namespace + ownerReferences: + - apiVersion: hypershift.openshift.io/v1beta1 + blockOwnerDeletion: true + controller: true + kind: HostedControlPlane + name: hcp + uid: "" + resourceVersion: "1" +spec: + tls: + insecureEdgeTerminationPolicy: None + termination: passthrough + to: + kind: Service + name: ignition-server-proxy + weight: null + wildcardPolicy: None +status: {} diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go index e40c73035f3..80de28e71cb 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go @@ -11,10 +11,6 @@ import ( ) func routePredicate(cpContext component.WorkloadContext) bool { - if cpContext.HCP.Spec.Platform.Type != hyperv1.IBMCloudPlatform { - return false - } - strategy := util.ServicePublishingStrategyByTypeForHCP(cpContext.HCP, hyperv1.Ignition) if strategy == nil { return false @@ -23,9 +19,16 @@ func routePredicate(cpContext component.WorkloadContext) bool { } func (ign *ignitionServer) adaptRoute(cpContext component.WorkloadContext, route *routev1.Route) error { + serviceName := "ignition-server-proxy" + // For IBM Cloud, we don't deploy the ignition server proxy. + // Instead, the ignition server service is directly exposed. + if cpContext.HCP.Spec.Platform.Type == hyperv1.IBMCloudPlatform { + serviceName = "ignition-server" + } + hcp := cpContext.HCP if util.IsPrivateHCP(hcp) { - return util.ReconcileInternalRoute(route, hcp.Name, ComponentName) + return util.ReconcileInternalRoute(route, hcp.Name, serviceName) } strategy := util.ServicePublishingStrategyByTypeForHCP(hcp, hyperv1.Ignition) @@ -37,5 +40,5 @@ func (ign *ignitionServer) adaptRoute(cpContext component.WorkloadContext, route if strategy.Route != nil { hostname = strategy.Route.Hostname } - return util.ReconcileExternalRoute(route, hostname, ign.defaultIngressDomain, ComponentName, util.UseDedicatedDNSForIgnition(hcp)) + return util.ReconcileExternalRoute(route, hostname, ign.defaultIngressDomain, serviceName, util.LabelHCPRoutes(hcp)) } diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/route.go b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/route.go index 28a403548a2..aee36ac7247 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/route.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/route.go @@ -33,5 +33,5 @@ func (ign *ignitionServerProxy) adaptRoute(cpContext component.WorkloadContext, if strategy.Route != nil { hostname = strategy.Route.Hostname } - return util.ReconcileExternalRoute(route, hostname, ign.defaultIngressDomain, ComponentName, util.UseDedicatedDNSForIgnition(hcp)) + return util.ReconcileExternalRoute(route, hostname, ign.defaultIngressDomain, ComponentName, util.LabelHCPRoutes(hcp)) } diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/router/component.go b/control-plane-operator/controllers/hostedcontrolplane/v2/router/component.go index d93dad1e4c1..acea4173c59 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/router/component.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/router/component.go @@ -1,6 +1,7 @@ package router import ( + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" oapiv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/oapi" "github.com/openshift/hypershift/hypershift-operator/controllers/sharedingress" component "github.com/openshift/hypershift/support/controlplane-component" @@ -46,15 +47,27 @@ func NewComponent() component.ControlPlaneComponent { Build() } -// useHCPRouter returns true if a dedicated common router is created for a HCP to handle ingress for the managed endpoints. -// This is true when the API input specifies intent for the following: -// 1 - AWS endpointAccess is private somehow (i.e. publicAndPrivate or private) or is public and configured with external DNS. -// 2 - When 1 is true, we recommend (and automate via CLI) ServicePublishingStrategy to be "Route" for all endpoints but the KAS -// which needs a dedicated Service type LB external to be exposed if no external DNS is supported. -// Otherwise, the Routes use the management cluster Domain and resolve through the default ingress controller. -func useHCPRouter(cpContext component.WorkloadContext) (bool, error) { +// UseHCPRouter returns true when the HCP routes should be served by a dedicated +// HCP router, as determined by util.LabelHCPRoutes. This occurs when: +// 1. The cluster has no public internet access (Private endpoint access), OR +// 2. The cluster has public internet access (Public or PublicAndPrivate endpoint access) +// but uses a dedicated Route for KAS DNS (rather than a LoadBalancer) +// +// Excludes shared ingress configurations and IBM Cloud platform. +func UseHCPRouter(hcp *hyperv1.HostedControlPlane) bool { if sharedingress.UseSharedIngress() { - return false, nil + return false + } + if hcp.Spec.Platform.Type == hyperv1.IBMCloudPlatform { + return false } - return util.IsPrivateHCP(cpContext.HCP) || util.IsPublicWithDNS(cpContext.HCP), nil + // Router infrastructure is needed when: + // 1. Cluster has private access (Private or PublicAndPrivate) - for internal routes, OR + // 2. External routes are labeled for HCP router (Public with KAS DNS) + return util.IsPrivateHCP(hcp) || util.LabelHCPRoutes(hcp) +} + +// useHCPRouter is an adapter for the component predicate interface. +func useHCPRouter(cpContext component.WorkloadContext) (bool, error) { + return UseHCPRouter(cpContext.HCP), nil } diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/router/component_test.go b/control-plane-operator/controllers/hostedcontrolplane/v2/router/component_test.go new file mode 100644 index 00000000000..b19c09d368e --- /dev/null +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/router/component_test.go @@ -0,0 +1,228 @@ +package router + +import ( + "testing" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/stretchr/testify/assert" +) + +func TestUseHCPRouter(t *testing.T) { + testsCases := []struct { + name string + hcp *hyperv1.HostedControlPlane + expectedResult bool + }{ + { + name: "Provider is IBM Cloud", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.IBMCloudPlatform, + }, + }, + }, + expectedResult: false, + }, + { + name: "Provider is NonePlatform, services are exposed with Routes", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.NonePlatform, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "cluster1.api.tenant1.com", + }, + }, + }, + { + Service: hyperv1.Konnectivity, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "cluster1.tunnel.tenant1.com", + }, + }, + }, + { + Service: hyperv1.Ignition, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "cluster1.ignition.tenant1.com", + }, + }, + }, + { + Service: hyperv1.OAuthServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "cluster1.oauth.tenant1.com", + }, + }, + }, + }, + }, + }, + expectedResult: true, + }, + { + name: "Provider is AWS, Public and Private, KAS Loadbalancer", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.PublicAndPrivate, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + expectedResult: true, // Router infrastructure needed for internal routes + }, + { + name: "Provider is AWS, Public and Private, KAS Route", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.PublicAndPrivate, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "cluster1.api.tenant1.com", + }, + }, + }, + }, + }, + }, + expectedResult: true, + }, + { + name: "Provider is AWS, Private", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.Private, + }, + }, + }, + }, + expectedResult: true, + }, + { + name: "Provider is Agent, KAS LoadBalancer", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AgentPlatform, + Agent: &hyperv1.AgentPlatformSpec{ + AgentNamespace: "agent-ns", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + expectedResult: false, // Router infrastructure not needed when KAS uses LoadBalancer + }, + { + name: "Provider is Agent, KAS Route", + hcp: &hyperv1.HostedControlPlane{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant1", + Name: "cluster1", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AgentPlatform, + Agent: &hyperv1.AgentPlatformSpec{ + AgentNamespace: "agent-ns", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "cluster1.api.tenant1.com", + }, + }, + }, + }, + }, + }, + expectedResult: true, + }, + } + for _, tc := range testsCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expectedResult, UseHCPRouter(tc.hcp)) + }) + } +} diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go index a658fe62831..d741b1152ed 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go @@ -4688,10 +4688,6 @@ func (r *HostedClusterReconciler) validateAWSConfig(hc *hyperv1.HostedCluster) e if !hyperutil.UseDedicatedDNSForKASByHC(hc) && kasPublishingStrategy.Type != hyperv1.LoadBalancer { errs = append(errs, fmt.Errorf("service type %v with publishing strategy %v is not supported, use Route or LoadBalancer", hyperv1.APIServer, kasPublishingStrategy.Type)) } - // When using dedicated DNS, the KAS should be exposed as Route. - if hyperutil.IsPublicWithDNSByHC(hc) && hyperutil.IsLBKASByHC(hc) { - errs = append(errs, fmt.Errorf("service type %v with publishing strategy %v is not supported when any service specifies external DNS, use Route", hyperv1.APIServer, kasPublishingStrategy.Type)) - } } return utilerrors.NewAggregate(errs) diff --git a/support/util/expose.go b/support/util/expose.go index 9ef85267439..485fae4c7c9 100644 --- a/support/util/expose.go +++ b/support/util/expose.go @@ -31,18 +31,6 @@ func UseDedicatedDNSForKAS(hcp *hyperv1.HostedControlPlane) bool { return UseDedicatedDNS(hcp, hyperv1.APIServer) } -func UseDedicatedDNSForOAuth(hcp *hyperv1.HostedControlPlane) bool { - return UseDedicatedDNS(hcp, hyperv1.OAuthServer) -} - -func UseDedicatedDNSForKonnectivity(hcp *hyperv1.HostedControlPlane) bool { - return UseDedicatedDNS(hcp, hyperv1.Konnectivity) -} - -func UseDedicatedDNSForIgnition(hcp *hyperv1.HostedControlPlane) bool { - return UseDedicatedDNS(hcp, hyperv1.Ignition) -} - func UseDedicatedDNS(hcp *hyperv1.HostedControlPlane, svcType hyperv1.ServiceType) bool { svc := ServicePublishingStrategyByTypeForHCP(hcp, svcType) return IsRoute(hcp, svcType) && diff --git a/support/util/route.go b/support/util/route.go index d5665a07886..7182d3bcaab 100644 --- a/support/util/route.go +++ b/support/util/route.go @@ -90,6 +90,8 @@ func hash(s string) string { func ReconcileExternalRoute(route *routev1.Route, hostname string, defaultIngressDomain string, serviceName string, labelHCPRoutes bool) error { if labelHCPRoutes { AddHCPRouteLabel(route) + } else { + RemoveHCPRouteLabel(route) } if hostname != "" { route.Spec.Host = hostname @@ -138,6 +140,14 @@ func AddHCPRouteLabel(target crclient.Object) { target.SetLabels(labels) } +func RemoveHCPRouteLabel(target crclient.Object) { + labels := target.GetLabels() + if labels != nil { + delete(labels, HCPRouteLabel) + target.SetLabels(labels) + } +} + func AddInternalRouteLabel(target crclient.Object) { labels := target.GetLabels() if labels == nil { diff --git a/support/util/route_test.go b/support/util/route_test.go index fd09810d8a2..ff9d4f7c422 100644 --- a/support/util/route_test.go +++ b/support/util/route_test.go @@ -5,6 +5,9 @@ import ( "math/rand" "testing" + routev1 "github.com/openshift/api/route/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kvalidation "k8s.io/apimachinery/pkg/util/validation" ) @@ -102,3 +105,364 @@ func randSeq(n int) string { } return string(b) } + +func TestReconcileExternalRoute(t *testing.T) { + tests := []struct { + name string + route *routev1.Route + hostname string + defaultDomain string + serviceName string + labelHCPRoutes bool + expectedLabels map[string]string + expectedHost string + expectedService string + }{ + { + name: "When labelHCPRoutes is true and route has no labels, it should add HCP label", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + }, + hostname: "test.example.com", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: true, + expectedLabels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + expectedHost: "test.example.com", + expectedService: "test-service", + }, + { + name: "When labelHCPRoutes is true and route has existing labels, it should add HCP label", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + "existing-label": "existing-value", + }, + }, + }, + hostname: "test.example.com", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: true, + expectedLabels: map[string]string{ + "existing-label": "existing-value", + HCPRouteLabel: "test-namespace", + }, + expectedHost: "test.example.com", + expectedService: "test-service", + }, + { + name: "When labelHCPRoutes is false and route has no labels, it should not add HCP label", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + }, + hostname: "test.example.com", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: false, + expectedLabels: nil, + expectedHost: "test.example.com", + expectedService: "test-service", + }, + { + name: "When labelHCPRoutes is false and route has HCP label, it should remove HCP label", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + HCPRouteLabel: "test-namespace", + "existing-label": "existing-value", + }, + }, + }, + hostname: "test.example.com", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: false, + expectedLabels: map[string]string{ + "existing-label": "existing-value", + }, + expectedHost: "test.example.com", + expectedService: "test-service", + }, + { + name: "When labelHCPRoutes is false and route has only HCP label, it should remove HCP label and leave empty map", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + }, + }, + hostname: "test.example.com", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: false, + expectedLabels: map[string]string{}, + expectedHost: "test.example.com", + expectedService: "test-service", + }, + { + name: "When hostname is empty and route name is short, it should leave hostname empty for default generation", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + }, + hostname: "", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: true, + expectedLabels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + expectedHost: "", // ShortenRouteHostnameIfNeeded returns empty when name is short enough + expectedService: "test-service", + }, + { + name: "When hostname is empty and route already has a hostname, it should preserve existing hostname", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: routev1.RouteSpec{ + Host: "existing.example.com", + }, + }, + hostname: "", + defaultDomain: "example.com", + serviceName: "test-service", + labelHCPRoutes: true, + expectedLabels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + expectedHost: "existing.example.com", + expectedService: "test-service", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ReconcileExternalRoute(tt.route, tt.hostname, tt.defaultDomain, tt.serviceName, tt.labelHCPRoutes) + if err != nil { + t.Fatalf("ReconcileExternalRoute() error = %v, wantErr false", err) + } + + // Check labels + if tt.expectedLabels == nil { + if len(tt.route.Labels) != 0 { + t.Errorf("Expected no labels, but got %v", tt.route.Labels) + } + } else { + if len(tt.route.Labels) != len(tt.expectedLabels) { + t.Errorf("Labels length mismatch: got %d, want %d. Got: %v, Want: %v", len(tt.route.Labels), len(tt.expectedLabels), tt.route.Labels, tt.expectedLabels) + } + for k, v := range tt.expectedLabels { + if got, ok := tt.route.Labels[k]; !ok || got != v { + t.Errorf("Label %s: got %v, want %v", k, got, v) + } + } + // Check that HCP label is not present when not expected + if _, ok := tt.expectedLabels[HCPRouteLabel]; !ok { + if _, hasLabel := tt.route.Labels[HCPRouteLabel]; hasLabel { + t.Errorf("Expected HCP label to be removed, but it still exists") + } + } + } + + // Check hostname + if tt.route.Spec.Host != tt.expectedHost { + t.Errorf("Host: got %v, want %v", tt.route.Spec.Host, tt.expectedHost) + } + + // Check service name + if tt.route.Spec.To.Name != tt.expectedService { + t.Errorf("Service name: got %v, want %v", tt.route.Spec.To.Name, tt.expectedService) + } + + // Check TLS config + if tt.route.Spec.TLS == nil { + t.Errorf("Expected TLS config to be set") + } else { + if tt.route.Spec.TLS.Termination != routev1.TLSTerminationPassthrough { + t.Errorf("TLS termination: got %v, want %v", tt.route.Spec.TLS.Termination, routev1.TLSTerminationPassthrough) + } + if tt.route.Spec.TLS.InsecureEdgeTerminationPolicy != routev1.InsecureEdgeTerminationPolicyNone { + t.Errorf("Insecure edge termination policy: got %v, want %v", tt.route.Spec.TLS.InsecureEdgeTerminationPolicy, routev1.InsecureEdgeTerminationPolicyNone) + } + } + }) + } +} + +func TestAddHCPRouteLabel(t *testing.T) { + tests := []struct { + name string + route *routev1.Route + expectedLabels map[string]string + }{ + { + name: "When route has no labels, it should add HCP label", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + }, + expectedLabels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + }, + { + name: "When route has existing labels, it should add HCP label without removing existing ones", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + "existing-label": "existing-value", + }, + }, + }, + expectedLabels: map[string]string{ + "existing-label": "existing-value", + HCPRouteLabel: "test-namespace", + }, + }, + { + name: "When route already has HCP label, it should update it", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + HCPRouteLabel: "old-namespace", + }, + }, + }, + expectedLabels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + AddHCPRouteLabel(tt.route) + + if len(tt.route.Labels) != len(tt.expectedLabels) { + t.Errorf("Labels length mismatch: got %d, want %d. Got: %v, Want: %v", len(tt.route.Labels), len(tt.expectedLabels), tt.route.Labels, tt.expectedLabels) + } + for k, v := range tt.expectedLabels { + if got, ok := tt.route.Labels[k]; !ok || got != v { + t.Errorf("Label %s: got %v, want %v", k, got, v) + } + } + }) + } +} + +func TestRemoveHCPRouteLabel(t *testing.T) { + tests := []struct { + name string + route *routev1.Route + expectedLabels map[string]string + }{ + { + name: "When route has no labels, it should not error", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + }, + expectedLabels: nil, + }, + { + name: "When route has HCP label only, it should remove it and leave empty map", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + HCPRouteLabel: "test-namespace", + }, + }, + }, + expectedLabels: map[string]string{}, + }, + { + name: "When route has HCP label and other labels, it should remove only HCP label", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + HCPRouteLabel: "test-namespace", + "existing-label": "existing-value", + "another-label": "another-value", + }, + }, + }, + expectedLabels: map[string]string{ + "existing-label": "existing-value", + "another-label": "another-value", + }, + }, + { + name: "When route does not have HCP label but has other labels, it should not modify labels", + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + Labels: map[string]string{ + "existing-label": "existing-value", + }, + }, + }, + expectedLabels: map[string]string{ + "existing-label": "existing-value", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RemoveHCPRouteLabel(tt.route) + + if tt.expectedLabels == nil { + if len(tt.route.Labels) != 0 { + t.Errorf("Expected no labels, but got %v", tt.route.Labels) + } + } else { + if len(tt.route.Labels) != len(tt.expectedLabels) { + t.Errorf("Labels length mismatch: got %d, want %d. Got: %v, Want: %v", len(tt.route.Labels), len(tt.expectedLabels), tt.route.Labels, tt.expectedLabels) + } + for k, v := range tt.expectedLabels { + if got, ok := tt.route.Labels[k]; !ok || got != v { + t.Errorf("Label %s: got %v, want %v", k, got, v) + } + } + // Ensure HCP label is not present + if _, hasLabel := tt.route.Labels[HCPRouteLabel]; hasLabel { + t.Errorf("Expected HCP label to be removed, but it still exists") + } + } + }) + } +} diff --git a/support/util/visibility.go b/support/util/visibility.go index 9c5918d3b0d..9fb085bebda 100644 --- a/support/util/visibility.go +++ b/support/util/visibility.go @@ -37,16 +37,95 @@ func IsPublicHC(hc *hyperv1.HostedCluster) bool { return access == hyperv1.PublicAndPrivate || access == hyperv1.Public } -func IsPublicWithDNS(hcp *hyperv1.HostedControlPlane) bool { - return IsPublicHCP(hcp) && (UseDedicatedDNS(hcp, hyperv1.APIServer) || - UseDedicatedDNS(hcp, hyperv1.OAuthServer) || - UseDedicatedDNS(hcp, hyperv1.Konnectivity) || - UseDedicatedDNS(hcp, hyperv1.Ignition)) -} +// LabelHCPRoutes determines if routes should be labeled for admission by the HCP router. +// Routes with the label "hypershift.openshift.io/hosted-control-plane" are served by a +// dedicated HCP router (HAProxy deployment in the HCP namespace). Routes without this label +// are served by the management cluster's default OpenShift ingress controller. +// +// This function is the single source of truth for route labeling decisions and is called by: +// - OAuth route reconciliation (external public/private routes) +// - Konnectivity route reconciliation (external routes) +// - Ignition server route reconciliation (external routes) +// - Router component predicate (determines if router Deployment/ConfigMap/PDB are created) +// - Router service creation (determines if public router LoadBalancer service is created) +// +// The HCP router infrastructure (Deployment, Services) is created when routes need to be labeled. +// This ensures routes and router services stay synchronized. +// +// # Platform-Specific Behavior +// +// AWS Platform: +// - Private: Always labels routes (no public access) +// - PublicAndPrivate + KAS LoadBalancer: Does NOT label external routes (uses mgmt cluster router) +// - PublicAndPrivate + KAS Route: Labels routes (uses HCP router for all routes) +// - Public + KAS LoadBalancer: Does NOT label routes (uses mgmt cluster router) +// - Public + KAS Route: Labels routes (uses HCP router) +// +// Agent Platform (bare metal): +// - No EndpointAccess field (no Private/PublicAndPrivate concept) +// - Labels routes ONLY when KAS uses Route with explicit hostname +// - KAS LoadBalancer/NodePort: Does NOT label routes (uses mgmt cluster router) +// +// KubeVirt, OpenStack, None Platforms: +// - Same behavior as Agent platform +// - Labels routes ONLY when KAS uses Route with explicit hostname +// +// IBM Cloud Platform: +// - Never labels routes (uses different routing mechanism) +// +// # Internal Routes +// +// Note that internal routes (*.apps..hypershift.local) are ALWAYS labeled for +// HCP router regardless of this function's return value. This function only controls +// EXTERNAL route labeling. Internal routes are handled separately in ReconcileInternalRoute(). +// +// # Architecture Reference +// +// For complete details on the HCP ingress architecture, see HCP_INGRESS_ARCHITECTURE.md +// in the repository root, which documents the full decision flow, code references, and +// interaction between route labeling and router service creation. +// +// Returns true when routes should be labeled for HCP router; false when routes should +// use the management cluster router. +func LabelHCPRoutes(hcp *hyperv1.HostedControlPlane) bool { + switch hcp.Spec.Platform.Type { + case hyperv1.AWSPlatform: + // AWS supports endpoint access modes (Private/PublicAndPrivate/Public). + // Label routes for HCP router when: + // 1. Cluster has no public access (Private-only), OR + // 2. Public cluster with dedicated DNS for KAS (KAS uses Route with hostname) + // + // For PublicAndPrivate clusters using LoadBalancer for KAS: + // - Internal routes (Konnectivity, Ignition) are served by internal HCP router + // - External routes (OAuth) use the management cluster router (no public HCP router needed) + // This avoids creating an unnecessary public LoadBalancer service. + return !IsPublicHCP(hcp) || UseDedicatedDNSForKAS(hcp) + + case hyperv1.AgentPlatform, hyperv1.KubevirtPlatform, hyperv1.OpenStackPlatform, hyperv1.NonePlatform: + // These platforms do not have endpoint access mode concepts (no Private/PublicAndPrivate). + // Label routes for HCP router ONLY when KAS explicitly uses Route with a hostname. + // + // This prevents creating HCP router infrastructure when: + // - KAS uses LoadBalancer (routes should use management cluster router) + // - KAS uses NodePort (routes should use management cluster router) + // + // When KAS uses Route with hostname, all routes are labeled for HCP router to ensure + // consistent routing through dedicated infrastructure. + return UseDedicatedDNSForKAS(hcp) + + case hyperv1.IBMCloudPlatform: + // IBM Cloud uses a different routing mechanism (shared ingress with HAProxy and + // kube-apiserver-proxy on worker nodes). Never use HCP router. + return false -func IsPublicWithDNSByHC(hc *hyperv1.HostedCluster) bool { - return IsPublicHC(hc) && (UseDedicatedDNSByHC(hc, hyperv1.APIServer) || - UseDedicatedDNSByHC(hc, hyperv1.OAuthServer) || - UseDedicatedDNSByHC(hc, hyperv1.Konnectivity) || - UseDedicatedDNSByHC(hc, hyperv1.Ignition)) + case hyperv1.PowerVSPlatform: + // PowerVS (IBM Cloud Power Virtual Servers) follows the same pattern as other + // platforms without endpoint access modes. + return UseDedicatedDNSForKAS(hcp) + + default: + // Conservative default for unknown platforms: do not create HCP router infrastructure. + // Routes will use the management cluster router. + return false + } } diff --git a/support/util/visibility_test.go b/support/util/visibility_test.go index 9dfb447ba46..ff88aadf4dd 100644 --- a/support/util/visibility_test.go +++ b/support/util/visibility_test.go @@ -165,3 +165,507 @@ func TestIsPublicHCP(t *testing.T) { }) } } + +func TestLabelHCPRoutes(t *testing.T) { + tests := []struct { + name string + hcp *hyperv1.HostedControlPlane + want bool + }{ + // AWS Platform Tests + { + name: "When AWS cluster is Private, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.Private, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: true, + }, + { + name: "When AWS cluster is PublicAndPrivate with KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.PublicAndPrivate, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When AWS cluster is PublicAndPrivate with KAS Route and hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.PublicAndPrivate, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + { + name: "When AWS cluster is Public with KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.Public, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When AWS cluster is Public with KAS Route and hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.Public, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + { + name: "When AWS cluster is Public with KAS Route but no hostname, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + EndpointAccess: hyperv1.Public, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + }, + }, + }, + }, + }, + want: false, + }, + + // Agent Platform Tests (Bare Metal) - Critical for OCPBUGS-70152 + { + name: "When Agent cluster has KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AgentPlatform, + Agent: &hyperv1.AgentPlatformSpec{ + AgentNamespace: "agent-ns", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When Agent cluster has KAS Route with hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AgentPlatform, + Agent: &hyperv1.AgentPlatformSpec{ + AgentNamespace: "agent-ns", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.agent.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + { + name: "When Agent cluster has KAS NodePort, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AgentPlatform, + Agent: &hyperv1.AgentPlatformSpec{ + AgentNamespace: "agent-ns", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.NodePort, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When Agent cluster has OAuth Route with hostname but KAS LoadBalancer, it should not label routes for HCP router (OCPBUGS-70152 bug scenario)", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AgentPlatform, + Agent: &hyperv1.AgentPlatformSpec{ + AgentNamespace: "agent-ns", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + { + Service: hyperv1.OAuthServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "oauth.agent.example.com", + }, + }, + }, + }, + }, + }, + want: false, // Should NOT create HCP router - this was the bug + }, + + // KubeVirt Platform Tests + { + name: "When KubeVirt cluster has KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.KubevirtPlatform, + Kubevirt: &hyperv1.KubevirtPlatformSpec{ + GenerateID: "kubevirt-cluster", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When KubeVirt cluster has KAS Route with hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.KubevirtPlatform, + Kubevirt: &hyperv1.KubevirtPlatformSpec{ + GenerateID: "kubevirt-cluster", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.kubevirt.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + + // OpenStack Platform Tests + { + name: "When OpenStack cluster has KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.OpenStackPlatform, + OpenStack: &hyperv1.OpenStackPlatformSpec{ + IdentityRef: hyperv1.OpenStackIdentityReference{ + Name: "openstack-creds", + }, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When OpenStack cluster has KAS Route with hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.OpenStackPlatform, + OpenStack: &hyperv1.OpenStackPlatformSpec{ + IdentityRef: hyperv1.OpenStackIdentityReference{ + Name: "openstack-creds", + }, + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.openstack.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + + // IBM Cloud Platform Tests + { + name: "When IBM Cloud cluster has KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.IBMCloudPlatform, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When IBM Cloud cluster has KAS Route with hostname, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.IBMCloudPlatform, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.ibmcloud.example.com", + }, + }, + }, + }, + }, + }, + want: false, // IBM Cloud never uses HCP router + }, + + // PowerVS Platform Tests + { + name: "When PowerVS cluster has KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.PowerVSPlatform, + PowerVS: &hyperv1.PowerVSPlatformSpec{ + AccountID: "test-account", + CISInstanceCRN: "test-crn", + ResourceGroup: "test-rg", + Region: "us-south", + Zone: "us-south-1", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When PowerVS cluster has KAS Route with hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.PowerVSPlatform, + PowerVS: &hyperv1.PowerVSPlatformSpec{ + AccountID: "test-account", + CISInstanceCRN: "test-crn", + ResourceGroup: "test-rg", + Region: "us-south", + Zone: "us-south-1", + }, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.powervs.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + + // None Platform Tests + { + name: "When None platform cluster has KAS LoadBalancer, it should not label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.NonePlatform, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.LoadBalancer, + }, + }, + }, + }, + }, + want: false, + }, + { + name: "When None platform cluster has KAS Route with hostname, it should label routes for HCP router", + hcp: &hyperv1.HostedControlPlane{ + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.NonePlatform, + }, + Services: []hyperv1.ServicePublishingStrategyMapping{ + { + Service: hyperv1.APIServer, + ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{ + Type: hyperv1.Route, + Route: &hyperv1.RoutePublishingStrategy{ + Hostname: "api.none.example.com", + }, + }, + }, + }, + }, + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := LabelHCPRoutes(tt.hcp) + if got != tt.want { + t.Errorf("LabelHCPRoutes() = %v, want %v", got, tt.want) + } + }) + } +}