should resync the grpc-server cert after clustermanager updated (#1126)

Signed-off-by: Wei Liu <liuweixa@redhat.com>

pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go

@@ -108,7 +108,7 @@ func (c certRotationController) sync(ctx context.Context, syncCtx factory.SyncCo
 
 		var errs []error
 		for i := range clustermanagers {
-			err = c.syncOne(ctx, syncCtx, clustermanagers[i])
+			err = c.syncOne(ctx, clustermanagers[i])
 			if err != nil {
 				errs = append(errs, err)
 			}
@@ -119,9 +119,10 @@ func (c certRotationController) sync(ctx context.Context, syncCtx factory.SyncCo
 		clustermanager, err := c.clusterManagerLister.Get(clustermanagerName)
 		// ClusterManager not found, could have been deleted, do nothing.
 		if errors.IsNotFound(err) {
-			return fmt.Errorf("no clustermanager for %s", clustermanagerName)
+			klog.V(4).Infof("ClusterManager %q not found; it may have been deleted", clustermanagerName)
+			return nil
 		}
-		err = c.syncOne(ctx, syncCtx, clustermanager)
+		err = c.syncOne(ctx, clustermanager)
 		if err != nil {
 			return err
 		}
@@ -129,7 +130,7 @@ func (c certRotationController) sync(ctx context.Context, syncCtx factory.SyncCo
 	}
 }
 
-func (c certRotationController) syncOne(ctx context.Context, syncCtx factory.SyncContext, clustermanager *operatorv1.ClusterManager) error {
+func (c certRotationController) syncOne(ctx context.Context, clustermanager *operatorv1.ClusterManager) error {
 	clustermanagerName := clustermanager.Name
 	clustermanagerNamespace := helpers.ClusterManagerNamespace(clustermanager.Name, clustermanager.Spec.DeployOption.Mode)
 
@@ -185,6 +186,11 @@ func (c certRotationController) syncOne(ctx context.Context, syncCtx factory.Syn
 
 	// delete the grpc serving secret if the grpc auth is disabled
 	if !helpers.GRPCAuthEnabled(clustermanager) {
+		if rotations, ok := c.rotationMap[clustermanager.Name]; ok {
+			rotations.targetRotations = removeRotation(rotations.targetRotations, helpers.GRPCServerSecret)
+			c.rotationMap[clustermanager.Name] = rotations
+		}
+
 		err = c.kubeClient.CoreV1().Secrets(clustermanagerNamespace).Delete(ctx, helpers.GRPCServerSecret, metav1.DeleteOptions{})
 		if err != nil && !errors.IsNotFound(err) {
 			return fmt.Errorf("clean up deleted cluster-manager, deleting grpc server secret failed, err:%s", err.Error())
@@ -226,19 +232,6 @@ func (c certRotationController) syncOne(ctx context.Context, syncCtx factory.Syn
 			},
 		}
 
-		if helpers.GRPCAuthEnabled(clustermanager) {
-			// maintain the grpc serving certs
-			// TODO may support user provided certs
-			targetRotations = append(targetRotations, certrotation.TargetRotation{
-				Namespace: clustermanagerNamespace,
-				Name:      helpers.GRPCServerSecret,
-				Validity:  TargetCertValidity,
-				HostNames: helpers.GRPCServerHostNames(clustermanagerNamespace, clustermanager),
-				Lister:    c.secretInformers[helpers.GRPCServerSecret].Lister(),
-				Client:    c.kubeClient.CoreV1(),
-			})
-		}
-
 		c.rotationMap[clustermanagerName] = rotations{
 			signingRotation:  signingRotation,
 			caBundleRotation: caBundleRotation,
@@ -247,7 +240,23 @@ func (c certRotationController) syncOne(ctx context.Context, syncCtx factory.Syn
 	}
 
 	// Ensure certificates are exists
-	rotations := c.rotationMap[clustermanagerName] // reconcile cert/key pair for signer
+	rotations := c.rotationMap[clustermanagerName]
+
+	if helpers.GRPCAuthEnabled(clustermanager) && !hasRotation(rotations.targetRotations, helpers.GRPCServerSecret) {
+		// maintain the grpc serving certs
+		// TODO may support user provided certs
+		rotations.targetRotations = append(rotations.targetRotations, certrotation.TargetRotation{
+			Namespace: clustermanagerNamespace,
+			Name:      helpers.GRPCServerSecret,
+			Validity:  TargetCertValidity,
+			HostNames: helpers.GRPCServerHostNames(clustermanagerNamespace, clustermanager),
+			Lister:    c.secretInformers[helpers.GRPCServerSecret].Lister(),
+			Client:    c.kubeClient.CoreV1(),
+		})
+		c.rotationMap[clustermanagerName] = rotations
+	}
+
+	// reconcile cert/key pair for signer
 	signingCertKeyPair, err := rotations.signingRotation.EnsureSigningCertKeyPair()
 	if err != nil {
 		return err
@@ -269,3 +278,22 @@ func (c certRotationController) syncOne(ctx context.Context, syncCtx factory.Syn
 
 	return errorhelpers.NewMultiLineAggregate(errs)
 }
+
+func hasRotation(targetRotations []certrotation.TargetRotation, name string) bool {
+	for _, rotation := range targetRotations {
+		if rotation.Name == name {
+			return true
+		}
+	}
+	return false
+}
+
+func removeRotation(targetRotations []certrotation.TargetRotation, name string) []certrotation.TargetRotation {
+	rs := []certrotation.TargetRotation{}
+	for _, rotation := range targetRotations {
+		if rotation.Name != name {
+			rs = append(rs, rotation)
+		}
+	}
+	return rs
+}

pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go

@@ -130,7 +130,7 @@ func TestCertRotation(t *testing.T) {
 			},
 		},
 		{
-			name: "Sync all clustermanagaers",
+			name: "Sync all clustermanagers",
 			clusterManagers: []*operatorapiv1.ClusterManager{
 				newClusterManager(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault),
 				newClusterManager(testClusterManagerNameHosted, operatorapiv1.InstallModeHosted),
@@ -202,6 +202,153 @@ func TestCertRotation(t *testing.T) {
 	}
 }
 
+func TestCertRotationGRPCAuth(t *testing.T) {
+	namespace := helpers.ClusterManagerNamespace(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault)
+
+	cases := []struct {
+		name                  string
+		clusterManager        *operatorapiv1.ClusterManager
+		updatedClusterManager *operatorapiv1.ClusterManager
+		existingObjects       []runtime.Object
+		validate              func(t *testing.T, kubeClient kubernetes.Interface, controller *certRotationController)
+	}{
+		{
+			name: "Enable GRPC",
+			clusterManager: func() *operatorapiv1.ClusterManager {
+				cm := newClusterManager(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault)
+				cm.Spec.RegistrationConfiguration = &operatorapiv1.RegistrationHubConfiguration{
+					RegistrationDrivers: []operatorapiv1.RegistrationDriverHub{
+						{
+							AuthType: commonhelpers.CSRAuthType,
+						},
+					},
+				}
+				return cm
+			}(),
+			updatedClusterManager: func() *operatorapiv1.ClusterManager {
+				return newClusterManager(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault)
+			}(),
+			existingObjects: []runtime.Object{
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: namespace,
+					},
+				},
+			},
+			validate: func(t *testing.T, kubeClient kubernetes.Interface, controller *certRotationController) {
+				// Check that GRPC server secret was created after update
+				_, err := kubeClient.CoreV1().Secrets(namespace).Get(context.Background(), helpers.GRPCServerSecret, metav1.GetOptions{})
+				if err != nil {
+					t.Fatalf("expected grpc server secret to be created after update, but got error: %v", err)
+				}
+
+				// Check that rotation was added to the map
+				rotations, ok := controller.rotationMap[testClusterManagerNameDefault]
+				if !ok {
+					t.Fatalf("expected rotations to exist in map")
+				}
+				if !hasRotation(rotations.targetRotations, helpers.GRPCServerSecret) {
+					t.Fatalf("expected grpc server rotation to be added after update, %v", rotations)
+				}
+			},
+		},
+		{
+			name: "Disable GRPC",
+			clusterManager: func() *operatorapiv1.ClusterManager {
+				return newClusterManager(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault)
+			}(),
+			updatedClusterManager: func() *operatorapiv1.ClusterManager {
+				cm := newClusterManager(testClusterManagerNameDefault, operatorapiv1.InstallModeDefault)
+				cm.Spec.RegistrationConfiguration = &operatorapiv1.RegistrationHubConfiguration{
+					RegistrationDrivers: []operatorapiv1.RegistrationDriverHub{
+						{
+							AuthType: commonhelpers.CSRAuthType,
+						},
+					},
+				}
+				return cm
+			}(),
+			existingObjects: []runtime.Object{
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: namespace,
+					},
+				},
+			},
+			validate: func(t *testing.T, kubeClient kubernetes.Interface, controller *certRotationController) {
+				// Check that GRPC server secret was deleted after update
+				_, err := kubeClient.CoreV1().Secrets(namespace).Get(context.Background(), helpers.GRPCServerSecret, metav1.GetOptions{})
+				if !errors.IsNotFound(err) {
+					t.Fatalf("expected GRPC server secret to be deleted after update, but got error: %v", err)
+				}
+
+				// Check that rotation was removed from the map
+				rotations, ok := controller.rotationMap[testClusterManagerNameDefault]
+				if ok && hasRotation(rotations.targetRotations, helpers.GRPCServerSecret) {
+					t.Fatalf("expected GRPC server rotation to be removed after update")
+				}
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			kubeClient := fakekube.NewSimpleClientset(c.existingObjects...)
+
+			newOnTermInformer := func(name string) kubeinformers.SharedInformerFactory {
+				return kubeinformers.NewSharedInformerFactoryWithOptions(kubeClient, 5*time.Minute,
+					kubeinformers.WithTweakListOptions(func(options *metav1.ListOptions) {
+						options.FieldSelector = fields.OneTermEqualSelector("metadata.name", name).String()
+					}))
+			}
+
+			secretInformers := map[string]corev1informers.SecretInformer{
+				helpers.SignerSecret:              newOnTermInformer(helpers.SignerSecret).Core().V1().Secrets(),
+				helpers.RegistrationWebhookSecret: newOnTermInformer(helpers.RegistrationWebhookSecret).Core().V1().Secrets(),
+				helpers.WorkWebhookSecret:         newOnTermInformer(helpers.WorkWebhookSecret).Core().V1().Secrets(),
+				helpers.GRPCServerSecret:          newOnTermInformer(helpers.GRPCServerSecret).Core().V1().Secrets(),
+			}
+
+			configmapInformer := newOnTermInformer(helpers.CaBundleConfigmap).Core().V1().ConfigMaps()
+
+			operatorClient := fakeoperatorclient.NewSimpleClientset(c.clusterManager)
+			operatorInformers := operatorinformers.NewSharedInformerFactory(operatorClient, 5*time.Minute)
+			clusterManagerStore := operatorInformers.Operator().V1().ClusterManagers().Informer().GetStore()
+			if err := clusterManagerStore.Add(c.clusterManager); err != nil {
+				t.Fatal(err)
+			}
+
+			syncContext := testingcommon.NewFakeSyncContext(t, testClusterManagerNameDefault)
+			recorder := syncContext.Recorder()
+
+			// Create the controller to check the rotation map
+			controller := &certRotationController{
+				rotationMap:          make(map[string]rotations),
+				kubeClient:           kubeClient,
+				secretInformers:      secretInformers,
+				configMapInformer:    configmapInformer,
+				recorder:             recorder,
+				clusterManagerLister: operatorInformers.Operator().V1().ClusterManagers().Lister(),
+			}
+
+			// First sync with initial configuration
+			if err := controller.sync(context.TODO(), syncContext); err != nil {
+				t.Fatal(err)
+			}
+
+			// update the cluster manager and sync again
+			if err := clusterManagerStore.Update(c.updatedClusterManager); err != nil {
+				t.Fatal(err)
+			}
+			if err := controller.sync(context.TODO(), syncContext); err != nil {
+				t.Fatal(err)
+			}
+
+			c.validate(t, kubeClient, controller)
+		})
+	}
+}
+
 func assertResourcesExistAndValid(t *testing.T, kubeClient kubernetes.Interface, namespace string) {
 	configmap, err := kubeClient.CoreV1().ConfigMaps(namespace).Get(context.Background(), "ca-bundle-configmap", metav1.GetOptions{})
 	if err != nil {