Update ClusterCertificateRotated to false if the signer name is inval…

…id (#507) Signed-off-by: zhujian <jiazhu@redhat.com>
open-cluster-management-io · Jun 11, 2024 · b38dc2c · b38dc2c
1 parent 9b36e11
commit b38dc2c
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 26 deletions.
diff --git a/pkg/registration/clientcert/cert_controller.go b/pkg/registration/clientcert/cert_controller.go
@@ -316,24 +316,39 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory.
 		return nil
 	}
 
-	// create a new private key
-	keyData, err := keyutil.MakeEllipticPrivateKeyPEM()
-	if err != nil {
-		return err
-	}
+	keyData, createdCSRName, err := func() ([]byte, string, error) {
+		// create a new private key
+		keyData, err := keyutil.MakeEllipticPrivateKeyPEM()
+		if err != nil {
+			return nil, "", err
+		}
 
-	privateKey, err := keyutil.ParsePrivateKeyPEM(keyData)
-	if err != nil {
-		return fmt.Errorf("invalid private key for certificate request: %w", err)
-	}
-	csrData, err := certutil.MakeCSR(privateKey, c.Subject, c.DNSNames, nil)
-	if err != nil {
-		return fmt.Errorf("unable to generate certificate request: %w", err)
-	}
-	createdCSRName, err := c.csrControl.create(ctx, syncCtx.Recorder(), c.ObjectMeta, csrData, c.SignerName, c.ExpirationSeconds)
+		privateKey, err := keyutil.ParsePrivateKeyPEM(keyData)
+		if err != nil {
+			return keyData, "", fmt.Errorf("invalid private key for certificate request: %w", err)
+		}
+		csrData, err := certutil.MakeCSR(privateKey, c.Subject, c.DNSNames, nil)
+		if err != nil {
+			return keyData, "", fmt.Errorf("unable to generate certificate request: %w", err)
+		}
+		createdCSRName, err := c.csrControl.create(ctx, syncCtx.Recorder(), c.ObjectMeta, csrData, c.SignerName, c.ExpirationSeconds)
+		if err != nil {
+			return keyData, "", err
+		}
+		return keyData, createdCSRName, nil
+	}()
 	if err != nil {
+		if updateErr := c.statusUpdater(ctx, metav1.Condition{
+			Type:    "ClusterCertificateRotated",
+			Status:  metav1.ConditionFalse,
+			Reason:  "ClientCertificateUpdateFailed",
+			Message: fmt.Sprintf("Failed to create CSR %v", err),
+		}); updateErr != nil {
+			return updateErr
+		}
 		return err
 	}
+
 	c.keyData = keyData
 	c.csrName = createdCSRName
 	return nil

diff --git a/test/integration/registration/addon_registration_test.go b/test/integration/registration/addon_registration_test.go
@@ -203,13 +203,25 @@ var _ = ginkgo.Describe("Addon Registration", func() {
 	assertClientCertCondition := func(clusterName, addonName string) {
 		ginkgo.By("Check clientcert addon status condition")
 		gomega.Eventually(func() bool {
-			addon, err := addOnClient.AddonV1alpha1().ManagedClusterAddOns(clusterName).Get(context.TODO(), addOnName, metav1.GetOptions{})
+			addon, err := addOnClient.AddonV1alpha1().ManagedClusterAddOns(clusterName).
+				Get(context.TODO(), addonName, metav1.GetOptions{})
 			if err != nil {
 				return false
 			}
 			return meta.IsStatusConditionTrue(addon.Status.Conditions, clientcert.ClusterCertificateRotatedCondition)
 		}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
 	}
+	assertClientCertConditionFalse := func(clusterName, addonName string) {
+		ginkgo.By("Check if clientcert addon status condition is false")
+		gomega.Eventually(func() bool {
+			addon, err := addOnClient.AddonV1alpha1().ManagedClusterAddOns(clusterName).
+				Get(context.TODO(), addonName, metav1.GetOptions{})
+			if err != nil {
+				return false
+			}
+			return meta.IsStatusConditionFalse(addon.Status.Conditions, clientcert.ClusterCertificateRotatedCondition)
+		}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
+	}
 
 	assertHasNoAddonLabel := func(clusterName, addonName string) {
 		ginkgo.By("Check if addon status label on managed cluster deleted")
@@ -227,7 +239,28 @@ var _ = ginkgo.Describe("Addon Registration", func() {
 		}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
 	}
 
-	assertSuccessAddOnBootstrap := func(signerName string) {
+	assertAddOnSignerUpdate := func(signerName string) {
+		gomega.Eventually(func() error {
+			addOn, err := addOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
+				Get(context.TODO(), addOnName, metav1.GetOptions{})
+			if err != nil {
+				return err
+			}
+
+			addOn.Status = addonv1alpha1.ManagedClusterAddOnStatus{
+				Registrations: []addonv1alpha1.RegistrationConfig{
+					{
+						SignerName: signerName,
+					},
+				},
+			}
+			_, err = addOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
+				UpdateStatus(context.TODO(), addOn, metav1.UpdateOptions{})
+			return err
+		}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
+	}
+
+	assertSuccessAddOnEnabling := func() {
 		ginkgo.By("Create ManagedClusterAddOn cr with required annotations")
 		// create addon namespace
 		ns := &corev1.Namespace{
@@ -251,18 +284,13 @@ var _ = ginkgo.Describe("Addon Registration", func() {
 		_, err = addOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).Create(context.TODO(), addOn, metav1.CreateOptions{})
 		gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
-		created, err := addOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).Get(context.TODO(), addOnName, metav1.GetOptions{})
-		gomega.Expect(err).NotTo(gomega.HaveOccurred())
-		created.Status = addonv1alpha1.ManagedClusterAddOnStatus{
-			Registrations: []addonv1alpha1.RegistrationConfig{
-				{
-					SignerName: signerName,
-				},
-			},
-		}
-		_, err = addOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).UpdateStatus(context.TODO(), created, metav1.UpdateOptions{})
+		_, err := addOnClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).Get(context.TODO(), addOnName, metav1.GetOptions{})
 		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	}
 
+	assertSuccessAddOnBootstrap := func(signerName string) {
+		assertSuccessAddOnEnabling()
+		assertAddOnSignerUpdate(signerName)
 		assertSuccessCSRApproval()
 		assertValidClientCertificate(addOnName, getSecretName(addOnName, signerName), signerName, expectedProxyURL)
 		assertAddonLabel(managedClusterName, addOnName, "unreachable")
@@ -364,6 +392,20 @@ var _ = ginkgo.Describe("Addon Registration", func() {
 			assertSuccessAddOnBootstrap(signerName)
 		})
 
+		ginkgo.It("should register addon failed with invalid custom signer", func() {
+			assertSuccessClusterBootstrap()
+			assertSuccessAddOnEnabling()
+			assertAddOnSignerUpdate("addon-xxx")
+			assertClientCertConditionFalse(managedClusterName, addOnName)
+
+			signerName := "example.com/signer1"
+			assertAddOnSignerUpdate(signerName)
+			assertSuccessCSRApproval()
+			assertValidClientCertificate(addOnName, getSecretName(addOnName, signerName), signerName, expectedProxyURL)
+			assertAddonLabel(managedClusterName, addOnName, "unreachable")
+			assertClientCertCondition(managedClusterName, addOnName)
+		})
+
 		ginkgo.It("should addon registraton config updated successfully", func() {
 			assertSuccessClusterBootstrap()
 			signerName := certificates.KubeAPIServerClientSignerName