Merge pull request #105 from elgnay/issue_7679

Refine the logic to get agent name

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) {

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
+}

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)
+			}
+		})
+	}
+}

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

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")
 				}
 			},

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
-}

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)

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

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)

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())
+	})
+})