From dc0979b65b8414738ccaced26f7a26ff6d41e61b Mon Sep 17 00:00:00 2001 From: Ian Howell Date: Fri, 7 Feb 2020 16:27:05 -0600 Subject: [PATCH] [#42] Refactor flag operations to be uniform This also increases unit test coverage Change-Id: I324954a1216f2204ff3977c05eb9bd087cd87795 --- cmd/config/get_authinfo.go | 10 +- cmd/config/get_cluster.go | 23 +- cmd/config/get_context.go | 24 +- cmd/config/set_authinfo.go | 64 ++--- cmd/config/set_cluster.go | 55 ++-- cmd/config/set_context.go | 60 +++-- ...config-cmd-set-context-too-few-args.golden | 10 +- ...onfig-cmd-set-context-too-many-args.golden | 10 +- .../config-cmd-set-context-with-help.golden | 10 +- .../missing.golden | 2 +- cmd/document/render.go | 52 +++- pkg/config/cmds.go | 131 +++------- pkg/config/cmds_test.go | 59 ----- pkg/config/cmds_types.go | 30 --- pkg/config/config.go | 10 +- pkg/config/options.go | 131 ++++++++++ pkg/config/options_test.go | 244 ++++++++++++++++++ 17 files changed, 609 insertions(+), 316 deletions(-) delete mode 100644 pkg/config/cmds_types.go create mode 100644 pkg/config/options.go create mode 100644 pkg/config/options_test.go diff --git a/cmd/config/get_authinfo.go b/cmd/config/get_authinfo.go index 883835ab9..48f345ebb 100644 --- a/cmd/config/get_authinfo.go +++ b/cmd/config/get_authinfo.go @@ -36,19 +36,19 @@ airshipctl config get-credential e2e` // An AuthInfo refers to a particular user for a cluster // NewCmdConfigGetAuthInfo returns a Command instance for 'config -AuthInfo' sub command func NewCmdConfigGetAuthInfo(rootSettings *environment.AirshipCTLSettings) *cobra.Command { - theAuthInfo := &config.AuthInfoOptions{} - getauthinfocmd := &cobra.Command{ + o := &config.AuthInfoOptions{} + cmd := &cobra.Command{ Use: "get-credentials NAME", Short: "Gets a user entry from the airshipctl config", Long: getAuthInfoLong, Example: getAuthInfoExample, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 1 { - theAuthInfo.Name = args[0] + o.Name = args[0] } - return config.RunGetAuthInfo(theAuthInfo, cmd.OutOrStdout(), rootSettings.Config()) + return config.RunGetAuthInfo(o, cmd.OutOrStdout(), rootSettings.Config()) }, } - return getauthinfocmd + return cmd } diff --git a/cmd/config/get_cluster.go b/cmd/config/get_cluster.go index f010bf087..f284b049d 100644 --- a/cmd/config/get_cluster.go +++ b/cmd/config/get_cluster.go @@ -38,32 +38,35 @@ airshipctl config get-cluster e2e --%v=ephemeral`, config.FlagClusterType) // NewCmdConfigGetCluster returns a Command instance for 'config -Cluster' sub command func NewCmdConfigGetCluster(rootSettings *environment.AirshipCTLSettings) *cobra.Command { - theCluster := &config.ClusterOptions{} - getclustercmd := &cobra.Command{ + o := &config.ClusterOptions{} + cmd := &cobra.Command{ Use: "get-cluster NAME", Short: getClusterLong, Example: getClusterExample, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 1 { - theCluster.Name = args[0] + o.Name = args[0] } - err := validate(theCluster) + err := validate(o) if err != nil { return err } - return config.RunGetCluster(theCluster, cmd.OutOrStdout(), rootSettings.Config()) + return config.RunGetCluster(o, cmd.OutOrStdout(), rootSettings.Config()) }, } - gcInitFlags(theCluster, getclustercmd) - - return getclustercmd + addGetClusterFlags(o, cmd) + return cmd } -func gcInitFlags(o *config.ClusterOptions, getclustercmd *cobra.Command) { - getclustercmd.Flags().StringVar(&o.ClusterType, config.FlagClusterType, "", +func addGetClusterFlags(o *config.ClusterOptions, cmd *cobra.Command) { + flags := cmd.Flags() + flags.StringVar( + &o.ClusterType, + config.FlagClusterType, + "", config.FlagClusterType+" for the cluster entry in airshipctl config") } diff --git a/cmd/config/get_context.go b/cmd/config/get_context.go index 89e3425c6..0cfb4ea7f 100644 --- a/cmd/config/get_context.go +++ b/cmd/config/get_context.go @@ -45,25 +45,29 @@ airshipctl config get-context e2e`, // NewCmdConfigGetContext returns a Command instance for 'config -Context' sub command func NewCmdConfigGetContext(rootSettings *environment.AirshipCTLSettings) *cobra.Command { - theContext := &config.ContextOptions{} - getcontextcmd := &cobra.Command{ + o := &config.ContextOptions{} + cmd := &cobra.Command{ Use: "get-context NAME", Short: getContextLong, Example: getContextExample, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 1 { - theContext.Name = args[0] + o.Name = args[0] } - return config.RunGetContext(theContext, cmd.OutOrStdout(), rootSettings.Config()) + return config.RunGetContext(o, cmd.OutOrStdout(), rootSettings.Config()) }, } - gctxInitFlags(theContext, getcontextcmd) - - return getcontextcmd + addGetContextFlags(o, cmd) + return cmd } -func gctxInitFlags(o *config.ContextOptions, getcontextcmd *cobra.Command) { - getcontextcmd.Flags().BoolVar(&o.CurrentContext, config.FlagCurrentContext, false, - config.FlagCurrentContext+" to retrieve the current context entry in airshipctl config") +func addGetContextFlags(o *config.ContextOptions, cmd *cobra.Command) { + flags := cmd.Flags() + + flags.BoolVar( + &o.CurrentContext, + config.FlagCurrentContext, + false, + "retrieve the current context entry in airshipctl config") } diff --git a/cmd/config/set_authinfo.go b/cmd/config/set_authinfo.go index c5b98e7a3..f852b382e 100644 --- a/cmd/config/set_authinfo.go +++ b/cmd/config/set_authinfo.go @@ -23,7 +23,6 @@ import ( "opendev.org/airship/airshipctl/pkg/config" "opendev.org/airship/airshipctl/pkg/environment" - "opendev.org/airship/airshipctl/pkg/log" ) var ( @@ -67,62 +66,69 @@ airshipctl config set-credentials cluster-admin --%v=~/.kube/admin.crt --%v=true // NewCmdConfigSetAuthInfo creates a command object for the "set-credentials" action, which // defines a new AuthInfo airship config. func NewCmdConfigSetAuthInfo(rootSettings *environment.AirshipCTLSettings) *cobra.Command { - theAuthInfo := &config.AuthInfoOptions{} + o := &config.AuthInfoOptions{} - setauthinfo := &cobra.Command{ + cmd := &cobra.Command{ Use: "set-credentials NAME", Short: "Sets a user entry in the airshipctl config", Long: setAuthInfoLong, Example: setAuthInfoExample, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - theAuthInfo.Name = args[0] - modified, err := config.RunSetAuthInfo(theAuthInfo, rootSettings.Config(), true) + o.Name = args[0] + modified, err := config.RunSetAuthInfo(o, rootSettings.Config(), true) if err != nil { return err } if modified { - fmt.Fprintf(cmd.OutOrStdout(), "User information %q modified.\n", theAuthInfo.Name) + fmt.Fprintf(cmd.OutOrStdout(), "User information %q modified.\n", o.Name) } else { - fmt.Fprintf(cmd.OutOrStdout(), "User information %q created.\n", theAuthInfo.Name) + fmt.Fprintf(cmd.OutOrStdout(), "User information %q created.\n", o.Name) } return nil }, } - err := suInitFlags(theAuthInfo, setauthinfo) - if err != nil { - log.Fatal(err) - } - return setauthinfo + addSetAuthInfoFlags(o, cmd) + return cmd } -func suInitFlags(o *config.AuthInfoOptions, setauthinfo *cobra.Command) error { - setauthinfo.Flags().StringVar(&o.ClientCertificate, config.FlagCertFile, o.ClientCertificate, +func addSetAuthInfoFlags(o *config.AuthInfoOptions, cmd *cobra.Command) { + flags := cmd.Flags() + + flags.StringVar( + &o.ClientCertificate, + config.FlagCertFile, + "", "Path to "+config.FlagCertFile+" file for the user entry in airshipctl") - err := setauthinfo.MarkFlagFilename(config.FlagCertFile) - if err != nil { - return err - } - setauthinfo.Flags().StringVar(&o.ClientKey, config.FlagKeyFile, o.ClientKey, + flags.StringVar( + &o.ClientKey, + config.FlagKeyFile, + "", "Path to "+config.FlagKeyFile+" file for the user entry in airshipctl") - err = setauthinfo.MarkFlagFilename(config.FlagKeyFile) - if err != nil { - return err - } - setauthinfo.Flags().StringVar(&o.Token, config.FlagBearerToken, o.Token, + flags.StringVar( + &o.Token, + config.FlagBearerToken, + "", config.FlagBearerToken+" for the user entry in airshipctl") - setauthinfo.Flags().StringVar(&o.Username, config.FlagUsername, o.Username, + flags.StringVar( + &o.Username, + config.FlagUsername, + "", config.FlagUsername+" for the user entry in airshipctl") - setauthinfo.Flags().StringVar(&o.Password, config.FlagPassword, o.Password, + flags.StringVar( + &o.Password, + config.FlagPassword, + "", config.FlagPassword+" for the user entry in airshipctl") - setauthinfo.Flags().BoolVar(&o.EmbedCertData, config.FlagEmbedCerts, false, + flags.BoolVar( + &o.EmbedCertData, + config.FlagEmbedCerts, + false, "Embed client cert/key for the user entry in airshipctl") - - return nil } diff --git a/cmd/config/set_cluster.go b/cmd/config/set_cluster.go index 1c32e577c..795058acd 100644 --- a/cmd/config/set_cluster.go +++ b/cmd/config/set_cluster.go @@ -57,52 +57,69 @@ airshipctl config set-cluster e2e --%v=target --%v=true --%v=".airship/cert_file // NewCmdConfigSetCluster creates a command object for the "set-cluster" action, which // defines a new cluster airshipctl config. func NewCmdConfigSetCluster(rootSettings *environment.AirshipCTLSettings) *cobra.Command { - theCluster := &config.ClusterOptions{} - - setclustercmd := &cobra.Command{ + o := &config.ClusterOptions{} + cmd := &cobra.Command{ Use: "set-cluster NAME", Short: "Sets a cluster entry in the airshipctl config", Long: setClusterLong, Example: setClusterExample, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - theCluster.Name = cmd.Flags().Args()[0] - modified, err := config.RunSetCluster(theCluster, rootSettings.Config(), true) + o.Name = args[0] + modified, err := config.RunSetCluster(o, rootSettings.Config(), true) if err != nil { return err } if modified { fmt.Fprintf(cmd.OutOrStdout(), "Cluster %q of type %q modified.\n", - theCluster.Name, theCluster.ClusterType) + o.Name, o.ClusterType) } else { fmt.Fprintf(cmd.OutOrStdout(), "Cluster %q of type %q created.\n", - theCluster.Name, theCluster.ClusterType) + o.Name, o.ClusterType) } return nil }, } - scInitFlags(theCluster, setclustercmd) - return setclustercmd + addSetClusterFlags(o, cmd) + return cmd } -func scInitFlags(o *config.ClusterOptions, setclustercmd *cobra.Command) { - setclustercmd.Flags().StringVar(&o.Server, config.FlagAPIServer, o.Server, +func addSetClusterFlags(o *config.ClusterOptions, cmd *cobra.Command) { + flags := cmd.Flags() + + flags.StringVar( + &o.Server, + config.FlagAPIServer, + "", config.FlagAPIServer+" for the cluster entry in airshipctl config") - setclustercmd.Flags().StringVar(&o.ClusterType, config.FlagClusterType, o.ClusterType, + flags.StringVar( + &o.ClusterType, + config.FlagClusterType, + "", config.FlagClusterType+" for the cluster entry in airshipctl config") - setclustercmd.Flags().BoolVar(&o.InsecureSkipTLSVerify, config.FlagInsecure, true, - config.FlagInsecure+" for the cluster entry in airshipctl config") - - setclustercmd.Flags().StringVar(&o.CertificateAuthority, config.FlagCAFile, o.CertificateAuthority, - "Path to "+config.FlagCAFile+" file for the cluster entry in airshipctl config") - err := setclustercmd.MarkFlagFilename(config.FlagCAFile) + err := cmd.MarkFlagRequired(config.FlagClusterType) if err != nil { log.Fatal(err) } - setclustercmd.Flags().BoolVar(&o.EmbedCAData, config.FlagEmbedCerts, false, + flags.BoolVar( + &o.InsecureSkipTLSVerify, + config.FlagInsecure, + true, + config.FlagInsecure+" for the cluster entry in airshipctl config") + + flags.StringVar( + &o.CertificateAuthority, + config.FlagCAFile, + "", + "Path to "+config.FlagCAFile+" file for the cluster entry in airshipctl config") + + flags.BoolVar( + &o.EmbedCAData, + config.FlagEmbedCerts, + false, config.FlagEmbedCerts+" for the cluster entry in airshipctl config") } diff --git a/cmd/config/set_context.go b/cmd/config/set_context.go index ac7713d21..f580fe117 100644 --- a/cmd/config/set_context.go +++ b/cmd/config/set_context.go @@ -44,11 +44,11 @@ airshipctl config set-context e2e`, ) // NewCmdConfigSetContext creates a command object for the "set-context" action, which -// defines a new Context airshipctl config. +// creates and modifies contexts in the airshipctl config func NewCmdConfigSetContext(rootSettings *environment.AirshipCTLSettings) *cobra.Command { - theContext := &config.ContextOptions{} + o := &config.ContextOptions{} - setcontextcmd := &cobra.Command{ + cmd := &cobra.Command{ Use: "set-context NAME", Short: "Switch to a new context or update context values in the airshipctl config", Long: setContextLong, @@ -58,40 +58,56 @@ func NewCmdConfigSetContext(rootSettings *environment.AirshipCTLSettings) *cobra nFlags := cmd.Flags().NFlag() if nFlags == 0 { // Change the current context to the provided context name if no flags are provided - theContext.CurrentContext = true + o.CurrentContext = true } - theContext.Name = cmd.Flags().Args()[0] - modified, err := config.RunSetContext(theContext, rootSettings.Config(), true) - + o.Name = args[0] + modified, err := config.RunSetContext(o, rootSettings.Config(), true) if err != nil { return err } if modified { - fmt.Fprintf(cmd.OutOrStdout(), "Context %q modified.\n", theContext.Name) + fmt.Fprintf(cmd.OutOrStdout(), "Context %q modified.\n", o.Name) } else { - fmt.Fprintf(cmd.OutOrStdout(), "Context %q created.\n", theContext.Name) + fmt.Fprintf(cmd.OutOrStdout(), "Context %q created.\n", o.Name) } return nil }, } - sctxInitFlags(theContext, setcontextcmd) - return setcontextcmd + addSetContextFlags(o, cmd) + return cmd } -func sctxInitFlags(o *config.ContextOptions, setcontextcmd *cobra.Command) { - setcontextcmd.Flags().StringVar(&o.Cluster, config.FlagClusterName, o.Cluster, - config.FlagClusterName+" for the context entry in airshipctl config") +func addSetContextFlags(o *config.ContextOptions, cmd *cobra.Command) { + flags := cmd.Flags() - setcontextcmd.Flags().StringVar(&o.AuthInfo, config.FlagAuthInfoName, o.AuthInfo, - config.FlagAuthInfoName+" for the context entry in airshipctl config") + flags.StringVar( + &o.Cluster, + config.FlagClusterName, + "", + "sets the "+config.FlagClusterName+" for the specified context in the airshipctl config") - setcontextcmd.Flags().StringVar(&o.Manifest, config.FlagManifest, o.Manifest, - config.FlagManifest+" for the context entry in airshipctl config") + flags.StringVar( + &o.AuthInfo, + config.FlagAuthInfoName, + "", + "sets the "+config.FlagAuthInfoName+" for the specified context in the airshipctl config") - setcontextcmd.Flags().StringVar(&o.Namespace, config.FlagNamespace, o.Namespace, - config.FlagNamespace+" for the context entry in airshipctl config") + flags.StringVar( + &o.Manifest, + config.FlagManifest, + "", + "sets the "+config.FlagManifest+" for the specified context in the airshipctl config") - setcontextcmd.Flags().StringVar(&o.ClusterType, config.FlagClusterType, "", - config.FlagClusterType+" for the context entry in airshipctl config") + flags.StringVar( + &o.Namespace, + config.FlagNamespace, + "", + "sets the "+config.FlagNamespace+" for the specified context in the airshipctl config") + + flags.StringVar( + &o.ClusterType, + config.FlagClusterType, + "", + "sets the "+config.FlagClusterType+" for the specified context in the airshipctl config") } diff --git a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-few-args.golden b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-few-args.golden index 89aa5d481..8d26f0832 100644 --- a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-few-args.golden +++ b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-few-args.golden @@ -11,10 +11,10 @@ airshipctl config set-context e2e --namespace=kube-system --manifest=manifest -- airshipctl config set-context e2e Flags: - --cluster string cluster for the context entry in airshipctl config - --cluster-type string cluster-type for the context entry in airshipctl config + --cluster string sets the cluster for the specified context in the airshipctl config + --cluster-type string sets the cluster-type for the specified context in the airshipctl config -h, --help help for set-context - --manifest string manifest for the context entry in airshipctl config - --namespace string namespace for the context entry in airshipctl config - --user string user for the context entry in airshipctl config + --manifest string sets the manifest for the specified context in the airshipctl config + --namespace string sets the namespace for the specified context in the airshipctl config + --user string sets the user for the specified context in the airshipctl config diff --git a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden index 70662d749..d509a9db6 100644 --- a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden +++ b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden @@ -11,10 +11,10 @@ airshipctl config set-context e2e --namespace=kube-system --manifest=manifest -- airshipctl config set-context e2e Flags: - --cluster string cluster for the context entry in airshipctl config - --cluster-type string cluster-type for the context entry in airshipctl config + --cluster string sets the cluster for the specified context in the airshipctl config + --cluster-type string sets the cluster-type for the specified context in the airshipctl config -h, --help help for set-context - --manifest string manifest for the context entry in airshipctl config - --namespace string namespace for the context entry in airshipctl config - --user string user for the context entry in airshipctl config + --manifest string sets the manifest for the specified context in the airshipctl config + --namespace string sets the namespace for the specified context in the airshipctl config + --user string sets the user for the specified context in the airshipctl config diff --git a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden index 31fb4c19f..edd4b19c7 100644 --- a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden +++ b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden @@ -14,9 +14,9 @@ airshipctl config set-context e2e --namespace=kube-system --manifest=manifest -- airshipctl config set-context e2e Flags: - --cluster string cluster for the context entry in airshipctl config - --cluster-type string cluster-type for the context entry in airshipctl config + --cluster string sets the cluster for the specified context in the airshipctl config + --cluster-type string sets the cluster-type for the specified context in the airshipctl config -h, --help help for set-context - --manifest string manifest for the context entry in airshipctl config - --namespace string namespace for the context entry in airshipctl config - --user string user for the context entry in airshipctl config + --manifest string sets the manifest for the specified context in the airshipctl config + --namespace string sets the namespace for the specified context in the airshipctl config + --user string sets the user for the specified context in the airshipctl config diff --git a/cmd/config/testdata/TestGetContextCmdGoldenOutput/missing.golden b/cmd/config/testdata/TestGetContextCmdGoldenOutput/missing.golden index 8a7ba8c89..694d3aa8d 100644 --- a/cmd/config/testdata/TestGetContextCmdGoldenOutput/missing.golden +++ b/cmd/config/testdata/TestGetContextCmdGoldenOutput/missing.golden @@ -13,6 +13,6 @@ airshipctl config get-context --current-context airshipctl config get-context e2e Flags: - --current-context current-context to retrieve the current context entry in airshipctl config + --current-context retrieve the current context entry in airshipctl config -h, --help help for get-context diff --git a/cmd/document/render.go b/cmd/document/render.go index 87a1d621b..cbc23fa15 100644 --- a/cmd/document/render.go +++ b/cmd/document/render.go @@ -8,16 +8,6 @@ import ( "opendev.org/airship/airshipctl/pkg/errors" ) -// InitFlags add flags for document render sub-command -func initRenderFlags(settings *render.Settings, cmd *cobra.Command) { - flags := cmd.Flags() - flags.StringArrayVarP(&settings.Label, "label", "l", nil, "Filter documents by Labels") - flags.StringArrayVarP(&settings.Annotation, "annotation", "a", nil, "Filter documents by Annotations") - flags.StringArrayVarP(&settings.GroupVersion, "apiversion", "g", nil, "Filter documents by API version") - flags.StringArrayVarP(&settings.Kind, "kind", "k", nil, "Filter documents by Kinds") - flags.StringVarP(&settings.RawFilter, "filter", "f", "", "Logical expression for document filtering") -} - // NewRenderCommand create a new command for document rendering func NewRenderCommand(rootSettings *environment.AirshipCTLSettings) *cobra.Command { renderSettings := &render.Settings{AirshipCTLSettings: rootSettings} @@ -29,6 +19,46 @@ func NewRenderCommand(rootSettings *environment.AirshipCTLSettings) *cobra.Comma }, } - initRenderFlags(renderSettings, renderCmd) + addRenderFlags(renderSettings, renderCmd) return renderCmd } + +// addRenderFlags adds flags for document render sub-command +func addRenderFlags(settings *render.Settings, cmd *cobra.Command) { + flags := cmd.Flags() + + flags.StringArrayVarP( + &settings.Label, + "label", + "l", + nil, + "Filter documents by Labels") + + flags.StringArrayVarP( + &settings.Annotation, + "annotation", + "a", + nil, + "Filter documents by Annotations") + + flags.StringArrayVarP( + &settings.GroupVersion, + "apiversion", + "g", + nil, + "Filter documents by API version") + + flags.StringArrayVarP( + &settings.Kind, + "kind", + "k", + nil, + "Filter documents by Kinds") + + flags.StringVarP( + &settings.RawFilter, + "filter", + "f", + "", + "Logical expression for document filtering") +} diff --git a/pkg/config/cmds.go b/pkg/config/cmds.go index 26c1e0ec8..3f880a47e 100644 --- a/pkg/config/cmds.go +++ b/pkg/config/cmds.go @@ -20,76 +20,9 @@ import ( "errors" "fmt" "io" - "io/ioutil" ) -// Validate that the arguments are correct -func (o *ClusterOptions) Validate() error { - if len(o.Name) == 0 { - return errors.New("you must specify a non-empty cluster name") - } - err := ValidClusterType(o.ClusterType) - if err != nil { - return err - } - if o.InsecureSkipTLSVerify && o.CertificateAuthority != "" { - return fmt.Errorf("you cannot specify a %s and %s mode at the same time", FlagCAFile, FlagInsecure) - } - - if !o.EmbedCAData { - return nil - } - caPath := o.CertificateAuthority - if caPath == "" { - return fmt.Errorf("you must specify a --%s to embed", FlagCAFile) - } - if _, err := ioutil.ReadFile(caPath); err != nil { - return fmt.Errorf("could not read %s data from %s: %v", FlagCAFile, caPath, err) - } - return nil -} - -func (o *ContextOptions) Validate() error { - if len(o.Name) == 0 { - return errors.New("you must specify a non-empty context name") - } - // Expect ClusterType only when this is not setting currentContext - if o.ClusterType != "" { - err := ValidClusterType(o.ClusterType) - if err != nil { - return err - } - } - // TODO Manifest, Cluster could be validated against the existing config maps - return nil -} - -func (o *AuthInfoOptions) Validate() error { - if len(o.Token) > 0 && (len(o.Username) > 0 || len(o.Password) > 0) { - return fmt.Errorf("you cannot specify more than one authentication method at the same time: --%v or --%v/--%v", - FlagBearerToken, FlagUsername, FlagPassword) - } - if !o.EmbedCertData { - return nil - } - certPath := o.ClientCertificate - if certPath == "" { - return fmt.Errorf("you must specify a --%s to embed", FlagCertFile) - } - if _, err := ioutil.ReadFile(certPath); err != nil { - return fmt.Errorf("error reading %s data from %s: %v", FlagCertFile, certPath, err) - } - keyPath := o.ClientKey - if keyPath == "" { - return fmt.Errorf("you must specify a --%s to embed", FlagKeyFile) - } - if _, err := ioutil.ReadFile(keyPath); err != nil { - return fmt.Errorf("error reading %s data from %s: %v", FlagKeyFile, keyPath, err) - } - return nil -} - -// runGetAuthInfo performs the execution of 'config get-credentials' sub command +// RunGetAuthInfo performs the execution of 'config get-credentials' sub command func RunGetAuthInfo(o *AuthInfoOptions, out io.Writer, airconfig *Config) error { if o.Name == "" { getAuthInfos(out, airconfig) @@ -117,7 +50,7 @@ func getAuthInfos(out io.Writer, airconfig *Config) { } } -// runGetCluster performs the execution of 'config get-cluster' sub command +// RunGetCluster performs the execution of 'config get-cluster' sub command func RunGetCluster(o *ClusterOptions, out io.Writer, airconfig *Config) error { if o.Name == "" { getClusters(out, airconfig) @@ -126,8 +59,7 @@ func RunGetCluster(o *ClusterOptions, out io.Writer, airconfig *Config) error { return getCluster(o.Name, o.ClusterType, out, airconfig) } -func getCluster(cName, cType string, - out io.Writer, airconfig *Config) error { +func getCluster(cName, cType string, out io.Writer, airconfig *Config) error { cluster, err := airconfig.GetCluster(cName, cType) if err != nil { return err @@ -147,7 +79,7 @@ func getClusters(out io.Writer, airconfig *Config) { } } -// runGetContext performs the execution of 'config get-Context' sub command +// RunGetContext performs the execution of 'config get-Context' sub command func RunGetContext(o *ContextOptions, out io.Writer, airconfig *Config) error { if o.Name == "" && !o.CurrentContext { getContexts(out, airconfig) @@ -157,11 +89,10 @@ func RunGetContext(o *ContextOptions, out io.Writer, airconfig *Config) error { } func getContext(o *ContextOptions, out io.Writer, airconfig *Config) error { - cName := o.Name if o.CurrentContext { - cName = airconfig.CurrentContext + o.Name = airconfig.CurrentContext } - context, err := airconfig.GetContext(cName) + context, err := airconfig.GetContext(o.Name) if err != nil { return err } @@ -180,19 +111,18 @@ func getContexts(out io.Writer, airconfig *Config) { } func RunSetAuthInfo(o *AuthInfoOptions, airconfig *Config, writeToStorage bool) (bool, error) { - authinfoWasModified := false + modified := false err := o.Validate() if err != nil { - return authinfoWasModified, err + return modified, err } - authinfoIWant := o.Name - authinfo, err := airconfig.GetAuthInfo(authinfoIWant) + authinfo, err := airconfig.GetAuthInfo(o.Name) if err != nil { var cerr ErrMissingConfig if !errors.As(err, &cerr) { // An error occurred, but it wasn't a "missing" config error. - return authinfoWasModified, err + return modified, err } // authinfo didn't exist, create it @@ -201,24 +131,24 @@ func RunSetAuthInfo(o *AuthInfoOptions, airconfig *Config, writeToStorage bool) } else { // AuthInfo exists, lets update airconfig.ModifyAuthInfo(authinfo, o) - authinfoWasModified = true + modified = true } // Update configuration file just in time persistence approach if writeToStorage { if err := airconfig.PersistConfig(); err != nil { // Error that it didnt persist the changes - return authinfoWasModified, ErrConfigFailed{} + return modified, ErrConfigFailed{} } } - return authinfoWasModified, nil + return modified, nil } func RunSetCluster(o *ClusterOptions, airconfig *Config, writeToStorage bool) (bool, error) { - clusterWasModified := false + modified := false err := o.Validate() if err != nil { - return clusterWasModified, err + return modified, err } cluster, err := airconfig.GetCluster(o.Name, o.ClusterType) @@ -226,22 +156,22 @@ func RunSetCluster(o *ClusterOptions, airconfig *Config, writeToStorage bool) (b var cerr ErrMissingConfig if !errors.As(err, &cerr) { // An error occurred, but it wasn't a "missing" config error. - return clusterWasModified, err + return modified, err } // Cluster didn't exist, create it _, err := airconfig.AddCluster(o) if err != nil { - return clusterWasModified, err + return modified, err } - clusterWasModified = false + modified = false } else { // Cluster exists, lets update _, err := airconfig.ModifyCluster(cluster, o) if err != nil { - return clusterWasModified, err + return modified, err } - clusterWasModified = true + modified = true } // Update configuration file @@ -251,31 +181,30 @@ func RunSetCluster(o *ClusterOptions, airconfig *Config, writeToStorage bool) (b // Some warning here , that it didnt persist the changes because of this // Or should we float this up // What would it mean? No value. - return clusterWasModified, err + return modified, err } } - return clusterWasModified, nil + return modified, nil } func RunSetContext(o *ContextOptions, airconfig *Config, writeToStorage bool) (bool, error) { - contextWasModified := false + modified := false err := o.Validate() if err != nil { - return contextWasModified, err + return modified, err } - contextIWant := o.Name - context, err := airconfig.GetContext(contextIWant) + context, err := airconfig.GetContext(o.Name) if err != nil { var cerr ErrMissingConfig if !errors.As(err, &cerr) { // An error occurred, but it wasn't a "missing" config error. - return contextWasModified, err + return modified, err } if o.CurrentContext { - return contextWasModified, ErrMissingConfig{} + return modified, ErrMissingConfig{} } // context didn't exist, create it // ignoring the returned added context @@ -289,15 +218,15 @@ func RunSetContext(o *ContextOptions, airconfig *Config, writeToStorage bool) (b // Context exists, lets update airconfig.ModifyContext(context, o) } - contextWasModified = true + modified = true } // Update configuration file just in time persistence approach if writeToStorage { if err := airconfig.PersistConfig(); err != nil { // Error that it didnt persist the changes - return contextWasModified, ErrConfigFailed{} + return modified, ErrConfigFailed{} } } - return contextWasModified, nil + return modified, nil } diff --git a/pkg/config/cmds_test.go b/pkg/config/cmds_test.go index 6feabe449..84a714b01 100644 --- a/pkg/config/cmds_test.go +++ b/pkg/config/cmds_test.go @@ -23,65 +23,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestValidateCluster(t *testing.T) { - co := DummyClusterOptions() - - // Assert that the initial dummy config is valid - err := co.Validate() - assert.NoError(t, err) - - // Validate with Embedded Data - // Empty CA - co.EmbedCAData = true - co.CertificateAuthority = "" - err = co.Validate() - assert.Error(t, err) - - // Lets add a CA - co.CertificateAuthority = "testdata/ca.crt" - err = co.Validate() - assert.NoError(t, err) - - // Lets add a CA but garbage - co.CertificateAuthority = "garbage" - err = co.Validate() - assert.Error(t, err) - - // Lets change the Insecure mode - co.InsecureSkipTLSVerify = true - err = co.Validate() - assert.Error(t, err) - - // Invalid Cluster Type - co.ClusterType = "Invalid" - err = co.Validate() - assert.Error(t, err) - - // Empty Cluster Name case - co.Name = "" - err = co.Validate() - assert.Error(t, err) -} - -func TestValidateContext(t *testing.T) { - co := DummyContextOptions() - // Valid Data case - err := co.Validate() - assert.NoError(t, err) -} - -func TestValidateAuthInfo(t *testing.T) { - co := DummyAuthInfoOptions() - // Token and cert error case - err := co.Validate() - assert.Error(t, err) - - // Valid Data case - co.Token = "" - err = co.Validate() - assert.NoError(t, err) -} - func TestRunGetAuthInfo(t *testing.T) { t.Run("testNonExistentAuthInfo", func(t *testing.T) { conf := DummyConfig() diff --git a/pkg/config/cmds_types.go b/pkg/config/cmds_types.go deleted file mode 100644 index 367a60ef7..000000000 --- a/pkg/config/cmds_types.go +++ /dev/null @@ -1,30 +0,0 @@ -package config - -type ClusterOptions struct { - Name string - ClusterType string - Server string - InsecureSkipTLSVerify bool - CertificateAuthority string - EmbedCAData bool -} - -type ContextOptions struct { - Name string - ClusterType string - CurrentContext bool - Cluster string - AuthInfo string - Manifest string - Namespace string -} - -type AuthInfoOptions struct { - Name string - ClientCertificate string - ClientKey string - Token string - Username string - Password string - EmbedCertData bool -} diff --git a/pkg/config/config.go b/pkg/config/config.go index 4f3bc6db4..7c0c04164 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -971,11 +971,13 @@ func (c *ClusterComplexName) SetDefaultType() { func (c *ClusterComplexName) String() string { return fmt.Sprintf("clusterName:%s, clusterType:%s", c.clusterName, c.clusterType) } -func ValidClusterType(ctype string) error { - if ctype == Ephemeral || ctype == Target { - return nil +func ValidClusterType(clusterType string) error { + for _, validType := range AllClusterTypes { + if clusterType == validType { + return nil + } } - return errors.New("Cluster Type must be specified. Valid values are :" + Ephemeral + " or " + Target + ".") + return fmt.Errorf("Cluster Type must be one of %v", AllClusterTypes) } /* ______________________________ diff --git a/pkg/config/options.go b/pkg/config/options.go new file mode 100644 index 000000000..576de94c9 --- /dev/null +++ b/pkg/config/options.go @@ -0,0 +1,131 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +// TODO(howell): Switch to strongly-typed errors + +import ( + "errors" + "fmt" + "os" +) + +type AuthInfoOptions struct { + Name string + ClientCertificate string + ClientKey string + Token string + Username string + Password string + EmbedCertData bool +} + +type ContextOptions struct { + Name string + ClusterType string + CurrentContext bool + Cluster string + AuthInfo string + Manifest string + Namespace string +} + +type ClusterOptions struct { + Name string + ClusterType string + Server string + InsecureSkipTLSVerify bool + CertificateAuthority string + EmbedCAData bool +} + +func (o *AuthInfoOptions) Validate() error { + if o.Token != "" && (o.Username != "" || o.Password != "") { + return fmt.Errorf("you cannot specify more than one authentication method at the same time: --%v or --%v/--%v", + FlagBearerToken, FlagUsername, FlagPassword) + } + + if !o.EmbedCertData { + return nil + } + + if err := checkExists(FlagCertFile, o.ClientCertificate); err != nil { + return err + } + + if err := checkExists(FlagKeyFile, o.ClientKey); err != nil { + return err + } + + return nil +} + +func (o *ContextOptions) Validate() error { + if o.Name == "" { + return errors.New("you must specify a non-empty context name") + } + + // If the user simply wants to change the current context, no further validation is needed + if o.CurrentContext { + return nil + } + + // If the cluster-type was specified, verify that it's valid + if o.ClusterType != "" { + if err := ValidClusterType(o.ClusterType); err != nil { + return err + } + } + + // TODO Manifest, Cluster could be validated against the existing config maps + return nil +} + +func (o *ClusterOptions) Validate() error { + if o.Name == "" { + return errors.New("you must specify a non-empty cluster name") + } + + err := ValidClusterType(o.ClusterType) + if err != nil { + return err + } + + if o.InsecureSkipTLSVerify && o.CertificateAuthority != "" { + return fmt.Errorf("you cannot specify a %s and %s mode at the same time", FlagCAFile, FlagInsecure) + } + + if !o.EmbedCAData { + return nil + } + + if err := checkExists(FlagCAFile, o.CertificateAuthority); err != nil { + return err + } + + return nil +} + +func checkExists(flagName, path string) error { + if path == "" { + return fmt.Errorf("you must specify a --%s to embed", flagName) + } + if _, err := os.Stat(path); err != nil { + return fmt.Errorf("could not read %s data from '%s': %v", flagName, path, err) + } + return nil +} diff --git a/pkg/config/options_test.go b/pkg/config/options_test.go new file mode 100644 index 000000000..932b37008 --- /dev/null +++ b/pkg/config/options_test.go @@ -0,0 +1,244 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config_test + +import ( + "io/ioutil" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "opendev.org/airship/airshipctl/pkg/config" +) + +func TestAuthInfoOptionsValidate(t *testing.T) { + aRealFile, err := ioutil.TempFile("", "a-real-file") + require.NoError(t, err) + + aRealFilename := aRealFile.Name() + defer os.Remove(aRealFilename) + + tests := []struct { + name string + testOptions config.AuthInfoOptions + expectError bool + }{ + { + name: "TokenAndUserPass", + testOptions: config.AuthInfoOptions{ + Token: "testToken", + Username: "testUser", + Password: "testPassword", + }, + expectError: true, + }, + { + name: "DontEmbed", + testOptions: config.AuthInfoOptions{ + EmbedCertData: false, + }, + expectError: false, + }, + { + name: "EmbedWithoutCert", + testOptions: config.AuthInfoOptions{ + EmbedCertData: true, + }, + expectError: true, + }, + { + name: "EmbedWithoutClientKey", + testOptions: config.AuthInfoOptions{ + EmbedCertData: true, + ClientCertificate: aRealFilename, + }, + expectError: true, + }, + { + name: "EmbedWithCertAndClientKey", + testOptions: config.AuthInfoOptions{ + EmbedCertData: true, + ClientCertificate: aRealFilename, + ClientKey: aRealFilename, + }, + expectError: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(subTest *testing.T) { + err := tt.testOptions.Validate() + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestContextOptionsValidate(t *testing.T) { + tests := []struct { + name string + testOptions config.ContextOptions + expectError bool + }{ + { + name: "MissingName", + testOptions: config.ContextOptions{ + Name: "", + }, + expectError: true, + }, + { + name: "InvalidClusterType", + testOptions: config.ContextOptions{ + Name: "testContext", + ClusterType: "badType", + }, + expectError: true, + }, + { + name: "SettingCurrentContext", + testOptions: config.ContextOptions{ + Name: "testContext", + CurrentContext: true, + }, + expectError: false, + }, + { + name: "NoClusterType", + testOptions: config.ContextOptions{ + Name: "testContext", + }, + expectError: false, + }, + { + name: "ValidClusterType", + testOptions: config.ContextOptions{ + Name: "testContext", + ClusterType: "target", + }, + expectError: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(subTest *testing.T) { + err := tt.testOptions.Validate() + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestClusterOptionsValidate(t *testing.T) { + aRealfile, err := ioutil.TempFile("", "a-real-file") + require.NoError(t, err) + + aRealFilename := aRealfile.Name() + defer os.Remove(aRealFilename) + + tests := []struct { + name string + testOptions config.ClusterOptions + expectError bool + }{ + { + name: "MissingName", + testOptions: config.ClusterOptions{ + Name: "", + }, + expectError: true, + }, + { + name: "InvalidClusterType", + testOptions: config.ClusterOptions{ + Name: "testCluster", + ClusterType: "badType", + }, + expectError: true, + }, + { + name: "InsecureSkipTLSVerifyAndCertificateAuthority", + testOptions: config.ClusterOptions{ + Name: "testCluster", + ClusterType: "target", + InsecureSkipTLSVerify: true, + CertificateAuthority: "cert_file", + }, + expectError: true, + }, + { + name: "DontEmbed", + testOptions: config.ClusterOptions{ + Name: "testCluster", + ClusterType: "target", + EmbedCAData: false, + }, + expectError: false, + }, + { + name: "EmbedWithoutCA", + testOptions: config.ClusterOptions{ + Name: "testCluster", + ClusterType: "target", + EmbedCAData: true, + }, + expectError: true, + }, + { + name: "EmbedWithFaultyCA", + testOptions: config.ClusterOptions{ + Name: "testCluster", + ClusterType: "target", + EmbedCAData: true, + CertificateAuthority: "not-a-real-file", + }, + expectError: true, + }, + { + name: "EmbedWithGoodCA", + testOptions: config.ClusterOptions{ + Name: "testCluster", + ClusterType: "target", + EmbedCAData: true, + CertificateAuthority: aRealFilename, + }, + expectError: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(subTest *testing.T) { + err := tt.testOptions.Validate() + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +}