From eb0f37cc8c762a411f3962c91f36c0d494444263 Mon Sep 17 00:00:00 2001 From: Mulham Raee Date: Mon, 16 Dec 2024 11:09:39 +0100 Subject: [PATCH 1/2] Move oauth kubeadminsecret hash annotation logic from HCCO to CPO This avoids the conflict between HCCO and CPO reconciling the same deployment. All HCP components reconciliation should be done in the CPO --- .../hostedcontrolplane_controller.go | 16 ++++----- .../hostedcontrolplane/oauth/deployment.go | 32 ++++++++++++----- .../controllers/resources/resources.go | 36 ++++--------------- .../controllers/resources/resources_test.go | 36 ++----------------- test/e2e/util/oauth.go | 6 ++-- 5 files changed, 42 insertions(+), 84 deletions(-) diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index dd76b70691..50813a41f4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go @@ -1201,6 +1201,13 @@ func (r *HostedControlPlaneReconciler) reconcile(ctx context.Context, hostedCont } if util.HCPOAuthEnabled(hostedControlPlane) { + // Reconcile kubeadmin password + r.Log.Info("Reconciling kubeadmin password secret") + explicitOauthConfig := hostedControlPlane.Spec.Configuration != nil && hostedControlPlane.Spec.Configuration.OAuth != nil + if err := r.reconcileKubeadminPassword(ctx, hostedControlPlane, explicitOauthConfig, createOrUpdate); err != nil { + return fmt.Errorf("failed to ensure control plane: %w", err) + } + if !r.IsCPOV2 { // Reconcile openshift oauth apiserver r.Log.Info("Reconciling OpenShift OAuth API Server") @@ -1317,15 +1324,6 @@ func (r *HostedControlPlaneReconciler) reconcile(ctx context.Context, hostedCont } } - // Reconcile kubeadmin password - if util.HCPOAuthEnabled(hostedControlPlane) { - r.Log.Info("Reconciling kubeadmin password secret") - explicitOauthConfig := hostedControlPlane.Spec.Configuration != nil && hostedControlPlane.Spec.Configuration.OAuth != nil - if err := r.reconcileKubeadminPassword(ctx, hostedControlPlane, explicitOauthConfig, createOrUpdate); err != nil { - return fmt.Errorf("failed to ensure control plane: %w", err) - } - } - if IsStorageAndCSIManaged(hostedControlPlane) { // Reconcile cloud csi driver r.Log.Info("Reconciling CSI Driver") diff --git a/control-plane-operator/controllers/hostedcontrolplane/oauth/deployment.go b/control-plane-operator/controllers/hostedcontrolplane/oauth/deployment.go index 127ac69d34..94ef45b9df 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/oauth/deployment.go +++ b/control-plane-operator/controllers/hostedcontrolplane/oauth/deployment.go @@ -7,24 +7,29 @@ import ( "strings" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/common" + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/kas" + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" + "github.com/openshift/hypershift/support/config" "github.com/openshift/hypershift/support/globalconfig" + "github.com/openshift/hypershift/support/util" + + configv1 "github.com/openshift/api/config/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "sigs.k8s.io/controller-runtime/pkg/client" - - configv1 "github.com/openshift/api/config/v1" - "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/kas" - "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" - "github.com/openshift/hypershift/support/config" - "github.com/openshift/hypershift/support/util" "k8s.io/utils/ptr" + + crclient "sigs.k8s.io/controller-runtime/pkg/client" ) const ( configHashAnnotation = "oauth.hypershift.openshift.io/config-hash" + KubeadminSecretHashAnnotation = "hypershift.openshift.io/kubeadmin-secret-hash" oauthNamedCertificateMountPathPrefix = "/etc/kubernetes/certs/named" socks5ProxyContainerName = "socks-proxy" @@ -71,7 +76,7 @@ func oauthLabels() map[string]string { } } -func ReconcileDeployment(ctx context.Context, client client.Client, deployment *appsv1.Deployment, auditWebhookRef *corev1.LocalObjectReference, ownerRef config.OwnerRef, config *corev1.ConfigMap, auditConfig *corev1.ConfigMap, image string, deploymentConfig config.DeploymentConfig, identityProviders []configv1.IdentityProvider, providerOverrides map[string]*ConfigOverride, availabilityProberImage string, namedCertificates []configv1.APIServerNamedServingCert, proxyImage string, proxyConfig *configv1.ProxySpec, clusterNoProxy string, oauthNoProxy []string, params *OAuthConfigParams, platformType hyperv1.PlatformType) error { +func ReconcileDeployment(ctx context.Context, client crclient.Client, deployment *appsv1.Deployment, auditWebhookRef *corev1.LocalObjectReference, ownerRef config.OwnerRef, config *corev1.ConfigMap, auditConfig *corev1.ConfigMap, image string, deploymentConfig config.DeploymentConfig, identityProviders []configv1.IdentityProvider, providerOverrides map[string]*ConfigOverride, availabilityProberImage string, namedCertificates []configv1.APIServerNamedServingCert, proxyImage string, proxyConfig *configv1.ProxySpec, clusterNoProxy string, oauthNoProxy []string, params *OAuthConfigParams, platformType hyperv1.PlatformType) error { ownerRef.ApplyTo(deployment) // preserve existing resource requirements for main oauth container @@ -105,6 +110,17 @@ func ReconcileDeployment(ctx context.Context, client client.Client, deployment * return fmt.Errorf("oauth server: configuration not found in configmap") } deployment.Spec.Template.ObjectMeta.Annotations[configHashAnnotation] = util.ComputeHash(configBytes) + + kubeadminPasswordSecret := common.KubeadminPasswordSecret(deployment.Namespace) + if err := client.Get(ctx, crclient.ObjectKeyFromObject(kubeadminPasswordSecret), kubeadminPasswordSecret); err != nil { + if !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to get kubeadmin password secret: %v", err) + } + delete(deployment.Spec.Template.ObjectMeta.Annotations, KubeadminSecretHashAnnotation) + } else { + deployment.Spec.Template.ObjectMeta.Annotations[KubeadminSecretHashAnnotation] = util.HashSimple(kubeadminPasswordSecret.Data) + } + deployment.Spec.Template.Spec = corev1.PodSpec{ AutomountServiceAccountToken: ptr.To(false), Containers: []corev1.Container{ diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 4a657aec8f..2a78eee9cb 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -2,7 +2,6 @@ package resources import ( "context" - "crypto/md5" "fmt" "reflect" "strings" @@ -85,7 +84,6 @@ import ( const ( ControllerName = "resources" - SecretHashAnnotation = "hypershift.openshift.io/kubeadmin-secret-hash" ConfigNamespace = "openshift-config" ConfigManagedNamespace = "openshift-config-managed" CloudProviderCMName = "cloud-provider-config" @@ -1341,31 +1339,20 @@ func (r *reconciler) reconcileKubeadminPasswordHashSecret(ctx context.Context, h if apierrors.IsNotFound(err) { // kubeAdminPasswordHash should not exist when a user specifies an explicit oauth config // delete kubeAdminPasswordHash if it exist - return r.deleteKubeadminPasswordHashSecret(ctx, hcp) + return r.deleteKubeadminPasswordHashSecret(ctx) } return fmt.Errorf("failed to get kubeadmin password secret: %w", err) } kubeadminPasswordHashSecret := manifests.KubeadminPasswordHashSecret() - if _, err := r.CreateOrUpdate(ctx, r.client, kubeadminPasswordHashSecret, func() error { + _, err := r.CreateOrUpdate(ctx, r.client, kubeadminPasswordHashSecret, func() error { return kubeadminpassword.ReconcileKubeadminPasswordHashSecret(kubeadminPasswordHashSecret, kubeadminPasswordSecret) - }); err != nil { - return err - } - oauthDeployment := manifests.OAuthDeployment(hcp.Namespace) - if _, err := r.CreateOrUpdate(ctx, r.cpClient, oauthDeployment, func() error { - if oauthDeployment.Spec.Template.ObjectMeta.Annotations == nil { - oauthDeployment.Spec.Template.ObjectMeta.Annotations = map[string]string{} - } - oauthDeployment.Spec.Template.ObjectMeta.Annotations[SecretHashAnnotation] = secretHash(kubeadminPasswordHashSecret.Data["kubeadmin"]) - return nil - }); err != nil { - return err - } - return nil + }) + + return err } -func (r *reconciler) deleteKubeadminPasswordHashSecret(ctx context.Context, hcp *hyperv1.HostedControlPlane) error { +func (r *reconciler) deleteKubeadminPasswordHashSecret(ctx context.Context) error { kubeadminPasswordHashSecret := manifests.KubeadminPasswordHashSecret() if err := r.client.Get(ctx, client.ObjectKeyFromObject(kubeadminPasswordHashSecret), kubeadminPasswordHashSecret); err != nil { if !apierrors.IsNotFound(err) { @@ -1377,20 +1364,9 @@ func (r *reconciler) deleteKubeadminPasswordHashSecret(ctx context.Context, hcp } } - oauthDeployment := manifests.OAuthDeployment(hcp.Namespace) - if _, err := r.CreateOrUpdate(ctx, r.cpClient, oauthDeployment, func() error { - delete(oauthDeployment.Spec.Template.ObjectMeta.Annotations, SecretHashAnnotation) - return nil - }); err != nil { - return err - } return nil } -func secretHash(data []byte) string { - return fmt.Sprintf("%x", md5.Sum(data)) -} - func (r *reconciler) reconcileOAuthServingCertCABundle(ctx context.Context, hcp *hyperv1.HostedControlPlane) error { sourceBundle := cpomanifests.OpenShiftOAuthMasterCABundle(hcp.Namespace) if err := r.cpClient.Get(ctx, client.ObjectKeyFromObject(sourceBundle), sourceBundle); err != nil { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index 52a3e1b95d..4a13f3bf82 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -440,20 +440,12 @@ func TestReconcileKubeadminPasswordHashSecret(t *testing.T) { testNamespace := "master-cluster1" testHCPName := "cluster1" - annotatedOauthDeployment := &appsv1.Deployment{ - ObjectMeta: manifests.OAuthDeployment(testNamespace).ObjectMeta, - } - annotatedOauthDeployment.Spec.Template.Annotations = map[string]string{ - SecretHashAnnotation: "fake-hash", - } - tests := map[string]struct { inputHCP *hyperv1.HostedControlPlane inputObjects []client.Object - expectedOauthServerAnnotations []string expectKubeadminPasswordHashSecretToExist bool }{ - "when kubeadminPasswordSecret exists the oauth server is annotated and the hash secret is created": { + "when kubeadminPasswordSecret exists the hash secret is created": { inputHCP: &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ Name: testHCPName, @@ -471,12 +463,9 @@ func TestReconcileKubeadminPasswordHashSecret(t *testing.T) { ObjectMeta: manifests.OAuthDeployment(testNamespace).ObjectMeta, }, }, - expectedOauthServerAnnotations: []string{ - SecretHashAnnotation, - }, expectKubeadminPasswordHashSecretToExist: true, }, - "when kubeadminPasswordSecret doesn't exist the oauth server is not annotated and the hash secret is not created": { + "when kubeadminPasswordSecret doesn't exist the hash secret is not created": { inputHCP: &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ Name: testHCPName, @@ -488,20 +477,6 @@ func TestReconcileKubeadminPasswordHashSecret(t *testing.T) { ObjectMeta: manifests.OAuthDeployment(testNamespace).ObjectMeta, }, }, - expectedOauthServerAnnotations: nil, - expectKubeadminPasswordHashSecretToExist: false, - }, - "when kubeadminPasswordSecret doesn't exist the oauth server SecretHashAnnotation annotation is deleted and the hash secret is not created": { - inputHCP: &hyperv1.HostedControlPlane{ - ObjectMeta: metav1.ObjectMeta{ - Name: testHCPName, - Namespace: testNamespace, - }, - }, - inputObjects: []client.Object{ - annotatedOauthDeployment, - }, - expectedOauthServerAnnotations: nil, expectKubeadminPasswordHashSecretToExist: false, }, } @@ -530,13 +505,6 @@ func TestReconcileKubeadminPasswordHashSecret(t *testing.T) { actualOauthDeployment := manifests.OAuthDeployment(testNamespace) err = r.cpClient.Get(context.TODO(), client.ObjectKeyFromObject(actualOauthDeployment), actualOauthDeployment) g.Expect(err).To(BeNil()) - if test.expectedOauthServerAnnotations == nil { - g.Expect(actualOauthDeployment.Spec.Template.Annotations).To(BeNil()) - } else { - for _, annotation := range test.expectedOauthServerAnnotations { - g.Expect(len(actualOauthDeployment.Spec.Template.Annotations[annotation]) > 0).To(BeTrue()) - } - } }) } } diff --git a/test/e2e/util/oauth.go b/test/e2e/util/oauth.go index 84c2f25f7b..3a6c992a20 100644 --- a/test/e2e/util/oauth.go +++ b/test/e2e/util/oauth.go @@ -14,7 +14,7 @@ import ( routev1 "github.com/openshift/api/route/v1" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" hcpmanifests "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" - "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources" + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/oauth" configmanifests "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" "github.com/openshift/hypershift/hypershift-operator/controllers/manifests" "github.com/openshift/hypershift/support/api" @@ -268,7 +268,7 @@ func validateClusterPreIDP(t *testing.T, ctx context.Context, client crclient.Cl err = client.Get(ctx, crclient.ObjectKeyFromObject(oauthDeployment), oauthDeployment) g.Expect(err).ToNot(HaveOccurred()) // validate oauthDeployment has kubeadmin password hash annotation - g.Expect(oauthDeployment.Spec.Template.ObjectMeta.Annotations).To(HaveKey(resources.SecretHashAnnotation)) + g.Expect(oauthDeployment.Spec.Template.ObjectMeta.Annotations).To(HaveKey(oauth.KubeadminSecretHashAnnotation)) } func validateClusterPostIDP(t *testing.T, ctx context.Context, client crclient.Client, hostedCluster *hyperv1.HostedCluster) { @@ -290,5 +290,5 @@ func validateClusterPostIDP(t *testing.T, ctx context.Context, client crclient.C err = client.Get(ctx, crclient.ObjectKeyFromObject(oauthDeployment), oauthDeployment) g.Expect(err).ToNot(HaveOccurred()) // validate oauthDeployment kubeadmin password hash annotation was removed - g.Expect(oauthDeployment.Spec.Template.ObjectMeta.Annotations).ToNot(HaveKey(resources.SecretHashAnnotation)) + g.Expect(oauthDeployment.Spec.Template.ObjectMeta.Annotations).ToNot(HaveKey(oauth.KubeadminSecretHashAnnotation)) } From 8fe412652ab990d298e8bec7ed6438bd415e7818 Mon Sep 17 00:00:00 2001 From: Mulham Raee Date: Mon, 6 Jan 2025 17:12:17 +0100 Subject: [PATCH 2/2] enable oauth v2 reconciliation --- .../hostedcontrolplane_controller.go | 13 ++++++++----- .../zz_fixture_TestControlPlaneComponents.yaml | 2 +- ...ControlPlaneComponents_TechPreviewNoUpgrade.yaml | 2 +- .../hostedcontrolplane/v2/oauth/component.go | 2 ++ 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index 50813a41f4..7620f3af17 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go +++ b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go @@ -80,6 +80,7 @@ import ( kcmv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/kcm" schedulerv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler" oapiv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/oapi" + oauthv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/oauth" oauthapiv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver" ocmv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/ocm" routecmv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/routecm" @@ -222,6 +223,7 @@ func (r *HostedControlPlaneReconciler) registerComponents() { autoscalerv2.NewComponent(), cvov2.NewComponent(r.EnableCVOManagementClusterMetricsAccess), ocmv2.NewComponent(), + oauthv2.NewComponent(), routecmv2.NewComponent(), clusterpolicyv2.NewComponent(), configoperatorv2.NewComponent(r.ReleaseProvider.GetRegistryOverrides(), r.ReleaseProvider.GetOpenShiftImageRegistryOverrides()), @@ -1214,12 +1216,13 @@ func (r *HostedControlPlaneReconciler) reconcile(ctx context.Context, hostedCont if err := r.reconcileOpenShiftOAuthAPIServer(ctx, hostedControlPlane, observedConfig, releaseImageProvider, createOrUpdate); err != nil { return fmt.Errorf("failed to reconcile openshift oauth apiserver: %w", err) } - } - // Reconcile oauth server - r.Log.Info("Reconciling OAuth Server") - if err := r.reconcileOAuthServer(ctx, hostedControlPlane, releaseImageProvider, infraStatus.OAuthHost, infraStatus.OAuthPort, createOrUpdate); err != nil { - return fmt.Errorf("failed to reconcile openshift oauth apiserver: %w", err) + // Reconcile oauth server + r.Log.Info("Reconciling OAuth Server") + if err := r.reconcileOAuthServer(ctx, hostedControlPlane, releaseImageProvider, infraStatus.OAuthHost, infraStatus.OAuthPort, createOrUpdate); err != nil { + return fmt.Errorf("failed to reconcile openshift oauth apiserver: %w", err) + } + } // TODO: move this up with the rest of conditions reconciliation logic? diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents.yaml index ef468ff5fd..13e7ee0f56 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents.yaml @@ -30,7 +30,7 @@ spec: annotations: cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes: logs component.hypershift.openshift.io/config-hash: 4ebc1fdd - hypershift.openshift.io/release-image: "" + hypershift.openshift.io/release-image: quay.io/openshift-release-dev/ocp-release:4.16.10-x86_64 creationTimestamp: null labels: app: oauth-openshift diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_TechPreviewNoUpgrade.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_TechPreviewNoUpgrade.yaml index ef468ff5fd..13e7ee0f56 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_TechPreviewNoUpgrade.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_TechPreviewNoUpgrade.yaml @@ -30,7 +30,7 @@ spec: annotations: cluster-autoscaler.kubernetes.io/safe-to-evict-local-volumes: logs component.hypershift.openshift.io/config-hash: 4ebc1fdd - hypershift.openshift.io/release-image: "" + hypershift.openshift.io/release-image: quay.io/openshift-release-dev/ocp-release:4.16.10-x86_64 creationTimestamp: null labels: app: oauth-openshift diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/component.go b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/component.go index 930ddd41a4..b72763ff19 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/component.go +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/component.go @@ -1,6 +1,7 @@ package oauth import ( + "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/common" oapiv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/oapi" component "github.com/openshift/hypershift/support/controlplane-component" "github.com/openshift/hypershift/support/util" @@ -68,6 +69,7 @@ func NewComponent() component.ControlPlaneComponent { ). WithDependencies(oapiv2.ComponentName). RolloutOnConfigMapChange("oauth-openshift"). + RolloutOnSecretChange(common.KubeadminPasswordSecret("").Name). InjectKonnectivityContainer(component.KonnectivityContainerOptions{ Mode: component.Dual, Socks5Options: component.Socks5Options{