fix: member agent validate opts, fix klog.ErrorS, and kubectl-fleet CLI improvements (#575)

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>

Author: ytimocin

Makefile

@@ -224,6 +224,7 @@ generate: $(CONTROLLER_GEN) ## Generate deep copy methods
 build: generate fmt vet ## Build agent binaries
 	go build -o bin/hubagent cmd/hubagent/main.go
 	go build -o bin/memberagent cmd/memberagent/main.go
+	go build -o bin/kubectl-fleet ./tools/fleet/
 
 .PHONY: run-hubagent
 run-hubagent: manifests generate fmt vet ## Run hub-agent from your host

cmd/memberagent/main.go

@@ -91,6 +91,11 @@ func main() {
 		klog.InfoS("flag:", "name", f.Name, "value", f.Value)
 	})
 
+	if errs := opts.Validate(); len(errs) != 0 {
+		klog.ErrorS(errs.ToAggregate(), "invalid parameter")
+		klog.FlushAndExit(klog.ExitFlushTimeout, 1)
+	}
+
 	// Set up controller-runtime logger
 	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
 
@@ -213,13 +218,16 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo
 			return err
 		})
 		if err != nil {
-			klog.ErrorS(err, "Failed to retrieve token file from the path %s", tokenFilePath)
+			klog.ErrorS(err, "Failed to retrieve token file", "path", tokenFilePath)
 			return nil, err
 		}
 		hubConfig.BearerTokenFile = tokenFilePath
 	}
 
 	hubConfig.TLSClientConfig.Insecure = tlsClientInsecure
