Skip to content

OCPBUGS-75932: [release-4.19] fix(cpo): Correct route labeling logic for HCP router infrastructure #7643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
csrwng wants to merge 1 commit into
base: release-4.19
Choose a base branch
from
Draft

OCPBUGS-75932: [release-4.19] fix(cpo): Correct route labeling logic for HCP router infrastructure #7643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 91 additions & 25 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -1229,15 +1283,15 @@ 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)
}
}
}

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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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))
Expand Down
10 changes: 4 additions & 6 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
}
Expand Down
26 changes: 26 additions & 0 deletions ...ver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: {}
3 changes: 3 additions & 0 deletions ...ontrolplane/testdata/ignition-server/zz_fixture_TestControlPlaneComponents_component.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions ...ignition-server/zz_fixture_TestControlPlaneComponents_component_TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 26 additions & 0 deletions ...testdata/ignition-server/zz_fixture_TestControlPlaneComponents_ignition_server_route.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: {}
15 changes: 9 additions & 6 deletions control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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))
}
2 changes: 1 addition & 1 deletion control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
31 changes: 22 additions & 9 deletions control-plane-operator/controllers/hostedcontrolplane/v2/router/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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
}
Loading