OCPBUGS-82192: fix(webhookcerts): handle upgrade from service-ca managed certs

hypershift-operator/controllers/webhookcerts/webhookcerts_controller.go

@@ -34,6 +34,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
+
+	"github.com/go-logr/logr"
 )
 
 const (
@@ -43,6 +45,14 @@ const (
 	ServingCertSecretName = "manager-serving-cert"
 
 	requeueInterval = 12 * time.Hour
+
+	// service-ca annotations used on the operator Service to trigger cert generation.
+	serviceCABetaAnnotation  = "service.beta.openshift.io/serving-cert-secret-name"
+	serviceCAAlphaAnnotation = "service.alpha.openshift.io/serving-cert-secret-name"
+
+	// service-ca annotations placed on secrets it creates.
+	originatingServiceBetaAnnotation  = "service.beta.openshift.io/originating-service-name"
+	originatingServiceAlphaAnnotation = "service.alpha.openshift.io/originating-service-name"
 )
 
 // WebhookCertReconciler reconciles the self-managed webhook CA and serving cert.
@@ -70,6 +80,15 @@ func (r *WebhookCertReconciler) SetupWithManager(mgr ctrl.Manager, createOrUpdat
 func (r *WebhookCertReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
+	// 0. Handle upgrade from service-ca managed certs.
+	// On existing OpenShift clusters, the service-ca operator may have created the
+	// serving cert secret and annotated the Service. We must remove these before
+	// reconciling our own certs, otherwise service-ca will keep overwriting the secret
+	// with a cert signed by a different CA than the one we inject into webhook configs.
+	if err := r.removeServiceCAResources(ctx, log); err != nil {
+		return ctrl.Result{}, err
+	}
+
 	// 1. Reconcile the self-signed CA.
 	caSecret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
@@ -203,6 +222,58 @@ func (r *WebhookCertReconciler) patchWebhookConfigsCABundle(ctx context.Context,
 	return nil
 }
 
+// removeServiceCAResources handles the upgrade from service-ca managed certs to self-managed certs.
+// It removes the service-ca annotations from the operator Service and deletes the serving cert
+// secret if it was created by service-ca, so the reconciler can recreate it with the self-managed CA.
+func (r *WebhookCertReconciler) removeServiceCAResources(ctx context.Context, log logr.Logger) error {
+	svc := &corev1.Service{}
+	if err := r.Client.Get(ctx, client.ObjectKey{Namespace: r.Namespace, Name: r.ServiceName}, svc); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to get operator service: %w", err)
+		}
+	} else {
+		changed := false
+		for _, annotation := range []string{serviceCABetaAnnotation, serviceCAAlphaAnnotation} {
+			if _, ok := svc.Annotations[annotation]; ok {
+				delete(svc.Annotations, annotation)
+				changed = true
+			}
+		}
+		if changed {
+			if err := r.Client.Update(ctx, svc); err != nil {
+				return fmt.Errorf("failed to remove service-ca annotations from operator service: %w", err)
+			}
+			log.Info("Removed service-ca annotations from operator service")
+		}
+	}
+
+	// If the existing serving cert secret was created by service-ca, delete it
+	// so we can recreate it signed by our self-managed CA.
+	existingSecret := &corev1.Secret{}
+	if err := r.Client.Get(ctx, client.ObjectKey{Namespace: r.Namespace, Name: ServingCertSecretName}, existingSecret); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to get existing serving cert secret: %w", err)
+		}
+	} else if isServiceCAManaged(existingSecret) {
+		if err := r.Client.Delete(ctx, existingSecret); err != nil && !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete service-ca managed serving cert secret: %w", err)
+		}
+		log.Info("Deleted service-ca managed serving cert secret")
+	}
+
+	return nil
+}
+
+// isServiceCAManaged returns true if the secret was created by the service-ca operator.
+func isServiceCAManaged(secret *corev1.Secret) bool {
+	if secret.Annotations == nil {
+		return false
+	}
+	_, hasBeta := secret.Annotations[originatingServiceBetaAnnotation]
+	_, hasAlpha := secret.Annotations[originatingServiceAlphaAnnotation]
+	return hasBeta || hasAlpha
+}
+
 // webhookDNSNames returns the DNS names for the webhook serving cert.
 func webhookDNSNames(serviceName, namespace string) []string {
 	return []string{

hypershift-operator/controllers/webhookcerts/webhookcerts_controller_test.go

@@ -229,6 +229,110 @@ func TestReconcile(t *testing.T) {
 		_, err := r.Reconcile(t.Context(), caRequest())
 		g.Expect(err).ToNot(HaveOccurred())
 	})
+
+	t.Run("When upgrading from service-ca it should remove service-ca annotations from the Service", func(t *testing.T) {
+		g := NewWithT(t)
+
+		svc := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "operator",
+				Namespace: "hypershift",
+				Annotations: map[string]string{
+					"service.beta.openshift.io/serving-cert-secret-name": "manager-serving-cert",
+				},
+			},
+			Spec: corev1.ServiceSpec{
+				Ports: []corev1.ServicePort{{Port: 443}},
+			},
+		}
+
+		cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(svc).Build()
+		r := newReconciler(cl)
+
+		_, err := r.Reconcile(t.Context(), caRequest())
+		g.Expect(err).ToNot(HaveOccurred())
+
+		updatedSvc := &corev1.Service{}
+		g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: "operator", Namespace: "hypershift"}, updatedSvc)).To(Succeed())
+		g.Expect(updatedSvc.Annotations).ToNot(HaveKey("service.beta.openshift.io/serving-cert-secret-name"))
+	})
+
+	t.Run("When upgrading from service-ca it should delete the service-ca managed serving cert and recreate it", func(t *testing.T) {
+		g := NewWithT(t)
+
+		// Simulate a serving cert secret created by service-ca.
+		serviceCACert := &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      ServingCertSecretName,
+				Namespace: "hypershift",
+				Annotations: map[string]string{
+					"service.beta.openshift.io/originating-service-name": "operator",
+				},
+			},
+			Type: corev1.SecretTypeTLS,
+			Data: map[string][]byte{
+				corev1.TLSCertKey:       []byte("service-ca-cert"),
+				corev1.TLSPrivateKeyKey: []byte("service-ca-key"),
+			},
+		}
+
+		svc := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "operator",
+				Namespace: "hypershift",
+				Annotations: map[string]string{
+					"service.beta.openshift.io/serving-cert-secret-name": "manager-serving-cert",
+				},
+			},
+			Spec: corev1.ServiceSpec{
+				Ports: []corev1.ServicePort{{Port: 443}},
+			},
+		}
+
+		cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(serviceCACert, svc).Build()
+		r := newReconciler(cl)
+
+		_, err := r.Reconcile(t.Context(), caRequest())
+		g.Expect(err).ToNot(HaveOccurred())
+
+		// The service-ca annotation should be removed from the Service.
+		updatedSvc := &corev1.Service{}
+		g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: "operator", Namespace: "hypershift"}, updatedSvc)).To(Succeed())
+		g.Expect(updatedSvc.Annotations).ToNot(HaveKey("service.beta.openshift.io/serving-cert-secret-name"))
+
+		// The serving cert should have been recreated without the service-ca annotation
+		// and signed by the self-managed CA.
+		servingSecret := &corev1.Secret{}
+		g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}, servingSecret)).To(Succeed())
+		g.Expect(servingSecret.Annotations).ToNot(HaveKey("service.beta.openshift.io/originating-service-name"))
+		g.Expect(servingSecret.Data[corev1.TLSCertKey]).ToNot(Equal([]byte("service-ca-cert")))
+		g.Expect(servingSecret.Data[corev1.TLSCertKey]).ToNot(BeEmpty())
+
+		// Verify the new cert is signed by the self-managed CA.
+		caSecret := &corev1.Secret{}
+		g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: CASecretName, Namespace: "hypershift"}, caSecret)).To(Succeed())
+		g.Expect(caSecret.Data).To(HaveKey(certs.CASignerCertMapKey))
+	})
+
+	t.Run("When the serving cert was not created by service-ca it should not delete it", func(t *testing.T) {
+		g := NewWithT(t)
+
+		// Pre-create valid self-managed secrets.
+		caSecret, servingSecret, _, err := GenerateInitialWebhookCerts("hypershift", "operator")
+		g.Expect(err).ToNot(HaveOccurred())
+		originalCert := servingSecret.Data[corev1.TLSCertKey]
+
+		cl := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(caSecret, servingSecret).Build()
+		r := newReconciler(cl)
+
+		_, err = r.Reconcile(t.Context(), caRequest())
+		g.Expect(err).ToNot(HaveOccurred())
+
+		// Cert should be unchanged.
+		updatedSecret := &corev1.Secret{}
+		g.Expect(cl.Get(t.Context(), client.ObjectKey{Name: ServingCertSecretName, Namespace: "hypershift"}, updatedSecret)).To(Succeed())
+		g.Expect(updatedSecret.Data[corev1.TLSCertKey]).To(Equal(originalCert))
+	})
 }
 
 func TestGenerateInitialWebhookCerts(t *testing.T) {