diff --git a/pkg/helpers/testing/assertion.go b/pkg/helpers/testing/assertion.go index 5a27fbe1c..da71eddc9 100644 --- a/pkg/helpers/testing/assertion.go +++ b/pkg/helpers/testing/assertion.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "os" "reflect" + "strings" "testing" clusterv1 "github.com/open-cluster-management/api/cluster/v1" @@ -36,6 +37,23 @@ func AssertError(t *testing.T, actual error, expectedErr string) { } } +// AssertError asserts the actual error representation starts with the expected prerfix, +// if the expected error prefix is empty, the actual should be nil +func AssertErrorWithPrefix(t *testing.T, actual error, expectedErrorPrefix string) { + if len(expectedErrorPrefix) > 0 && actual == nil { + t.Errorf("expected error with prefix %q", expectedErrorPrefix) + return + } + if len(expectedErrorPrefix) > 0 && actual != nil && !strings.HasPrefix(actual.Error(), expectedErrorPrefix) { + t.Errorf("expected error with prefix %q, but got %q", expectedErrorPrefix, actual.Error()) + return + } + if len(expectedErrorPrefix) == 0 && actual != nil { + t.Errorf("unexpected err: %v", actual) + return + } +} + // AssertActions asserts the actual actions have the expected action verb func AssertActions(t *testing.T, actualActions []clienttesting.Action, expectedVerbs ...string) { if len(actualActions) != len(expectedVerbs) { diff --git a/pkg/spoke/hubclientcert/certificate.go b/pkg/spoke/hubclientcert/certificate.go index 6429ab59a..2b581cfde 100644 --- a/pkg/spoke/hubclientcert/certificate.go +++ b/pkg/spoke/hubclientcert/certificate.go @@ -3,6 +3,7 @@ package hubclientcert import ( "errors" "fmt" + "strings" "time" certificates "k8s.io/api/certificates/v1beta1" @@ -18,7 +19,9 @@ import ( // 1. KubeconfigFile exists // 2. TLSKeyFile exists // 3. TLSCertFile exists and the certificate is not expired -func hasValidKubeconfig(secret *corev1.Secret) bool { +// 4. If not empty, the given commonName matches the common name of the subject in the +// certificate stored in TLSCertFile +func hasValidKubeconfig(secret *corev1.Secret, commonName string) bool { if secret.Data == nil { klog.V(4).Infof("No kubeconfig found in secret %q", secret.Namespace+"/"+secret.Name) return false @@ -46,7 +49,25 @@ func hasValidKubeconfig(secret *corev1.Secret) bool { return false } - return valid + if len(commonName) == 0 || !valid { + return valid + } + + // check the common name of the subject in certification + certs, err := certutil.ParseCertsPEM(certData) + if err != nil { + klog.V(4).Infof("unable to parse certificate: %v", err) + return false + } + + for _, cert := range certs { + if cert.Subject.CommonName == commonName { + return true + } + } + + klog.V(4).Infof("certificate is not issued for %q", commonName) + return false } // IsCertificateValid return true if all certs in client certificate are not expired. @@ -154,3 +175,25 @@ func isCSRApproved(csr *certificates.CertificateSigningRequest) bool { return approved } + +// GetClusterAgentNamesFromCertificate returns the cluster name and agent name by parsing +// the common name of the certification +func GetClusterAgentNamesFromCertificate(certData []byte) (clusterName, agentName string, err error) { + certs, err := certutil.ParseCertsPEM(certData) + if err != nil { + return "", "", fmt.Errorf("unable to parse certificate: %w", err) + } + + for _, cert := range certs { + if ok := strings.HasPrefix(cert.Subject.CommonName, subjectPrefix); !ok { + continue + } + names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, subjectPrefix), ":") + if len(names) != 2 { + continue + } + return names[0], names[1], nil + } + + return "", "", nil +} diff --git a/pkg/spoke/hubclientcert/certificate_test.go b/pkg/spoke/hubclientcert/certificate_test.go index 5e8943892..6bbb513ca 100644 --- a/pkg/spoke/hubclientcert/certificate_test.go +++ b/pkg/spoke/hubclientcert/certificate_test.go @@ -44,9 +44,10 @@ func TestCSRApproved(t *testing.T) { func TestValidKubeconfig(t *testing.T) { cases := []struct { - name string - secret *corev1.Secret - isValid bool + name string + secret *corev1.Secret + commonName string + isValid bool }{ { name: "no data", @@ -74,17 +75,25 @@ func TestValidKubeconfig(t *testing.T) { KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), }), }, + { + name: "unmatched common name", + secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ + KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), + }), + commonName: "wrong-common-name", + }, { name: "valid hub config", secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), }), - isValid: true, + commonName: "test", + isValid: true, }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - isValid := hasValidKubeconfig(c.secret) + isValid := hasValidKubeconfig(c.secret, c.commonName) if isValid != c.isValid { t.Errorf("expected %t, but got %t", c.isValid, isValid) } @@ -141,3 +150,43 @@ func TestGetCertValidityPeriod(t *testing.T) { }) } } + +func TestGetClusterAgentNamesFromCertificate(t *testing.T) { + cases := []struct { + name string + certData []byte + expectedClusterName string + expectedAgentName string + expectedErrorPrefix string + }{ + { + name: "cert data is invalid", + certData: []byte("invalid cert"), + expectedErrorPrefix: "unable to parse certificate:", + }, + { + name: "cert with invalid commmon name", + certData: testinghelpers.NewTestCert("test", 60*time.Second).Cert, + }, + { + name: "valid cert with correct common name", + certData: testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second).Cert, + expectedClusterName: "cluster1", + expectedAgentName: "agent1", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + clusterName, agentName, err := GetClusterAgentNamesFromCertificate(c.certData) + testinghelpers.AssertErrorWithPrefix(t, err, c.expectedErrorPrefix) + + if clusterName != c.expectedClusterName { + t.Errorf("expect %v, but got %v", c.expectedClusterName, clusterName) + } + + if agentName != c.expectedAgentName { + t.Errorf("expect %v, but got %v", c.expectedAgentName, agentName) + } + }) + } +} diff --git a/pkg/spoke/hubclientcert/controller.go b/pkg/spoke/hubclientcert/controller.go index e75ee5b7a..3d681f74a 100644 --- a/pkg/spoke/hubclientcert/controller.go +++ b/pkg/spoke/hubclientcert/controller.go @@ -179,9 +179,9 @@ func (c *ClientCertForHubController) sync(ctx context.Context, syncCtx factory.S } // create a csr to request new client certificate if - // a. there is no client certificate + // a. there is no valid client certificate issued for the current cluster/agent // b. client certificate exists and has less than 20% of its life remaining - if hasValidKubeconfig(secret) { + if hasValidKubeconfig(secret, fmt.Sprintf("%s%s:%s", subjectPrefix, c.clusterName, c.agentName)) { notBefore, notAfter, err := getCertValidityPeriod(secret) if err != nil { return err diff --git a/pkg/spoke/hubclientcert/controller_test.go b/pkg/spoke/hubclientcert/controller_test.go index bb0c332c4..672ff3c10 100644 --- a/pkg/spoke/hubclientcert/controller_test.go +++ b/pkg/spoke/hubclientcert/controller_test.go @@ -83,7 +83,7 @@ func TestSync(t *testing.T) { testinghelpers.AssertActions(t, hubActions, "get") testinghelpers.AssertActions(t, agentActions, "get", "update") actual := agentActions[1].(clienttesting.UpdateActionImpl).Object - if !hasValidKubeconfig(actual.(*corev1.Secret)) { + if !hasValidKubeconfig(actual.(*corev1.Secret), "") { t.Error("kubeconfig secret is invalid") } }, diff --git a/pkg/spoke/hubclientcert/secret_controller.go b/pkg/spoke/hubclientcert/secret_controller.go index 0e9bb2c7d..c771a69e2 100644 --- a/pkg/spoke/hubclientcert/secret_controller.go +++ b/pkg/spoke/hubclientcert/secret_controller.go @@ -18,7 +18,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" corev1informers "k8s.io/client-go/informers/core/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" - corev1lister "k8s.io/client-go/listers/core/v1" "k8s.io/klog/v2" ) @@ -29,7 +28,6 @@ type hubKubeconfigSecretController struct { hubKubeconfigSecretNamespace string hubKubeconfigSecretName string spokeCoreClient corev1client.CoreV1Interface - spokeSecretLister corev1lister.SecretLister } // NewHubKubeconfigSecretController returns a new HubKubeconfigSecretController @@ -43,7 +41,6 @@ func NewHubKubeconfigSecretController( hubKubeconfigSecretNamespace: hubKubeconfigSecretNamespace, hubKubeconfigSecretName: hubKubeconfigSecretName, spokeCoreClient: spokeCoreClient, - spokeSecretLister: spokeSecretInformer.Lister(), } return factory.New(). @@ -70,55 +67,52 @@ func NewHubKubeconfigSecretController( func (s *hubKubeconfigSecretController) sync(ctx context.Context, syncCtx factory.SyncContext) error { klog.V(4).Infof("Reconciling Hub KubeConfig secret %q", s.hubKubeconfigSecretName) - secret, err := s.spokeCoreClient.Secrets(s.hubKubeconfigSecretNamespace).Get(ctx, s.hubKubeconfigSecretName, metav1.GetOptions{}) + return DumpSecret(s.spokeCoreClient, s.hubKubeconfigSecretNamespace, s.hubKubeconfigSecretName, s.hubKubeconfigDir, ctx, syncCtx.Recorder()) +} + +// DumpSecret dumps the data in the given seccret into a directory in file system. +// The output directory will be created if not exists. +// TO DO: remove the file once the corresponding key is removed from secret. +func DumpSecret( + coreV1Client corev1client.CoreV1Interface, + secretNamespace, secretName, outputDir string, + ctx context.Context, + recorder events.Recorder) error { + secret, err := coreV1Client.Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) if errors.IsNotFound(err) { return nil } if err != nil { - return fmt.Errorf("unable to get secret %s/%s : %w", s.hubKubeconfigSecretNamespace, s.hubKubeconfigSecretName, err) + return fmt.Errorf("unable to get secret %s/%s : %w", secretNamespace, secretName, err) } - // if the secret is invalid, ignore it - if !hasValidKubeconfig(secret) { - return nil + if err := os.MkdirAll(outputDir, 0700); err != nil { + return fmt.Errorf("unable to create dir %q : %w", outputDir, err) } - if err := os.MkdirAll(s.hubKubeconfigDir, 0700); err != nil { - return fmt.Errorf("unable to create dir %q : %w", s.hubKubeconfigDir, err) - } - - // create/update configuration files from the secret + // create/update files from the secret for key, data := range secret.Data { - configFilePath := path.Join(s.hubKubeconfigDir, key) - if err := writeConfigFile(configFilePath, data, syncCtx.Recorder()); err != nil { - return fmt.Errorf("unable to write config file %q: %w", configFilePath, err) + filename := path.Clean(path.Join(outputDir, key)) + lastData, err := ioutil.ReadFile(filename) + switch { + case os.IsNotExist(err): + // create file + if err := ioutil.WriteFile(filename, data, 0600); err != nil { + return fmt.Errorf("unable to write file %q: %w", filename, err) + } + recorder.Event("FileCreated", fmt.Sprintf("File %q is created from secret %s/%s", filename, secretNamespace, secretName)) + case err != nil: + return fmt.Errorf("unable to read file %q: %w", filename, err) + case bytes.Equal(lastData, data): + // skip file without any change + continue + default: + // update file + if err := ioutil.WriteFile(path.Clean(filename), data, 0600); err != nil { + return fmt.Errorf("unable to write file %q: %w", filename, err) + } + recorder.Event("FileUpdated", fmt.Sprintf("File %q is updated from secret %s/%s", filename, secretNamespace, secretName)) } } return nil } - -// writeConfigFile creates or updates a specified file and record an event to log it. -func writeConfigFile(filename string, data []byte, recorder events.Recorder) error { - lastData, err := ioutil.ReadFile(path.Clean(filename)) - if os.IsNotExist(err) { - if err := ioutil.WriteFile(path.Clean(filename), data, 0600); err != nil { - return err - } - recorder.Event("HubKubeConfigFileCreated", fmt.Sprintf("Hub config file %q is created from hub kubeconfig secret", filename)) - return nil - } - if err != nil { - return err - } - - if bytes.Equal(lastData, data) { - return nil - } - - if err := ioutil.WriteFile(path.Clean(filename), data, 0600); err != nil { - return err - } - - recorder.Event("HubKubeConfigFileUpdated", fmt.Sprintf("Hub config file %q is updated from hub kubeconfig secret", filename)) - return nil -} diff --git a/pkg/spoke/hubclientcert/secret_controller_test.go b/pkg/spoke/hubclientcert/secret_controller_test.go index 5574ed748..51d5b1986 100644 --- a/pkg/spoke/hubclientcert/secret_controller_test.go +++ b/pkg/spoke/hubclientcert/secret_controller_test.go @@ -10,15 +10,15 @@ import ( "time" testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" + "github.com/openshift/library-go/pkg/operator/events/eventstesting" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/rand" - kubeinformers "k8s.io/client-go/informers" kubefake "k8s.io/client-go/kubernetes/fake" ) -func TestHubKubeconfigSecretSync(t *testing.T) { - testDir, err := ioutil.TempDir("", "testhubkubeconfigsecretsync") +func TestDumpSecret(t *testing.T) { + testDir, err := ioutil.TempDir("", "dumpsecret") if err != nil { t.Errorf("unexpected error: %v", err) } @@ -47,20 +47,6 @@ func TestHubKubeconfigSecretSync(t *testing.T) { } }, }, - { - name: "invalid secret", - queueKey: testSecretName, - secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", nil, map[string][]byte{}), - validateFiles: func(t *testing.T, hubKubeconfigDir string) { - files, err := ioutil.ReadDir(hubKubeconfigDir) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if len(files) != 0 { - t.Errorf("expect no files, but get %d files", len(files)) - } - }, - }, { name: "secret is created", queueKey: testSecretName, @@ -110,11 +96,6 @@ func TestHubKubeconfigSecretSync(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { kubeClient := kubefake.NewSimpleClientset(c.secret) - kubeInformerFactory := kubeinformers.NewSharedInformerFactory(kubeClient, time.Minute*10) - secretStore := kubeInformerFactory.Core().V1().Secrets().Informer().GetStore() - if c.secret != nil { - secretStore.Add(c.secret) - } hubKubeconfigDir := path.Join(testDir, fmt.Sprintf("/%s/hub-kubeconfig", rand.String(6))) if err := os.MkdirAll(hubKubeconfigDir, 0755); err != nil { @@ -124,16 +105,9 @@ func TestHubKubeconfigSecretSync(t *testing.T) { testinghelpers.WriteFile(path.Join(hubKubeconfigDir, k), v) } - ctrl := hubKubeconfigSecretController{ - hubKubeconfigDir: hubKubeconfigDir, - hubKubeconfigSecretName: testSecretName, - hubKubeconfigSecretNamespace: testNamespace, - spokeCoreClient: kubeClient.CoreV1(), - spokeSecretLister: kubeInformerFactory.Core().V1().Secrets().Lister(), - } - syncErr := ctrl.sync(context.TODO(), testinghelpers.NewFakeSyncContext(t, c.queueKey)) - if syncErr != nil { - t.Errorf("unexpected err: %v", syncErr) + err = DumpSecret(kubeClient.CoreV1(), testNamespace, testSecretName, hubKubeconfigDir, context.TODO(), eventstesting.NewTestingEventRecorder(t)) + if err != nil { + t.Errorf("unexpected err: %v", err) } c.validateFiles(t, hubKubeconfigDir) diff --git a/pkg/spoke/spokeagent.go b/pkg/spoke/spokeagent.go index 061ce8f50..b6291cff2 100644 --- a/pkg/spoke/spokeagent.go +++ b/pkg/spoke/spokeagent.go @@ -16,6 +16,7 @@ import ( "github.com/open-cluster-management/registration/pkg/spoke/managedcluster" "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/operator/events" "github.com/spf13/pflag" @@ -26,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" @@ -84,7 +86,13 @@ func NewSpokeAgentOptions() *SpokeAgentOptions { // create a valid hub kubeconfig. Once the hub kubeconfig is valid, the // temporary controller is stopped and the main controllers are started. func (o *SpokeAgentOptions) RunSpokeAgent(ctx context.Context, controllerContext *controllercmd.ControllerContext) error { - if err := o.Complete(); err != nil { + // create kube client + spokeKubeClient, err := kubernetes.NewForConfig(controllerContext.KubeConfig) + if err != nil { + return err + } + + if err := o.Complete(spokeKubeClient.CoreV1(), ctx, controllerContext.EventRecorder); err != nil { klog.Fatal(err) } @@ -94,11 +102,7 @@ func (o *SpokeAgentOptions) RunSpokeAgent(ctx context.Context, controllerContext klog.Infof("Cluster name is %q and agent name is %q", o.ClusterName, o.AgentName) - // create kube client and shared informer factory for spoke cluster - spokeKubeClient, err := kubernetes.NewForConfig(controllerContext.KubeConfig) - if err != nil { - return err - } + // create shared informer factory for spoke cluster spokeKubeInformerFactory := informers.NewSharedInformerFactory(spokeKubeClient, 10*time.Minute) // get spoke cluster CA bundle @@ -331,7 +335,7 @@ func (o *SpokeAgentOptions) Validate() error { } // Complete fills in missing values. -func (o *SpokeAgentOptions) Complete() error { +func (o *SpokeAgentOptions) Complete(coreV1Client corev1client.CoreV1Interface, ctx context.Context, recorder events.Recorder) error { // get component namespace of spoke agent nsBytes, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") if err != nil { @@ -340,6 +344,13 @@ func (o *SpokeAgentOptions) Complete() error { o.ComponentNamespace = string(nsBytes) } + // dump data in hub kubeconfig secret into file system if it exists + err = hubclientcert.DumpSecret(coreV1Client, o.ComponentNamespace, o.HubKubeconfigSecret, + o.HubKubeconfigDir, ctx, recorder) + if err != nil { + return err + } + // load or generate cluster/agent names o.ClusterName, o.AgentName = o.getOrGenerateClusterAgentNames() @@ -356,10 +367,15 @@ func generateAgentName() string { return utilrand.String(spokeAgentNameLength) } -// hasValidHubClientConfig returns ture if the conditions below are met: -// 1. KubeconfigFile exists -// 2. TLSKeyFile exists -// 3. TLSCertFile exists and the certificate is not expired +// hasValidHubClientConfig returns ture if all the conditions below are met: +// 1. KubeconfigFile exists; +// 2. TLSKeyFile exists; +// 3. TLSCertFile exists; +// 4. Certificate in TLSCertFile is issued for the current cluster/agent; +// 5. Certificate in TLSCertFile is not expired; +// Normally, KubeconfigFile/TLSKeyFile/TLSCertFile will be created once the bootstrap process +// completes. Changing the name of the cluster will make the existing hub kubeconfig invalid, +// because certificate in TLSCertFile is issued to a specific cluster/agent. func (o *SpokeAgentOptions) hasValidHubClientConfig() (bool, error) { kubeconfigPath := path.Join(o.HubKubeconfigDir, hubclientcert.KubeconfigFile) if _, err := os.Stat(kubeconfigPath); os.IsNotExist(err) { @@ -380,17 +396,42 @@ func (o *SpokeAgentOptions) hasValidHubClientConfig() (bool, error) { return false, nil } + // check if the tls certificate is issued for the current cluster/agent + clusterName, agentName, err := hubclientcert.GetClusterAgentNamesFromCertificate(certData) + if err != nil { + return false, nil + } + if clusterName != o.ClusterName || agentName != o.AgentName { + klog.V(4).Infof("Certificate in file %q is issued for agent %q instead of %q", + certPath, fmt.Sprintf("%s:%s", clusterName, agentName), + fmt.Sprintf("%s:%s", o.ClusterName, o.AgentName)) + return false, nil + } + return hubclientcert.IsCertificateValid(certData) } // getOrGenerateClusterAgentNames returns cluster name and agent name. -// Rules for picking up cluster/agent names: -// -// 1. take clusterName from input arguments if it is not empty -// 2. TODO: read cluster name from openshift struct if the spoke agent is running in an openshift cluster -// 3. take cluster/agent names from the mounted secret if they exist -// 4. generate random cluster/agent names then +// Rules for picking up cluster name: +// 1. Use cluster name from input arguments if 'cluster-name' is specified; +// 2. Parse cluster name from the common name of the certification subject if the certification exists; +// 3. Fallback to cluster name in the mounted secret if it exists; +// 4. TODO: Read cluster name from openshift struct if the agent is running in an openshift cluster; +// 5. Generate a random cluster name then; + +// Rules for picking up agent name: +// 1. Parse agent name from the common name of the certification subject if the certification exists; +// 2. Fallback to agent name in the mounted secret if it exists; +// 3. Generate a random agent name then; func (o *SpokeAgentOptions) getOrGenerateClusterAgentNames() (string, string) { + // try to load cluster/agent name from tls certification + var clusterNameInCert, agentNameInCert string + certPath := path.Join(o.HubKubeconfigDir, hubclientcert.TLSCertFile) + certData, certErr := ioutil.ReadFile(path.Clean(certPath)) + if certErr == nil { + clusterNameInCert, agentNameInCert, _ = hubclientcert.GetClusterAgentNamesFromCertificate(certData) + } + clusterName := o.ClusterName // if cluster name is not specified with input argument, try to load it from file if clusterName == "" { @@ -399,11 +440,19 @@ func (o *SpokeAgentOptions) getOrGenerateClusterAgentNames() (string, string) { // and then load the cluster name from the mounted secret clusterNameFilePath := path.Join(o.HubKubeconfigDir, hubclientcert.ClusterNameFile) clusterNameBytes, err := ioutil.ReadFile(path.Clean(clusterNameFilePath)) - if err != nil { - // generate random cluster name if faild - clusterName = generateClusterName() - } else { + switch { + case len(clusterNameInCert) > 0: + // use cluster name loaded from the tls certification + clusterName = clusterNameInCert + if clusterNameInCert != string(clusterNameBytes) { + klog.Warningf("Use cluster name %q in certification instead of %q in the mounted secret", clusterNameInCert, string(clusterNameBytes)) + } + case err == nil: + // use cluster name load from the mounted secret clusterName = string(clusterNameBytes) + default: + // generate random cluster name + clusterName = generateClusterName() } } @@ -411,11 +460,19 @@ func (o *SpokeAgentOptions) getOrGenerateClusterAgentNames() (string, string) { agentNameFilePath := path.Join(o.HubKubeconfigDir, hubclientcert.AgentNameFile) agentNameBytes, err := ioutil.ReadFile(path.Clean(agentNameFilePath)) var agentName string - if err != nil { - // generate random agent name if faild - agentName = generateAgentName() - } else { + switch { + case len(agentNameInCert) > 0: + // use agent name loaded from the tls certification + agentName = agentNameInCert + if agentNameInCert != string(agentNameBytes) { + klog.Warningf("Use agent name %q in certification instead of %q in the mounted secret", agentNameInCert, string(agentNameBytes)) + } + case err == nil: + // use agent name loaded from the mounted secret agentName = string(agentNameBytes) + default: + // generate random agent name + agentName = generateAgentName() } return clusterName, agentName diff --git a/pkg/spoke/spokeagent_test.go b/pkg/spoke/spokeagent_test.go index 1550be3b5..2a41ded4f 100644 --- a/pkg/spoke/spokeagent_test.go +++ b/pkg/spoke/spokeagent_test.go @@ -2,6 +2,7 @@ package spoke import ( "bytes" + "context" "io/ioutil" "os" "path" @@ -10,23 +11,127 @@ import ( testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" "github.com/open-cluster-management/registration/pkg/spoke/hubclientcert" + "github.com/openshift/library-go/pkg/operator/events/eventstesting" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + kubefake "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" ) func TestComplete(t *testing.T) { - options := NewSpokeAgentOptions() - if err := options.Complete(); err != nil { - t.Errorf("unexpected error: %v", err) + // get component namespace + var componentNamespace string + nsBytes, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + if err != nil { + componentNamespace = defaultSpokeComponentNamespace + } else { + componentNamespace = string(nsBytes) } - if options.ComponentNamespace == "" { - t.Error("component namespace should not be empty") + + cases := []struct { + name string + clusterName string + secret *corev1.Secret + expectedClusterName string + expectedAgentName string + }{ + { + name: "generate random cluster/agent name", + }, + { + name: "specify cluster name", + clusterName: "cluster1", + expectedClusterName: "cluster1", + }, + { + name: "override cluster name in secret with specified value", + clusterName: "cluster1", + secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", nil, map[string][]byte{ + "cluster-name": []byte("cluster2"), + "agent-name": []byte("agent2"), + }), + expectedClusterName: "cluster1", + expectedAgentName: "agent2", + }, + { + name: "override cluster name in cert with specified value", + clusterName: "cluster1", + secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", testinghelpers.NewTestCert("system:open-cluster-management:cluster2:agent2", 60*time.Second), map[string][]byte{ + "kubeconfig": testinghelpers.NewKubeconfig(nil, nil), + "cluster-name": []byte("cluster3"), + "agent-name": []byte("agent3"), + }), + expectedClusterName: "cluster1", + expectedAgentName: "agent2", + }, + { + name: "take cluster/agent name from secret", + secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", nil, map[string][]byte{ + "cluster-name": []byte("cluster1"), + "agent-name": []byte("agent1"), + }), + expectedClusterName: "cluster1", + expectedAgentName: "agent1", + }, + { + name: "take cluster/agent name from cert", + secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second), map[string][]byte{}), + expectedClusterName: "cluster1", + expectedAgentName: "agent1", + }, + { + name: "override cluster name in secret with value from cert", + secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second), map[string][]byte{ + "cluster-name": []byte("cluster2"), + "agent-name": []byte("agent2"), + }), + expectedClusterName: "cluster1", + expectedAgentName: "agent1", + }, } - if options.ClusterName == "" { - t.Error("cluster name should not be empty") - } - if options.AgentName == "" { - t.Error("agent name should not be empty") + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + // setup kube client + objects := []runtime.Object{} + if c.secret != nil { + objects = append(objects, c.secret) + } + kubeClient := kubefake.NewSimpleClientset(objects...) + + // create a tmp dir to dump hub kubeconfig + dir, err := ioutil.TempDir("", "hub-kubeconfig") + if err != nil { + t.Error("unable to create a tmp dir") + } + defer os.RemoveAll(dir) + + options := &SpokeAgentOptions{ + ClusterName: c.clusterName, + HubKubeconfigSecret: "hub-kubeconfig-secret", + HubKubeconfigDir: dir, + } + + if err := options.Complete(kubeClient.CoreV1(), context.TODO(), eventstesting.NewTestingEventRecorder(t)); err != nil { + t.Errorf("unexpected error: %v", err) + } + if options.ComponentNamespace == "" { + t.Error("component namespace should not be empty") + } + if options.ClusterName == "" { + t.Error("cluster name should not be empty") + } + if options.AgentName == "" { + t.Error("agent name should not be empty") + } + if len(c.expectedClusterName) > 0 && options.ClusterName != c.expectedClusterName { + t.Errorf("expect cluster name %q but got %q", c.expectedClusterName, options.ClusterName) + } + if len(c.expectedAgentName) > 0 && options.AgentName != c.expectedAgentName { + t.Errorf("expect agent name %q but got %q", c.expectedAgentName, options.AgentName) + } + }) } } @@ -97,15 +202,19 @@ func TestHasValidHubClientConfig(t *testing.T) { } defer os.RemoveAll(tempDir) - cert := testinghelpers.NewTestCert("test", 60*time.Second) - kubeconfig := testinghelpers.NewKubeconfig(cert.Key, cert.Cert) + cert1 := testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second) + cert2 := testinghelpers.NewTestCert("test", 60*time.Second) + + kubeconfig := testinghelpers.NewKubeconfig(nil, nil) cases := []struct { - name string - kubeconfig []byte - tlsCert []byte - tlsKey []byte - isValid bool + name string + clusterName string + agentName string + kubeconfig []byte + tlsCert []byte + tlsKey []byte + isValid bool }{ { name: "no kubeconfig", @@ -119,15 +228,26 @@ func TestHasValidHubClientConfig(t *testing.T) { { name: "no tls cert", kubeconfig: kubeconfig, - tlsKey: cert.Key, + tlsKey: cert1.Key, isValid: false, }, { - name: "valid hub client config", - kubeconfig: kubeconfig, - tlsKey: cert.Key, - tlsCert: cert.Cert, - isValid: true, + name: "cert is not issued for cluster1:agent1", + clusterName: "cluster1", + agentName: "agent1", + kubeconfig: kubeconfig, + tlsKey: cert2.Key, + tlsCert: cert2.Cert, + isValid: false, + }, + { + name: "valid hub client config", + clusterName: "cluster1", + agentName: "agent1", + kubeconfig: kubeconfig, + tlsKey: cert1.Key, + tlsCert: cert1.Cert, + isValid: true, }, } for _, c := range cases { @@ -142,7 +262,11 @@ func TestHasValidHubClientConfig(t *testing.T) { testinghelpers.WriteFile(path.Join(tempDir, "tls.crt"), c.tlsCert) } - options := &SpokeAgentOptions{HubKubeconfigDir: tempDir} + options := &SpokeAgentOptions{ + ClusterName: c.clusterName, + AgentName: c.agentName, + HubKubeconfigDir: tempDir, + } valid, err := options.hasValidHubClientConfig() if err != nil { t.Errorf("unexpected error: %v", err) diff --git a/test/integration/spokeagent_restart_test.go b/test/integration/spokeagent_restart_test.go new file mode 100644 index 000000000..867801177 --- /dev/null +++ b/test/integration/spokeagent_restart_test.go @@ -0,0 +1,314 @@ +package integration_test + +import ( + "context" + "path" + "time" + + "github.com/onsi/ginkgo" + "github.com/onsi/gomega" + + clusterv1 "github.com/open-cluster-management/api/cluster/v1" + "github.com/open-cluster-management/registration/pkg/spoke" + "github.com/open-cluster-management/registration/test/integration/util" + + "github.com/openshift/library-go/pkg/controller/controllercmd" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var _ = ginkgo.Describe("Agent Restart", func() { + + ginkgo.It("restart agent", func() { + var err error + managedClusterName := "restart-test-cluster1" + + hubKubeconfigSecret := "restart-test-hub-kubeconfig-secret" + hubKubeconfigDir := path.Join(util.TestDir, "restart-test", "hub-kubeconfig") + + bootstrapFile := path.Join(util.TestDir, "restart-test", "kubeconfig") + + ginkgo.By("Create bootstrap kubeconfig") + err = util.CreateBootstrapKubeConfigWithCertAge(bootstrapFile, securePort, 20*time.Second) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("run registration agent") + ctx, stopAgent := context.WithCancel(context.Background()) + go func() { + agentOptions := spoke.SpokeAgentOptions{ + ClusterName: managedClusterName, + BootstrapKubeconfig: bootstrapFile, + HubKubeconfigSecret: hubKubeconfigSecret, + HubKubeconfigDir: hubKubeconfigDir, + ClusterHealthCheckPeriod: 1 * time.Minute, + } + err := agentOptions.RunSpokeAgent(ctx, &controllercmd.ControllerContext{ + KubeConfig: spokeCfg, + EventRecorder: util.NewIntegrationTestEventRecorder("restart-test"), + }) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + }() + + ginkgo.By("Check existence of csr and ManagedCluster") + // the csr should be created + gomega.Eventually(func() bool { + if _, err := util.FindUnapprovedSpokeCSR(kubeClient, managedClusterName); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + // the spoke cluster should be created + gomega.Eventually(func() bool { + if _, err := util.GetManagedCluster(clusterClient, managedClusterName); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Accept ManagedCluster and approve csr") + err = util.AcceptManagedCluster(clusterClient, managedClusterName) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + err = util.ApproveSpokeClusterCSR(kubeClient, managedClusterName, time.Second*20) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("Check if hub kubeconfig secret is updated") + // the hub kubeconfig secret should be filled after the csr is approved + gomega.Eventually(func() bool { + if _, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Check if ManagedCluster joins the hub") + // the spoke cluster should have joined condition finally + gomega.Eventually(func() bool { + spokeCluster, err := util.GetManagedCluster(clusterClient, managedClusterName) + if err != nil { + return false + } + joined := meta.FindStatusCondition(spokeCluster.Status.Conditions, clusterv1.ManagedClusterConditionJoined) + if joined == nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Stop registration agent and wait for a grace period") + stopAgent() + time.Sleep(5 * time.Second) + + // remove the join condition. A new join condition will be added once the registration agent + // is restarted successfully + spokeCluster, err := util.GetManagedCluster(clusterClient, managedClusterName) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + conditions := []metav1.Condition{} + for _, condition := range spokeCluster.Status.Conditions { + if condition.Type == clusterv1.ManagedClusterConditionJoined { + continue + } + conditions = append(conditions, condition) + } + spokeCluster.Status.Conditions = conditions + spokeCluster, err = clusterClient.ClusterV1().ManagedClusters().UpdateStatus(context.TODO(), spokeCluster, metav1.UpdateOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("Restart registration agent") + ctx, stopAgent = context.WithCancel(context.Background()) + defer stopAgent() + + go func() { + agentOptions := spoke.SpokeAgentOptions{ + ClusterName: managedClusterName, + BootstrapKubeconfig: bootstrapFile, + HubKubeconfigSecret: hubKubeconfigSecret, + HubKubeconfigDir: hubKubeconfigDir, + ClusterHealthCheckPeriod: 1 * time.Minute, + } + err := agentOptions.RunSpokeAgent(ctx, &controllercmd.ControllerContext{ + KubeConfig: spokeCfg, + EventRecorder: util.NewIntegrationTestEventRecorder("restart-test"), + }) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + }() + + ginkgo.By("Check if ManagedCluster joins the hub") + // the spoke cluster should have joined condition finally + gomega.Eventually(func() bool { + spokeCluster, err := util.GetManagedCluster(clusterClient, managedClusterName) + if err != nil { + return false + } + joined := meta.FindStatusCondition(spokeCluster.Status.Conditions, clusterv1.ManagedClusterConditionJoined) + if joined == nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Check the existence of the renewal csr") + // The renewal csr is approved automaically on hub, which indicates the + // cluster/agent names keep the same + gomega.Eventually(func() bool { + _, err = util.FindAutoApprovedSpokeCSR(kubeClient, managedClusterName) + if err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + }) + + // This case happens when registration agent is restarted with a new cluster name by specifing + // argument 'cluster-name' and the agent has already had a hub kubecofig with a different + // cluster name. A bootstrap process is expected. + ginkgo.It("restart agent with a different cluster name", func() { + var err error + managedClusterName := "restart-test-cluster2" + + hubKubeconfigSecret := "restart-test-hub-kubeconfig-secret" + hubKubeconfigDir := path.Join(util.TestDir, "restart-test", "hub-kubeconfig") + + bootstrapFile := path.Join(util.TestDir, "restart-test", "kubeconfig") + + ginkgo.By("Create bootstrap kubeconfig") + err = util.CreateBootstrapKubeConfigWithCertAge(bootstrapFile, securePort, 20*time.Second) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("run registration agent") + ctx, stopAgent := context.WithCancel(context.Background()) + go func() { + agentOptions := spoke.SpokeAgentOptions{ + ClusterName: managedClusterName, + BootstrapKubeconfig: bootstrapFile, + HubKubeconfigSecret: hubKubeconfigSecret, + HubKubeconfigDir: hubKubeconfigDir, + ClusterHealthCheckPeriod: 1 * time.Minute, + } + err := agentOptions.RunSpokeAgent(ctx, &controllercmd.ControllerContext{ + KubeConfig: spokeCfg, + EventRecorder: util.NewIntegrationTestEventRecorder("restart-test"), + }) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + }() + + ginkgo.By("Check existence of csr and ManagedCluster") + // the csr should be created + gomega.Eventually(func() bool { + if _, err := util.FindUnapprovedSpokeCSR(kubeClient, managedClusterName); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + // the spoke cluster should be created + gomega.Eventually(func() bool { + if _, err := util.GetManagedCluster(clusterClient, managedClusterName); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Accept ManagedCluster and approve csr") + err = util.AcceptManagedCluster(clusterClient, managedClusterName) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + err = util.ApproveSpokeClusterCSR(kubeClient, managedClusterName, time.Second*20) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("Check if hub kubeconfig secret is updated") + // the hub kubeconfig secret should be filled after the csr is approved + gomega.Eventually(func() bool { + if _, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Check if ManagedCluster joins the hub") + // the spoke cluster should have joined condition finally + gomega.Eventually(func() bool { + spokeCluster, err := util.GetManagedCluster(clusterClient, managedClusterName) + if err != nil { + return false + } + joined := meta.FindStatusCondition(spokeCluster.Status.Conditions, clusterv1.ManagedClusterConditionJoined) + if joined == nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Stop registration agent and wait for a grace period") + stopAgent() + time.Sleep(5 * time.Second) + + ginkgo.By("Restart registration agent with a new cluster name") + ctx, stopAgent = context.WithCancel(context.Background()) + defer stopAgent() + + managedClusterName = "restart-test-cluster3" + go func() { + agentOptions := spoke.SpokeAgentOptions{ + ClusterName: managedClusterName, + BootstrapKubeconfig: bootstrapFile, + HubKubeconfigSecret: hubKubeconfigSecret, + HubKubeconfigDir: hubKubeconfigDir, + ClusterHealthCheckPeriod: 1 * time.Minute, + } + err := agentOptions.RunSpokeAgent(ctx, &controllercmd.ControllerContext{ + KubeConfig: spokeCfg, + EventRecorder: util.NewIntegrationTestEventRecorder("restart-test"), + }) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + }() + + ginkgo.By("Check the existence of csr and the new ManagedCluster") + // the csr should be created + gomega.Eventually(func() bool { + if _, err := util.FindUnapprovedSpokeCSR(kubeClient, managedClusterName); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + // the spoke cluster should be created + gomega.Eventually(func() bool { + if _, err := util.GetManagedCluster(clusterClient, managedClusterName); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Accept the new ManagedCluster and approve csr") + err = util.AcceptManagedCluster(clusterClient, managedClusterName) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + err = util.ApproveSpokeClusterCSR(kubeClient, managedClusterName, time.Second*20) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("Check if hub kubeconfig secret is updated") + // the hub kubeconfig secret should be filled after the csr is approved + gomega.Eventually(func() bool { + if _, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret); err != nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + + ginkgo.By("Check if the new ManagedCluster joins the hub") + // the spoke cluster should have joined condition finally + gomega.Eventually(func() bool { + spokeCluster, err := util.GetManagedCluster(clusterClient, managedClusterName) + if err != nil { + return false + } + joined := meta.FindStatusCondition(spokeCluster.Status.Conditions, clusterv1.ManagedClusterConditionJoined) + if joined == nil { + return false + } + return true + }, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue()) + }) +})