Merge pull request #6253 from zhzhuang-zju/uniquecluster

fix: A new pull-mode cluster may overwrite the existing member clusters

cmd/agent/app/agent.go

@@ -164,22 +164,15 @@ func run(ctx context.Context, opts *options.Options) error {
 		ClusterConfig:      clusterConfig,
 	}
 
-	id, err := util.ObtainClusterID(clusterKubeClient)
+	registerOption.ClusterID, err = util.ObtainClusterID(clusterKubeClient)
 	if err != nil {
 		return err
 	}
 
-	ok, name, err := util.IsClusterIdentifyUnique(karmadaClient, id)
+	if err = registerOption.Validate(karmadaClient, true); err != nil {
-	if err != nil {
 		return err
 	}
 
-	if !ok && opts.ClusterName != name {
-		return fmt.Errorf("the same cluster has been registered with name %s", name)
-	}
-
-	registerOption.ClusterID = id
-
 	clusterSecret, impersonatorSecret, err := util.ObtainCredentialsFromMemberCluster(clusterKubeClient, registerOption)
 	if err != nil {
 		return err

pkg/karmadactl/join/join.go

@@ -228,22 +228,15 @@ func (j *CommandJoinOption) RunJoinCluster(controlPlaneRestConfig, clusterConfig
 		ClusterConfig:      clusterConfig,
 	}
 
-	id, err := util.ObtainClusterID(clusterKubeClient)
+	registerOption.ClusterID, err = util.ObtainClusterID(clusterKubeClient)
 	if err != nil {
 		return err
 	}
 
-	ok, name, err := util.IsClusterIdentifyUnique(karmadaClient, id)
+	if err = registerOption.Validate(karmadaClient, false); err != nil {
-	if err != nil {
 		return err
 	}
 
-	if !ok {
-		return fmt.Errorf("the same cluster has been registered with name %s", name)
-	}
-
-	registerOption.ClusterID = id
-
 	clusterSecret, impersonatorSecret, err := util.ObtainCredentialsFromMemberCluster(clusterKubeClient, registerOption)
 	if err != nil {
 		return err

pkg/karmadactl/join/join_test.go

@@ -105,7 +105,7 @@ func TestRunJoinCluster(t *testing.T) {
 		prep                                 func(karmadaClient karmadaclientset.Interface, controlKubeClient kubeclient.Interface, clusterKubeClient kubeclient.Interface, opts *CommandJoinOption, clusterID types.UID, clusterName string) error
 		verify                               func(karmadaClient karmadaclientset.Interface, controlKubeClient kubeclient.Interface, clusterKubeClint kubeclient.Interface, opts *CommandJoinOption, clusterID types.UID) error
 		wantErr                              bool
-		errMsg                               string
+		errMsg                               func(opts *CommandJoinOption, clusterID types.UID) string
 	}{
 		{
 			name:                "RunJoinCluster_RegisterTheSameClusterWithSameID_TheSameClusterHasBeenRegistered",
@@ -136,7 +136,9 @@ func TestRunJoinCluster(t *testing.T) {
 				return nil
 			},
 			wantErr: true,
-			errMsg:  "the same cluster has been registered with name member1",
+			errMsg: func(opts *CommandJoinOption, clusterID types.UID) string {
+				return fmt.Sprintf("the cluster ID %s or the cluster name %s has been registered", clusterID, opts.ClusterName)
+			},
 		},
 		{
 			name: "RunJoinCluster_RegisterClusterInControllerPlane_ClusterRegisteredInControllerPlane",
@@ -170,8 +172,8 @@ func TestRunJoinCluster(t *testing.T) {
 			if err != nil && !test.wantErr {
 				t.Errorf("unexpected error, got: %v", err)
 			}
-			if err != nil && test.wantErr && !strings.Contains(err.Error(), test.errMsg) {
+			if err != nil && test.wantErr && !strings.Contains(err.Error(), test.errMsg(test.joinOpts, test.clusterID)) {
-				t.Errorf("expected error message %s to be in %s", test.errMsg, err.Error())
+				t.Errorf("expected error message %s to be in %s", test.errMsg(test.joinOpts, test.clusterID), err.Error())
 			}
 			if err := test.verify(test.karmadaClient, test.controlKubeClient, test.clusterKubeClient, test.joinOpts, test.clusterID); err != nil {
 				t.Errorf("failed to verify joining the cluster, got error: %v", err)

pkg/util/cluster.go

@@ -52,6 +52,7 @@ const (
 type ClusterRegisterOption struct {
 	ClusterNamespace   string
 	ClusterName        string
+	ClusterID          string
 	ReportSecrets      []string
 	ClusterAPIEndpoint string
 	ProxyServerAddress string
@@ -64,11 +65,10 @@ type ClusterRegisterOption struct {
 	ClusterConfig      *rest.Config
 	Secret             corev1.Secret
 	ImpersonatorSecret corev1.Secret
-	ClusterID          string
 }
 
 // IsKubeCredentialsEnabled represents whether report secret
-func (r ClusterRegisterOption) IsKubeCredentialsEnabled() bool {
+func (r *ClusterRegisterOption) IsKubeCredentialsEnabled() bool {
 	for _, sct := range r.ReportSecrets {
 		if sct == KubeCredentials {
 			return true
@@ -78,7 +78,7 @@ func (r ClusterRegisterOption) IsKubeCredentialsEnabled() bool {
 }
 
 // IsKubeImpersonatorEnabled represents whether report impersonator secret
-func (r ClusterRegisterOption) IsKubeImpersonatorEnabled() bool {
+func (r *ClusterRegisterOption) IsKubeImpersonatorEnabled() bool {
 	for _, sct := range r.ReportSecrets {
 		if sct == KubeImpersonator {
 			return true
@@ -87,6 +87,49 @@ func (r ClusterRegisterOption) IsKubeImpersonatorEnabled() bool {
 	return false
 }
 
+// Validate validates the cluster register option, including clusterID, cluster name and so on.
+func (r *ClusterRegisterOption) Validate(karmadaClient karmadaclientset.Interface, isAgent bool) error {
+	clusterList, err := karmadaClient.ClusterV1alpha1().Clusters().List(context.TODO(), metav1.ListOptions{})
+	if err != nil {
+		return err
+	}
+	clusterIDUsed, clusterNameUsed, sameCluster := r.validateCluster(clusterList)
+	if err != nil {
+		return err
+	}
+
+	if isAgent && sameCluster {
+		return nil
+	}
+
+	if clusterIDUsed || clusterNameUsed {
+		return fmt.Errorf("the cluster ID %s or the cluster name %s has been registered", r.ClusterID, r.ClusterName)
+	}
+
+	return nil
+}
+
+// validateCluster validates the cluster register option whether the cluster name and cluster ID are unique.
+// 1. When registering a cluster for the first time, the metrics `clusterIDUsed` and `clusterNameUsed` can be used
+// to check if the cluster ID and the cluster name have already been used, which can avoid duplicate registrations.
+// 2. In cases where the agent is restarted, the metric `sameCluster` can be used to determine if the cluster
+// specified in the `RegisterOption` has already been registered, aiming to achieve the purpose of re-entering and updating the cluster.
+func (r *ClusterRegisterOption) validateCluster(clusterList *clusterv1alpha1.ClusterList) (clusterIDUsed, clusterNameUsed, sameCluster bool) {
+	for _, cluster := range clusterList.Items {
+		if cluster.Spec.ID == r.ClusterID && cluster.GetName() == r.ClusterName {
+			return true, true, true
+		}
+		if cluster.Spec.ID == r.ClusterID {
+			clusterIDUsed = true
+		}
+		if cluster.GetName() == r.ClusterName {
+			clusterNameUsed = true
+		}
+	}
+
+	return clusterIDUsed, clusterNameUsed, false
+}
+
 // IsClusterReady tells whether the cluster status in 'Ready' condition.
 func IsClusterReady(clusterStatus *clusterv1alpha1.ClusterStatus) bool {
 	return meta.IsStatusConditionTrue(clusterStatus.Conditions, clusterv1alpha1.ClusterConditionReady)
@@ -208,21 +251,6 @@ func ObtainClusterID(clusterKubeClient kubernetes.Interface) (string, error) {
 	return string(ns.UID), nil
 }
 
-// IsClusterIdentifyUnique checks whether the ClusterID exists in the karmada control plane.
-func IsClusterIdentifyUnique(controlPlaneClient karmadaclientset.Interface, id string) (bool, string, error) {
-	clusterList, err := controlPlaneClient.ClusterV1alpha1().Clusters().List(context.TODO(), metav1.ListOptions{})
-	if err != nil {
-		return false, "", err
-	}
-
-	for _, cluster := range clusterList.Items {
-		if cluster.Spec.ID == id {
-			return false, cluster.Name, nil
-		}
-	}
-	return true, "", nil
-}
-
 // ClusterAccessCredentialChanged checks whether the cluster access credential changed
 func ClusterAccessCredentialChanged(newSpec, oldSpec clusterv1alpha1.ClusterSpec) bool {
 	if oldSpec.APIEndpoint == newSpec.APIEndpoint &&

pkg/util/cluster_test.go

@@ -22,7 +22,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -54,11 +53,6 @@ func withSyncMode(cluster *clusterv1alpha1.Cluster, syncMode clusterv1alpha1.Clu
 	return cluster
 }
 
-func withID(cluster *clusterv1alpha1.Cluster, id string) *clusterv1alpha1.Cluster {
-	cluster.Spec.ID = id
-	return cluster
-}
-
 func TestCreateOrUpdateClusterObject(t *testing.T) {
 	type args struct {
 		controlPlaneClient karmadaclientset.Interface
@@ -167,48 +161,6 @@ func TestCreateOrUpdateClusterObject(t *testing.T) {
 	}
 }
 
-func TestIsClusterIDUnique(t *testing.T) {
-	tests := []struct {
-		name           string
-		existedCluster []runtime.Object
-		id             string
-		want           bool
-		clustername    string
-	}{
-		{
-			name: "no cluster", id: "1", want: true,
-			existedCluster: []runtime.Object{},
-		},
-		{
-			name: "existed id", id: "1", want: false, clustername: "cluster-1",
-			existedCluster: []runtime.Object{withID(newCluster("cluster-1"), "1")},
-		},
-		{
-			name: "unique id", id: "2", want: true,
-			existedCluster: []runtime.Object{withID(newCluster("cluster-1"), "1")},
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			fakeClient := karmadaclientsetfake.NewSimpleClientset(tc.existedCluster...)
-
-			ok, name, err := IsClusterIdentifyUnique(fakeClient, tc.id)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			if ok != tc.want {
-				t.Errorf("expected value: %v, but got: %v", tc.want, ok)
-			}
-
-			if !ok && name != tc.clustername {
-				t.Errorf("expected clustername: %v, but got: %v", tc.clustername, name)
-			}
-		})
-	}
-}
-
 func TestClusterRegisterOption_IsKubeCredentialsEnabled(t *testing.T) {
 	type fields struct {
 		ReportSecrets []string
@@ -323,6 +275,106 @@ func TestClusterRegisterOption_IsKubeImpersonatorEnabled(t *testing.T) {
 	}
 }
 
+func TestClusterRegisterOption_ValidateCluster(t *testing.T) {
+	registeredClusterList := &clusterv1alpha1.ClusterList{
+		Items: []clusterv1alpha1.Cluster{
+			{
+				Spec: clusterv1alpha1.ClusterSpec{
+					ID: "1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "member1",
+				},
+			},
+			{
+				Spec: clusterv1alpha1.ClusterSpec{
+					ID: "2",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "member2",
+				},
+			},
+			{
+				Spec: clusterv1alpha1.ClusterSpec{
+					ID: "3",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "member3",
+				},
+			},
+		},
+	}
+
+	testItems := []struct {
+		name                    string
+		clusterList             *clusterv1alpha1.ClusterList
+		opts                    ClusterRegisterOption
+		expectedClusterIDUsed   bool
+		expectedClusterNameUsed bool
+		expectedSameCluster     bool
+	}{
+		{
+			name:        "registering a brand new cluster",
+			clusterList: registeredClusterList,
+			opts: ClusterRegisterOption{
+				ClusterID:   "4",
+				ClusterName: "member4",
+			},
+			expectedClusterIDUsed:   false,
+			expectedClusterNameUsed: false,
+			expectedSameCluster:     false,
+		},
+		{
+			name:        "clusterName is used",
+			clusterList: registeredClusterList,
+			opts: ClusterRegisterOption{
+				ClusterID:   "4",
+				ClusterName: "member2",
+			},
+			expectedClusterIDUsed:   false,
+			expectedClusterNameUsed: true,
+			expectedSameCluster:     false,
+		},
+		{
+			name:        "clusterID is used",
+			clusterList: registeredClusterList,
+			opts: ClusterRegisterOption{
+				ClusterID:   "2",
+				ClusterName: "member4",
+			},
+			expectedClusterIDUsed:   true,
+			expectedClusterNameUsed: false,
+			expectedSameCluster:     false,
+		},
+		{
+			name:        "the same cluster",
+			clusterList: registeredClusterList,
+			opts: ClusterRegisterOption{
+				ClusterID:   "2",
+				ClusterName: "member2",
+			},
+			expectedClusterIDUsed:   true,
+			expectedClusterNameUsed: true,
+			expectedSameCluster:     true,
+		},
+	}
+
+	for _, item := range testItems {
+		t.Run(item.name, func(t *testing.T) {
+			clusterIDUsed, clusterNameUsed, sameCluster := item.opts.validateCluster(item.clusterList)
+			if clusterIDUsed != item.expectedClusterIDUsed {
+				t.Errorf("clusterNameUsed = %v, want %v", clusterIDUsed, item.expectedClusterIDUsed)
+			}
+			if clusterNameUsed != item.expectedClusterNameUsed {
+				t.Errorf("clusterNameUsed = %v, want %v", clusterNameUsed, item.expectedClusterNameUsed)
+			}
+			if sameCluster != item.expectedSameCluster {
+				t.Errorf("clusterNameUsed = %v, want %v", sameCluster, item.expectedSameCluster)
+			}
+		})
+	}
+}
+
 func TestCreateClusterObject(t *testing.T) {
 	type args struct {
 		controlPlaneClient karmadaclientset.Interface