[Federation] Review comment fixes for add override flags options to kubefed init

federation/pkg/kubefed/init/BUILD

@@ -54,6 +54,7 @@ go_test(
         "//vendor:k8s.io/apimachinery/pkg/api/resource",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apimachinery/pkg/util/diff",
+        "//vendor:k8s.io/apimachinery/pkg/util/sets",
         "//vendor:k8s.io/client-go/dynamic",
         "//vendor:k8s.io/client-go/rest/fake",
         "//vendor:k8s.io/client-go/tools/clientcmd",

federation/pkg/kubefed/init/init.go

@@ -117,20 +117,6 @@ var (
 	}
 
 	hyperkubeImageName = "gcr.io/google_containers/hyperkube-amd64"
-
-	apiserverDefaultArgs = map[string]string{
-		"--bind-address":         "0.0.0.0",
-		"--etcd-servers":         "http://localhost:2379",
-		"--secure-port":          "443",
-		"--client-ca-file":       "/etc/federation/apiserver/ca.crt",
-		"--tls-cert-file":        "/etc/federation/apiserver/server.crt",
-		"--tls-private-key-file": "/etc/federation/apiserver/server.key",
-	}
-
-	cmDefaultArgs = map[string]string{
-		"--kubeconfig":          "/etc/federation/controller-manager/kubeconfig",
-		"--dns-provider-config": "",
-	}
 )
 
 // NewCmdInit defines the `init` command that bootstraps a federation
