Merge pull request #17516 from kargakis/tighten-generator-validation

Auto commit by PR queue bot

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

@@ -58,9 +58,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)
@@ -95,8 +94,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
@@ -123,19 +122,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
@@ -147,8 +158,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)
@@ -157,9 +166,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()
@@ -317,9 +325,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{}{}