diff --git a/pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go b/pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go index 4f184ccc9..519cec76b 100644 --- a/pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller.go +++ b/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 +} diff --git a/pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go b/pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go index 2606daa93..330533156 100644 --- a/pkg/operator/operators/clustermanager/controllers/certrotationcontroller/certrotation_controller_test.go +++ b/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 {