diff --git a/cmd/agent/app/agent.go b/cmd/agent/app/agent.go index fc35ef8c5..fa67234f0 100644 --- a/cmd/agent/app/agent.go +++ b/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 != nil { + if err = registerOption.Validate(karmadaClient, true); 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 diff --git a/pkg/karmadactl/join/join.go b/pkg/karmadactl/join/join.go index 9803051fe..21097b4a2 100644 --- a/pkg/karmadactl/join/join.go +++ b/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 != nil { + if err = registerOption.Validate(karmadaClient, false); 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 diff --git a/pkg/karmadactl/join/join_test.go b/pkg/karmadactl/join/join_test.go index f13fedb7f..b9e768849 100644 --- a/pkg/karmadactl/join/join_test.go +++ b/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) { - t.Errorf("expected error message %s to be in %s", test.errMsg, err.Error()) + 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(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) diff --git a/pkg/util/cluster.go b/pkg/util/cluster.go index 974b3615f..a57e39b58 100644 --- a/pkg/util/cluster.go +++ b/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 && diff --git a/pkg/util/cluster_test.go b/pkg/util/cluster_test.go index e4f7ab85b..0ea8fc6bf 100644 --- a/pkg/util/cluster_test.go +++ b/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