From 71934b76430d6de1fb69a0169eeed6750fb1d256 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Thu, 19 Nov 2015 19:14:10 +0100 Subject: [PATCH] kubectl: Validate flag usage against generators This commit moves the generators into separate buckets and creates separate calls for each one group. This helps in providing just the necessary generators to each generator command. Validation of flags against generators is also added so that flags that are not meant to be used with a specific generator will result in error. --- pkg/kubectl/cmd/autoscale.go | 7 +- pkg/kubectl/cmd/cmd_test.go | 118 +------------------------------- pkg/kubectl/cmd/expose.go | 7 +- pkg/kubectl/cmd/expose_test.go | 30 -------- pkg/kubectl/cmd/run.go | 9 ++- pkg/kubectl/cmd/util/factory.go | 57 ++++++++------- pkg/kubectl/generate.go | 53 ++++++++++++++ 7 files changed, 106 insertions(+), 175 deletions(-) diff --git a/pkg/kubectl/cmd/autoscale.go b/pkg/kubectl/cmd/autoscale.go index d311dc8e350..b8a14827faa 100644 --- a/pkg/kubectl/cmd/autoscale.go +++ b/pkg/kubectl/cmd/autoscale.go @@ -101,7 +101,8 @@ func RunAutoscale(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args [] // Get the generator, setup and validate all required parameters generatorName := cmdutil.GetFlagString(cmd, "generator") - generator, found := f.Generator(generatorName) + generators := f.Generators("autoscale") + generator, found := generators[generatorName] if !found { return cmdutil.UsageError(cmd, fmt.Sprintf("generator %q not found.", generatorName)) } @@ -117,6 +118,10 @@ func RunAutoscale(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args [] if err = kubectl.ValidateParams(names, params); err != nil { return err } + // Check for invalid flags used against the present generator. + if err := kubectl.EnsureFlagsValid(cmd, generators, generatorName); err != nil { + return err + } // Generate new object object, err := generator.Generate(params) diff --git a/pkg/kubectl/cmd/cmd_test.go b/pkg/kubectl/cmd/cmd_test.go index 7a105b5c761..3b5b50dac81 100644 --- a/pkg/kubectl/cmd/cmd_test.go +++ b/pkg/kubectl/cmd/cmd_test.go @@ -24,7 +24,6 @@ import ( "io/ioutil" "os" "reflect" - "strconv" "testing" "time" @@ -40,7 +39,6 @@ import ( "k8s.io/kubernetes/pkg/kubectl/resource" "k8s.io/kubernetes/pkg/runtime" "k8s.io/kubernetes/pkg/util" - "k8s.io/kubernetes/pkg/util/intstr" ) type internalType struct { @@ -218,8 +216,7 @@ func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) { t := &testFactory{ Validator: validation.NullSchema{}, } - generators := cmdutil.DefaultGenerators() - generators["service/test"] = testServiceGenerator{} + f := &cmdutil.Factory{ Object: func() (meta.RESTMapper, runtime.ObjectTyper) { return testapi.Default.RESTMapper(), api.Scheme @@ -249,9 +246,8 @@ func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) { ClientConfig: func() (*client.Config, error) { return t.ClientConfig, t.Err }, - Generator: func(name string) (kubectl.Generator, bool) { - generator, ok := generators[name] - return generator, ok + Generators: func(cmdName string) map[string]kubectl.Generator { + return cmdutil.DefaultGenerators(cmdName) }, LogsForObject: func(object, options runtime.Object) (*client.Request, error) { fakeClient := t.Client.(*fake.RESTClient) @@ -612,111 +608,3 @@ func TestNormalizationFuncGlobalExistence(t *testing.T) { t.Fatal("child and root commands should have the same normalization functions") } } - -type testServiceGenerator struct{} - -func (testServiceGenerator) ParamNames() []kubectl.GeneratorParam { - return []kubectl.GeneratorParam{ - {"default-name", true}, - {"name", false}, - {"port", true}, - {"labels", false}, - {"public-ip", false}, - {"create-external-load-balancer", false}, - {"type", false}, - {"protocol", false}, - {"container-port", false}, // alias of target-port - {"target-port", false}, - {"port-name", false}, - {"session-affinity", false}, - } -} - -func (testServiceGenerator) Generate(genericParams map[string]interface{}) (runtime.Object, error) { - params := map[string]string{} - for key, value := range genericParams { - strVal, isString := value.(string) - if !isString { - return nil, fmt.Errorf("expected string, saw %v for '%s'", value, key) - } - params[key] = strVal - } - labelsString, found := params["labels"] - var labels map[string]string - var err error - if found && len(labelsString) > 0 { - labels, err = kubectl.ParseLabels(labelsString) - if err != nil { - return nil, err - } - } - - name, found := params["name"] - if !found || len(name) == 0 { - name, found = params["default-name"] - if !found || len(name) == 0 { - return nil, fmt.Errorf("'name' is a required parameter.") - } - } - portString, found := params["port"] - if !found { - return nil, fmt.Errorf("'port' is a required parameter.") - } - port, err := strconv.Atoi(portString) - if err != nil { - return nil, err - } - servicePortName, found := params["port-name"] - if !found { - // Leave the port unnamed. - servicePortName = "" - } - service := api.Service{ - ObjectMeta: api.ObjectMeta{ - Name: name, - Labels: labels, - }, - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Name: servicePortName, - Port: port, - Protocol: api.Protocol(params["protocol"]), - }, - }, - }, - } - targetPort, found := params["target-port"] - if !found { - targetPort, found = params["container-port"] - } - if found && len(targetPort) > 0 { - if portNum, err := strconv.Atoi(targetPort); err != nil { - service.Spec.Ports[0].TargetPort = intstr.FromString(targetPort) - } else { - service.Spec.Ports[0].TargetPort = intstr.FromInt(portNum) - } - } else { - service.Spec.Ports[0].TargetPort = intstr.FromInt(port) - } - if params["create-external-load-balancer"] == "true" { - service.Spec.Type = api.ServiceTypeLoadBalancer - } - if len(params["external-ip"]) > 0 { - service.Spec.ExternalIPs = []string{params["external-ip"]} - } - if len(params["type"]) != 0 { - service.Spec.Type = api.ServiceType(params["type"]) - } - if len(params["session-affinity"]) != 0 { - switch api.ServiceAffinity(params["session-affinity"]) { - case api.ServiceAffinityNone: - service.Spec.SessionAffinity = api.ServiceAffinityNone - case api.ServiceAffinityClientIP: - service.Spec.SessionAffinity = api.ServiceAffinityClientIP - default: - return nil, fmt.Errorf("unknown session affinity: %s", params["session-affinity"]) - } - } - return &service, nil -} diff --git a/pkg/kubectl/cmd/expose.go b/pkg/kubectl/cmd/expose.go index d0341eca662..1de43f6da39 100644 --- a/pkg/kubectl/cmd/expose.go +++ b/pkg/kubectl/cmd/expose.go @@ -131,7 +131,8 @@ func RunExpose(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []str // Get the generator, setup and validate all required parameters generatorName := cmdutil.GetFlagString(cmd, "generator") - generator, found := f.Generator(generatorName) + generators := f.Generators("expose") + generator, found := generators[generatorName] if !found { return cmdutil.UsageError(cmd, fmt.Sprintf("generator %q not found.", generatorName)) } @@ -179,6 +180,10 @@ func RunExpose(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []str if err = kubectl.ValidateParams(names, params); err != nil { return err } + // Check for invalid flags used against the present generator. + if err := kubectl.EnsureFlagsValid(cmd, generators, generatorName); err != nil { + return err + } // Generate new object object, err := generator.Generate(params) diff --git a/pkg/kubectl/cmd/expose_test.go b/pkg/kubectl/cmd/expose_test.go index a9d6639f5be..6bad24357f4 100644 --- a/pkg/kubectl/cmd/expose_test.go +++ b/pkg/kubectl/cmd/expose_test.go @@ -198,36 +198,6 @@ func TestRunExposeService(t *testing.T) { }, status: 200, }, - { - name: "expose-external-service", - args: []string{"service", "baz"}, - ns: "test", - calls: map[string]string{ - "GET": "/namespaces/test/services/baz", - "POST": "/namespaces/test/services", - }, - input: &api.Service{ - ObjectMeta: api.ObjectMeta{Name: "baz", Namespace: "test", ResourceVersion: "12"}, - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{}, - }, - }, - // Even if we specify --selector, since service/test doesn't need one it will ignore it - flags: map[string]string{"selector": "svc=fromexternal", "port": "90", "labels": "svc=fromexternal", "name": "frombaz", "generator": "service/test", "dry-run": "true"}, - output: &api.Service{ - ObjectMeta: api.ObjectMeta{Name: "frombaz", Namespace: "", Labels: map[string]string{"svc": "fromexternal"}}, - Spec: api.ServiceSpec{ - Ports: []api.ServicePort{ - { - Protocol: api.ProtocolTCP, - Port: 90, - TargetPort: intstr.FromInt(90), - }, - }, - }, - }, - status: 200, - }, { name: "expose-from-file", args: []string{}, diff --git a/pkg/kubectl/cmd/run.go b/pkg/kubectl/cmd/run.go index b6f9ae90216..a87efcee0d6 100644 --- a/pkg/kubectl/cmd/run.go +++ b/pkg/kubectl/cmd/run.go @@ -153,9 +153,10 @@ func Run(f *cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cob generatorName = "job/v1beta1" } } - generator, found := f.Generator(generatorName) + generators := f.Generators("run") + generator, found := generators[generatorName] if !found { - return cmdutil.UsageError(cmd, fmt.Sprintf("Generator: %s not found.", generatorName)) + return cmdutil.UsageError(cmd, fmt.Sprintf("generator %q not found.", generatorName)) } names := generator.ParamNames() params := kubectl.MakeParams(cmd, names) @@ -342,7 +343,8 @@ func getRestartPolicy(cmd *cobra.Command, interactive bool) (api.RestartPolicy, } func generateService(f *cmdutil.Factory, cmd *cobra.Command, args []string, serviceGenerator string, paramsIn map[string]interface{}, namespace string, out io.Writer) error { - generator, found := f.Generator(serviceGenerator) + generators := f.Generators("expose") + generator, found := generators[serviceGenerator] if !found { return fmt.Errorf("missing service generator: %s", serviceGenerator) } @@ -394,6 +396,7 @@ func createGeneratedObject(f *cmdutil.Factory, cmd *cobra.Command, generator kub return nil, "", nil, nil, err } + // TODO: Validate flag usage against selected generator. More tricky since --expose was added. obj, err := generator.Generate(params) if err != nil { return nil, "", nil, nil, err diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 61b5d69febf..72a12323879 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -57,9 +57,8 @@ const ( // TODO: pass the various interfaces on the factory directly into the command constructors (so the // commands are decoupled from the factory). type Factory struct { - clients *ClientCache - flags *pflag.FlagSet - generators map[string]kubectl.Generator + clients *ClientCache + flags *pflag.FlagSet // Returns interfaces for dealing with arbitrary runtime.Objects. Object func() (meta.RESTMapper, runtime.ObjectTyper) @@ -92,8 +91,8 @@ type Factory struct { // other namespace is specified and whether the namespace was // overriden. DefaultNamespace func() (string, bool, error) - // Returns the generator for the provided generator name - Generator func(name string) (kubectl.Generator, bool) + // Generators returns the generators for the provided command + Generators func(cmdName string) map[string]kubectl.Generator // Check whether the kind of resources could be exposed CanBeExposed func(kind unversioned.GroupKind) error // Check whether the kind of resources could be autoscaled @@ -120,19 +119,31 @@ const ( ) // DefaultGenerators returns the set of default generators for use in Factory instances -func DefaultGenerators() map[string]kubectl.Generator { - return map[string]kubectl.Generator{ - RunV1GeneratorName: kubectl.BasicReplicationController{}, - RunPodV1GeneratorName: kubectl.BasicPod{}, - ServiceV1GeneratorName: kubectl.ServiceGeneratorV1{}, - ServiceV2GeneratorName: kubectl.ServiceGeneratorV2{}, - HorizontalPodAutoscalerV1Beta1GeneratorName: kubectl.HorizontalPodAutoscalerV1Beta1{}, - DeploymentV1Beta1GeneratorName: kubectl.DeploymentV1Beta1{}, - JobV1Beta1GeneratorName: kubectl.JobV1Beta1{}, - NamespaceV1GeneratorName: kubectl.NamespaceGeneratorV1{}, - SecretV1GeneratorName: kubectl.SecretGeneratorV1{}, - SecretForDockerRegistryV1GeneratorName: kubectl.SecretForDockerRegistryGeneratorV1{}, +func DefaultGenerators(cmdName string) map[string]kubectl.Generator { + generators := map[string]map[string]kubectl.Generator{} + generators["expose"] = map[string]kubectl.Generator{ + ServiceV1GeneratorName: kubectl.ServiceGeneratorV1{}, + ServiceV2GeneratorName: kubectl.ServiceGeneratorV2{}, } + generators["run"] = map[string]kubectl.Generator{ + RunV1GeneratorName: kubectl.BasicReplicationController{}, + RunPodV1GeneratorName: kubectl.BasicPod{}, + DeploymentV1Beta1GeneratorName: kubectl.DeploymentV1Beta1{}, + JobV1Beta1GeneratorName: kubectl.JobV1Beta1{}, + } + generators["autoscale"] = map[string]kubectl.Generator{ + HorizontalPodAutoscalerV1Beta1GeneratorName: kubectl.HorizontalPodAutoscalerV1Beta1{}, + } + generators["namespace"] = map[string]kubectl.Generator{ + NamespaceV1GeneratorName: kubectl.NamespaceGeneratorV1{}, + } + generators["secret"] = map[string]kubectl.Generator{ + SecretV1GeneratorName: kubectl.SecretGeneratorV1{}, + } + generators["secret-for-docker-registry"] = map[string]kubectl.Generator{ + SecretForDockerRegistryV1GeneratorName: kubectl.SecretForDockerRegistryGeneratorV1{}, + } + return generators[cmdName] } // NewFactory creates a factory with the default Kubernetes resources defined @@ -144,8 +155,6 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { flags := pflag.NewFlagSet("", pflag.ContinueOnError) flags.SetNormalizeFunc(util.WarnWordSepNormalizeFunc) // Warn for "_" flags - generators := DefaultGenerators() - clientConfig := optionalClientConfig if optionalClientConfig == nil { clientConfig = DefaultClientConfig(flags) @@ -154,9 +163,8 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { clients := NewClientCache(clientConfig) return &Factory{ - clients: clients, - flags: flags, - generators: generators, + clients: clients, + flags: flags, Object: func() (meta.RESTMapper, runtime.ObjectTyper) { cfg, err := clientConfig.ClientConfig() @@ -311,9 +319,8 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { DefaultNamespace: func() (string, bool, error) { return clientConfig.Namespace() }, - Generator: func(name string) (kubectl.Generator, bool) { - generator, ok := generators[name] - return generator, ok + Generators: func(cmdName string) map[string]kubectl.Generator { + return DefaultGenerators(cmdName) }, CanBeExposed: func(kind unversioned.GroupKind) error { switch kind { diff --git a/pkg/kubectl/generate.go b/pkg/kubectl/generate.go index 8c1ec434d9a..6a71b619d80 100644 --- a/pkg/kubectl/generate.go +++ b/pkg/kubectl/generate.go @@ -23,6 +23,7 @@ import ( "strings" "github.com/spf13/cobra" + "github.com/spf13/pflag" "k8s.io/kubernetes/pkg/runtime" utilerrors "k8s.io/kubernetes/pkg/util/errors" ) @@ -69,6 +70,58 @@ func ValidateParams(paramSpec []GeneratorParam, params map[string]interface{}) e return utilerrors.NewAggregate(allErrs) } +// AnnotateFlags annotates all flags that are used by generators. +func AnnotateFlags(cmd *cobra.Command, generators map[string]Generator) { + // Iterate over all generators and mark any flags used by them. + for name, generator := range generators { + generatorParams := map[string]struct{}{} + for _, param := range generator.ParamNames() { + generatorParams[param.Name] = struct{}{} + } + + cmd.Flags().VisitAll(func(flag *pflag.Flag) { + if _, found := generatorParams[flag.Name]; !found { + // This flag is not used by the current generator + // so skip it. + return + } + if flag.Annotations == nil { + flag.Annotations = map[string][]string{} + } + if annotations := flag.Annotations["generator"]; annotations == nil { + flag.Annotations["generator"] = []string{} + } + flag.Annotations["generator"] = append(flag.Annotations["generator"], name) + }) + } +} + +// EnsureFlagsValid ensures that no invalid flags are being used against a generator. +func EnsureFlagsValid(cmd *cobra.Command, generators map[string]Generator, generatorInUse string) error { + AnnotateFlags(cmd, generators) + + allErrs := []error{} + cmd.Flags().VisitAll(func(flag *pflag.Flag) { + // If the flag hasn't changed, don't validate it. + if !flag.Changed { + return + } + // Look into the flag annotations for the generators that can use it. + if annotations := flag.Annotations["generator"]; len(annotations) > 0 { + annotationMap := map[string]struct{}{} + for _, ann := range annotations { + annotationMap[ann] = struct{}{} + } + // If the current generator is not annotated, then this flag shouldn't + // be used with it. + if _, found := annotationMap[generatorInUse]; !found { + allErrs = append(allErrs, fmt.Errorf("cannot use --%s with --generator=%s", flag.Name, generatorInUse)) + } + } + }) + return utilerrors.NewAggregate(allErrs) +} + // MakeParams is a utility that creates generator parameters from a command line func MakeParams(cmd *cobra.Command, params []GeneratorParam) map[string]interface{} { result := map[string]interface{}{}