diff --git a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go b/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go index 17339fad0d..131dbcd2ea 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go +++ b/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") 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/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{ 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)) }