diff --git a/pkg/registration/register/csr/cert_controller.go b/pkg/registration/register/csr/cert_controller.go deleted file mode 100644 index c7f3ed55d..000000000 --- a/pkg/registration/register/csr/cert_controller.go +++ /dev/null @@ -1,392 +0,0 @@ -package csr - -import ( - "context" - "crypto/tls" - "crypto/x509/pkix" - "fmt" - "math/rand" - "reflect" - "time" - - "github.com/openshift/library-go/pkg/controller/factory" - "github.com/openshift/library-go/pkg/operator/events" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - corev1client "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/client-go/tools/cache" - certutil "k8s.io/client-go/util/cert" - "k8s.io/client-go/util/keyutil" - "k8s.io/klog/v2" - - "open-cluster-management.io/ocm/pkg/registration/register" -) - -// ControllerResyncInterval is exposed so that integration tests can crank up the constroller sync speed. -var ControllerResyncInterval = 5 * time.Minute - -type secretOption struct { - // SecretNamespace is the namespace of the secret containing client certificate. - secretNamespace string - // SecretName is the name of the secret containing client certificate. The secret will be created if - // it does not exist. - secretName string - // AdditonalSecretData contains data that will be added into the secret - additionalSecretData map[string][]byte -} - -// clientCertificateController implements the common logic of hub client certification creation/rotation. It -// creates a client certificate and rotates it before it becomes expired by using csrs. The client -// certificate generated is stored in a specific secret with the keys below: -// 1). tls.key: tls key file -// 2). tls.crt: tls cert file -type clientCertificateController struct { - secretOption - CSROption - controllerName string - - // csrName is the name of csr created by controller and waiting for approval. - csrName string - - // keyData is the private key data used to created a csr - // csrName and keyData store the internal state of the controller. They are set after controller creates a new csr - // and cleared once the csr is approved and processed by controller. There are 4 combination of their values: - // 1. csrName empty, keyData empty: means we aren't trying to create a new client cert, our current one is valid - // 2. csrName set, keyData empty: there was bug - // 3. csrName set, keyData set: we are waiting for a new cert to be signed. - // 4. csrName empty, keydata set: the CSR failed to create, this shouldn't happen, it's a bug. - keyData []byte - - managementCoreClient corev1client.CoreV1Interface - statusUpdater register.StatusUpdateFunc -} - -// NewClientCertificateController return an instance of clientCertificateController -func NewClientCertificateController( - secretNamespace string, - secretName string, - additionalSecretData map[string][]byte, - csrOption CSROption, - statusUpdater register.StatusUpdateFunc, - managementSecretInformer cache.SharedIndexInformer, - managementCoreClient corev1client.CoreV1Interface, - recorder events.Recorder, - controllerName string, -) factory.Controller { - c := clientCertificateController{ - secretOption: secretOption{ - secretName: secretName, - secretNamespace: secretNamespace, - additionalSecretData: additionalSecretData, - }, - CSROption: csrOption, - controllerName: controllerName, - statusUpdater: statusUpdater, - managementCoreClient: managementCoreClient, - } - - return factory.New(). - WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string { - return factory.DefaultQueueKey - }, func(obj interface{}) bool { - accessor, err := meta.Accessor(obj) - if err != nil { - return false - } - // only enqueue a specific secret - if accessor.GetNamespace() == c.secretNamespace && accessor.GetName() == c.secretName { - return true - } - return false - }, managementSecretInformer). - WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string { - return factory.DefaultQueueKey - }, c.EventFilterFunc, csrOption.CSRControl.Informer()). - WithSync(c.sync). - ResyncEvery(ControllerResyncInterval). - ToController(controllerName, recorder) -} - -func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory.SyncContext) error { - logger := klog.FromContext(ctx) - // get secret containing client certificate - secret, err := c.managementCoreClient.Secrets(c.secretNamespace).Get(ctx, c.secretName, metav1.GetOptions{}) - switch { - case apierrors.IsNotFound(err): - secret = &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: c.secretNamespace, - Name: c.secretName, - }, - } - case err != nil: - return fmt.Errorf("unable to get secret %q: %w", c.secretNamespace+"/"+c.secretName, err) - } - - // reconcile pending csr if exists - if len(c.csrName) > 0 { - // build a secret data map if the csr is approved - newSecretConfig, err := func() (map[string][]byte, error) { - // skip if there is no ongoing csr - if len(c.csrName) == 0 { - return nil, fmt.Errorf("no ongoing csr") - } - - // skip if csr is not approved yet - isApproved, err := c.CSROption.CSRControl.isApproved(c.csrName) - if err != nil { - return nil, err - } - if !isApproved { - return nil, nil - } - - // skip if csr is not issued - certData, err := c.CSROption.CSRControl.getIssuedCertificate(c.csrName) - if err != nil { - return nil, err - } - if len(certData) == 0 { - return nil, nil - } - - logger.V(4).Info("Sync csr", "name", c.csrName) - // check if cert in csr status matches with the corresponding private key - if c.keyData == nil { - return nil, fmt.Errorf("no private key found for certificate in csr: %s", c.csrName) - } - _, err = tls.X509KeyPair(certData, c.keyData) - if err != nil { - return nil, fmt.Errorf("private key does not match with the certificate in csr: %s", c.csrName) - } - - data := map[string][]byte{ - TLSCertFile: certData, - TLSKeyFile: c.keyData, - } - - return data, nil - }() - - if err != nil { - c.reset() - if updateErr := c.statusUpdater(ctx, metav1.Condition{ - Type: "ClusterCertificateRotated", - Status: metav1.ConditionFalse, - Reason: "ClientCertificateUpdateFailed", - Message: fmt.Sprintf("Failed to rotated client certificate %v", err), - }); updateErr != nil { - return updateErr - } - return err - } - if len(newSecretConfig) == 0 { - return nil - } - // append additional data into client certificate secret - for k, v := range c.additionalSecretData { - newSecretConfig[k] = v - } - secret.Data = newSecretConfig - // save the changes into secret - if err := saveSecret(c.managementCoreClient, c.secretNamespace, secret); err != nil { - if updateErr := c.statusUpdater(ctx, metav1.Condition{ - Type: "ClusterCertificateRotated", - Status: metav1.ConditionFalse, - Reason: "ClientCertificateUpdateFailed", - Message: fmt.Sprintf("Failed to rotated client certificate %v", err), - }); updateErr != nil { - return updateErr - } - return err - } - - notBefore, notAfter, err := getCertValidityPeriod(secret) - - cond := metav1.Condition{ - Type: "ClusterCertificateRotated", - Status: metav1.ConditionTrue, - Reason: "ClientCertificateUpdated", - Message: fmt.Sprintf("client certificate rotated starting from %v to %v", *notBefore, *notAfter), - } - - if err != nil { - cond = metav1.Condition{ - Type: "ClusterCertificateRotated", - Status: metav1.ConditionFalse, - Reason: "ClientCertificateUpdateFailed", - Message: fmt.Sprintf("Failed to rotated client certificate %v", err), - } - } - if updateErr := c.statusUpdater(ctx, cond); updateErr != nil { - return updateErr - } - - if err != nil { - c.reset() - return err - } - - syncCtx.Recorder().Eventf("ClientCertificateCreated", "A new client certificate for %s is available", c.controllerName) - c.reset() - return nil - } - - // create a csr to request new client certificate if - // a. there is no valid client certificate issued for the current cluster/agent; - // b. client certificate is sensitive to the additional secret data and the data changes; - // c. client certificate exists and has less than a random percentage range from 20% to 25% of its life remaining; - shouldCreate, err := shouldCreateCSR( - logger, - c.controllerName, - secret, - syncCtx.Recorder(), - c.Subject, - c.additionalSecretData) - if err != nil { - return err - } - if !shouldCreate { - return nil - } - - shouldHalt := c.CSROption.HaltCSRCreation() - if shouldHalt { - if updateErr := c.statusUpdater(ctx, metav1.Condition{ - Type: "ClusterCertificateRotated", - Status: metav1.ConditionFalse, - Reason: "ClientCertificateUpdateFailed", - Message: "Stop creating csr since there are too many csr created already on hub", - }); updateErr != nil { - return updateErr - } - syncCtx.Recorder().Eventf("ClientCertificateCreationHalted", "Stop creating csr since there are too many csr created already on hub", c.controllerName) - return nil - } - - 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.CSROption.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 -} - -func saveSecret(spokeCoreClient corev1client.CoreV1Interface, secretNamespace string, secret *corev1.Secret) error { - var err error - if secret.ResourceVersion == "" { - _, err = spokeCoreClient.Secrets(secretNamespace).Create(context.Background(), secret, metav1.CreateOptions{}) - return err - } - _, err = spokeCoreClient.Secrets(secretNamespace).Update(context.Background(), secret, metav1.UpdateOptions{}) - return err -} - -func (c *clientCertificateController) reset() { - c.csrName = "" - c.keyData = nil -} - -func shouldCreateCSR( - logger klog.Logger, - controllerName string, - secret *corev1.Secret, - recorder events.Recorder, - subject *pkix.Name, - additionalSecretData map[string][]byte) (bool, error) { - // create a csr to request new client certificate if - // a.there is no valid client certificate issued for the current cluster/agent - valid, err := isCertificateValid(logger, secret.Data[TLSCertFile], subject) - if err != nil { - recorder.Eventf("CertificateValidationFailed", "Failed to validate client certificate for %s: %v", controllerName, err) - return true, nil - } - if !valid { - recorder.Eventf("NoValidCertificateFound", "No valid client certificate for %s is found. Bootstrap is required", controllerName) - return true, nil - } - - // b.client certificate is sensitive to the additional secret data and the data changes - if err := hasAdditionalSecretData(additionalSecretData, secret); err != nil { - recorder.Eventf("AdditonalSecretDataChanged", "The additional secret data is changed for %v. Re-create the client certificate for %s", err, controllerName) - return true, nil - } - - // c.client certificate exists and has less than a random percentage range from 20% to 25% of its life remaining - notBefore, notAfter, err := getCertValidityPeriod(secret) - if err != nil { - return false, err - } - total := notAfter.Sub(*notBefore) - remaining := time.Until(*notAfter) - logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, - "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) - threshold := jitter(0.2, 0.25) - if remaining.Seconds()/total.Seconds() > threshold { - // Do nothing if the client certificate is valid and has more than a random percentage range from 20% to 25% of its life remaining - logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, - "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) - return false, nil - } - recorder.Eventf("CertificateRotationStarted", - "The current client certificate for %s expires in %v. Start certificate rotation", - controllerName, remaining.Round(time.Second)) - return true, nil -} - -// hasAdditonalSecretData checks if the secret includes the expected additional secret data. -func hasAdditionalSecretData(additionalSecretData map[string][]byte, secret *corev1.Secret) error { - for k, v := range additionalSecretData { - value, ok := secret.Data[k] - if !ok { - return fmt.Errorf("key %q not found in secret %q", k, secret.Namespace+"/"+secret.Name) - } - - if !reflect.DeepEqual(v, value) { - return fmt.Errorf("key %q in secret %q does not match the expected value", - k, secret.Namespace+"/"+secret.Name) - } - } - return nil -} - -func jitter(percentage float64, maxFactor float64) float64 { - if maxFactor <= 0.0 { - maxFactor = 1.0 - } - newPercentage := percentage + percentage*rand.Float64()*maxFactor //#nosec G404 - return newPercentage -} diff --git a/pkg/registration/register/csr/certificate.go b/pkg/registration/register/csr/certificate.go index eb973bb89..fa9b433e6 100644 --- a/pkg/registration/register/csr/certificate.go +++ b/pkg/registration/register/csr/certificate.go @@ -36,7 +36,7 @@ import ( func isCertificateValid(logger klog.Logger, certData []byte, subject *pkix.Name) (bool, error) { certs, err := certutil.ParseCertsPEM(certData) if err != nil { - return false, errors.New("unable to parse certificate") + return false, fmt.Errorf("unable to parse certificate: %v", err) } if len(certs) == 0 { diff --git a/pkg/registration/register/csr/certificate_test.go b/pkg/registration/register/csr/certificate_test.go index c1c9150c0..2595cfc72 100644 --- a/pkg/registration/register/csr/certificate_test.go +++ b/pkg/registration/register/csr/certificate_test.go @@ -265,10 +265,7 @@ func TestBuildKubeconfig(t *testing.T) { } registerImpl := &CSRDriver{} - kubeconfig, err := registerImpl.BuildKubeConfigFromBootstrap(bootstrapKubeconfig) - if err != nil { - t.Fatal(err) - } + kubeconfig := registerImpl.BuildKubeConfigFromTemplate(bootstrapKubeconfig) currentContext, ok := kubeconfig.Contexts[kubeconfig.CurrentContext] if !ok { t.Errorf("current context %q not found: %v", kubeconfig.CurrentContext, kubeconfig) diff --git a/pkg/registration/register/csr/controller_test.go b/pkg/registration/register/csr/controller_test.go deleted file mode 100644 index 738f844db..000000000 --- a/pkg/registration/register/csr/controller_test.go +++ /dev/null @@ -1,309 +0,0 @@ -package csr - -import ( - "context" - "crypto/x509/pkix" - "fmt" - "testing" - "time" - - "github.com/openshift/library-go/pkg/operator/events" - certificates "k8s.io/api/certificates/v1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/rand" - kubefake "k8s.io/client-go/kubernetes/fake" - clienttesting "k8s.io/client-go/testing" - "k8s.io/client-go/tools/cache" - "k8s.io/klog/v2/ktesting" - - testingcommon "open-cluster-management.io/ocm/pkg/common/testing" - testinghelpers "open-cluster-management.io/ocm/pkg/registration/helpers/testing" - "open-cluster-management.io/ocm/pkg/registration/hub/user" - "open-cluster-management.io/ocm/pkg/registration/register" -) - -const ( - testNamespace = "testns" - testAgentName = "testagent" - testSecretName = "testsecret" - testCSRName = "testcsr" -) - -var commonName = fmt.Sprintf("%s%s:%s", user.SubjectPrefix, testinghelpers.TestManagedClusterName, testAgentName) - -func TestSync(t *testing.T) { - testSubject := &pkix.Name{ - CommonName: commonName, - } - - cases := []struct { - name string - queueKey string - secrets []runtime.Object - approvedCSRCert *testinghelpers.TestCert - keyDataExpected bool - csrNameExpected bool - expectedCondition *metav1.Condition - validateActions func(t *testing.T, hubActions, agentActions []clienttesting.Action) - }{ - { - name: "agent bootstrap", - secrets: []runtime.Object{}, - queueKey: "key", - keyDataExpected: true, - csrNameExpected: true, - validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) { - testingcommon.AssertActions(t, hubActions, "create") - actual := hubActions[0].(clienttesting.CreateActionImpl).Object - if _, ok := actual.(*unstructured.Unstructured); !ok { - t.Errorf("expected csr was created, but failed") - } - testingcommon.AssertActions(t, agentActions, "get") - }, - }, - { - name: "syc csr after bootstrap", - queueKey: testSecretName, - secrets: []runtime.Object{ - testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "1", nil, map[string][]byte{ - register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), - register.AgentNameFile: []byte(testAgentName), - }, - ), - }, - expectedCondition: &metav1.Condition{ - Type: ClusterCertificateRotatedCondition, - Status: metav1.ConditionTrue, - }, - approvedCSRCert: testinghelpers.NewTestCert(commonName, 10*time.Second), - validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) { - logger, _ := ktesting.NewTestContext(t) - testingcommon.AssertActions(t, hubActions, "get", "get") - testingcommon.AssertActions(t, agentActions, "get", "update") - actual := agentActions[1].(clienttesting.UpdateActionImpl).Object - secret := actual.(*corev1.Secret) - valid, err := isCertificateValid(logger, secret.Data[TLSCertFile], testSubject) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if !valid { - t.Error("client certificate is invalid") - } - }, - }, - { - name: "sync a valid hub kubeconfig secret", - queueKey: testSecretName, - secrets: []runtime.Object{ - testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "1", testinghelpers.NewTestCert(commonName, 10000*time.Second), map[string][]byte{ - register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), - register.AgentNameFile: []byte(testAgentName), - register.KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - }, - validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) { - testingcommon.AssertNoActions(t, hubActions) - testingcommon.AssertActions(t, agentActions, "get") - }, - }, - { - name: "sync an expiring hub kubeconfig secret", - queueKey: testSecretName, - secrets: []runtime.Object{ - testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "1", testinghelpers.NewTestCert(commonName, -3*time.Second), map[string][]byte{ - register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), - register.AgentNameFile: []byte(testAgentName), - register.KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - }, - keyDataExpected: true, - csrNameExpected: true, - validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) { - testingcommon.AssertActions(t, hubActions, "create") - actual := hubActions[0].(clienttesting.CreateActionImpl).Object - if _, ok := actual.(*unstructured.Unstructured); !ok { - t.Errorf("expected csr was created, but failed") - } - testingcommon.AssertActions(t, agentActions, "get") - }, - }, - { - name: "sync when additional secret data changes", - queueKey: testSecretName, - secrets: []runtime.Object{ - testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "1", testinghelpers.NewTestCert(commonName, 10000*time.Second), map[string][]byte{ - register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), - register.AgentNameFile: []byte("invalid-name"), - }), - }, - keyDataExpected: true, - csrNameExpected: true, - validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) { - testingcommon.AssertActions(t, hubActions, "create") - actual := hubActions[0].(clienttesting.CreateActionImpl).Object - if _, ok := actual.(*unstructured.Unstructured); !ok { - t.Errorf("expected csr was created, but failed") - } - testingcommon.AssertActions(t, agentActions, "get") - }, - }, - } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - ctrl := &mockCSRControl{} - var csrs []runtime.Object - if c.approvedCSRCert != nil { - csr := testinghelpers.NewApprovedCSR(testinghelpers.CSRHolder{Name: testCSRName}) - csr.Status.Certificate = c.approvedCSRCert.Cert - csrs = append(csrs, csr) - ctrl.approved = true - ctrl.issuedCertData = c.approvedCSRCert.Cert - } - hubKubeClient := kubefake.NewSimpleClientset(csrs...) - ctrl.csrClient = &hubKubeClient.Fake - - // GenerateName is not working for fake clent, we set the name with prepend reactor - hubKubeClient.PrependReactor( - "create", - "certificatesigningrequests", - func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { - return true, testinghelpers.NewCSR(testinghelpers.CSRHolder{Name: testCSRName}), nil - }, - ) - agentKubeClient := kubefake.NewSimpleClientset(c.secrets...) - - clientCertOption := secretOption{ - secretNamespace: testNamespace, - secretName: testSecretName, - additionalSecretData: map[string][]byte{ - register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), - register.AgentNameFile: []byte(testAgentName), - }, - } - csrOption := CSROption{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Subject: testSubject, - SignerName: certificates.KubeAPIServerClientSignerName, - HaltCSRCreation: func() bool { return false }, - CSRControl: ctrl, - } - - updater := &fakeStatusUpdater{} - - controller := &clientCertificateController{ - secretOption: clientCertOption, - CSROption: csrOption, - managementCoreClient: agentKubeClient.CoreV1(), - controllerName: "test-agent", - statusUpdater: updater.update, - } - - if c.approvedCSRCert != nil { - controller.csrName = testCSRName - controller.keyData = c.approvedCSRCert.Key - } - - err := controller.sync(context.TODO(), testingcommon.NewFakeSyncContext(t, c.queueKey)) - if err != nil { - t.Errorf("unexpected error %v", err) - } - - hasKeyData := controller.keyData != nil - if c.keyDataExpected != hasKeyData { - t.Error("controller.keyData should be set") - } - - hasCSRName := controller.csrName != "" - if c.csrNameExpected != hasCSRName { - t.Error("controller.csrName should be set") - } - - if !conditionEqual(c.expectedCondition, updater.cond) { - t.Errorf("condition is not correct, expected %v, got %v", c.expectedCondition, updater.cond) - } - - c.validateActions(t, hubKubeClient.Actions(), agentKubeClient.Actions()) - }) - } -} - -var _ CSRControl = &mockCSRControl{} - -func conditionEqual(expected, actual *metav1.Condition) bool { - if expected == nil && actual == nil { - return true - } - - if expected == nil || actual == nil { - return false - } - - if expected.Type != actual.Type { - return false - } - - if string(expected.Status) != string(actual.Status) { - return false - } - - return true -} - -type fakeStatusUpdater struct { - cond *metav1.Condition -} - -func (f *fakeStatusUpdater) update(_ context.Context, cond metav1.Condition) error { - f.cond = cond.DeepCopy() - return nil -} - -type mockCSRControl struct { - approved bool - issuedCertData []byte - csrClient *clienttesting.Fake -} - -func (m *mockCSRControl) create( - _ context.Context, _ events.Recorder, objMeta metav1.ObjectMeta, _ []byte, _ string, _ *int32) (string, error) { - mockCSR := &unstructured.Unstructured{} - _, err := m.csrClient.Invokes(clienttesting.CreateActionImpl{ - ActionImpl: clienttesting.ActionImpl{ - Verb: "create", - }, - Object: mockCSR, - }, nil) - return objMeta.Name + rand.String(4), err -} - -func (m *mockCSRControl) isApproved(name string) (bool, error) { - _, err := m.csrClient.Invokes(clienttesting.GetActionImpl{ - ActionImpl: clienttesting.ActionImpl{ - Verb: "get", - Resource: certificates.SchemeGroupVersion.WithResource("certificatesigningrequests"), - }, - Name: name, - }, nil) - - return m.approved, err -} - -func (m *mockCSRControl) getIssuedCertificate(name string) ([]byte, error) { - _, err := m.csrClient.Invokes(clienttesting.GetActionImpl{ - ActionImpl: clienttesting.ActionImpl{ - Verb: "get", - Resource: certificates.SchemeGroupVersion.WithResource("certificatesigningrequests"), - }, - Name: name, - }, nil) - return m.issuedCertData, err -} - -func (m *mockCSRControl) Informer() cache.SharedIndexInformer { - panic("implement me") -} diff --git a/pkg/registration/register/csr/csr.go b/pkg/registration/register/csr/csr.go index 89e19d5f3..c43c8fa26 100644 --- a/pkg/registration/register/csr/csr.go +++ b/pkg/registration/register/csr/csr.go @@ -2,16 +2,24 @@ package csr import ( "context" + "crypto/tls" "crypto/x509/pkix" "fmt" + "math/rand" "os" "path" + "reflect" + "time" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/client-go/tools/cache" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + certutil "k8s.io/client-go/util/cert" + "k8s.io/client-go/util/keyutil" "k8s.io/klog/v2" "open-cluster-management.io/ocm/pkg/registration/register" @@ -60,38 +68,200 @@ type CSROption struct { HaltCSRCreation func() bool } -type CSRDriver struct{} +type CSRDriver struct { + // csrName is the name of csr created by controller and waiting for approval. + csrName string -func (c *CSRDriver) Start( - ctx context.Context, - name string, - updater register.StatusUpdateFunc, - recorder events.Recorder, - secretOption register.SecretOption, - option any, - addtionalData map[string][]byte) { - csrOption, ok := option.(*CSROption) + // keyData is the private key data used to created a csr + // csrName and keyData store the internal state of the controller. They are set after controller creates a new csr + // and cleared once the csr is approved and processed by controller. There are 4 combination of their values: + // 1. csrName empty, keyData empty: means we aren't trying to create a new client cert, our current one is valid + // 2. csrName set, keyData empty: there was bug + // 3. csrName set, keyData set: we are waiting for a new cert to be signed. + // 4. csrName empty, keydata set: the CSR failed to create, this shouldn't happen, it's a bug. + keyData []byte +} + +func (c *CSRDriver) Process( + ctx context.Context, controllerName string, secret *corev1.Secret, additionalSecretData map[string][]byte, + recorder events.Recorder, opt any) (*corev1.Secret, *metav1.Condition, error) { + logger := klog.FromContext(ctx) + csrOption, ok := opt.(*CSROption) if !ok { - utilruntime.Must(fmt.Errorf("option type is not correct")) + return nil, nil, fmt.Errorf("option type is not correct") } - ctrl := NewClientCertificateController( - secretOption.SecretNamespace, secretOption.SecretName, addtionalData, *csrOption, updater, - secretOption.ManagementSecretInformer, secretOption.ManagementCoreClient, recorder, name) - ctrl.Run(ctx, 1) -} -func (c *CSRDriver) BuildKubeConfigFromBootstrap(bootstrapConfig *clientcmdapi.Config) (*clientcmdapi.Config, error) { - kubeConfig, err := register.BaseKubeConfigFromBootStrap(bootstrapConfig) + // reconcile pending csr if exists + if len(c.csrName) > 0 { + // build a secret data map if the csr is approved + newSecretConfig, err := func() (map[string][]byte, error) { + // skip if there is no ongoing csr + if len(c.csrName) == 0 { + return nil, fmt.Errorf("no ongoing csr") + } + + // skip if csr is not approved yet + isApproved, err := csrOption.CSRControl.isApproved(c.csrName) + if err != nil { + return nil, err + } + if !isApproved { + return nil, nil + } + + // skip if csr is not issued + certData, err := csrOption.CSRControl.getIssuedCertificate(c.csrName) + if err != nil { + return nil, err + } + if len(certData) == 0 { + return nil, nil + } + + logger.Info("Sync csr", "name", c.csrName) + // check if cert in csr status matches with the corresponding private key + if c.keyData == nil { + return nil, fmt.Errorf("no private key found for certificate in csr: %s", c.csrName) + } + _, err = tls.X509KeyPair(certData, c.keyData) + if err != nil { + return nil, fmt.Errorf("private key does not match with the certificate in csr: %s", c.csrName) + } + + data := map[string][]byte{ + TLSCertFile: certData, + TLSKeyFile: c.keyData, + } + + return data, nil + }() + + if err != nil { + c.reset() + return secret, &metav1.Condition{ + Type: "ClusterCertificateRotated", + Status: metav1.ConditionFalse, + Reason: "ClientCertificateUpdateFailed", + Message: fmt.Sprintf("Failed to rotated client certificate %v", err), + }, err + } + if len(newSecretConfig) == 0 { + return nil, nil, nil + } + // append additional data into client certificate secret + for k, v := range newSecretConfig { + secret.Data[k] = v + } + + notBefore, notAfter, err := getCertValidityPeriod(secret) + + cond := &metav1.Condition{ + Type: "ClusterCertificateRotated", + Status: metav1.ConditionTrue, + Reason: "ClientCertificateUpdated", + Message: fmt.Sprintf("client certificate rotated starting from %v to %v", *notBefore, *notAfter), + } + + if err != nil { + cond = &metav1.Condition{ + Type: "ClusterCertificateRotated", + Status: metav1.ConditionFalse, + Reason: "ClientCertificateUpdateFailed", + Message: fmt.Sprintf("Failed to rotated client certificate %v", err), + } + } else { + recorder.Eventf("ClientCertificateCreated", "A new client certificate for %s is available", controllerName) + } + c.reset() + return secret, cond, err + } + + // create a csr to request new client certificate if + // a. there is no valid client certificate issued for the current cluster/agent; + // b. client certificate is sensitive to the additional secret data and the data changes; + // c. client certificate exists and has less than a random percentage range from 20% to 25% of its life remaining; + shouldCreate, err := shouldCreateCSR( + logger, + controllerName, + secret, + recorder, + csrOption.Subject, + additionalSecretData) + if err != nil { + return secret, nil, err + } + if !shouldCreate { + return nil, nil, nil + } + + shouldHalt := csrOption.HaltCSRCreation() + if shouldHalt { + recorder.Eventf("ClientCertificateCreationHalted", + "Stop creating csr since there are too many csr created already on hub", controllerName) + return nil, &metav1.Condition{ + Type: "ClusterCertificateRotated", + Status: metav1.ConditionFalse, + Reason: "ClientCertificateUpdateFailed", + Message: "Stop creating csr since there are too many csr created already on hub", + }, nil + } + + 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, csrOption.Subject, csrOption.DNSNames, nil) + if err != nil { + return keyData, "", fmt.Errorf("unable to generate certificate request: %w", err) + } + createdCSRName, err := csrOption.CSRControl.create( + ctx, recorder, csrOption.ObjectMeta, csrData, csrOption.SignerName, csrOption.ExpirationSeconds) + if err != nil { + return keyData, "", err + } + return keyData, createdCSRName, nil + }() if err != nil { - return nil, err + return nil, &metav1.Condition{ + Type: "ClusterCertificateRotated", + Status: metav1.ConditionFalse, + Reason: "ClientCertificateUpdateFailed", + Message: fmt.Sprintf("Failed to create CSR %v", err), + }, err } + c.keyData = keyData + c.csrName = createdCSRName + return nil, nil, nil +} + +func (c *CSRDriver) reset() { + c.csrName = "" + c.keyData = nil +} + +func (c *CSRDriver) BuildKubeConfigFromTemplate(kubeConfig *clientcmdapi.Config) *clientcmdapi.Config { kubeConfig.AuthInfos = map[string]*clientcmdapi.AuthInfo{register.DefaultKubeConfigAuth: { ClientCertificate: TLSCertFile, ClientKey: TLSKeyFile, }} - return kubeConfig, nil + return kubeConfig +} + +func (c *CSRDriver) InformerHandler(option any) (cache.SharedIndexInformer, factory.EventFilterFunc) { + csrOption, ok := option.(*CSROption) + if !ok { + utilruntime.Must(fmt.Errorf("option type is not correct")) + } + return csrOption.CSRControl.Informer(), csrOption.EventFilterFunc } func (c *CSRDriver) IsHubKubeConfigValid(ctx context.Context, secretOption register.SecretOption) (bool, error) { @@ -132,3 +302,74 @@ func (c *CSRDriver) IsHubKubeConfigValid(ctx context.Context, secretOption regis func NewCSRDriver() register.RegisterDriver { return &CSRDriver{} } + +func shouldCreateCSR( + logger klog.Logger, + controllerName string, + secret *corev1.Secret, + recorder events.Recorder, + subject *pkix.Name, + additionalSecretData map[string][]byte) (bool, error) { + // create a csr to request new client certificate if + // a.there is no valid client certificate issued for the current cluster/agent + valid, err := isCertificateValid(logger, secret.Data[TLSCertFile], subject) + if err != nil { + recorder.Eventf("CertificateValidationFailed", "Failed to validate client certificate for %s: %v", controllerName, err) + return true, nil + } + if !valid { + recorder.Eventf("NoValidCertificateFound", "No valid client certificate for %s is found. Bootstrap is required", controllerName) + return true, nil + } + + // b.client certificate is sensitive to the additional secret data and the data changes + if err := hasAdditionalSecretData(additionalSecretData, secret); err != nil { + recorder.Eventf("AdditonalSecretDataChanged", "The additional secret data is changed for %v. Re-create the client certificate for %s", err, controllerName) + return true, nil + } + + // c.client certificate exists and has less than a random percentage range from 20% to 25% of its life remaining + notBefore, notAfter, err := getCertValidityPeriod(secret) + if err != nil { + return false, err + } + total := notAfter.Sub(*notBefore) + remaining := time.Until(*notAfter) + logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, + "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) + threshold := jitter(0.2, 0.25) + if remaining.Seconds()/total.Seconds() > threshold { + // Do nothing if the client certificate is valid and has more than a random percentage range from 20% to 25% of its life remaining + logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, + "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) + return false, nil + } + recorder.Eventf("CertificateRotationStarted", + "The current client certificate for %s expires in %v. Start certificate rotation", + controllerName, remaining.Round(time.Second)) + return true, nil +} + +// hasAdditonalSecretData checks if the secret includes the expected additional secret data. +func hasAdditionalSecretData(additionalSecretData map[string][]byte, secret *corev1.Secret) error { + for k, v := range additionalSecretData { + value, ok := secret.Data[k] + if !ok { + return fmt.Errorf("key %q not found in secret %q", k, secret.Namespace+"/"+secret.Name) + } + + if !reflect.DeepEqual(v, value) { + return fmt.Errorf("key %q in secret %q does not match the expected value", + k, secret.Namespace+"/"+secret.Name) + } + } + return nil +} + +func jitter(percentage float64, maxFactor float64) float64 { + if maxFactor <= 0.0 { + maxFactor = 1.0 + } + newPercentage := percentage + percentage*rand.Float64()*maxFactor //#nosec G404 + return newPercentage +} diff --git a/pkg/registration/register/csr/csr_test.go b/pkg/registration/register/csr/csr_test.go index 6dd6cf028..5b28320ea 100644 --- a/pkg/registration/register/csr/csr_test.go +++ b/pkg/registration/register/csr/csr_test.go @@ -2,17 +2,281 @@ package csr import ( "context" + "crypto/x509/pkix" + "fmt" "os" "path" "testing" "time" + "github.com/openshift/library-go/pkg/operator/events" + certificates "k8s.io/api/certificates/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/rand" + kubefake "k8s.io/client-go/kubernetes/fake" + clienttesting "k8s.io/client-go/testing" + "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/clientcmd" + "k8s.io/klog/v2/ktesting" + testingcommon "open-cluster-management.io/ocm/pkg/common/testing" testinghelpers "open-cluster-management.io/ocm/pkg/registration/helpers/testing" + "open-cluster-management.io/ocm/pkg/registration/hub/user" "open-cluster-management.io/ocm/pkg/registration/register" ) +const ( + testNamespace = "testns" + testAgentName = "testagent" + testSecretName = "testsecret" + testCSRName = "testcsr" +) + +var commonName = fmt.Sprintf("%s%s:%s", user.SubjectPrefix, testinghelpers.TestManagedClusterName, testAgentName) + +func TestProcess(t *testing.T) { + testSubject := &pkix.Name{ + CommonName: commonName, + } + + cases := []struct { + name string + queueKey string + secret *corev1.Secret + approvedCSRCert *testinghelpers.TestCert + keyDataExpected bool + csrNameExpected bool + expectedCondition *metav1.Condition + validateActions func(t *testing.T, hubActions []clienttesting.Action, secret *corev1.Secret) + }{ + { + name: "syc csr after bootstrap", + queueKey: testSecretName, + secret: testinghelpers.NewHubKubeconfigSecret( + testNamespace, testSecretName, "1", nil, + map[string][]byte{ + register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), + register.AgentNameFile: []byte(testAgentName), + }), + expectedCondition: &metav1.Condition{ + Type: ClusterCertificateRotatedCondition, + Status: metav1.ConditionTrue, + }, + approvedCSRCert: testinghelpers.NewTestCert(commonName, 10*time.Second), + validateActions: func(t *testing.T, hubActions []clienttesting.Action, secret *corev1.Secret) { + logger, _ := ktesting.NewTestContext(t) + testingcommon.AssertActions(t, hubActions, "get", "get") + valid, err := isCertificateValid(logger, secret.Data[TLSCertFile], testSubject) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if !valid { + t.Error("client certificate is invalid") + } + }, + }, + { + name: "sync a valid hub kubeconfig secret", + queueKey: testSecretName, + secret: testinghelpers.NewHubKubeconfigSecret( + testNamespace, testSecretName, "1", + testinghelpers.NewTestCert(commonName, 10000*time.Second), map[string][]byte{ + register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), + register.AgentNameFile: []byte(testAgentName), + register.KubeconfigFile: testinghelpers.NewKubeconfig( + "c1", "https://127.0.0.1:6001", "", nil, nil, nil), + }), + validateActions: func(t *testing.T, hubActions []clienttesting.Action, secret *corev1.Secret) { + testingcommon.AssertNoActions(t, hubActions) + if secret != nil { + t.Errorf("expect the secret not to be generated") + } + }, + }, + { + name: "sync an expiring hub kubeconfig secret", + queueKey: testSecretName, + secret: testinghelpers.NewHubKubeconfigSecret( + testNamespace, testSecretName, "1", + testinghelpers.NewTestCert(commonName, -3*time.Second), + map[string][]byte{ + register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), + register.AgentNameFile: []byte(testAgentName), + register.KubeconfigFile: testinghelpers.NewKubeconfig( + "c1", "https://127.0.0.1:6001", "", nil, nil, nil), + }), + keyDataExpected: true, + csrNameExpected: true, + validateActions: func(t *testing.T, hubActions []clienttesting.Action, secret *corev1.Secret) { + testingcommon.AssertActions(t, hubActions, "create") + actual := hubActions[0].(clienttesting.CreateActionImpl).Object + if _, ok := actual.(*unstructured.Unstructured); !ok { + t.Errorf("expected csr was created, but failed") + } + }, + }, + { + name: "sync when additional secret data changes", + queueKey: testSecretName, + secret: testinghelpers.NewHubKubeconfigSecret( + testNamespace, testSecretName, "1", + testinghelpers.NewTestCert(commonName, 10000*time.Second), + map[string][]byte{ + register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), + register.AgentNameFile: []byte("invalid-name"), + }), + keyDataExpected: true, + csrNameExpected: true, + validateActions: func(t *testing.T, hubActions []clienttesting.Action, secret *corev1.Secret) { + testingcommon.AssertActions(t, hubActions, "create") + actual := hubActions[0].(clienttesting.CreateActionImpl).Object + if _, ok := actual.(*unstructured.Unstructured); !ok { + t.Errorf("expected csr was created, but failed") + } + }, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ctrl := &mockCSRControl{} + var csrs []runtime.Object + if c.approvedCSRCert != nil { + csr := testinghelpers.NewApprovedCSR(testinghelpers.CSRHolder{Name: testCSRName}) + csr.Status.Certificate = c.approvedCSRCert.Cert + csrs = append(csrs, csr) + ctrl.approved = true + ctrl.issuedCertData = c.approvedCSRCert.Cert + } + hubKubeClient := kubefake.NewSimpleClientset(csrs...) + ctrl.csrClient = &hubKubeClient.Fake + + // GenerateName is not working for fake clent, we set the name with prepend reactor + hubKubeClient.PrependReactor( + "create", + "certificatesigningrequests", + func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + return true, testinghelpers.NewCSR(testinghelpers.CSRHolder{Name: testCSRName}), nil + }, + ) + + additionalSecretData := map[string][]byte{ + register.ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), + register.AgentNameFile: []byte(testAgentName), + } + csrOption := &CSROption{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Subject: testSubject, + SignerName: certificates.KubeAPIServerClientSignerName, + HaltCSRCreation: func() bool { return false }, + CSRControl: ctrl, + } + + driver := &CSRDriver{} + + if c.approvedCSRCert != nil { + driver.csrName = testCSRName + driver.keyData = c.approvedCSRCert.Key + } + + syncCtx := testingcommon.NewFakeSyncContext(t, "test") + + secret, cond, err := driver.Process( + context.TODO(), "test", c.secret, additionalSecretData, syncCtx.Recorder(), csrOption) + if err != nil { + t.Errorf("unexpected error %v", err) + } + + hasKeyData := driver.keyData != nil + if c.keyDataExpected != hasKeyData { + t.Error("controller.keyData should be set") + } + + hasCSRName := driver.csrName != "" + if c.csrNameExpected != hasCSRName { + t.Error("controller.csrName should be set") + } + + if !conditionEqual(c.expectedCondition, cond) { + t.Errorf("condition is not correct, expected %v, got %v", c.expectedCondition, cond) + } + + c.validateActions(t, hubKubeClient.Actions(), secret) + }) + } +} + +var _ CSRControl = &mockCSRControl{} + +func conditionEqual(expected, actual *metav1.Condition) bool { + if expected == nil && actual == nil { + return true + } + + if expected == nil || actual == nil { + return false + } + + if expected.Type != actual.Type { + return false + } + + if string(expected.Status) != string(actual.Status) { + return false + } + + return true +} + +type mockCSRControl struct { + approved bool + issuedCertData []byte + csrClient *clienttesting.Fake +} + +func (m *mockCSRControl) create( + _ context.Context, _ events.Recorder, objMeta metav1.ObjectMeta, _ []byte, _ string, _ *int32) (string, error) { + mockCSR := &unstructured.Unstructured{} + _, err := m.csrClient.Invokes(clienttesting.CreateActionImpl{ + ActionImpl: clienttesting.ActionImpl{ + Verb: "create", + }, + Object: mockCSR, + }, nil) + return objMeta.Name + rand.String(4), err +} + +func (m *mockCSRControl) isApproved(name string) (bool, error) { + _, err := m.csrClient.Invokes(clienttesting.GetActionImpl{ + ActionImpl: clienttesting.ActionImpl{ + Verb: "get", + Resource: certificates.SchemeGroupVersion.WithResource("certificatesigningrequests"), + }, + Name: name, + }, nil) + + return m.approved, err +} + +func (m *mockCSRControl) getIssuedCertificate(name string) ([]byte, error) { + _, err := m.csrClient.Invokes(clienttesting.GetActionImpl{ + ActionImpl: clienttesting.ActionImpl{ + Verb: "get", + Resource: certificates.SchemeGroupVersion.WithResource("certificatesigningrequests"), + }, + Name: name, + }, nil) + return m.issuedCertData, err +} + +func (m *mockCSRControl) Informer() cache.SharedIndexInformer { + panic("implement me") +} + func TestIsHubKubeConfigValidFunc(t *testing.T) { tempDir, err := os.MkdirTemp("", "testvalidhubclientconfig") if err != nil { @@ -119,7 +383,6 @@ func TestIsHubKubeConfigValidFunc(t *testing.T) { HubKubeconfigDir: tempDir, HubKubeconfigFile: path.Join(tempDir, "kubeconfig"), } - registerImpl := register.NewRegister(driver) if c.kubeconfig != nil { testinghelpers.WriteFile(path.Join(tempDir, "kubeconfig"), c.kubeconfig) } @@ -137,7 +400,7 @@ func TestIsHubKubeConfigValidFunc(t *testing.T) { secretOption.BootStrapKubeConfig = bootstrapKubeconfig } - valid, err := registerImpl.IsHubKubeConfigValidFunc(secretOption)(context.TODO()) + valid, err := register.IsHubKubeConfigValidFunc(driver, secretOption)(context.TODO()) if err != nil { t.Errorf("unexpected error: %v", err) } diff --git a/pkg/registration/register/interface.go b/pkg/registration/register/interface.go index 16f74998b..d19074b01 100644 --- a/pkg/registration/register/interface.go +++ b/pkg/registration/register/interface.go @@ -4,9 +4,10 @@ import ( "context" "os" + "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/tools/cache" @@ -48,47 +49,31 @@ type SecretOption struct { ManagementCoreClient corev1client.CoreV1Interface } -// A function to update the condition of the corresponding object. +// StatusUpdateFunc is A function to update the condition of the corresponding object. type StatusUpdateFunc func(ctx context.Context, cond metav1.Condition) error -// Register is to start a process that maintains a secret defined in the secretOption. -type Register interface { - // Start starts the bootstrap/creds rotate process for the agent. - Start(ctx context.Context, name string, statusUpdater StatusUpdateFunc, recorder events.Recorder, secretOption SecretOption, option any) - - // IsHubKubeConfigValidFunc returns a func to check if the current hube-kubeconfig is valid. It is called before - // and after bootstrap to confirm if the bootstrap is finished. - IsHubKubeConfigValidFunc(secretOption SecretOption) wait.ConditionWithContextFunc -} - // RegisterDriver is the interface that each driver should implement type RegisterDriver interface { - // IsHubKubeConfigValidFunc returns a func to check if the current hube-kubeconfig is valid. It is called before + // IsHubKubeConfigValid is to check if the current hube-kubeconfig is valid. It is called before // and after bootstrap to confirm if the bootstrap is finished. IsHubKubeConfigValid(ctx context.Context, secretOption SecretOption) (bool, error) - BuildKubeConfigFromBootstrap(config *clientcmdapi.Config) (*clientcmdapi.Config, error) + // BuildKubeConfigFromTemplate builds the kubeconfig from the template kubeconfig + BuildKubeConfigFromTemplate(config *clientcmdapi.Config) *clientcmdapi.Config - Start(ctx context.Context, + // Process update secret with credentials + Process( + ctx context.Context, name string, - statusUpdater StatusUpdateFunc, - recorder events.Recorder, - secretOpt SecretOption, - opt any, - additionalData map[string][]byte) -} + secret *corev1.Secret, + additionalSecretData map[string][]byte, + recorder events.Recorder, opt any) (*corev1.Secret, *metav1.Condition, error) -type registerImpl struct { - driver RegisterDriver + // InformerHandler returns informer related object + InformerHandler(option any) (cache.SharedIndexInformer, factory.EventFilterFunc) } -func NewRegister(driver RegisterDriver) Register { - return ®isterImpl{ - driver: driver, - } -} - -func (r *registerImpl) IsHubKubeConfigValidFunc(secretOption SecretOption) wait.ConditionWithContextFunc { +func IsHubKubeConfigValidFunc(driver RegisterDriver, secretOption SecretOption) wait.ConditionWithContextFunc { return func(ctx context.Context) (bool, error) { logger := klog.FromContext(ctx) if _, err := os.Stat(secretOption.HubKubeconfigFile); os.IsNotExist(err) { @@ -106,38 +91,6 @@ func (r *registerImpl) IsHubKubeConfigValidFunc(secretOption SecretOption) wait. return valid, err } - return r.driver.IsHubKubeConfigValid(ctx, secretOption) + return driver.IsHubKubeConfigValid(ctx, secretOption) } } - -func (r *registerImpl) Start( - ctx context.Context, - name string, - statusUpdater StatusUpdateFunc, - recorder events.Recorder, - secretOption SecretOption, option any) { - additionalSercretData := map[string][]byte{} - if secretOption.BootStrapKubeConfig != nil { - kubeConfig, err := r.driver.BuildKubeConfigFromBootstrap(secretOption.BootStrapKubeConfig) - if err != nil { - utilruntime.Must(err) - } - if kubeConfig != nil { - kubeconfigData, err := clientcmd.Write(*kubeConfig) - if err != nil { - utilruntime.Must(err) - } - additionalSercretData[KubeconfigFile] = kubeconfigData - } - } - - if len(secretOption.ClusterName) > 0 { - additionalSercretData[ClusterNameFile] = []byte(secretOption.ClusterName) - } - - if len(secretOption.AgentName) > 0 { - additionalSercretData[AgentNameFile] = []byte(secretOption.AgentName) - } - - r.driver.Start(ctx, name, statusUpdater, recorder, secretOption, option, additionalSercretData) -} diff --git a/pkg/registration/register/secret_controller.go b/pkg/registration/register/secret_controller.go new file mode 100644 index 000000000..0304f8657 --- /dev/null +++ b/pkg/registration/register/secret_controller.go @@ -0,0 +1,164 @@ +package register + +import ( + "context" + "fmt" + "time" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/client-go/tools/clientcmd" +) + +// ControllerResyncInterval is exposed so that integration tests can crank up the constroller sync speed. +var ControllerResyncInterval = 5 * time.Minute + +type secretController struct { + SecretOption + option any + driver RegisterDriver + controllerName string + statusUpdater StatusUpdateFunc + additionalSecretData map[string][]byte + secretToSave *corev1.Secret +} + +// NewSecretController return an instance of secretController +func NewSecretController( + secretOption SecretOption, + option any, + driver RegisterDriver, + statusUpdater StatusUpdateFunc, + recorder events.Recorder, + controllerName string, +) factory.Controller { + additionalSecretData := map[string][]byte{} + if secretOption.BootStrapKubeConfig != nil { + kubeConfigTemplate, err := BaseKubeConfigFromBootStrap(secretOption.BootStrapKubeConfig) + if err != nil { + utilruntime.Must(err) + } + kubeConfig := driver.BuildKubeConfigFromTemplate(kubeConfigTemplate) + if kubeConfig != nil { + kubeconfigData, err := clientcmd.Write(*kubeConfig) + if err != nil { + utilruntime.Must(err) + } + additionalSecretData[KubeconfigFile] = kubeconfigData + } + } + + if len(secretOption.ClusterName) > 0 { + additionalSecretData[ClusterNameFile] = []byte(secretOption.ClusterName) + } + + if len(secretOption.AgentName) > 0 { + additionalSecretData[AgentNameFile] = []byte(secretOption.AgentName) + } + + c := secretController{ + SecretOption: secretOption, + driver: driver, + controllerName: controllerName, + statusUpdater: statusUpdater, + additionalSecretData: additionalSecretData, + option: option, + } + + f := factory.New(). + WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string { + return factory.DefaultQueueKey + }, func(obj interface{}) bool { + accessor, err := meta.Accessor(obj) + if err != nil { + return false + } + // only enqueue a specific secret + if accessor.GetNamespace() == c.SecretNamespace && accessor.GetName() == c.SecretName { + return true + } + return false + }, secretOption.ManagementSecretInformer) + + driverInformer, driverFilter := driver.InformerHandler(option) + if driverInformer != nil && driverFilter != nil { + f = f.WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string { + return factory.DefaultQueueKey + }, driverFilter, driverInformer) + } + + return f.WithSync(c.sync). + ResyncEvery(ControllerResyncInterval). + ToController(controllerName, recorder) +} + +func (c *secretController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + // get secret containing client certificate + secret, err := c.ManagementCoreClient.Secrets(c.SecretNamespace).Get(ctx, c.SecretName, metav1.GetOptions{}) + switch { + case apierrors.IsNotFound(err): + secret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: c.SecretNamespace, + Name: c.SecretName, + }, + } + case err != nil: + return fmt.Errorf("unable to get secret %q: %w", c.SecretNamespace+"/"+c.SecretName, err) + } + + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + + if c.secretToSave == nil { + secret, cond, err := c.driver.Process(ctx, c.controllerName, secret, c.additionalSecretData, syncCtx.Recorder(), c.option) + if cond != nil { + if updateErr := c.statusUpdater(ctx, *cond); updateErr != nil { + return updateErr + } + } + if err != nil { + return err + } + if secret == nil { + return nil + } + + if len(c.additionalSecretData) > 0 { + // append additional data into client certificate secret + for k, v := range c.additionalSecretData { + secret.Data[k] = v + } + } + c.secretToSave = secret + } + + // save the changes into secret + if err := saveSecret(c.ManagementCoreClient, c.SecretNamespace, c.secretToSave); err != nil { + return err + } + syncCtx.Recorder().Eventf("SecretSave", "Secret %s/%s for %s is updated", + c.SecretNamespace, c.SecretName, c.controllerName) + // clean the cached secret. + c.secretToSave = nil + + return nil +} + +func saveSecret(spokeCoreClient corev1client.CoreV1Interface, secretNamespace string, secret *corev1.Secret) error { + var err error + if secret.ResourceVersion == "" { + _, err = spokeCoreClient.Secrets(secretNamespace).Create(context.Background(), secret, metav1.CreateOptions{}) + return err + } + _, err = spokeCoreClient.Secrets(secretNamespace).Update(context.Background(), secret, metav1.UpdateOptions{}) + return err +} diff --git a/pkg/registration/register/secret_controller_test.go b/pkg/registration/register/secret_controller_test.go new file mode 100644 index 000000000..26a31aaf7 --- /dev/null +++ b/pkg/registration/register/secret_controller_test.go @@ -0,0 +1,197 @@ +package register + +import ( + "context" + "testing" + "time" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/informers" + kubefake "k8s.io/client-go/kubernetes/fake" + clienttesting "k8s.io/client-go/testing" + "k8s.io/client-go/tools/cache" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + + testingcommon "open-cluster-management.io/ocm/pkg/common/testing" + testinghelpers "open-cluster-management.io/ocm/pkg/registration/helpers/testing" +) + +func TestSync(t *testing.T) { + commonName := "test" + testCases := []struct { + name string + option SecretOption + secrets []runtime.Object + driver *fakeDriver + expectedCond *metav1.Condition + validatAction func(t *testing.T, actions []clienttesting.Action) + }{ + { + name: "create secret without additional data", + option: SecretOption{ + SecretName: "test", + SecretNamespace: "test", + }, + secrets: []runtime.Object{}, + driver: newFakeDriver( + testinghelpers.NewHubKubeconfigSecret( + "test", "test", "", + testinghelpers.NewTestCert(commonName, 100*time.Second), map[string][]byte{}), + &metav1.Condition{Type: "Created", Status: metav1.ConditionTrue}, nil, + ), + validatAction: func(t *testing.T, actions []clienttesting.Action) { + testingcommon.AssertActions(t, actions, "get", "create") + }, + expectedCond: &metav1.Condition{Type: "Created", Status: metav1.ConditionTrue}, + }, + { + name: "update secret without additional data", + option: SecretOption{ + SecretName: "test", + SecretNamespace: "test", + }, + secrets: []runtime.Object{ + testinghelpers.NewHubKubeconfigSecret( + "test", "test", "0", + testinghelpers.NewTestCert(commonName, 100*time.Second), map[string][]byte{}), + }, + driver: newFakeDriver( + testinghelpers.NewHubKubeconfigSecret( + "test", "test", "1", + testinghelpers.NewTestCert(commonName, 200*time.Second), map[string][]byte{}), + &metav1.Condition{Type: "Created", Status: metav1.ConditionTrue}, nil, + ), + validatAction: func(t *testing.T, actions []clienttesting.Action) { + testingcommon.AssertActions(t, actions, "get", "update") + }, + expectedCond: &metav1.Condition{Type: "Created", Status: metav1.ConditionTrue}, + }, + { + name: "nothing to create if there is no secret generated", + option: SecretOption{ + SecretName: "test", + SecretNamespace: "test", + }, + secrets: []runtime.Object{}, + driver: newFakeDriver(nil, nil, nil), + validatAction: func(t *testing.T, actions []clienttesting.Action) { + testingcommon.AssertActions(t, actions, "get") + }, + }, + { + name: "addition secret data", + option: SecretOption{ + SecretName: "test", + SecretNamespace: "test", + ClusterName: "cluster1", + AgentName: "agent1", + BootStrapKubeConfig: &clientcmdapi.Config{ + Clusters: map[string]*clientcmdapi.Cluster{"test-cluster": { + Server: "localhost", + InsecureSkipTLSVerify: true, + }}, + Contexts: map[string]*clientcmdapi.Context{"test-context": { + Cluster: "test-cluster", + AuthInfo: "test-user", + }}, + AuthInfos: map[string]*clientcmdapi.AuthInfo{ + "test-user": { + Token: "test-token", + }, + }, + CurrentContext: "test-context", + }, + }, + secrets: []runtime.Object{}, + driver: newFakeDriver(testinghelpers.NewHubKubeconfigSecret( + "test", "test", "", + testinghelpers.NewTestCert(commonName, 100*time.Second), map[string][]byte{}), nil, nil), + validatAction: func(t *testing.T, actions []clienttesting.Action) { + testingcommon.AssertActions(t, actions, "get", "create") + secret := actions[1].(clienttesting.CreateActionImpl).Object.(*corev1.Secret) + cluster, ok := secret.Data[ClusterNameFile] + if !ok || string(cluster) != "cluster1" { + t.Errorf("cluster name not correct") + } + agent, ok := secret.Data[AgentNameFile] + if !ok || string(agent) != "agent1" { + t.Errorf("agent name not correct") + } + _, ok = secret.Data[KubeconfigFile] + if !ok { + t.Errorf("kubeconfig file should exist") + } + }, + }, + } + + for _, c := range testCases { + t.Run(c.name, func(t *testing.T) { + syncCtx := testingcommon.NewFakeSyncContext(t, "test") + kubeClient := kubefake.NewSimpleClientset(c.secrets...) + c.option.ManagementCoreClient = kubeClient.CoreV1() + informerFactory := informers.NewSharedInformerFactory(kubeClient, 10*time.Minute) + c.option.ManagementSecretInformer = informerFactory.Core().V1().Secrets().Informer() + updater := &fakeStatusUpdater{} + ctrl := NewSecretController( + c.option, nil, c.driver, updater.update, syncCtx.Recorder(), "test") + err := ctrl.Sync(context.Background(), syncCtx) + if err != nil { + t.Fatal(err) + } + c.validatAction(t, kubeClient.Actions()) + if !apiequality.Semantic.DeepEqual(c.expectedCond, updater.cond) { + t.Errorf("Condition update not correct") + } + }) + } +} + +type fakeStatusUpdater struct { + cond *metav1.Condition +} + +func (f *fakeStatusUpdater) update(_ context.Context, cond metav1.Condition) error { + f.cond = cond.DeepCopy() + return nil +} + +type fakeDriver struct { + secret *corev1.Secret + err error + cond *metav1.Condition +} + +func newFakeDriver(secret *corev1.Secret, cond *metav1.Condition, err error) *fakeDriver { + return &fakeDriver{ + secret: secret, + cond: cond, + err: err, + } +} + +func (f *fakeDriver) IsHubKubeConfigValid(_ context.Context, _ SecretOption) (bool, error) { + return true, nil +} + +func (f *fakeDriver) BuildKubeConfigFromTemplate(config *clientcmdapi.Config) *clientcmdapi.Config { + return config +} + +func (f *fakeDriver) Process( + _ context.Context, + _ string, + _ *corev1.Secret, + _ map[string][]byte, + _ events.Recorder, _ any) (*corev1.Secret, *metav1.Condition, error) { + return f.secret, f.cond, f.err +} + +func (f *fakeDriver) InformerHandler(_ any) (cache.SharedIndexInformer, factory.EventFilterFunc) { + return nil, nil +} diff --git a/pkg/registration/spoke/addon/registration_controller.go b/pkg/registration/spoke/addon/registration_controller.go index be7c5b556..396c12567 100644 --- a/pkg/registration/spoke/addon/registration_controller.go +++ b/pkg/registration/spoke/addon/registration_controller.go @@ -224,7 +224,6 @@ func (c *addOnRegistrationController) startRegistration(ctx context.Context, con } driver := csr.NewCSRDriver() - registerImpl := register.NewRegister(driver) csrOption := &csr.CSROption{ ObjectMeta: metav1.ObjectMeta{ GenerateName: fmt.Sprintf("addon-%s-%s-", c.clusterName, config.addOnName), @@ -244,9 +243,11 @@ func (c *addOnRegistrationController) startRegistration(ctx context.Context, con controllerName := fmt.Sprintf("ClientCertController@addon:%s:signer:%s", config.addOnName, config.registration.SignerName) statusUpdater := c.generateStatusUpdate(c.clusterName, config.addOnName) + secretController := register.NewSecretController( + secretOption, csrOption, driver, statusUpdater, c.recorder, controllerName) go kubeInformerFactory.Start(ctx.Done()) - go registerImpl.Start(ctx, controllerName, statusUpdater, c.recorder, secretOption, csrOption) + go secretController.Run(ctx, 1) return stopFunc } diff --git a/pkg/registration/spoke/spokeagent.go b/pkg/registration/spoke/spokeagent.go index f16e0ea9e..9c30d4739 100644 --- a/pkg/registration/spoke/spokeagent.go +++ b/pkg/registration/spoke/spokeagent.go @@ -46,11 +46,13 @@ type SpokeAgentConfig struct { agentOptions *commonoptions.AgentOptions registrationOption *SpokeAgentOptions - RegisterImpl register.Register + driver register.RegisterDriver // currentBootstrapKubeConfig is the selected bootstrap kubeconfig file path. // Only used in MultipleHubs feature. currentBootstrapKubeConfig string + + internalHubConfigValidFunc wait.ConditionWithContextFunc } // NewSpokeAgentConfig returns a SpokeAgentConfig @@ -59,7 +61,7 @@ func NewSpokeAgentConfig(commonOpts *commonoptions.AgentOptions, opts *SpokeAgen return &SpokeAgentConfig{ agentOptions: commonOpts, registrationOption: opts, - RegisterImpl: register.NewRegister(registerDriver), + driver: registerDriver, } } @@ -251,9 +253,8 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context, HubKubeconfigDir: o.agentOptions.HubKubeconfigDir, BootStrapKubeConfig: kubeconfig, } - hasValidHubClientConfig := o.RegisterImpl.IsHubKubeConfigValidFunc(secretOption) - - ok, err := hasValidHubClientConfig(ctx) + o.internalHubConfigValidFunc = register.IsHubKubeConfigValidFunc(o.driver, secretOption) + ok, err := o.internalHubConfigValidFunc(ctx) if err != nil { return err } @@ -279,15 +280,15 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context, controllerName := fmt.Sprintf("BootstrapClientCertController@cluster:%s", o.agentOptions.SpokeClusterName) bootstrapCtx, stopBootstrap := context.WithCancel(ctx) - - go o.RegisterImpl.Start(bootstrapCtx, - controllerName, registration.GenerateBootstrapStatusUpdater(), recorder, secretOption, csrOption) + secretController := register.NewSecretController( + secretOption, csrOption, o.driver, registration.GenerateBootstrapStatusUpdater(), recorder, controllerName) go bootstrapInformerFactory.Start(bootstrapCtx.Done()) + go secretController.Run(bootstrapCtx, 1) // wait for the hub client config is ready. logger.Info("Waiting for hub client config and managed cluster to be ready") - if err := wait.PollUntilContextCancel(bootstrapCtx, 1*time.Second, true, hasValidHubClientConfig); err != nil { + if err := wait.PollUntilContextCancel(bootstrapCtx, 1*time.Second, true, o.internalHubConfigValidFunc); err != nil { // TODO need run the bootstrap CSR forever to re-establish the client-cert if it is ever lost. stopBootstrap() return fmt.Errorf("failed to wait for hub client config for managed cluster to be ready: %w", err) @@ -352,11 +353,11 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context, // create another ClientCertForHubController for client certificate rotation controllerName := fmt.Sprintf("ClientCertController@cluster:%s", o.agentOptions.SpokeClusterName) - go o.RegisterImpl.Start(ctx, - controllerName, registration.GenerateStatusUpdater( + secretController := register.NewSecretController( + secretOption, csrOption, o.driver, registration.GenerateStatusUpdater( hubClusterClient, hubClusterInformerFactory.Cluster().V1().ManagedClusters().Lister(), - o.agentOptions.SpokeClusterName), recorder, secretOption, csrOption) + o.agentOptions.SpokeClusterName), recorder, controllerName) // create ManagedClusterLeaseController to keep the spoke cluster heartbeat managedClusterLeaseController := lease.NewManagedClusterLeaseController( @@ -447,6 +448,7 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context, go spokeClusterInformerFactory.Start(ctx.Done()) } + go secretController.Run(ctx, 1) go managedClusterLeaseController.Run(ctx, 1) go managedClusterHealthCheckController.Run(ctx, 1) if features.SpokeMutableFeatureGate.Enabled(ocmfeature.AddonManagement) { @@ -469,6 +471,13 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context, return nil } +func (o *SpokeAgentConfig) IsHubKubeConfigValid(ctx context.Context) (bool, error) { + if o.internalHubConfigValidFunc == nil { + return false, nil + } + return o.internalHubConfigValidFunc(ctx) +} + // getSpokeClusterCABundle returns the spoke cluster Kubernetes client CA data when SpokeExternalServerURLs is specified func (o *SpokeAgentConfig) getSpokeClusterCABundle(kubeConfig *rest.Config) ([]byte, error) { if len(o.registrationOption.SpokeExternalServerURLs) == 0 { diff --git a/pkg/singleton/spoke/agent.go b/pkg/singleton/spoke/agent.go index d2cb6eda2..9a9050a60 100644 --- a/pkg/singleton/spoke/agent.go +++ b/pkg/singleton/spoke/agent.go @@ -9,7 +9,6 @@ import ( "k8s.io/klog/v2" commonoptions "open-cluster-management.io/ocm/pkg/common/options" - "open-cluster-management.io/ocm/pkg/registration/register" registration "open-cluster-management.io/ocm/pkg/registration/spoke" work "open-cluster-management.io/ocm/pkg/work/spoke" ) @@ -40,19 +39,9 @@ func (a *AgentConfig) RunSpokeAgent(ctx context.Context, controllerContext *cont } }() - secretOption := register.SecretOption{ - SecretNamespace: a.agentOption.ComponentNamespace, - SecretName: a.registrationOption.HubKubeconfigSecret, - ClusterName: a.agentOption.SpokeClusterName, - AgentName: a.agentOption.AgentID, - HubKubeconfigFile: a.agentOption.HubKubeconfigFile, - HubKubeconfigDir: a.agentOption.HubKubeconfigDir, - } - hasValidClientConfig := registrationCfg.RegisterImpl.IsHubKubeConfigValidFunc(secretOption) - // wait for the hub client config ready. klog.Info("Waiting for hub client config and managed cluster to be ready") - if err := wait.PollUntilContextCancel(ctx, 1*time.Second, true, hasValidClientConfig); err != nil { + if err := wait.PollUntilContextCancel(ctx, 1*time.Second, true, registrationCfg.IsHubKubeConfigValid); err != nil { return err } diff --git a/test/integration/registration/addon_registration_test.go b/test/integration/registration/addon_registration_test.go index b6bc220d3..756c4f04d 100644 --- a/test/integration/registration/addon_registration_test.go +++ b/test/integration/registration/addon_registration_test.go @@ -350,13 +350,17 @@ var _ = ginkgo.Describe("Addon Registration", func() { assertSuccessCSRApproval() ginkgo.By("Wait for addon namespace") - gomega.Consistently(func() bool { + gomega.Consistently(func() error { csrs, err := util.FindAddOnCSRs(kubeClient, managedClusterName, addOnName) if err != nil { - return false + return err } - return len(csrs) == 1 - }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + if len(csrs) != 1 { + return fmt.Errorf("the number of CSRs is not correct, got %d", len(csrs)) + } + return nil + }, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed()) ginkgo.By("Create addon namespace") // create addon namespace diff --git a/test/integration/registration/integration_suite_test.go b/test/integration/registration/integration_suite_test.go index 5c839f2a3..cef3b4ff4 100644 --- a/test/integration/registration/integration_suite_test.go +++ b/test/integration/registration/integration_suite_test.go @@ -28,7 +28,7 @@ import ( commonoptions "open-cluster-management.io/ocm/pkg/common/options" "open-cluster-management.io/ocm/pkg/features" "open-cluster-management.io/ocm/pkg/registration/hub" - "open-cluster-management.io/ocm/pkg/registration/register/csr" + "open-cluster-management.io/ocm/pkg/registration/register" "open-cluster-management.io/ocm/pkg/registration/spoke" "open-cluster-management.io/ocm/pkg/registration/spoke/addon" "open-cluster-management.io/ocm/pkg/registration/spoke/registration" @@ -111,7 +111,7 @@ var _ = ginkgo.BeforeSuite(func() { // crank up the sync speed transport.CertCallbackRefreshDuration = 5 * time.Second - csr.ControllerResyncInterval = 5 * time.Second + register.ControllerResyncInterval = 5 * time.Second registration.CreatingControllerSyncInterval = 1 * time.Second // crank up the addon lease sync and udpate speed diff --git a/test/integration/registration/spokeagent_recovery_test.go b/test/integration/registration/spokeagent_recovery_test.go index f2e32001e..02c34058b 100644 --- a/test/integration/registration/spokeagent_recovery_test.go +++ b/test/integration/registration/spokeagent_recovery_test.go @@ -95,12 +95,12 @@ var _ = ginkgo.Describe("Agent Recovery", func() { gomega.Expect(err).NotTo(gomega.HaveOccurred()) // the hub kubeconfig secret should be filled after the csr is approved - gomega.Eventually(func() bool { + gomega.Eventually(func() error { if _, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret); err != nil { - return false + return err } - return true - }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + return nil + }, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed()) // the spoke cluster should have joined condition finally gomega.Eventually(func() error {