+	if tlsClientInsecure {
+		klog.Warning("TLS verification is disabled for hub cluster connection. This is insecure and should not be used in production.")
+	}
 	if !tlsClientInsecure {
 		caBundle, ok := os.LookupEnv("CA_BUNDLE")
 		if ok && caBundle == "" {
@@ -257,7 +265,7 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo
 		r := textproto.NewReader(bufio.NewReader(strings.NewReader(header)))
 		h, err := r.ReadMIMEHeader()
 		if err != nil && !errors.Is(err, io.EOF) {
-			klog.ErrorS(err, "Failed to parse HUB_KUBE_HEADER %q", header)
+			klog.ErrorS(err, "Failed to parse HUB_KUBE_HEADER", "header", header)
 			return nil, err
 		}
 		hubConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper {

test/e2e/cluster_staged_updaterun_test.go

@@ -2203,7 +2203,7 @@ func approveClusterApprovalRequest(stageName, updateRunName, stageTask string) {
 
 	// Use kubectl-fleet approve plugin to approve the request
 	cmd := exec.Command(fleetBinaryPath, "approve", "clusterapprovalrequest",
-		"--hubClusterContext", "kind-hub",
+		"--hub-cluster-context", "kind-hub",
 		"--name", approvalRequestName)
 	output, err := cmd.CombinedOutput()
 	Expect(err).ToNot(HaveOccurred(), "kubectl-fleet approve failed: %s", string(output))

test/e2e/drain_tool_test.go

@@ -360,17 +360,17 @@ var _ = Describe("Drain is allowed on one cluster, blocked on others - ClusterRe
 func runDrainClusterBinary(hubClusterName, memberClusterName string) {
 	By(fmt.Sprintf("draining cluster %s", memberClusterName))
 	cmd := exec.Command(fleetBinaryPath, "draincluster",
-		"--hubClusterContext", hubClusterName,
-		"--clusterName", memberClusterName)
+		"--hub-cluster-context", hubClusterName,
+		"--cluster-name", memberClusterName)
 	_, err := cmd.CombinedOutput()
 	Expect(err).ToNot(HaveOccurred(), "Drain command failed with error: %v", err)
 }
 
 func runUncordonClusterBinary(hubClusterName, memberClusterName string) {
 	By(fmt.Sprintf("uncordoning cluster %s", memberClusterName))
 	cmd := exec.Command(fleetBinaryPath, "uncordoncluster",
-		"--hubClusterContext", hubClusterName,
-		"--clusterName", memberClusterName)
+		"--hub-cluster-context", hubClusterName,
+		"--cluster-name", memberClusterName)
 	_, err := cmd.CombinedOutput()
 	Expect(err).ToNot(HaveOccurred(), "Uncordon command failed with error: %v", err)
 }

tools/fleet/README.md

@@ -72,51 +72,51 @@ Use the `approve` subcommand to approve approval request resources for staged up
 
 **Cluster-scoped (ClusterApprovalRequest):**
 ```bash
-kubectl fleet approve clusterapprovalrequest --hubClusterContext <hub-cluster-context> --name <approval-request-name>
+kubectl fleet approve clusterapprovalrequest --hub-cluster-context <hub-cluster-context> --name <approval-request-name>
 # Or using alias:
-kubectl fleet approve careq --hubClusterContext <hub-cluster-context> --name <approval-request-name>
+kubectl fleet approve careq --hub-cluster-context <hub-cluster-context> --name <approval-request-name>
 ```
 
 **Namespace-scoped (ApprovalRequest):**
 ```bash
-kubectl fleet approve approvalrequest --hubClusterContext <hub-cluster-context> --name <approval-request-name> --namespace <namespace>
+kubectl fleet approve approvalrequest --hub-cluster-context <hub-cluster-context> --name <approval-request-name> --namespace <namespace>
 # Or using alias:
-kubectl fleet approve areq --hubClusterContext <hub-cluster-context> --name <approval-request-name> -n <namespace>
+kubectl fleet approve areq --hub-cluster-context <hub-cluster-context> --name <approval-request-name> -n <namespace>
 ```
 
 Example:
 ```bash
 # Approve a ClusterApprovalRequest
-kubectl fleet approve clusterapprovalrequest --hubClusterContext hub --name my-approval-request
+kubectl fleet approve clusterapprovalrequest --hub-cluster-context hub --name my-approval-request
 
 # Approve an ApprovalRequest in a specific namespace
-kubectl fleet approve approvalrequest --hubClusterContext hub --name my-approval-request --namespace my-namespace
+kubectl fleet approve approvalrequest --hub-cluster-context hub --name my-approval-request --namespace my-namespace
 ```
 
 ### Drain a Member Cluster
 
 Use the `draincluster` subcommand to remove all resources propagated to a member cluster from the hub cluster by any `Placement` resource. This is useful when you want to temporarily move all workloads off a member cluster in preparation for an event like upgrade or reconfiguration.
 
 ```bash
-kubectl fleet draincluster --hubClusterContext <hub-cluster-context> --clusterName <memberClusterName>
+kubectl fleet draincluster --hub-cluster-context <hub-cluster-context> --cluster-name <memberClusterName>
 ```
 
 Example:
 ```bash
-kubectl fleet draincluster --hubClusterContext hub --clusterName member-cluster-1
+kubectl fleet draincluster --hub-cluster-context hub --cluster-name member-cluster-1
 ```
 
 ### Uncordon a Member Cluster
 
 Use the `uncordoncluster` subcommand to uncordon a member cluster that has been previously drained, allowing resources to be propagated to the cluster again.
 
 ```bash
-kubectl fleet uncordoncluster --hubClusterContext <hub-cluster-context> --clusterName <memberClusterName>
+kubectl fleet uncordoncluster --hub-cluster-context <hub-cluster-context> --cluster-name <memberClusterName>
 ```
 
 Example:
 ```bash
-kubectl fleet uncordoncluster --hubClusterContext hub --clusterName member-cluster-1
+kubectl fleet uncordoncluster --hub-cluster-context hub --cluster-name member-cluster-1
 ```
 
 ## Subcommands
@@ -156,13 +156,13 @@ If the `cordon` taint is not present on the member cluster, the command will hav
 ## Flags
 
 The `approve` subcommand uses the following flags:
-- `--hubClusterContext`: kubectl context for the hub cluster (required)
+- `--hub-cluster-context`: kubectl context for the hub cluster (required)
 - `--name`: name of the resource to approve (required)
 - `--namespace`, `-n`: namespace of the resource to approve (required for `approvalrequest`, not allowed for `clusterapprovalrequest`)
 
 Both `draincluster` and `uncordoncluster` subcommands use the following flags:
-- `--hubClusterContext`: kubectl context for the hub cluster (required)
-- `--clusterName`: name of the member cluster to operate on (required)
+- `--hub-cluster-context`: kubectl context for the hub cluster (required)
+- `--cluster-name`: name of the member cluster to operate on (required)
 
 ## Examples
 
@@ -173,31 +173,31 @@ Both `draincluster` and `uncordoncluster` subcommands use the following flags:
 kubectl config get-contexts
 
 # 2. Drain a cluster for maintenance
-kubectl fleet draincluster --hubClusterContext production-hub --clusterName worker-node-1
+kubectl fleet draincluster --hub-cluster-context production-hub --cluster-name worker-node-1
 
 # 3. Perform maintenance on the worker-node-1 cluster
 # ... maintenance operations ...
 
 # 4. After maintenance, uncordon the cluster to allow workloads back
-kubectl fleet uncordoncluster --hubClusterContext production-hub --clusterName worker-node-1
+kubectl fleet uncordoncluster --hub-cluster-context production-hub --cluster-name worker-node-1
 ```
 
 ### Additional Examples
 
 ```bash
 # Approve a ClusterApprovalRequest for staged updates
-kubectl fleet approve clusterapprovalrequest --hubClusterContext hub --name update-approval-stage-1
+kubectl fleet approve clusterapprovalrequest --hub-cluster-context hub --name update-approval-stage-1
 
 # Approve a ApprovalRequest for staged updates
-kubectl fleet approve approvalrequest --hubClusterContext hub --name update-approval-stage-1 --namespace test-namespace
+kubectl fleet approve approvalrequest --hub-cluster-context hub --name update-approval-stage-1 --namespace test-namespace
 
 # Drain multiple clusters (run separately for each cluster)
-kubectl fleet draincluster --hubClusterContext hub --clusterName east-cluster
-kubectl fleet draincluster --hubClusterContext hub --clusterName west-cluster
+kubectl fleet draincluster --hub-cluster-context hub --cluster-name east-cluster
+kubectl fleet draincluster --hub-cluster-context hub --cluster-name west-cluster
 
 # Uncordon clusters after maintenance
-kubectl fleet uncordoncluster --hubClusterContext hub --clusterName east-cluster
-kubectl fleet uncordoncluster --hubClusterContext hub --clusterName west-cluster
+kubectl fleet uncordoncluster --hub-cluster-context hub --cluster-name east-cluster
+kubectl fleet uncordoncluster --hub-cluster-context hub --cluster-name west-cluster
 ```
 
 ## Troubleshooting

tools/fleet/cmd/approve/approve.go

@@ -20,11 +20,11 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"time"
 
 	"github.com/spf13/cobra"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -87,6 +87,7 @@ type approveOptions struct {
 	hubClusterContext string
 	name              string
 	namespace         string
+	timeout           time.Duration
 
 	hubClient client.Client
 }
@@ -119,16 +120,19 @@ For namespace-scoped resources (approvalrequest), you must also specify the --na
 			if err := o.setupClient(); err != nil {
 				return err
 			}
-			return o.run(cmd.Context(), cfg)
+			ctx, cancel := context.WithTimeout(cmd.Context(), o.timeout)
+			defer cancel()
+			return o.run(ctx, cfg)
 		},
 	}
 
-	cmd.Flags().StringVar(&o.hubClusterContext, "hubClusterContext", "", "The name of the kubeconfig context to use for the hub cluster")
+	cmd.Flags().StringVar(&o.hubClusterContext, "hub-cluster-context", "", "The name of the kubeconfig context to use for the hub cluster")
 	cmd.Flags().StringVar(&o.name, "name", "", "The name of the resource to approve")
 	cmd.Flags().StringVarP(&o.namespace, "namespace", "n", "", "The namespace of the resource to approve (required for namespace-scoped resources)")
+	cmd.Flags().DurationVar(&o.timeout, "timeout", 5*time.Minute, "Maximum time to wait for the operation to complete")
 
 	// Mark required flags.
-	_ = cmd.MarkFlagRequired("hubClusterContext")
+	_ = cmd.MarkFlagRequired("hub-cluster-context")
 	_ = cmd.MarkFlagRequired("name")
 
 	return cmd
@@ -208,10 +212,9 @@ func (o *approveOptions) approveApprovalRequest(ctx context.Context) error {
 
 // setupClient creates and configures the Kubernetes client
 func (o *approveOptions) setupClient() error {
-	scheme := runtime.NewScheme()
-
-	if err := placementv1beta1.AddToScheme(scheme); err != nil {
-		return fmt.Errorf("failed to add custom APIs (placement) to the runtime scheme: %w", err)
+	scheme, err := toolsutils.NewFleetScheme()
+	if err != nil {
+		return fmt.Errorf("failed to create runtime scheme: %w", err)
 	}
 
 	hubClient, err := toolsutils.GetClusterClientFromClusterContext(o.hubClusterContext, scheme)

tools/fleet/cmd/draincluster/draincluster.go

@@ -20,11 +20,11 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"time"
 
 	"github.com/spf13/cobra"
 	k8errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -49,6 +49,7 @@ const (
 type drainOptions struct {
 	hubClusterContext string
 	clusterName       string
+	timeout           time.Duration
 
 	hubClient client.Client
 }
@@ -70,18 +71,20 @@ func NewCmdDrainCluster() *cobra.Command {
 	}
 
 	// Add flags specific to drain command
-	cmd.Flags().StringVar(&o.hubClusterContext, "hubClusterContext", "", "kubectl context for the hub cluster (required)")
-	cmd.Flags().StringVar(&o.clusterName, "clusterName", "", "name of the member cluster (required)")
+	cmd.Flags().StringVar(&o.hubClusterContext, "hub-cluster-context", "", "kubectl context for the hub cluster (required)")
+	cmd.Flags().StringVar(&o.clusterName, "cluster-name", "", "name of the member cluster (required)")
+	cmd.Flags().DurationVar(&o.timeout, "timeout", 5*time.Minute, "Maximum time to wait for the operation to complete")
 
 	// Mark required flags
-	_ = cmd.MarkFlagRequired("hubClusterContext")
-	_ = cmd.MarkFlagRequired("clusterName")
+	_ = cmd.MarkFlagRequired("hub-cluster-context")
+	_ = cmd.MarkFlagRequired("cluster-name")
 
 	return cmd
 }
 
 func (o *drainOptions) runDrain() error {
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), o.timeout)
+	defer cancel()
 
 	isDrainSuccessful, err := o.drain(ctx)
 	if err != nil {
@@ -111,13 +114,9 @@ func (o *drainOptions) runDrain() error {
 
 // setupClient creates and configures the Kubernetes client
 func (o *drainOptions) setupClient() error {
-	scheme := runtime.NewScheme()
-
-	if err := clusterv1beta1.AddToScheme(scheme); err != nil {
-		return fmt.Errorf("failed to add custom APIs (cluster) to the runtime scheme: %w", err)
-	}
-	if err := placementv1beta1.AddToScheme(scheme); err != nil {
-		return fmt.Errorf("failed to add custom APIs (placement) to the runtime scheme: %w", err)
+	scheme, err := toolsutils.NewFleetScheme()
+	if err != nil {
+		return fmt.Errorf("failed to create runtime scheme: %w", err)
 	}
 
 	hubClient, err := toolsutils.GetClusterClientFromClusterContext(o.hubClusterContext, scheme)

tools/fleet/cmd/uncordoncluster/uncordoncluster.go

@@ -20,22 +20,22 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"time"
 
 	"github.com/spf13/cobra"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
-	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
 	toolsutils "github.com/kubefleet-dev/kubefleet/tools/utils"
 )
 
 // uncordonOptions wraps common cluster connection parameters
 type uncordonOptions struct {
 	hubClusterContext string
 	clusterName       string
+	timeout           time.Duration
 
 	hubClient client.Client
 }
@@ -57,18 +57,21 @@ func NewCmdUncordonCluster() *cobra.Command {
 	}
 
 	// Add flags specific to uncordon command
-	cmd.Flags().StringVar(&o.hubClusterContext, "hubClusterContext", "", "kubectl context for the hub cluster (required)")
-	cmd.Flags().StringVar(&o.clusterName, "clusterName", "", "name of the member cluster (required)")
+	cmd.Flags().StringVar(&o.hubClusterContext, "hub-cluster-context", "", "kubectl context for the hub cluster (required)")
+	cmd.Flags().StringVar(&o.clusterName, "cluster-name", "", "name of the member cluster (required)")
+	cmd.Flags().DurationVar(&o.timeout, "timeout", 5*time.Minute, "Maximum time to wait for the operation to complete")
 
 	// Mark required flags
-	_ = cmd.MarkFlagRequired("hubClusterContext")
-	_ = cmd.MarkFlagRequired("clusterName")
+	_ = cmd.MarkFlagRequired("hub-cluster-context")
+	_ = cmd.MarkFlagRequired("cluster-name")
 
 	return cmd
 }
 
 func (o *uncordonOptions) runUncordon() error {
-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), o.timeout)
+	defer cancel()
+
 	if err := o.uncordon(ctx); err != nil {
 		return fmt.Errorf("failed to uncordon cluster %s: %w", o.clusterName, err)
 	}
@@ -79,13 +82,9 @@ func (o *uncordonOptions) runUncordon() error {
 
 // setupClient creates and configures the Kubernetes client
 func (o *uncordonOptions) setupClient() error {
-	scheme := runtime.NewScheme()
-
-	if err := clusterv1beta1.AddToScheme(scheme); err != nil {
-		return fmt.Errorf("failed to add custom APIs (cluster) to the runtime scheme: %w", err)
-	}
-	if err := placementv1beta1.AddToScheme(scheme); err != nil {
-		return fmt.Errorf("failed to add custom APIs (placement) to the runtime scheme: %w", err)
+	scheme, err := toolsutils.NewFleetScheme()
+	if err != nil {
+		return fmt.Errorf("failed to create runtime scheme: %w", err)
 	}
 
 	hubClient, err := toolsutils.GetClusterClientFromClusterContext(o.hubClusterContext, scheme)