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