From b38dc2cceffdcd4e7e873aded3b8b084a9659dcb Mon Sep 17 00:00:00 2001 From: Jian Zhu Date: Tue, 11 Jun 2024 09:56:43 +0800 Subject: [PATCH] Update ClusterCertificateRotated to false if the signer name is invalid (#507) Signed-off-by: zhujian --- .../clientcert/cert_controller.go | 43 ++++++++---- .../registration/addon_registration_test.go | 66 +++++++++++++++---- 2 files changed, 83 insertions(+), 26 deletions(-) diff --git a/pkg/registration/clientcert/cert_controller.go b/pkg/registration/clientcert/cert_controller.go index fc67bbcf4..f9421a0ac 100644 --- 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() + 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 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 } - 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) - if err != nil { - 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 index 51c61dced..ca6100a4a 100644 --- 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