@@ -156,8 +142,8 @@ func NewCmdInit(cmdOut io.Writer, config util.AdminConfig) *cobra.Command {
 	cmd.Flags().String("etcd-pv-capacity", "10Gi", "Size of persistent volume claim to be used for etcd.")
 	cmd.Flags().Bool("etcd-persistent-storage", true, "Use persistent volume for etcd. Defaults to 'true'.")
 	cmd.Flags().Bool("dry-run", false, "dry run without sending commands to server.")
-	cmd.Flags().String("apiserver-arg-overrides", "", "comma sapareted list of all or subset of the federation-apiserver arguments as \"--arg1=value1,--arg2=value2...\"")
+	cmd.Flags().String("apiserver-arg-overrides", "", "comma separated list of federation-apiserver arguments to override: Example \"--arg1=value1,--arg2=value2...\"")
-	cmd.Flags().String("controllermgr-arg-overrides", "", "comma sapareted list of all or subset of the federation-controller-manager arguments as \"--arg1=value1,--arg2=value2...\"")
+	cmd.Flags().String("controllermanager-arg-overrides", "", "comma separated list of federation-controller-manager arguments to override: Example \"--arg1=value1,--arg2=value2...\"")
 	cmd.Flags().String("storage-backend", "etcd2", "The storage backend for persistence. Options: 'etcd2' (default), 'etcd3'.")
 	cmd.Flags().String(apiserverServiceTypeFlag, string(v1.ServiceTypeLoadBalancer), "The type of service to create for federation API server. Options: 'LoadBalancer' (default), 'NodePort'.")
 	cmd.Flags().String(apiserverAdvertiseAddressFlag, "", "Preferred address to advertise api server nodeport service. Valid only if '"+apiserverServiceTypeFlag+"=NodePort'.")
@@ -185,8 +171,6 @@ func initFederation(cmdOut io.Writer, config util.AdminConfig, cmd *cobra.Comman
 	etcdPVCapacity := cmdutil.GetFlagString(cmd, "etcd-pv-capacity")
 	etcdPersistence := cmdutil.GetFlagBool(cmd, "etcd-persistent-storage")
 	dryRun := cmdutil.GetDryRunFlag(cmd)
-	apiserverArgOverrides := cmdutil.GetFlagString(cmd, "apiserver-arg-overrides")
-	cmArgOverrides := cmdutil.GetFlagString(cmd, "controllermgr-arg-overrides")
 	storageBackend := cmdutil.GetFlagString(cmd, "storage-backend")
 	apiserverServiceType := v1.ServiceType(cmdutil.GetFlagString(cmd, apiserverServiceTypeFlag))
 	apiserverAdvertiseAddress := cmdutil.GetFlagString(cmd, apiserverAdvertiseAddressFlag)
@@ -203,6 +187,14 @@ func initFederation(cmdOut io.Writer, config util.AdminConfig, cmd *cobra.Comman
 			return fmt.Errorf("%s should be passed only with '%s=NodePort'", apiserverAdvertiseAddressFlag, apiserverServiceTypeFlag)
 		}
 	}
+	apiserverArgOverrides, err := marshallOverrides(cmdutil.GetFlagString(cmd, "apiserver-arg-overrides"))
+	if err != nil {
+		return fmt.Errorf("Error marshalling --apiserver-arg-overrides: %v", err)
+	}
+	cmArgOverrides, err := marshallOverrides(cmdutil.GetFlagString(cmd, "controllermanager-arg-overrides"))
+	if err != nil {
+		return fmt.Errorf("Error marshalling --controllermanager-arg-overrides: %v", err)
+	}
 
 	hostFactory := config.HostFactory(initFlags.Host, initFlags.Kubeconfig)
 	hostClientset, err := hostFactory.ClientSet()
@@ -537,34 +529,28 @@ func createPVC(clientset *client.Clientset, namespace, svcName, etcdPVCapacity s
 	return clientset.Core().PersistentVolumeClaims(namespace).Create(pvc)
 }
 
-func createAPIServer(clientset *client.Clientset, namespace, name, image, credentialsName, advertiseAddress, storageBackend, apiserverArgOverrides string, pvc *api.PersistentVolumeClaim, dryRun bool) (*extensions.Deployment, error) {
+func createAPIServer(clientset *client.Clientset, namespace, name, image, credentialsName, advertiseAddress, storageBackend string, argOverrides map[string]string, pvc *api.PersistentVolumeClaim, dryRun bool) (*extensions.Deployment, error) {
 	command := []string{
 		"/hyperkube",
 		"federation-apiserver",
 	}
-	apiserverArgs := []string{}
+	argsMap := map[string]string{
-	apiserverArgsMap := apiserverDefaultArgs
+		"--bind-address":         "0.0.0.0",
+		"--etcd-servers":         "http://localhost:2379",
+		"--secure-port":          "443",
+		"--client-ca-file":       "/etc/federation/apiserver/ca.crt",
+		"--tls-cert-file":        "/etc/federation/apiserver/server.crt",
+		"--tls-private-key-file": "/etc/federation/apiserver/server.key",
+		"--admission-control":    "NamespaceLifecycle",
+	}
 
-	apiserverArgsMap["--storage-backend"] = storageBackend
+	argsMap["--storage-backend"] = storageBackend
 	if advertiseAddress != "" {
-		// This will be overridden with the override arg if user specifies one
+		argsMap["--advertise-address"] = advertiseAddress
-		apiserverArgsMap["--advertise-address"] = advertiseAddress
 	}
 
-	if apiserverArgOverrides != "" {
+	args := argMapsToArgStrings(argsMap, argOverrides)
-		var err error
+	command = append(command, args...)
-		apiserverArgsMap, err = overrideArgs(apiserverArgsMap, apiserverArgOverrides)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	for key, value := range apiserverArgsMap {
-		apiserverArgs = append(apiserverArgs, fmt.Sprintf("%s=%s", key, value))
-	}
-	// This is needed for the unit test deep copy to get an exact match
-	sort.Strings(apiserverArgs)
-	command = append(command, apiserverArgs...)
 
 	dep := &extensions.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
@@ -706,33 +692,23 @@ func createRoleBindings(clientset *client.Clientset, namespace, saName string, d
 	return newRole, newRolebinding, err
 }
 
-func createControllerManager(clientset *client.Clientset, namespace, name, svcName, cmName, image, kubeconfigName, dnsZoneName, dnsProvider, saName, cmArgOverrides string, dryRun bool) (*extensions.Deployment, error) {
+func createControllerManager(clientset *client.Clientset, namespace, name, svcName, cmName, image, kubeconfigName, dnsZoneName, dnsProvider, saName string, argOverrides map[string]string, dryRun bool) (*extensions.Deployment, error) {
 	command := []string{
 		"/hyperkube",
 		"federation-controller-manager",
 	}
-	cmArgs := []string{}
+	argsMap := map[string]string{
-	cmArgsMap := cmDefaultArgs
+		"--kubeconfig":          "/etc/federation/controller-manager/kubeconfig",
-
+		"--dns-provider-config": "",
-	cmArgsMap["--master"] = fmt.Sprintf("https://%s", svcName)
-	cmArgsMap["--dns-provider"] = dnsProvider
-	cmArgsMap["--federation-name"] = name
-	cmArgsMap["--zone-name"] = dnsZoneName
-
-	if cmArgOverrides != "" {
-		var err error
-		cmArgsMap, err = overrideArgs(cmArgsMap, cmArgOverrides)
-		if err != nil {
-			return nil, err
-		}
 	}
 
-	for key, value := range cmArgsMap {
+	argsMap["--master"] = fmt.Sprintf("https://%s", svcName)
-		cmArgs = append(cmArgs, fmt.Sprintf("%s=%s", key, value))
+	argsMap["--dns-provider"] = dnsProvider
-	}
+	argsMap["--federation-name"] = name
-	// This is needed for the unit test deep copy to get an exact match
+	argsMap["--zone-name"] = dnsZoneName
-	sort.Strings(cmArgs)
+
-	command = append(command, cmArgs...)
+	args := argMapsToArgStrings(argsMap, argOverrides)
+	command = append(command, args...)
 
 	dep := &extensions.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
@@ -794,34 +770,41 @@ func createControllerManager(clientset *client.Clientset, namespace, name, svcNa
 	return clientset.Extensions().Deployments(namespace).Create(dep)
 }
 
-func overrideArgs(serverArgs map[string]string, argOverrides string) (map[string]string, error) {
+func marshallOverrides(overrideArgString string) (map[string]string, error) {
-	overrideArgs := strings.Split(argOverrides, ",")
+	if overrideArgString == "" {
-	argsMap := serverArgs
+		return nil, nil
+	}
 
+	argsMap := make(map[string]string)
+	overrideArgs := strings.Split(overrideArgString, ",")
 	for _, overrideArg := range overrideArgs {
-		overrideArgSplitted := strings.Split(overrideArg, "=")
+		splitArg := strings.Split(overrideArg, "=")
-		if len(overrideArgSplitted) != 2 {
+		if len(splitArg) != 2 {
-			return nil, fmt.Errorf("Wrong format for override arg: %s", overrideArg)
+			return nil, fmt.Errorf("wrong format for override arg: %s", overrideArg)
 		}
-		found := false
+		key := strings.TrimSpace(splitArg[0])
-		for key := range argsMap {
+		val := strings.TrimSpace(splitArg[1])
-			if key == overrideArgSplitted[0] {
+		if len(key) == 0 {
-				// Replace the default value by user specified value
+			return nil, fmt.Errorf("wrong format for override arg: %s, arg name cannot be empty", overrideArg)
-				argsMap[key] = overrideArgSplitted[1]
-				found = true
-				break
-			}
-		}
-		// This arg is not in defaults, append it.
-		// We do not care if the argument name or value used is not correct
-		// To verify the wrong args is the particular server apps job.
-		if found == false {
-			argsMap[overrideArgSplitted[0]] = overrideArgSplitted[1]
 		}
+		argsMap[key] = val
 	}
 	return argsMap, nil
 }
 
+func argMapsToArgStrings(argsMap, overrides map[string]string) []string {
+	for key, val := range overrides {
+		argsMap[key] = val
+	}
+	args := []string{}
+	for key, value := range argsMap {
+		args = append(args, fmt.Sprintf("%s=%s", key, value))
+	}
+	// This is needed for the unit test deep copy to get an exact match
+	sort.Strings(args)
+	return args
+}
+
 func waitForPods(clientset *client.Clientset, fedPods []string, namespace string) error {
 	err := wait.PollInfinite(podWaitInterval, func() (bool, error) {
 		podCheck := len(fedPods)

federation/pkg/kubefed/init/init_test.go

@@ -26,8 +26,8 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"strconv"
 	"sort"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -37,6 +37,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/rest/fake"
 	"k8s.io/client-go/tools/clientcmd"
@@ -77,39 +78,39 @@ func TestInitFederation(t *testing.T) {
 	defer kubefedtesting.RemoveFakeKubeconfigFiles(fakeKubeFiles)
 
 	testCases := []struct {
-		federation           string
+		federation            string
-		kubeconfigGlobal     string
+		kubeconfigGlobal      string
-		kubeconfigExplicit   string
+		kubeconfigExplicit    string
-		dnsZoneName          string
+		dnsZoneName           string
-		lbIP                 string
+		lbIP                  string
-		apiserverServiceType v1.ServiceType
+		apiserverServiceType  v1.ServiceType
-		advertiseAddress     string
+		advertiseAddress      string
-		image                string
+		image                 string
-		etcdPVCapacity       string
+		etcdPVCapacity        string
-		etcdPersistence      string
+		etcdPersistence       string
-		expectedErr          string
+		expectedErr           string
-		dnsProvider          string
+		dnsProvider           string
-		storageBackend       string
+		storageBackend        string
-		dryRun               string
+		dryRun                string
 		apiserverArgOverrides string
 		cmArgOverrides        string
 	}{
 		{
-			federation:           "union",
+			federation:            "union",
-			kubeconfigGlobal:     fakeKubeFiles[0],
+			kubeconfigGlobal:      fakeKubeFiles[0],
-			kubeconfigExplicit:   "",
+			kubeconfigExplicit:    "",
-			dnsZoneName:          "example.test.",
+			dnsZoneName:           "example.test.",
-			lbIP:                 lbIP,
+			lbIP:                  lbIP,
-			apiserverServiceType: v1.ServiceTypeLoadBalancer,
+			apiserverServiceType:  v1.ServiceTypeLoadBalancer,
-			image:                "example.test/foo:bar",
+			image:                 "example.test/foo:bar",
-			etcdPVCapacity:       "5Gi",
+			etcdPVCapacity:        "5Gi",
-			etcdPersistence:      "true",
+			etcdPersistence:       "true",
-			expectedErr:          "",
+			expectedErr:           "",
-			dnsProvider:          "test-dns-provider",
+			dnsProvider:           "test-dns-provider",
-			storageBackend:       "etcd2",
+			storageBackend:        "etcd2",
-			dryRun:               "",
+			dryRun:                "",
 			apiserverArgOverrides: "--client-ca-file=override,--log-dir=override",
-			cmArgOverrides:        "",
+			cmArgOverrides:        "--dns-provider=override,--log-dir=override",
 		},
 		{
 			federation:           "union",
@@ -125,8 +126,6 @@ func TestInitFederation(t *testing.T) {
 			dnsProvider:          "", //test for default value of dns provider
 			storageBackend:       "etcd2",
 			dryRun:               "",
-			apiserverArgOverrides: "",
-			cmArgOverrides:        "--dns-provider=override,--log-dir=override",
 		},
 		{
 			federation:           "union",
@@ -142,8 +141,6 @@ func TestInitFederation(t *testing.T) {
 			dnsProvider:          "test-dns-provider",
 			storageBackend:       "etcd2",
 			dryRun:               "valid-run",
-			apiserverArgOverrides: "--log-dir=override",
-			cmArgOverrides:        "--log-dir=override",
 		},
 		{
 			federation:           "union",
@@ -159,8 +156,6 @@ func TestInitFederation(t *testing.T) {
 			dnsProvider:          "test-dns-provider",
 			storageBackend:       "etcd3",
 			dryRun:               "",
-			apiserverArgOverrides: "",
-			cmArgOverrides:        "",
 		},
 		{
 			federation:           "union",
@@ -175,8 +170,6 @@ func TestInitFederation(t *testing.T) {
 			dnsProvider:          "test-dns-provider",
 			storageBackend:       "etcd3",
 			dryRun:               "",
-			apiserverArgOverrides: "",
-			cmArgOverrides:        "",			
 		},
 		{
 			federation:           "union",
@@ -192,8 +185,6 @@ func TestInitFederation(t *testing.T) {
 			dnsProvider:          "test-dns-provider",
 			storageBackend:       "etcd3",
 			dryRun:               "",
-			apiserverArgOverrides: "",
-			cmArgOverrides:        "",		
 		},
 	}
 
@@ -226,7 +217,7 @@ func TestInitFederation(t *testing.T) {
 		cmd.Flags().Set("dns-zone-name", tc.dnsZoneName)
 		cmd.Flags().Set("image", tc.image)
 		cmd.Flags().Set("apiserver-arg-overrides", tc.apiserverArgOverrides)
-		cmd.Flags().Set("controllermgr-arg-overrides", tc.cmArgOverrides)
+		cmd.Flags().Set("controllermanager-arg-overrides", tc.cmArgOverrides)
 
 		if tc.storageBackend != "" {
 			cmd.Flags().Set("storage-backend", tc.storageBackend)
@@ -277,6 +268,64 @@ func TestInitFederation(t *testing.T) {
 	}
 }
 
+func TestMarshallAndMergeOverrides(t *testing.T) {
+	testCases := []struct {
+		overrideParams string
+		expectedSet    sets.String
+		expectedErr    string
+	}{
+		{
+			overrideParams: "valid-format-param1=override1,valid-format-param2=override2",
+			expectedSet:    sets.NewString("arg2=val2", "arg1=val1", "valid-format-param1=override1", "valid-format-param2=override2"),
+			expectedErr:    "",
+		},
+		{
+			overrideParams: "valid-format-param1=override1,arg1=override1",
+			expectedSet:    sets.NewString("arg2=val2", "arg1=override1", "valid-format-param1=override1"),
+			expectedErr:    "",
+		},
+		{
+			overrideParams: "zero-value-arg=",
+			expectedSet:    sets.NewString("arg2=val2", "arg1=val1", "zero-value-arg="),
+			expectedErr:    "",
+		},
+		{
+			overrideParams: "wrong-format-arg",
+			expectedErr:    "wrong format for override arg: wrong-format-arg",
+		},
+		{
+			overrideParams: "wrong-format-arg=override=wrong-format-arg=override",
+			expectedErr:    "wrong format for override arg: wrong-format-arg=override=wrong-format-arg=override",
+		},
+		{
+			overrideParams: "=wrong-format-only-value",
+			expectedErr:    "wrong format for override arg: =wrong-format-only-value, arg name cannot be empty",
+		},
+	}
+
+	for i, tc := range testCases {
+		args, err := marshallOverrides(tc.overrideParams)
+		if tc.expectedErr == "" {
+			origArgs := map[string]string{
+				"arg1": "val1",
+				"arg2": "val2",
+			}
+			merged := argMapsToArgStrings(origArgs, args)
+
+			got := sets.NewString(merged...)
+			want := tc.expectedSet
+
+			if !got.Equal(want) {
+				t.Errorf("[%d] unexpected output: got: %v, want: %v", i, got, want)
+			}
+		} else {
+			if err.Error() != tc.expectedErr {
+				t.Errorf("[%d] unexpected error output: got: %s, want: %s", i, err.Error(), tc.expectedErr)
+			}
+		}
+	}
+}
+
 // TestCertsTLS tests TLS handshake with client authentication for any server
 // name. There is a separate test below to test the certificate generation
 // end-to-end over HTTPS.
@@ -710,27 +759,28 @@ func fakeInitHostFactory(apiserverServiceType v1.ServiceType, federationName, na
 			address = nodeIP
 		}
 	}
-	//apiserver.Spec.Template.Spec.Containers[0].Command = append(apiserver.Spec.Template.Spec.Containers[0].Command, fmt.Sprintf("--advertise-address=%s", address))
-
 
 	apiserverCommand := []string{
 		"/hyperkube",
 		"federation-apiserver",
 	}
-	apiserverArgs := []string{}
+	apiserverArgs := []string{
-	apiserverArgsMap := apiserverDefaultArgs
+		"--bind-address=0.0.0.0",
-	apiserverArgsMap["--storage-backend"] = storageProvider
+		"--etcd-servers=http://localhost:2379",
-	//apiserverArgsMap["--advertise-address"] = ip
+		"--secure-port=443",
-	apiserverArgsMap["--advertise-address"] = address
+		"--tls-cert-file=/etc/federation/apiserver/server.crt",
-	if strings.Contains(apiserverOverrideArg, "--client-ca-file=override") {
+		"--tls-private-key-file=/etc/federation/apiserver/server.key",
-		apiserverArgsMap["--client-ca-file"] = "override"
+		"--admission-control=NamespaceLifecycle",
-	}
+		fmt.Sprintf("--storage-backend=%s", storageProvider),
-	if strings.Contains(apiserverOverrideArg, "--log-dir=override") {
+		fmt.Sprintf("--advertise-address=%s", address),
-		apiserverArgsMap["--log-dir"] = "override"
 	}
 
-	for key, value := range apiserverArgsMap {
+	if apiserverOverrideArg != "" {
-		apiserverArgs = append(apiserverArgs, fmt.Sprintf("%s=%s", key, value))
+		apiserverArgs = append(apiserverArgs, "--client-ca-file=override")
+		apiserverArgs = append(apiserverArgs, "--log-dir=override")
+
+	} else {
+		apiserverArgs = append(apiserverArgs, "--client-ca-file=/etc/federation/apiserver/ca.crt")
 	}
 	sort.Strings(apiserverArgs)
 	apiserverCommand = append(apiserverCommand, apiserverArgs...)
@@ -828,23 +878,22 @@ func fakeInitHostFactory(apiserverServiceType v1.ServiceType, federationName, na
 		"/hyperkube",
 		"federation-controller-manager",
 	}
-	cmArgs := []string{}
+
-	cmArgsMap := cmDefaultArgs
+	cmArgs := []string{
-	cmArgsMap["--federation-name"] = federationName
+		"--kubeconfig=/etc/federation/controller-manager/kubeconfig",
-	cmArgsMap["--zone-name"] = dnsZoneName
+		"--dns-provider-config=",
-	cmArgsMap["--master"] = "https://" + svcName
+		fmt.Sprintf("--federation-name=%s", federationName),
-	if strings.Contains(cmOverrideArg, "--dns-provider=override") {
+		fmt.Sprintf("--zone-name=%s", dnsZoneName),
-		cmArgsMap["--dns-provider"] = "override"
+		fmt.Sprintf("--master=https://%s", svcName),
-	} else {
-		cmArgsMap["--dns-provider"] = dnsProvider
-	}
-	if strings.Contains(cmOverrideArg, "--log-dir=override") {
-		cmArgsMap["--log-dir"] = "override"
 	}
 
-	for key, value := range cmArgsMap {
+	if cmOverrideArg != "" {
-		cmArgs = append(cmArgs, fmt.Sprintf("%s=%s", key, value))
+		cmArgs = append(cmArgs, "--dns-provider=override")
+		cmArgs = append(cmArgs, "--log-dir=override")
+	} else {
+		cmArgs = append(cmArgs, fmt.Sprintf("--dns-provider=%s", dnsProvider))
 	}
+
 	sort.Strings(cmArgs)
 	cmCommand = append(cmCommand, cmArgs...)
 

hack/verify-flags/known-flags.txt

@@ -117,7 +117,7 @@ container-runtime
 container-runtime-endpoint
 contain-pod-resources
 contention-profiling
-controllermgr-arg-overrides
+controllermanager-arg-overrides
 controller-start-interval
 cors-allowed-origins
 cpu-cfs-quota