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

@@ -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)

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) {
+		Generators: func(cmdName string) map[string]kubectl.Generator {
-			generator, ok := generators[name]
+			return cmdutil.DefaultGenerators(cmdName)
-			return generator, ok
 		},
 		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
-}

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)

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{},

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

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
+	clients *ClientCache
-	flags      *pflag.FlagSet
+	flags   *pflag.FlagSet
-	generators map[string]kubectl.Generator
 
 	// 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
+	// Generators returns the generators for the provided command
-	Generator func(name string) (kubectl.Generator, bool)
+	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 {
+func DefaultGenerators(cmdName string) map[string]kubectl.Generator {
-	return map[string]kubectl.Generator{
+	generators := map[string]map[string]kubectl.Generator{}
-		RunV1GeneratorName:                          kubectl.BasicReplicationController{},
+	generators["expose"] = map[string]kubectl.Generator{
-		RunPodV1GeneratorName:                       kubectl.BasicPod{},
+		ServiceV1GeneratorName: kubectl.ServiceGeneratorV1{},
-		ServiceV1GeneratorName:                      kubectl.ServiceGeneratorV1{},
+		ServiceV2GeneratorName: kubectl.ServiceGeneratorV2{},
-		ServiceV2GeneratorName:                      kubectl.ServiceGeneratorV2{},
-		HorizontalPodAutoscalerV1Beta1GeneratorName: kubectl.HorizontalPodAutoscalerV1Beta1{},
-		DeploymentV1Beta1GeneratorName:              kubectl.DeploymentV1Beta1{},
-		JobV1Beta1GeneratorName:                     kubectl.JobV1Beta1{},
-		NamespaceV1GeneratorName:                    kubectl.NamespaceGeneratorV1{},
-		SecretV1GeneratorName:                       kubectl.SecretGeneratorV1{},
-		SecretForDockerRegistryV1GeneratorName:      kubectl.SecretForDockerRegistryGeneratorV1{},
 	}
+	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,
+		clients: clients,
-		flags:      flags,
+		flags:   flags,
-		generators: generators,
 
 		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) {
+		Generators: func(cmdName string) map[string]kubectl.Generator {
-			generator, ok := generators[name]
+			return DefaultGenerators(cmdName)
-			return generator, ok
 		},
 		CanBeExposed: func(kind unversioned.GroupKind) error {
 			switch kind {

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{}{}