OCPBUGS-58145: reload serving cert on rotation

pkg/operator/controller/canary/controller.go

@@ -35,6 +35,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
+
+	"k8s.io/client-go/tools/record"
 )
 
 const (
@@ -80,6 +82,7 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) {
 	reconciler := &reconciler{
 		config:                    config,
 		client:                    mgr.GetClient(),
+		recorder:                  mgr.GetEventRecorderFor(canaryControllerName),
 		enableCanaryRouteRotation: false,
 	}
 	c, err := controller.New(canaryControllerName, mgr, controller.Options{Reconciler: reconciler})
@@ -161,6 +164,16 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) {
 		return nil, err
 	}
 
+	// Watch the canary serving cert secret and enqueue the default ingress controller so
+	// that changes to the serving cert cause the canary daemonset to be reconciled.
+	canarySecretPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		name := operatorcontroller.CanaryCertificateName()
+		return o.GetNamespace() == name.Namespace && o.GetName() == name.Name
+	})
+	if err := c.Watch(source.Kind[client.Object](operatorCache, &corev1.Secret{}, enqueueRequestForDefaultIngressController(config.Namespace), canarySecretPredicate)); err != nil {
+		return nil, err
+	}
+
 	return c, nil
 }
 
@@ -183,13 +196,13 @@ func enqueueRequestForDefaultIngressController(namespace string) handler.EventHa
 func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
 	result := reconcile.Result{}
 
