From eb0fc28aeaf0b81c0a5d607707bebb06e989fbfa Mon Sep 17 00:00:00 2001 From: jace-ys Date: Mon, 5 Feb 2024 17:41:50 +0000 Subject: [PATCH 1/4] Allow use of label selector with namespaces/all-namespaces Signed-off-by: jace-ys --- pkg/renew/renew.go | 4 ++-- pkg/renew/renew_test.go | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/pkg/renew/renew.go b/pkg/renew/renew.go index 19ddf40..f605896 100644 --- a/pkg/renew/renew.go +++ b/pkg/renew/renew.go @@ -112,8 +112,8 @@ func (o *Options) Validate(cmd *cobra.Command, args []string) error { return errors.New("cannot specify --namespace flag in conjunction with --all flag") } - if !o.All && len(args) == 0 { - return errors.New("please supply one or more Certificate resource names or use the --all flag to renew all Certificate resources") + if !o.All && len(args) == 0 && len(o.LabelSelector) == 0 { + return errors.New("please either supply one or more Certificate resource names, label selectors, or use the --all flag to renew all Certificate resources") } return nil diff --git a/pkg/renew/renew_test.go b/pkg/renew/renew_test.go index b93f228..2603b89 100644 --- a/pkg/renew/renew_test.go +++ b/pkg/renew/renew_test.go @@ -95,6 +95,29 @@ func TestValidate(t *testing.T) { }, expErr: false, }, + "If --label-selector specified with --all, error": { + options: &Options{ + LabelSelector: "foo=bar", + All: true, + }, + expErr: true, + }, + "If --label-selector specified with --all-namespaces, don't error": { + options: &Options{ + AllNamespaces: true, + LabelSelector: "foo=bar", + }, + expErr: false, + }, + "If --label-selector specified with --namespace, don't error": { + options: &Options{ + LabelSelector: "foo=bar", + }, + setStringFlags: []stringFlag{ + {name: "namespace", value: "foo"}, + }, + expErr: false, + }, } for name, test := range tests { From 1196365488e5376e897a6d7e95176f5b7b8b5ee7 Mon Sep 17 00:00:00 2001 From: jace-ys Date: Mon, 5 Feb 2024 17:50:07 +0000 Subject: [PATCH 2/4] Refactor validation Signed-off-by: jace-ys --- pkg/renew/renew.go | 26 ++++++++++++++------------ pkg/renew/renew_test.go | 11 +++++++++-- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/pkg/renew/renew.go b/pkg/renew/renew.go index f605896..af852ca 100644 --- a/pkg/renew/renew.go +++ b/pkg/renew/renew.go @@ -96,24 +96,26 @@ func NewCmdRenew(ctx context.Context, ioStreams genericclioptions.IOStreams) *co // Validate validates the provided options func (o *Options) Validate(cmd *cobra.Command, args []string) error { - if len(o.LabelSelector) > 0 && len(args) > 0 { - return errors.New("cannot specify Certificate names in conjunction with label selectors") - } - if len(o.LabelSelector) > 0 && o.All { return errors.New("cannot specify label selectors in conjunction with --all flag") } - if o.All && len(args) > 0 { - return errors.New("cannot specify Certificate names in conjunction with --all flag") - } + if len(args) == 0 { + if !o.All && len(o.LabelSelector) == 0 { + return errors.New("please either supply one or more Certificate resource names, label selectors, or use the --all flag to renew all Certificate resources") + } + } else { + if len(o.LabelSelector) > 0 { + return errors.New("cannot specify Certificate names in conjunction with label selectors") + } - if o.All && cmd.PersistentFlags().Changed("namespace") { - return errors.New("cannot specify --namespace flag in conjunction with --all flag") - } + if o.All { + return errors.New("cannot specify Certificate names in conjunction with --all flag") + } - if !o.All && len(args) == 0 && len(o.LabelSelector) == 0 { - return errors.New("please either supply one or more Certificate resource names, label selectors, or use the --all flag to renew all Certificate resources") + if o.AllNamespaces { + return errors.New("cannot specify Certificate names in conjunction with --all-namespaces flag") + } } return nil diff --git a/pkg/renew/renew_test.go b/pkg/renew/renew_test.go index 2603b89..828af8d 100644 --- a/pkg/renew/renew_test.go +++ b/pkg/renew/renew_test.go @@ -41,6 +41,13 @@ func TestValidate(t *testing.T) { args: []string{"abc"}, expErr: true, }, + "If there are arguments, as well as --all-namespaces, error": { + options: &Options{ + AllNamespaces: true, + }, + args: []string{"abc"}, + expErr: true, + }, "If there are all certificates selected, as well as label selector, error": { options: &Options{ LabelSelector: "foo=bar", @@ -63,14 +70,14 @@ func TestValidate(t *testing.T) { }, expErr: false, }, - "If --namespace and --all namespace specified, error": { + "If --namespace and --all specified, don't error": { options: &Options{ All: true, }, setStringFlags: []stringFlag{ {name: "namespace", value: "foo"}, }, - expErr: true, + expErr: false, }, "If --namespace specified without arguments, error": { options: &Options{}, From 5babd3ebdd128d93a0d4c2f9ccbaa442ec53d516 Mon Sep 17 00:00:00 2001 From: jace-ys Date: Mon, 5 Feb 2024 17:49:55 +0000 Subject: [PATCH 3/4] Fix test bug Signed-off-by: jace-ys --- pkg/renew/renew_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/renew/renew_test.go b/pkg/renew/renew_test.go index 828af8d..92957e3 100644 --- a/pkg/renew/renew_test.go +++ b/pkg/renew/renew_test.go @@ -53,7 +53,6 @@ func TestValidate(t *testing.T) { LabelSelector: "foo=bar", All: true, }, - args: []string{""}, expErr: true, }, "If there are all certificates selected, as well as arguments, error": { From 11ee430e1719882fc39d599956d1aae25c1efed9 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Thu, 15 Feb 2024 10:20:57 +0100 Subject: [PATCH 4/4] dynamically build error string Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/renew/renew.go | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/pkg/renew/renew.go b/pkg/renew/renew.go index af852ca..5f7e26f 100644 --- a/pkg/renew/renew.go +++ b/pkg/renew/renew.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "strings" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" @@ -96,26 +97,37 @@ func NewCmdRenew(ctx context.Context, ioStreams genericclioptions.IOStreams) *co // Validate validates the provided options func (o *Options) Validate(cmd *cobra.Command, args []string) error { - if len(o.LabelSelector) > 0 && o.All { - return errors.New("cannot specify label selectors in conjunction with --all flag") + // The --all, --selector and args are mutually exclusive. + // - --all is equivalent to a match-all label selector + // - --selector filters by label selector + // - args are an explicit list of Certificate names + // However, there must always be one of the three specified. + + var flags []string + if len(args) > 0 { + flags = append(flags, fmt.Sprintf("the Certificate resource names %q", args)) + } + if o.All { + flags = append(flags, "the --all flag") + } + if len(o.LabelSelector) > 0 { + flags = append(flags, "a label selector") } - if len(args) == 0 { - if !o.All && len(o.LabelSelector) == 0 { - return errors.New("please either supply one or more Certificate resource names, label selectors, or use the --all flag to renew all Certificate resources") - } - } else { - if len(o.LabelSelector) > 0 { - return errors.New("cannot specify Certificate names in conjunction with label selectors") - } + // Ensure that only one of the three flags are specified + if len(flags) > 1 { + return fmt.Errorf("cannot specify %s in conjunction with %s", flags[0], strings.Join(flags[1:], " and ")) + } - if o.All { - return errors.New("cannot specify Certificate names in conjunction with --all flag") - } + if len(flags) == 0 { + return errors.New("please either supply one or more Certificate resource names, label selectors, or use the --all flag to renew all Certificate resources") + } - if o.AllNamespaces { - return errors.New("cannot specify Certificate names in conjunction with --all-namespaces flag") - } + // The --all-namespaces flag overrides the --namespace flag. + // Additionally, we can only use --all-namespaces when not specifying a list of Certificate names + + if o.AllNamespaces && len(args) > 0 { + return errors.New("cannot specify Certificate names in conjunction with --all-namespaces flag") } return nil