diff --git a/federation/pkg/kubefed/init/BUILD b/federation/pkg/kubefed/init/BUILD index d216563f3d1..7c844d37147 100644 --- a/federation/pkg/kubefed/init/BUILD +++ b/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", diff --git a/federation/pkg/kubefed/init/init.go b/federation/pkg/kubefed/init/init.go index 93786a436b8..6307c752b86 100644 --- a/federation/pkg/kubefed/init/init.go +++ b/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("controllermgr-arg-overrides", "", "comma sapareted list of all or subset of the federation-controller-manager 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("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{} - apiserverArgsMap := apiserverDefaultArgs + argsMap := 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", + "--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 - apiserverArgsMap["--advertise-address"] = advertiseAddress + argsMap["--advertise-address"] = advertiseAddress } - if apiserverArgOverrides != "" { - var err error - 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...) + args := argMapsToArgStrings(argsMap, argOverrides) + command = append(command, args...) 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{} - cmArgsMap := cmDefaultArgs - - 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 - } + argsMap := map[string]string{ + "--kubeconfig": "/etc/federation/controller-manager/kubeconfig", + "--dns-provider-config": "", } - for key, value := range cmArgsMap { - cmArgs = append(cmArgs, fmt.Sprintf("%s=%s", key, value)) - } - // This is needed for the unit test deep copy to get an exact match - sort.Strings(cmArgs) - command = append(command, cmArgs...) + argsMap["--master"] = fmt.Sprintf("https://%s", svcName) + argsMap["--dns-provider"] = dnsProvider + argsMap["--federation-name"] = name + argsMap["--zone-name"] = dnsZoneName + + 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) { - overrideArgs := strings.Split(argOverrides, ",") - argsMap := serverArgs +func marshallOverrides(overrideArgString string) (map[string]string, error) { + if overrideArgString == "" { + return nil, nil + } + argsMap := make(map[string]string) + overrideArgs := strings.Split(overrideArgString, ",") for _, overrideArg := range overrideArgs { - overrideArgSplitted := strings.Split(overrideArg, "=") - if len(overrideArgSplitted) != 2 { - return nil, fmt.Errorf("Wrong format for override arg: %s", overrideArg) + splitArg := strings.Split(overrideArg, "=") + if len(splitArg) != 2 { + return nil, fmt.Errorf("wrong format for override arg: %s", overrideArg) } - found := false - for key := range argsMap { - if key == overrideArgSplitted[0] { - // Replace the default value by user specified value - 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] + key := strings.TrimSpace(splitArg[0]) + val := strings.TrimSpace(splitArg[1]) + if len(key) == 0 { + return nil, fmt.Errorf("wrong format for override arg: %s, arg name cannot be empty", overrideArg) } + 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) diff --git a/federation/pkg/kubefed/init/init_test.go b/federation/pkg/kubefed/init/init_test.go index 7bbafda291b..85e5eec273f 100644 --- a/federation/pkg/kubefed/init/init_test.go +++ b/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 - kubeconfigGlobal string - kubeconfigExplicit string - dnsZoneName string - lbIP string - apiserverServiceType v1.ServiceType - advertiseAddress string - image string - etcdPVCapacity string - etcdPersistence string - expectedErr string - dnsProvider string - storageBackend string - dryRun string + federation string + kubeconfigGlobal string + kubeconfigExplicit string + dnsZoneName string + lbIP string + apiserverServiceType v1.ServiceType + advertiseAddress string + image string + etcdPVCapacity string + etcdPersistence string + expectedErr string + dnsProvider string + storageBackend string + dryRun string apiserverArgOverrides string cmArgOverrides string }{ { - federation: "union", - kubeconfigGlobal: fakeKubeFiles[0], - kubeconfigExplicit: "", - dnsZoneName: "example.test.", - lbIP: lbIP, - apiserverServiceType: v1.ServiceTypeLoadBalancer, - image: "example.test/foo:bar", - etcdPVCapacity: "5Gi", - etcdPersistence: "true", - expectedErr: "", - dnsProvider: "test-dns-provider", - storageBackend: "etcd2", - dryRun: "", + federation: "union", + kubeconfigGlobal: fakeKubeFiles[0], + kubeconfigExplicit: "", + dnsZoneName: "example.test.", + lbIP: lbIP, + apiserverServiceType: v1.ServiceTypeLoadBalancer, + image: "example.test/foo:bar", + etcdPVCapacity: "5Gi", + etcdPersistence: "true", + expectedErr: "", + dnsProvider: "test-dns-provider", + storageBackend: "etcd2", + 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{} - apiserverArgsMap := apiserverDefaultArgs - apiserverArgsMap["--storage-backend"] = storageProvider - //apiserverArgsMap["--advertise-address"] = ip - apiserverArgsMap["--advertise-address"] = address - if strings.Contains(apiserverOverrideArg, "--client-ca-file=override") { - apiserverArgsMap["--client-ca-file"] = "override" - } - if strings.Contains(apiserverOverrideArg, "--log-dir=override") { - apiserverArgsMap["--log-dir"] = "override" + apiserverArgs := []string{ + "--bind-address=0.0.0.0", + "--etcd-servers=http://localhost:2379", + "--secure-port=443", + "--tls-cert-file=/etc/federation/apiserver/server.crt", + "--tls-private-key-file=/etc/federation/apiserver/server.key", + "--admission-control=NamespaceLifecycle", + fmt.Sprintf("--storage-backend=%s", storageProvider), + fmt.Sprintf("--advertise-address=%s", address), } - for key, value := range apiserverArgsMap { - apiserverArgs = append(apiserverArgs, fmt.Sprintf("%s=%s", key, value)) + if apiserverOverrideArg != "" { + 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 - cmArgsMap["--federation-name"] = federationName - cmArgsMap["--zone-name"] = dnsZoneName - cmArgsMap["--master"] = "https://" + svcName - if strings.Contains(cmOverrideArg, "--dns-provider=override") { - cmArgsMap["--dns-provider"] = "override" - } else { - cmArgsMap["--dns-provider"] = dnsProvider - } - if strings.Contains(cmOverrideArg, "--log-dir=override") { - cmArgsMap["--log-dir"] = "override" + + cmArgs := []string{ + "--kubeconfig=/etc/federation/controller-manager/kubeconfig", + "--dns-provider-config=", + fmt.Sprintf("--federation-name=%s", federationName), + fmt.Sprintf("--zone-name=%s", dnsZoneName), + fmt.Sprintf("--master=https://%s", svcName), } - for key, value := range cmArgsMap { - cmArgs = append(cmArgs, fmt.Sprintf("%s=%s", key, value)) + if cmOverrideArg != "" { + 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...) diff --git a/hack/verify-flags/known-flags.txt b/hack/verify-flags/known-flags.txt index 3af526ff358..0c55cfa71b1 100644 --- a/hack/verify-flags/known-flags.txt +++ b/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