-	if _, _, err := r.ensureCanaryNamespace(); err != nil {
+	if _, _, err := r.ensureCanaryNamespace(ctx); err != nil {
 		// Return if the canary namespace cannot be created since
 		// resource creation in a namespace that does not exist will fail.
 		return result, fmt.Errorf("failed to ensure canary namespace: %v", err)
 	}
 
-	haveDs, daemonset, err := r.ensureCanaryDaemonSet()
+	haveDs, daemonset, err := r.ensureCanaryDaemonSet(ctx)
 	if err != nil {
 		return result, fmt.Errorf("failed to ensure canary daemonset: %v", err)
 	} else if !haveDs {
@@ -205,14 +218,14 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		Controller: &trueVar,
 	}
 
-	haveService, service, err := r.ensureCanaryService(daemonsetRef)
+	haveService, service, err := r.ensureCanaryService(ctx, daemonsetRef)
 	if err != nil {
 		return result, fmt.Errorf("failed to ensure canary service: %v", err)
 	} else if !haveService {
 		return result, fmt.Errorf("failed to get canary service: %v", err)
 	}
 
-	haveRoute, _, err := r.ensureCanaryRoute(service)
+	haveRoute, _, err := r.ensureCanaryRoute(ctx, service)
 	if err != nil {
 		return result, fmt.Errorf("failed to ensure canary route: %v", err)
 	} else if !haveRoute {
@@ -222,7 +235,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	// Get the canary route rotation annotation value
 	// from the default ingress controller.
 	ic := &operatorv1.IngressController{}
-	if err := r.client.Get(context.TODO(), request.NamespacedName, ic); err != nil {
+	if err := r.client.Get(ctx, request.NamespacedName, ic); err != nil {
 		return result, fmt.Errorf("failed to get ingress controller %s: %v", request.NamespacedName.Name, err)
 	}
 
@@ -252,7 +265,8 @@ type Config struct {
 type reconciler struct {
 	config Config
 
-	client client.Client
+	client   client.Client
+	recorder record.EventRecorder
 
 	// Use a mutex so enableCanaryRotation is
 	// go-routine safe.
@@ -289,8 +303,9 @@ func (r *reconciler) startCanaryRoutePolling(stop <-chan struct{}) error {
 
 	// using wait.NonSlidingUntil so that the canary runs every canaryCheckFrequency, regardless of how long the function takes
 	go wait.NonSlidingUntil(func() {
+		ctx := context.TODO()
 		// Get the current canary route every iteration in case it has been modified
-		haveRoute, route, err := r.currentCanaryRoute()
+		haveRoute, route, err := r.currentCanaryRoute(ctx)
 		if err != nil {
 			log.Error(err, "failed to get current canary route for canary check")
 			return
@@ -338,15 +353,15 @@ func (r *reconciler) startCanaryRoutePolling(stop <-chan struct{}) error {
 		if rotationEnabled {
 			checkCount++
 			if checkCount >= canaryCheckCycleCount {
-				haveService, service, err := r.currentCanaryService()
+				haveService, service, err := r.currentCanaryService(ctx)
 				if err != nil {
 					log.Error(err, "failed to get canary service")
 					return
 				} else if !haveService {
 					log.Info("canary check service does not exist")
 					return
 				}
-				route, err = r.rotateRouteEndpoint(service, route)
+				route, err = r.rotateRouteEndpoint(ctx, service, route)
 				if err != nil {
 					log.Error(err, "failed to rotate canary route endpoint")
 					return
@@ -480,13 +495,13 @@ func (r *reconciler) setCanaryStatusCondition(cond operatorv1.OperatorCondition)
 // Switch the current RoutePort that the route points to.
 // Use this function to periodically update the canary route endpoint
 // to verify if the router has wedged.
-func (r *reconciler) rotateRouteEndpoint(service *corev1.Service, current *routev1.Route) (*routev1.Route, error) {
+func (r *reconciler) rotateRouteEndpoint(ctx context.Context, service *corev1.Service, current *routev1.Route) (*routev1.Route, error) {
 	updated, err := cycleServicePort(service, current)
 	if err != nil {
 		return nil, fmt.Errorf("failed to rotate route port: %v", err)
 	}
 
-	if changed, err := r.updateCanaryRoute(current, updated); err != nil {
+	if changed, err := r.updateCanaryRoute(ctx, current, updated); err != nil {
 		return current, err
 	} else if !changed {
 		return current, fmt.Errorf("expected canary route to be updated: No relevant changes detected")

pkg/operator/controller/canary/daemonset.go

@@ -15,35 +15,61 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 )
 
+const (
+	// CanaryServingCertHashAnnotation is the annotation key used on the
+	// canary DaemonSet PodTemplate to force a rollout when the canary
+	// serving cert secret changes.
+	CanaryServingCertHashAnnotation = "ingress.operator.openshift.io/canary-serving-cert-hash"
+)
+
 // ensureCanaryDaemonSet ensures the canary daemonset exists
-func (r *reconciler) ensureCanaryDaemonSet() (bool, *appsv1.DaemonSet, error) {
-	desired := desiredCanaryDaemonSet(r.config.CanaryImage)
-	haveDs, current, err := r.currentCanaryDaemonSet()
+func (r *reconciler) ensureCanaryDaemonSet(ctx context.Context) (bool, *appsv1.DaemonSet, error) {
+	// Attempt to read the canary serving cert secret and compute a content hash.
+	// If the secret is missing or incomplete, proceed without the annotation but
+	// surface a log entry so operators can investigate.
+	var certHash string
+	secret := &corev1.Secret{}
+	if err := r.client.Get(ctx, controller.CanaryCertificateName(), secret); err != nil {
+		if errors.IsNotFound(err) {
+			log.Info("canary serving cert secret not found; skipping canary-serving-cert-hash annotation")
+		} else {
+			return false, nil, fmt.Errorf("failed to get canary serving cert secret: %v", err)
+		}
+	} else {
+		if h, err := ComputeTLSSecretHash(secret); err != nil {
+			log.Info("canary serving cert secret is incomplete; skipping canary-serving-cert-hash annotation", "error", err)
+		} else {
+			certHash = h
+		}
+	}
+
+	desired := desiredCanaryDaemonSet(r.config.CanaryImage, certHash)
+	haveDs, current, err := r.currentCanaryDaemonSet(ctx)
 	if err != nil {
 		return false, nil, err
 	}
 
 	switch {
 	case !haveDs:
-		if err := r.createCanaryDaemonSet(desired); err != nil {
+		if err := r.createCanaryDaemonSet(ctx, desired); err != nil {
 			return false, nil, err
 		}
-		return r.currentCanaryDaemonSet()
+		return r.currentCanaryDaemonSet(ctx)
 	case haveDs:
-		if updated, err := r.updateCanaryDaemonSet(current, desired); err != nil {
+		if updated, err := r.updateCanaryDaemonSet(ctx, current, desired); err != nil {
 			return true, current, err
 		} else if updated {
-			return r.currentCanaryDaemonSet()
+			return r.currentCanaryDaemonSet(ctx)
 		}
 	}
 
 	return true, current, nil
 }
 
 // currentCanaryDaemonSet returns the current canary daemonset
-func (r *reconciler) currentCanaryDaemonSet() (bool, *appsv1.DaemonSet, error) {
+func (r *reconciler) currentCanaryDaemonSet(ctx context.Context) (bool, *appsv1.DaemonSet, error) {
 	daemonset := &appsv1.DaemonSet{}
-	if err := r.client.Get(context.TODO(), controller.CanaryDaemonSetName(), daemonset); err != nil {
+	if err := r.client.Get(ctx, controller.CanaryDaemonSetName(), daemonset); err != nil {
 		if errors.IsNotFound(err) {
 			return false, nil, nil
 		}
@@ -53,8 +79,8 @@ func (r *reconciler) currentCanaryDaemonSet() (bool, *appsv1.DaemonSet, error) {
 }
 
 // createCanaryDaemonSet creates the given daemonset resource
-func (r *reconciler) createCanaryDaemonSet(daemonset *appsv1.DaemonSet) error {
-	if err := r.client.Create(context.TODO(), daemonset); err != nil {
+func (r *reconciler) createCanaryDaemonSet(ctx context.Context, daemonset *appsv1.DaemonSet) error {
+	if err := r.client.Create(ctx, daemonset); err != nil {
 		return fmt.Errorf("failed to create canary daemonset %s/%s: %v", daemonset.Namespace, daemonset.Name, err)
 	}
 
@@ -64,23 +90,46 @@ func (r *reconciler) createCanaryDaemonSet(daemonset *appsv1.DaemonSet) error {
 
 // updateCanaryDaemonSet updates the canary daemonset if an appropriate change
 // has been detected
-func (r *reconciler) updateCanaryDaemonSet(current, desired *appsv1.DaemonSet) (bool, error) {
+func (r *reconciler) updateCanaryDaemonSet(ctx context.Context, current, desired *appsv1.DaemonSet) (bool, error) {
 	changed, updated := canaryDaemonSetChanged(current, desired)
 	if !changed {
 		return false, nil
 	}
 
+	// Capture annotation change for events.
+	var oldHash, newHash string
+	if current.Spec.Template.Annotations != nil {
+		oldHash = current.Spec.Template.Annotations[CanaryServingCertHashAnnotation]
+	}
+	if updated.Spec.Template.Annotations != nil {
+		newHash = updated.Spec.Template.Annotations[CanaryServingCertHashAnnotation]
+	}
+
 	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
-	if err := r.client.Update(context.TODO(), updated); err != nil {
+	if err := r.client.Update(ctx, updated); err != nil {
 		return false, fmt.Errorf("failed to update canary daemonset %s/%s: %v", updated.Namespace, updated.Name, err)
 	}
+
 	log.Info("updated canary daemonset", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
+
+	// If the only meaningful change (or one of the changes) was the canary cert
+	// annotation, emit an event for traceability.
+	if newHash != "" && newHash != oldHash {
+		short := newHash
+		if len(short) > 8 {
+			short = short[:8]
+		}
+		if r.recorder != nil {
+			r.recorder.Eventf(updated, "Normal", "CanaryCertRotated", "Canary serving cert rotated, updated pod template annotation hash: %s", short)
+		}
+	}
+
 	return true, nil
 }
 
 // desiredCanaryDaemonSet returns the desired canary daemonset read in
 // from manifests
-func desiredCanaryDaemonSet(canaryImage string) *appsv1.DaemonSet {
+func desiredCanaryDaemonSet(canaryImage string, certHash string) *appsv1.DaemonSet {
 	daemonset := manifests.CanaryDaemonSet()
 	name := controller.CanaryDaemonSetName()
 	daemonset.Name = name.Name
@@ -97,6 +146,13 @@ func desiredCanaryDaemonSet(canaryImage string) *appsv1.DaemonSet {
 	daemonset.Spec.Template.Spec.Containers[0].Image = canaryImage
 	daemonset.Spec.Template.Spec.Containers[0].Command = []string{"ingress-operator", CanaryHealthcheckCommand}
 
+	if certHash != "" {
+		if daemonset.Spec.Template.Annotations == nil {
+			daemonset.Spec.Template.Annotations = map[string]string{}
+		}
+		daemonset.Spec.Template.Annotations[CanaryServingCertHashAnnotation] = certHash
+	}
+
 	return daemonset
 }
 
@@ -163,6 +219,26 @@ func canaryDaemonSetChanged(current, expected *appsv1.DaemonSet) (bool, *appsv1.
 		changed = true
 	}
 
+	// Update when the canary-serving-cert hash annotation changes on the pod template.
+	var currentHash, expectedHash string
+	if current.Spec.Template.Annotations != nil {
+		currentHash = current.Spec.Template.Annotations[CanaryServingCertHashAnnotation]
+	}
+	if expected.Spec.Template.Annotations != nil {
+		expectedHash = expected.Spec.Template.Annotations[CanaryServingCertHashAnnotation]
+	}
+	if currentHash != expectedHash {
+		if updated.Spec.Template.Annotations == nil {
+			updated.Spec.Template.Annotations = map[string]string{}
+		}
+		if expectedHash == "" {
+			delete(updated.Spec.Template.Annotations, CanaryServingCertHashAnnotation)
+		} else {
+			updated.Spec.Template.Annotations[CanaryServingCertHashAnnotation] = expectedHash
+		}
+		changed = true
+	}
+
 	if !changed {
 		return false, nil
 	}

pkg/operator/controller/canary/daemonset_test.go

@@ -16,7 +16,7 @@ import (
 func Test_desiredCanaryDaemonSet(t *testing.T) {
 	// canaryImageName is the ingress-operator image
 	canaryImageName := "openshift/origin-cluster-ingress-operator:latest"
-	daemonset := desiredCanaryDaemonSet(canaryImageName)
+	daemonset := desiredCanaryDaemonSet(canaryImageName, "")
 
 	expectedDaemonSetName := controller.CanaryDaemonSetName()
 
@@ -225,11 +225,21 @@ func Test_canaryDaemonsetChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if canary serving cert annotation changes",
+			mutate: func(ds *appsv1.DaemonSet) {
+				if ds.Spec.Template.Annotations == nil {
+					ds.Spec.Template.Annotations = map[string]string{}
+				}
+				ds.Spec.Template.Annotations[CanaryServingCertHashAnnotation] = "d34db33f"
+			},
+			expect: true,
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			original := desiredCanaryDaemonSet("")
+			original := desiredCanaryDaemonSet("", "")
 			mutated := original.DeepCopy()
 			tc.mutate(mutated)
 			if changed, updated := canaryDaemonSetChanged(original, mutated); changed != tc.expect {