Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict the cluster sync mode for the unjoin and unregister command #6081

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/karmadactl/unjoin/unjoin.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/klog/v2"
"k8s.io/kubectl/pkg/util/templates"

clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
karmadaclientset "github.com/karmada-io/karmada/pkg/generated/clientset/versioned"
"github.com/karmada-io/karmada/pkg/karmadactl/options"
cmdutil "github.com/karmada-io/karmada/pkg/karmadactl/util"
Expand Down Expand Up @@ -194,8 +195,16 @@ func (j *CommandUnjoinOption) RunUnJoinCluster(controlPlaneRestConfig, clusterCo
controlPlaneKarmadaClient := controlPlaneKarmadaClientBuilder(controlPlaneRestConfig)
controlPlaneKubeClient := controlPlaneKubeClientBuilder(controlPlaneRestConfig)

syncMode, err := cmdutil.GetClusterSyncMode(controlPlaneKarmadaClient, j.ClusterName)
if err != nil {
return err
}
if syncMode == clusterv1alpha1.Pull {
return fmt.Errorf("cluster %s is a %s mode member cluster, please use command `unregister` if you want to unregister cluster", j.ClusterName, syncMode)
}

// delete the cluster object in host cluster that associates the unjoining cluster
err := cmdutil.DeleteClusterObject(controlPlaneKubeClient, controlPlaneKarmadaClient, j.ClusterName, j.Wait, j.DryRun, j.forceDeletion)
err = cmdutil.DeleteClusterObject(controlPlaneKubeClient, controlPlaneKarmadaClient, j.ClusterName, j.Wait, j.DryRun, j.forceDeletion)
if err != nil {
klog.Errorf("Failed to delete cluster object. cluster name: %s, error: %v", j.ClusterName, err)
return err
Expand Down
2 changes: 1 addition & 1 deletion pkg/karmadactl/unjoin/unjoin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestRunUnJoinCluster(t *testing.T) {
clusterConfig: &rest.Config{},
karmadaClient: fakekarmadaclient.NewSimpleClientset(),
prep: func(_ kubeclient.Interface, _ kubeclient.Interface, karmadaClient karmadaclientset.Interface, _ *CommandUnjoinOption) error {
karmadaClient.(*fakekarmadaclient.Clientset).Fake.PrependReactor("delete", "clusters", func(coretesting.Action) (bool, runtime.Object, error) {
karmadaClient.(*fakekarmadaclient.Clientset).Fake.PrependReactor("get", "clusters", func(coretesting.Action) (bool, runtime.Object, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is delete changed to get because a query is added?

return true, nil, errors.New("unexpected error: encountered a network issue while deleting the clusters")
})
controlPlaneKarmadaClientBuilder = func(*rest.Config) karmadaclientset.Interface {
Expand Down
11 changes: 10 additions & 1 deletion pkg/karmadactl/unregister/unregister.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"k8s.io/klog/v2"
"k8s.io/kubectl/pkg/util/templates"

clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
"github.com/karmada-io/karmada/pkg/apis/cluster/validation"
karmadaclientset "github.com/karmada-io/karmada/pkg/generated/clientset/versioned"
"github.com/karmada-io/karmada/pkg/karmadactl/options"
Expand Down Expand Up @@ -310,6 +311,14 @@ func (j *CommandUnregisterOption) getKarmadaAgentConfig(agent *appsv1.Deployment

// RunUnregisterCluster unregister the pull mode cluster from karmada.
func (j *CommandUnregisterOption) RunUnregisterCluster() error {
syncMode, err := cmdutil.GetClusterSyncMode(j.ControlPlaneClient, j.ClusterName)
if err != nil {
return err
}
if syncMode == clusterv1alpha1.Push {
return fmt.Errorf("cluster %s is a %s mode member cluster, please use command `unjoin` if you want to unregister cluster", j.ClusterName, syncMode)
}

if j.DryRun {
return nil
}
Expand All @@ -318,7 +327,7 @@ func (j *CommandUnregisterOption) RunUnregisterCluster() error {
// 1. delete the work object from the Karmada control plane
// When deleting a cluster, the deletion triggers the removal of executionSpace, which can lead to the deletion of RBAC roles related to work.
// Therefore, the deletion of work should be performed before deleting the cluster.
err := cmdutil.EnsureWorksDeleted(j.ControlPlaneClient, names.GenerateExecutionSpaceName(j.ClusterName), j.Wait)
err = cmdutil.EnsureWorksDeleted(j.ControlPlaneClient, names.GenerateExecutionSpaceName(j.ClusterName), j.Wait)
if err != nil {
klog.Errorf("Failed to delete works object. cluster name: %s, error: %v", j.ClusterName, err)
return err
Expand Down
14 changes: 14 additions & 0 deletions pkg/karmadactl/util/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
karmadaclientset "github.com/karmada-io/karmada/pkg/generated/clientset/versioned"
"github.com/karmada-io/karmada/pkg/util"
"github.com/karmada-io/karmada/pkg/util/names"
Expand Down Expand Up @@ -95,6 +96,19 @@ func DeleteClusterObject(controlPlaneKubeClient kubeclient.Interface, controlPla
return err
}

// GetClusterSyncMode return the sync mode of the member cluster.
func GetClusterSyncMode(controlPlaneKarmadaClient karmadaclientset.Interface, clusterName string) (clusterv1alpha1.ClusterSyncMode, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this function doesn't feel very necessary, and inconvenient to test

target, err := controlPlaneKarmadaClient.ClusterV1alpha1().Clusters().Get(context.TODO(), clusterName, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
return clusterv1alpha1.Push, fmt.Errorf("no cluster object %s found in karmada control Plane", clusterName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better for sycnmode to return an empty string when the returned err is not null?

The first is that it's easier to read and understand, and the second is that the handler of the mode function is improperly handled.

}
if err != nil {
klog.Errorf("Failed to get cluster object. cluster name: %s, error: %v", clusterName, err)
return clusterv1alpha1.Push, err
}
return target.Spec.SyncMode, nil
}

// removeWorkFinalizer removes the finalizer of works in the executionSpace.
func removeWorkFinalizer(executionSpaceName string, controlPlaneKarmadaClient karmadaclientset.Interface) error {
list, err := controlPlaneKarmadaClient.WorkV1alpha1().Works(executionSpaceName).List(context.TODO(), metav1.ListOptions{})
Expand Down