From 1e4cba79ef8972248ac360b4ac5b21b085f6a8dc Mon Sep 17 00:00:00 2001 From: Sachin Kumar Singh Date: Tue, 22 Dec 2020 01:50:52 +0530 Subject: [PATCH] Remove the dependency between create poddisruptionbudget and generators --- .../src/k8s.io/kubectl/pkg/cmd/create/BUILD | 3 + .../kubectl/pkg/cmd/create/create_pdb.go | 222 ++++++++++++--- .../kubectl/pkg/cmd/create/create_pdb_test.go | 262 ++++++++++++++---- 3 files changed, 398 insertions(+), 89 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/create/BUILD b/staging/src/k8s.io/kubectl/pkg/cmd/create/BUILD index d58150fb1cb..e586029bdfc 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/create/BUILD +++ b/staging/src/k8s.io/kubectl/pkg/cmd/create/BUILD @@ -30,6 +30,7 @@ go_library( "//staging/src/k8s.io/api/batch/v1beta1:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/api/networking/v1:go_default_library", + "//staging/src/k8s.io/api/policy/v1beta1:go_default_library", "//staging/src/k8s.io/api/rbac/v1:go_default_library", "//staging/src/k8s.io/api/scheduling/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", @@ -51,6 +52,7 @@ go_library( "//staging/src/k8s.io/client-go/kubernetes/typed/batch/v1beta1:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/typed/core/v1:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/typed/networking/v1:go_default_library", + "//staging/src/k8s.io/client-go/kubernetes/typed/policy/v1beta1:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/typed/rbac/v1:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/typed/scheduling/v1:go_default_library", "//staging/src/k8s.io/component-base/cli/flag:go_default_library", @@ -97,6 +99,7 @@ go_test( "//staging/src/k8s.io/api/batch/v1beta1:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/api/networking/v1:go_default_library", + "//staging/src/k8s.io/api/policy/v1beta1:go_default_library", "//staging/src/k8s.io/api/rbac/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb.go b/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb.go index 569895d11eb..2e2518bb40f 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb.go @@ -17,12 +17,22 @@ limitations under the License. package create import ( + "context" + "fmt" + "regexp" + "github.com/spf13/cobra" + policyv1beta1 "k8s.io/api/policy/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/cli-runtime/pkg/genericclioptions" + resourcecli "k8s.io/cli-runtime/pkg/resource" + policyclient "k8s.io/client-go/kubernetes/typed/policy/v1beta1" cmdutil "k8s.io/kubectl/pkg/cmd/util" - "k8s.io/kubectl/pkg/generate" - generateversioned "k8s.io/kubectl/pkg/generate/versioned" + "k8s.io/kubectl/pkg/scheme" + "k8s.io/kubectl/pkg/util" "k8s.io/kubectl/pkg/util/i18n" "k8s.io/kubectl/pkg/util/templates" ) @@ -43,14 +53,40 @@ var ( // PodDisruptionBudgetOpts holds the command-line options for poddisruptionbudget sub command type PodDisruptionBudgetOpts struct { - CreateSubcommandOptions *CreateSubcommandOptions + // PrintFlags holds options necessary for obtaining a printer + PrintFlags *genericclioptions.PrintFlags + PrintObj func(obj runtime.Object) error + // Name of resource being created + Name string + + MinAvailable string + MaxUnavailable string + + // A label selector to use for this budget + Selector string + CreateAnnotation bool + FieldManager string + Namespace string + EnforceNamespace bool + + Client *policyclient.PolicyV1beta1Client + DryRunStrategy cmdutil.DryRunStrategy + DryRunVerifier *resourcecli.DryRunVerifier + + genericclioptions.IOStreams +} + +// NewPodDisruptionBudgetOpts creates a new *PodDisruptionBudgetOpts with sane defaults +func NewPodDisruptionBudgetOpts(ioStreams genericclioptions.IOStreams) *PodDisruptionBudgetOpts { + return &PodDisruptionBudgetOpts{ + PrintFlags: genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme), + IOStreams: ioStreams, + } } // NewCmdCreatePodDisruptionBudget is a macro command to create a new pod disruption budget. func NewCmdCreatePodDisruptionBudget(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.Command { - options := &PodDisruptionBudgetOpts{ - CreateSubcommandOptions: NewCreateSubcommandOptions(ioStreams), - } + o := NewPodDisruptionBudgetOpts(ioStreams) cmd := &cobra.Command{ Use: "poddisruptionbudget NAME --selector=SELECTOR --min-available=N [--dry-run=server|client|none]", @@ -60,54 +96,172 @@ func NewCmdCreatePodDisruptionBudget(f cmdutil.Factory, ioStreams genericcliopti Long: pdbLong, Example: pdbExample, Run: func(cmd *cobra.Command, args []string) { - cmdutil.CheckErr(options.Complete(f, cmd, args)) - cmdutil.CheckErr(options.Run()) + cmdutil.CheckErr(o.Complete(f, cmd, args)) + cmdutil.CheckErr(o.Validate()) + cmdutil.CheckErr(o.Run()) }, } - options.CreateSubcommandOptions.PrintFlags.AddFlags(cmd) + o.PrintFlags.AddFlags(cmd) cmdutil.AddApplyAnnotationFlags(cmd) cmdutil.AddValidateFlags(cmd) - cmdutil.AddGeneratorFlags(cmd, generateversioned.PodDisruptionBudgetV2GeneratorName) + cmdutil.AddDryRunFlag(cmd) - cmd.Flags().String("min-available", "", i18n.T("The minimum number or percentage of available pods this budget requires.")) - cmd.Flags().String("max-unavailable", "", i18n.T("The maximum number or percentage of unavailable pods this budget requires.")) - cmd.Flags().String("selector", "", i18n.T("A label selector to use for this budget. Only equality-based selector requirements are supported.")) - cmdutil.AddFieldManagerFlagVar(cmd, &options.CreateSubcommandOptions.FieldManager, "kubectl-create") + cmd.Flags().StringVar(&o.MinAvailable, "min-available", o.MinAvailable, i18n.T("The minimum number or percentage of available pods this budget requires.")) + cmd.Flags().StringVar(&o.MaxUnavailable, "max-unavailable", o.MaxUnavailable, i18n.T("The maximum number or percentage of unavailable pods this budget requires.")) + cmd.Flags().StringVar(&o.Selector, "selector", o.Selector, i18n.T("A label selector to use for this budget. Only equality-based selector requirements are supported.")) + cmdutil.AddFieldManagerFlagVar(cmd, &o.FieldManager, "kubectl-create") return cmd } // Complete completes all the required options func (o *PodDisruptionBudgetOpts) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string) error { - name, err := NameFromCommandArgs(cmd, args) + var err error + o.Name, err = NameFromCommandArgs(cmd, args) if err != nil { return err } - var generator generate.StructuredGenerator - switch generatorName := cmdutil.GetFlagString(cmd, "generator"); generatorName { - case generateversioned.PodDisruptionBudgetV1GeneratorName: - generator = &generateversioned.PodDisruptionBudgetV1Generator{ - Name: name, - MinAvailable: cmdutil.GetFlagString(cmd, "min-available"), - Selector: cmdutil.GetFlagString(cmd, "selector"), - } - case generateversioned.PodDisruptionBudgetV2GeneratorName: - generator = &generateversioned.PodDisruptionBudgetV2Generator{ - Name: name, - MinAvailable: cmdutil.GetFlagString(cmd, "min-available"), - MaxUnavailable: cmdutil.GetFlagString(cmd, "max-unavailable"), - Selector: cmdutil.GetFlagString(cmd, "selector"), - } - default: - return errUnsupportedGenerator(cmd, generatorName) + restConfig, err := f.ToRESTConfig() + if err != nil { + return err + } + o.Client, err = policyclient.NewForConfig(restConfig) + if err != nil { + return err } - return o.CreateSubcommandOptions.Complete(f, cmd, args, generator) + o.CreateAnnotation = cmdutil.GetFlagBool(cmd, cmdutil.ApplyAnnotationsFlag) + + o.DryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd) + if err != nil { + return err + } + dynamicClient, err := f.DynamicClient() + if err != nil { + return err + } + discoveryClient, err := f.ToDiscoveryClient() + if err != nil { + return err + } + o.DryRunVerifier = resourcecli.NewDryRunVerifier(dynamicClient, discoveryClient) + + o.Namespace, o.EnforceNamespace, err = f.ToRawKubeConfigLoader().Namespace() + if err != nil { + return err + } + + cmdutil.PrintFlagsWithDryRunStrategy(o.PrintFlags, o.DryRunStrategy) + + printer, err := o.PrintFlags.ToPrinter() + if err != nil { + return err + } + + o.PrintObj = func(obj runtime.Object) error { + return printer.PrintObj(obj, o.Out) + } + + return nil +} + +// Validate checks to the PodDisruptionBudgetOpts to see if there is sufficient information run the command +func (o *PodDisruptionBudgetOpts) Validate() error { + if len(o.Name) == 0 { + return fmt.Errorf("name must be specified") + } + + if len(o.Selector) == 0 { + return fmt.Errorf("a selector must be specified") + } + + if len(o.MaxUnavailable) == 0 && len(o.MinAvailable) == 0 { + return fmt.Errorf("one of min-available or max-unavailable must be specified") + } + + if len(o.MaxUnavailable) > 0 && len(o.MinAvailable) > 0 { + return fmt.Errorf("min-available and max-unavailable cannot be both specified") + } + + // The following regex matches the following values: + // 10, 20, 30%, 50% (number and percentage) + // but not 10Gb, 20Mb + re := regexp.MustCompile(`^[0-9]+%?$`) + + switch { + case len(o.MinAvailable) > 0 && !re.MatchString(o.MinAvailable): + return fmt.Errorf("invalid format specified for min-available") + case len(o.MaxUnavailable) > 0 && !re.MatchString(o.MaxUnavailable): + return fmt.Errorf("invalid format specified for max-unavailable") + } + + return nil } // Run calls the CreateSubcommandOptions.Run in PodDisruptionBudgetOpts instance func (o *PodDisruptionBudgetOpts) Run() error { - return o.CreateSubcommandOptions.Run() + podDisruptionBudget, err := o.createPodDisruptionBudgets() + if err != nil { + return err + } + + if err := util.CreateOrUpdateAnnotation(o.CreateAnnotation, podDisruptionBudget, scheme.DefaultJSONEncoder()); err != nil { + return err + } + + if o.DryRunStrategy != cmdutil.DryRunClient { + createOptions := metav1.CreateOptions{} + if o.FieldManager != "" { + createOptions.FieldManager = o.FieldManager + } + if o.DryRunStrategy == cmdutil.DryRunServer { + if err := o.DryRunVerifier.HasSupport(podDisruptionBudget.GroupVersionKind()); err != nil { + return err + } + createOptions.DryRun = []string{metav1.DryRunAll} + } + podDisruptionBudget, err = o.Client.PodDisruptionBudgets(o.Namespace).Create(context.TODO(), podDisruptionBudget, createOptions) + if err != nil { + return fmt.Errorf("failed to create poddisruptionbudgets: %v", err) + } + } + return o.PrintObj(podDisruptionBudget) +} + +func (o *PodDisruptionBudgetOpts) createPodDisruptionBudgets() (*policyv1beta1.PodDisruptionBudget, error) { + namespace := "" + if o.EnforceNamespace { + namespace = o.Namespace + } + + podDisruptionBudget := &policyv1beta1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + APIVersion: policyv1beta1.SchemeGroupVersion.String(), + Kind: "PodDisruptionBudget", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: o.Name, + Namespace: namespace, + }, + } + + selector, err := metav1.ParseToLabelSelector(o.Selector) + if err != nil { + return nil, err + } + + podDisruptionBudget.Spec.Selector = selector + + switch { + case len(o.MinAvailable) > 0: + minAvailable := intstr.Parse(o.MinAvailable) + podDisruptionBudget.Spec.MinAvailable = &minAvailable + case len(o.MaxUnavailable) > 0: + maxUnavailable := intstr.Parse(o.MaxUnavailable) + podDisruptionBudget.Spec.MaxUnavailable = &maxUnavailable + } + + return podDisruptionBudget, nil } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb_test.go index bcb9bc00c06..addd403b9e8 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/create/create_pdb_test.go @@ -17,69 +17,221 @@ limitations under the License. package create import ( - "bytes" - "io/ioutil" - "net/http" "testing" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/cli-runtime/pkg/genericclioptions" - restclient "k8s.io/client-go/rest" - "k8s.io/client-go/rest/fake" - cmdtesting "k8s.io/kubectl/pkg/cmd/testing" - "k8s.io/kubectl/pkg/scheme" + policyv1beta1 "k8s.io/api/policy/v1beta1" + apiequality "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) -func TestCreatePdb(t *testing.T) { - pdbName := "my-pdb" - tf := cmdtesting.NewTestFactory().WithNamespace("test") - defer tf.Cleanup() +func TestCreatePdbValidation(t *testing.T) { + selectorOpts := "app=nginx" + podAmountNumber := "3" + podAmountPercent := "50%" - ns := scheme.Codecs.WithoutConversion() - - tf.Client = &fake.RESTClient{ - GroupVersion: schema.GroupVersion{Group: "policy", Version: "v1beta1"}, - NegotiatedSerializer: ns, - Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusOK, - Body: ioutil.NopCloser(&bytes.Buffer{}), - }, nil - }), - } - tf.ClientConfigVal = &restclient.Config{} - - outputFormat := "name" - - ioStreams, _, buf, _ := genericclioptions.NewTestIOStreams() - cmd := NewCmdCreatePodDisruptionBudget(tf, ioStreams) - cmd.Flags().Set("min-available", "1") - cmd.Flags().Set("selector", "app=rails") - cmd.Flags().Set("dry-run", "client") - cmd.Flags().Set("output", outputFormat) - - printFlags := genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme) - printFlags.OutputFormat = &outputFormat - - options := &PodDisruptionBudgetOpts{ - CreateSubcommandOptions: &CreateSubcommandOptions{ - PrintFlags: printFlags, - Name: pdbName, - IOStreams: ioStreams, + tests := map[string]struct { + options *PodDisruptionBudgetOpts + expected string + }{ + "test-missing-name-param": { + options: &PodDisruptionBudgetOpts{ + Selector: selectorOpts, + MinAvailable: podAmountNumber, + }, + expected: "name must be specified", + }, + "test-missing-selector-param": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + MinAvailable: podAmountNumber, + }, + expected: "a selector must be specified", + }, + "test-missing-max-unavailable-max-unavailable-param": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + }, + expected: "one of min-available or max-unavailable must be specified", + }, + "test-both-min-available-max-unavailable-param": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MinAvailable: podAmountNumber, + MaxUnavailable: podAmountPercent, + }, + expected: "min-available and max-unavailable cannot be both specified", + }, + "test-invalid-min-available-format": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MinAvailable: "10GB", + }, + expected: "invalid format specified for min-available", + }, + "test-invalid-max-unavailable-format": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MaxUnavailable: "10GB", + }, + expected: "invalid format specified for max-unavailable", + }, + "test-valid-min-available-format": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MaxUnavailable: podAmountNumber, + }, + expected: "", + }, + "test-valid-max-unavailable-format": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MaxUnavailable: podAmountPercent, + }, + expected: "", }, } - err := options.Complete(tf, cmd, []string{pdbName}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - err = options.Run() - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { - expectedOutput := "poddisruptionbudget.policy/" + pdbName + "\n" - if buf.String() != expectedOutput { - t.Errorf("expected output: %s, but got: %s", expectedOutput, buf.String()) + o := &PodDisruptionBudgetOpts{ + Name: tc.options.Name, + Selector: tc.options.Selector, + MinAvailable: tc.options.MinAvailable, + MaxUnavailable: tc.options.MaxUnavailable, + } + + err := o.Validate() + if err != nil && err.Error() != tc.expected { + t.Errorf("unexpected error: %v", err) + } + if tc.expected != "" && err == nil { + t.Errorf("expected error, got no error") + } + }) + } +} + +func TestCreatePdb(t *testing.T) { + selectorOpts := "app=nginx" + podAmountNumber := "3" + podAmountPercent := "50%" + + selector, err := metav1.ParseToLabelSelector(selectorOpts) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + minAvailableNumber := intstr.Parse(podAmountNumber) + minAvailablePercent := intstr.Parse(podAmountPercent) + + minUnavailableNumber := intstr.Parse(podAmountNumber) + minUnavailablePercent := intstr.Parse(podAmountPercent) + + tests := map[string]struct { + options *PodDisruptionBudgetOpts + expected *policyv1beta1.PodDisruptionBudget + }{ + "test-valid-min-available-pods-number": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MinAvailable: podAmountNumber, + }, + expected: &policyv1beta1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + Kind: "PodDisruptionBudget", + APIVersion: "policy/v1beta1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pdb", + }, + Spec: policyv1beta1.PodDisruptionBudgetSpec{ + Selector: selector, + MinAvailable: &minAvailableNumber, + }, + }, + }, + "test-valid-min-available-pods-percentage": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MinAvailable: podAmountPercent, + }, + expected: &policyv1beta1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + Kind: "PodDisruptionBudget", + APIVersion: "policy/v1beta1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pdb", + }, + Spec: policyv1beta1.PodDisruptionBudgetSpec{ + Selector: selector, + MinAvailable: &minAvailablePercent, + }, + }, + }, + "test-valid-max-unavailable-pods-number": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MinAvailable: podAmountNumber, + }, + expected: &policyv1beta1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + Kind: "PodDisruptionBudget", + APIVersion: "policy/v1beta1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pdb", + }, + Spec: policyv1beta1.PodDisruptionBudgetSpec{ + Selector: selector, + MinAvailable: &minUnavailableNumber, + }, + }, + }, + "test-valid-max-unavailable-pods-percentage": { + options: &PodDisruptionBudgetOpts{ + Name: "my-pdb", + Selector: selectorOpts, + MinAvailable: podAmountPercent, + }, + expected: &policyv1beta1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + Kind: "PodDisruptionBudget", + APIVersion: "policy/v1beta1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pdb", + }, + Spec: policyv1beta1.PodDisruptionBudgetSpec{ + Selector: selector, + MinAvailable: &minUnavailablePercent, + }, + }, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + + podDisruptionBudget, err := tc.options.createPodDisruptionBudgets() + if err != nil { + t.Errorf("unexpected error:\n%#v\n", err) + return + } + if !apiequality.Semantic.DeepEqual(podDisruptionBudget, tc.expected) { + t.Errorf("expected:\n%#v\ngot:\n%#v", tc.expected, podDisruptionBudget) + } + }) } }