fix: address a misconfigured default value in hub agent flag setup (#523)

Author: michaelawyu

cmd/hubagent/options/options_test.go

@@ -706,6 +706,123 @@ func TestPlacementManagementOptions(t *testing.T) {
 	}
 }
 
+// TestWebhookOptions tests the parsing and validation logic of the webhook options defined in WebhookOptions.
+func TestWebhookOptions(t *testing.T) {
+	testCases := []struct {
+		name             string
+		flagSetName      string
+		args             []string
+		wantWebhookOpts  WebhookOptions
+		wantErred        bool
+		wantErrMsgSubStr string
+	}{
+		{
+			name:        "all default",
+			flagSetName: "allDefault",
+			args:        []string{},
+			wantWebhookOpts: WebhookOptions{
+				EnableWebhooks:                         true,
+				ClientConnectionType:                   "url",
+				ServiceName:                            "fleetwebhook",
+				EnableGuardRail:                        false,
+				GuardRailWhitelistedUsers:              "",
+				GuardRailDenyModifyMemberClusterLabels: false,
+				EnableWorkload:                         false,
+				UseCertManager:                         false,
+			},
+		},
+		{
+			name:        "all specified",
+			flagSetName: "allSpecified",
+			args: []string{
+				"--enable-webhook=false",
+				"--webhook-client-connection-type=service",
+				"--webhook-service-name=customwebhook",
+				"--enable-guard-rail=true",
+				"--whitelisted-users=user1,user2",
+				"--deny-modify-member-cluster-labels=true",
+				"--enable-workload=true",
+				"--use-cert-manager=true",
+			},
+			wantWebhookOpts: WebhookOptions{
+				EnableWebhooks:                         false,
+				ClientConnectionType:                   "service",
+				ServiceName:                            "customwebhook",
+				EnableGuardRail:                        true,
+				GuardRailWhitelistedUsers:              "user1,user2",
+				GuardRailDenyModifyMemberClusterLabels: true,
+				EnableWorkload:                         true,
+				UseCertManager:                         true,
+			},
+		},
+		{
+			name:        "webhook client connection type URL (case-insensitive)",
+			flagSetName: "webhookClientConnTypeURL",
+			args:        []string{"--webhook-client-connection-type=URL"},
+			wantWebhookOpts: WebhookOptions{
+				EnableWebhooks:                         true,
+				ClientConnectionType:                   "url",
+				ServiceName:                            "fleetwebhook",
+				EnableGuardRail:                        false,
+				GuardRailWhitelistedUsers:              "",
+				GuardRailDenyModifyMemberClusterLabels: false,
+				EnableWorkload:                         false,
+				UseCertManager:                         false,
+			},
+		},
+		{
+			name:        "webhook client connection type service (case-insensitive)",
+			flagSetName: "webhookClientConnTypeService",
+			args:        []string{"--webhook-client-connection-type=Service"},
+			wantWebhookOpts: WebhookOptions{
+				EnableWebhooks:                         true,
+				ClientConnectionType:                   "service",
+				ServiceName:                            "fleetwebhook",
+				EnableGuardRail:                        false,
+				GuardRailWhitelistedUsers:              "",
+				GuardRailDenyModifyMemberClusterLabels: false,
+				EnableWorkload:                         false,
+				UseCertManager:                         false,
+			},
+		},
+		{
+			name:             "invalid webhook client connection type",
+			flagSetName:      "webhookClientConnTypeInvalid",
+			args:             []string{"--webhook-client-connection-type=ftp"},
+			wantErred:        true,
+			wantErrMsgSubStr: "invalid webhook client connection type",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			flags := flag.NewFlagSet(tc.flagSetName, flag.ContinueOnError)
+			webhookOpts := WebhookOptions{}
+			webhookOpts.AddFlags(flags)
+
+			err := flags.Parse(tc.args)
+			if tc.wantErred {
+				if err == nil {
+					t.Fatalf("flag Parse() = nil, want erred")
+				}
+
+				if !strings.Contains(err.Error(), tc.wantErrMsgSubStr) {
+					t.Fatalf("flag Parse() error = %v, want error msg with sub-string %s", err, tc.wantErrMsgSubStr)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("flag Parse() = %v, want nil", err)
+			}
+
+			if diff := cmp.Diff(webhookOpts, tc.wantWebhookOpts); diff != "" {
+				t.Errorf("webhook options diff (-got, +want):\n%s", diff)
+			}
+		})
+	}
+}
+
 // TestRateLimitOptions tests the parsing and validation logic of the rate limit options defined in RateLimitOptions.
 func TestRateLimitOptions(t *testing.T) {
 	testCases := []struct {

cmd/hubagent/options/webhooks.go

@@ -71,22 +71,10 @@ func (o *WebhookOptions) AddFlags(flags *flag.FlagSet) {
 		"Enable the KubeFleet webhooks or not.",
 	)
 
-	flags.Func(
+	flags.Var(
+		newWebhookClientConnTypeValueWithValidation(string(URL), &o.ClientConnectionType),
 		"webhook-client-connection-type",
 		"The connection type used by the webhook client. Valid values are `url` and `service`. Defaults to `url`. This option only applies if webhooks are enabled.",
-		func(s string) error {
-			if len(s) == 0 {
-				o.ClientConnectionType = "url"
-				return nil
-			}
-
-			parsedStr, err := parseWebhookClientConnectionString(s)
-			if err != nil {
-				return fmt.Errorf("invalid webhook client connection type: %w", err)
-			}
-			o.ClientConnectionType = string(parsedStr)
-			return nil
-		},
 	)
 
 	flags.StringVar(
@@ -131,3 +119,28 @@ func (o *WebhookOptions) AddFlags(flags *flag.FlagSet) {
 		"Use the cert-manager project for managing KubeFleet webhook server certificates or not. If set to false, the system will use self-signed certificates. If set to true, the EnableWorkload option must be set to true as well. This option only applies if webhooks are enabled.",
 	)
 }
+
+type WebhookClientConnTypeValueWithValidation string
+
+func (v *WebhookClientConnTypeValueWithValidation) String() string {
+	return string(*v)
+}
+
+func (v *WebhookClientConnTypeValueWithValidation) Set(s string) error {
+	if len(s) == 0 {
+		*v = "url"
+		return nil
+	}
+
+	parsedStr, err := parseWebhookClientConnectionString(s)
+	if err != nil {
+		return fmt.Errorf("invalid webhook client connection type: %w", err)
+	}
+	*v = WebhookClientConnTypeValueWithValidation(parsedStr)
+	return nil
+}
+
+func newWebhookClientConnTypeValueWithValidation(defaultVal string, p *string) *WebhookClientConnTypeValueWithValidation {
+	*p = defaultVal
+	return (*WebhookClientConnTypeValueWithValidation)(p)
+}