Merge pull request #5286 from muraee/move-oauth-hash-cpo

NO-JIRA: Move oauth kubeadminsecret hash annotation logic from HCCO to CPO

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -81,6 +81,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"
@@ -223,6 +224,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()),
@@ -1203,18 +1205,26 @@ 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")
 			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?
@@ -1319,15 +1329,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")

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{

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

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

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{

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 {

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())
-				}
-			}
 		})
 	}
 }

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))
 }