set default for flag --skip-tls=false

Ankita Thomas · Ankita Thomas · commit 36ce1c63fdca · 2020-08-18T10:08:19.000-04:00
diff --git a/cmd/opm/index/add.go b/cmd/opm/index/add.go
@@ -53,7 +53,6 @@ func addIndexAddCmd(parent *cobra.Command) {
 	if err := indexCmd.MarkFlagRequired("bundles"); err != nil {
 		logrus.Panic("Failed to set required `bundles` flag for `index add`")
 	}
-	indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling bundles or index")
 	indexCmd.Flags().StringP("binary-image", "i", "", "container image for on-image `opm` command")
 	indexCmd.Flags().StringP("container-tool", "c", "", "tool to interact with container images (save, build, etc.). One of: [docker, podman]")
 	indexCmd.Flags().StringP("build-tool", "u", "", "tool to build container images. One of: [docker, podman]. Defaults to podman. Overrides part of container-tool.")
@@ -108,13 +107,9 @@ func runIndexAddCmdFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 
 	mode, err := cmd.Flags().GetString("mode")
diff --git a/cmd/opm/index/cmd.go b/cmd/opm/index/cmd.go
@@ -18,9 +18,15 @@ func AddCommand(parent *cobra.Command) {
 			}
 			return nil
 		},
+		PersistentPreRun: func(cmd *cobra.Command, args []string) {
+			if skipTLS, err := cmd.Flags().GetBool("skip-tls"); err == nil && skipTLS {
+				logrus.Warn("--skip-tls flag is set: this mode is insecure and meant for development purposes only.")
+			}
+		},
 	}
 
 	parent.AddCommand(cmd)
+	parent.PersistentFlags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling bundles or index")
 	cmd.AddCommand(newIndexDeleteCmd())
 	addIndexAddCmd(cmd)
 	cmd.AddCommand(newIndexExportCmd())
diff --git a/cmd/opm/index/delete.go b/cmd/opm/index/delete.go
@@ -41,7 +41,6 @@ func newIndexDeleteCmd() *cobra.Command {
 	indexCmd.Flags().StringP("pull-tool", "p", "", "tool to pull container images. One of: [none, docker, podman]. Defaults to none. Overrides part of container-tool.")
 	indexCmd.Flags().StringP("tag", "t", "", "custom tag for container image being built")
 	indexCmd.Flags().Bool("permissive", false, "allow registry load errors")
-	indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling index")
 
 	if err := indexCmd.Flags().MarkHidden("debug"); err != nil {
 		logrus.Panic(err.Error())
@@ -92,13 +91,9 @@ func runIndexDeleteCmdFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 
 	logger := logrus.WithFields(logrus.Fields{"operators": operators})
diff --git a/cmd/opm/index/deprecate.go b/cmd/opm/index/deprecate.go
@@ -57,7 +57,6 @@ func newIndexDeprecateTruncateCmd() *cobra.Command {
 	indexCmd.Flags().StringP("pull-tool", "p", "", "tool to pull container images. One of: [none, docker, podman]. Defaults to none. Overrides part of container-tool.")
 	indexCmd.Flags().StringP("tag", "t", "", "custom tag for container image being built")
 	indexCmd.Flags().Bool("permissive", false, "allow registry load errors")
-	indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling index")
 	if err := indexCmd.Flags().MarkHidden("debug"); err != nil {
 		logrus.Panic(err.Error())
 	}
@@ -106,13 +105,9 @@ func runIndexDeprecateTruncateCmdFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 
 	logger := logrus.WithFields(logrus.Fields{"bundles": bundles})
diff --git a/cmd/opm/index/export.go b/cmd/opm/index/export.go
@@ -46,7 +46,6 @@ func newIndexExportCmd() *cobra.Command {
 	}
 	indexCmd.Flags().StringP("download-folder", "f", "downloaded", "directory where downloaded operator bundle(s) will be stored")
 	indexCmd.Flags().StringP("container-tool", "c", "none", "tool to interact with container images (save, build, etc.). One of: [none, docker, podman]")
-	indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling index")
 	if err := indexCmd.Flags().MarkHidden("debug"); err != nil {
 		logrus.Panic(err.Error())
 	}
@@ -76,13 +75,9 @@ func runIndexExportCmdFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 
 	logger := logrus.WithFields(logrus.Fields{"index": index, "package": packageName})
@@ -96,7 +91,7 @@ func runIndexExportCmdFunc(cmd *cobra.Command, args []string) error {
 		Package:       packageName,
 		DownloadPath:  downloadPath,
 		ContainerTool: containertools.NewContainerTool(containerTool, containertools.NoneTool),
-		SkipTLS:           skipTLS,
+		SkipTLS:       skipTLS,
 	}
 
 	err = indexExporter.ExportFromIndex(request)
diff --git a/cmd/opm/index/prune.go b/cmd/opm/index/prune.go
@@ -41,7 +41,6 @@ func newIndexPruneCmd() *cobra.Command {
 	indexCmd.Flags().StringP("container-tool", "c", "podman", "tool to interact with container images (save, build, etc.). One of: [docker, podman]")
 	indexCmd.Flags().StringP("tag", "t", "", "custom tag for container image being built")
 	indexCmd.Flags().Bool("permissive", false, "allow registry load errors")
-	indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling index")
 
 	if err := indexCmd.Flags().MarkHidden("debug"); err != nil {
 		logrus.Panic(err.Error())
@@ -96,13 +95,9 @@ func runIndexPruneCmdFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 
 	logger := logrus.WithFields(logrus.Fields{"packages": packages})
diff --git a/cmd/opm/index/prunestranded.go b/cmd/opm/index/prunestranded.go
@@ -36,7 +36,6 @@ func newIndexPruneStrandedCmd() *cobra.Command {
 	indexCmd.Flags().StringP("binary-image", "i", "", "container image for on-image `opm` command")
 	indexCmd.Flags().StringP("container-tool", "c", "podman", "tool to interact with container images (save, build, etc.). One of: [docker, podman]")
 	indexCmd.Flags().StringP("tag", "t", "", "custom tag for container image being built")
-	indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling index")
 
 	if err := indexCmd.Flags().MarkHidden("debug"); err != nil {
 		logrus.Panic(err.Error())
@@ -81,13 +80,9 @@ func runIndexPruneStrandedCmdFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 
 	logger := logrus.WithFields(logrus.Fields{})
diff --git a/cmd/opm/registry/add.go b/cmd/opm/registry/add.go
@@ -41,13 +41,9 @@ func addFunc(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	var skipTLS *bool
-	if cmd.Flags().Changed("skip-tls") {
-		skipTLSVal, err := cmd.Flags().GetBool("skip-tls")
-		if err != nil {
-			return err
-		}
-		skipTLS = &skipTLSVal
+	skipTLS, err := cmd.Flags().GetBool("skip-tls")
+	if err != nil {
+		return err
 	}
 	fromFilename, err := cmd.Flags().GetString("database")
 	if err != nil {
@@ -81,6 +77,10 @@ func addFunc(cmd *cobra.Command, args []string) error {
 
 	logger := logrus.WithFields(logrus.Fields{"bundles": bundleImages})
 
+	if skipTLS {
+		logger.Warn("--skip-tls flag is set: this mode is insecure and meant for development purposes only.")
+	}
+
 	logger.Info("adding to the registry")
 
 	registryAdder := registry.NewRegistryAdder(logger)
diff --git a/pkg/containertools/factory_docker.go b/pkg/containertools/factory_docker.go
@@ -22,10 +22,6 @@ func (d *DockerCommandFactory) BuildCommand(o BuildOptions) (*exec.Cmd, error) {
 		args = append(args, "-t", tag)
 	}
 
-	if o.secure {
-		args = append(args, "--tls")
-	}
-
 	if o.context == "" {
 		return nil, fmt.Errorf("context not provided")
 	}
diff --git a/pkg/containertools/factory_podman.go b/pkg/containertools/factory_podman.go
@@ -24,10 +24,6 @@ func (p *PodmanCommandFactory) BuildCommand(o BuildOptions) (*exec.Cmd, error) {
 		args = append(args, "-t", tag)
 	}
 
-	if !o.secure {
-		args = append(args, "--tls-verify=false")
-	}
-
 	if o.context == "" {
 		return nil, fmt.Errorf("context not provided")
 	}
diff --git a/pkg/containertools/factory_test.go b/pkg/containertools/factory_test.go
@@ -77,7 +77,7 @@ func TestBuildCommand(t *testing.T) {
 			Factory: &PodmanCommandFactory{},
 			Options: DefaultBuildOptions(),
 			Args: []string{
-				"podman", "build", "--format", "docker", "--tls-verify=false", ".",
+				"podman", "build", "--format", "docker", ".",
 			},
 		},
 		{
@@ -88,7 +88,7 @@ func TestBuildCommand(t *testing.T) {
 				format:  "oci",
 			},
 			Args: []string{
-				"podman", "build", "--format", "oci", "--tls-verify=false", ".",
+				"podman", "build", "--format", "oci", ".",
 			},
 		},
 		{
@@ -98,7 +98,7 @@ func TestBuildCommand(t *testing.T) {
 				context: "foo",
 			},
 			Args: []string{
-				"podman", "build", "--format", "docker", "--tls-verify=false", "foo",
+				"podman", "build", "--format", "docker", "foo",
 			},
 		},
 		{
@@ -109,7 +109,7 @@ func TestBuildCommand(t *testing.T) {
 				dockerfile: "foo",
 			},
 			Args: []string{
-				"podman", "build", "--format", "docker", "-f", "foo", "--tls-verify=false", ".",
+				"podman", "build", "--format", "docker", "-f", "foo", ".",
 			},
 		},
 		{
@@ -120,7 +120,7 @@ func TestBuildCommand(t *testing.T) {
 				tags:    []string{"foo"},
 			},
 			Args: []string{
-				"podman", "build", "--format", "docker", "-t", "foo", "--tls-verify=false", ".",
+				"podman", "build", "--format", "docker", "-t", "foo", ".",
 			},
 		},
 		{
@@ -131,7 +131,7 @@ func TestBuildCommand(t *testing.T) {
 				tags:    []string{"foo", "bar"},
 			},
 			Args: []string{
-				"podman", "build", "--format", "docker", "-t", "foo", "-t", "bar", "--tls-verify=false", ".",
+				"podman", "build", "--format", "docker", "-t", "foo", "-t", "bar", ".",
 			},
 		},
 		{
diff --git a/pkg/containertools/option_build.go b/pkg/containertools/option_build.go
@@ -5,7 +5,6 @@ type BuildOptions struct {
 	tags       []string
 	dockerfile string
 	context    string
-	secure    bool
 }
 
 func (o *BuildOptions) SetFormatDocker() {
@@ -28,10 +27,6 @@ func (o *BuildOptions) SetContext(context string) {
 	o.context = context
 }
 
-func (o *BuildOptions) SetSkipTLS(skipTLS bool) {
-	o.secure = !skipTLS
-}
-
 func DefaultBuildOptions() BuildOptions {
 	var o BuildOptions
 	o.SetFormatDocker()
diff --git a/pkg/containertools/runner.go b/pkg/containertools/runner.go
@@ -31,11 +31,9 @@ type RunnerConfig struct {
 
 type RunnerOption func(config *RunnerConfig)
 
-func SkipTLS(skip *bool) RunnerOption {
+func SkipTLS(skip bool) RunnerOption {
 	return func(config *RunnerConfig) {
-		if skip != nil {
-			config.SkipTLS = *skip
-		}
+		config.SkipTLS = skip
 	}
 }
 
@@ -45,52 +43,32 @@ func (r *RunnerConfig) apply(options []RunnerOption) {
 	}
 }
 
-func (r *ContainerCommandRunner) argsForCmd(cmd string, args... string) []string {
+func (r *ContainerCommandRunner) argsForCmd(cmd string, args ...string) []string {
 	cmdArgs := []string{cmd}
 	switch r.containerTool {
 	case PodmanTool:
 		switch cmd {
-		case "build", "pull", "push", "login", "search":
+		case "pull", "push", "login", "search":
 			// --tls-verify is a valid flag for these podman subcommands
 			if r.config.SkipTLS {
 				cmdArgs = append(cmdArgs, "--tls-verify=false")
 			}
 		}
-	case DockerTool:
-		if !r.config.SkipTLS {
-			cmdArgs = append(cmdArgs, "--tls")
-		}
 	default:
 	}
 	cmdArgs = append(cmdArgs, args...)
 	return cmdArgs
 }
 
-func defaultConfig(toolName string) *RunnerConfig {
-	switch toolName {
-	case "docker":
-		// docker disables tls verify by default, mimic that behavior
-		return &RunnerConfig{
-			SkipTLS: true,
-		}
-	case "podman":
-		return &RunnerConfig{
-			SkipTLS: false,
-		}
-	default:
-		return &RunnerConfig{}
-	}
-}
-
 // NewCommandRunner takes the containerTool as an input string and returns a
 // CommandRunner to run commands with that cli tool
-func NewCommandRunner(containerTool ContainerTool, logger *logrus.Entry, opts... RunnerOption) *ContainerCommandRunner {
-	config := defaultConfig(containerTool.String())
+func NewCommandRunner(containerTool ContainerTool, logger *logrus.Entry, opts ...RunnerOption) *ContainerCommandRunner {
+	var config RunnerConfig
 	config.apply(opts)
 	r := &ContainerCommandRunner{
 		logger:        logger,
 		containerTool: containerTool,
-		config:        config,
+		config:        &config,
 	}
 	return r
 }
@@ -126,7 +104,6 @@ func (r *ContainerCommandRunner) Build(dockerfile, tag string) error {
 	}
 	o.SetDockerfile(dockerfile)
 	o.SetContext(".")
-	o.SetSkipTLS(r.config.SkipTLS)
 	command, err := r.containerTool.CommandFactory().BuildCommand(o)
 	if err != nil {
 		return fmt.Errorf("unable to perform build: %v", err)
@@ -160,7 +137,7 @@ func (r *ContainerCommandRunner) Unpack(image, src, dst string) error {
 	}
 
 	id := strings.TrimSuffix(string(out), "\n")
-	args = r.argsForCmd("cp", id + ":" + src, dst)
+	args = r.argsForCmd("cp", id+":"+src, dst)
 	command = exec.Command(r.containerTool.String(), args...)
 
 	r.logger.Infof("running %s cp", r.containerTool)
diff --git a/pkg/image/execregistry/registry.go b/pkg/image/execregistry/registry.go
@@ -26,7 +26,7 @@ type Registry struct {
 var _ image.Registry = &Registry{}
 
 // NewRegistry instantiates and returns a new registry which manipulates images via exec podman/docker commands.
-func NewRegistry(tool containertools.ContainerTool, logger *logrus.Entry, opts... containertools.RunnerOption) (registry *Registry, err error) {
+func NewRegistry(tool containertools.ContainerTool, logger *logrus.Entry, opts ...containertools.RunnerOption) (registry *Registry, err error) {
 	return &Registry{
 		log: logger,
 		cmd: containertools.NewCommandRunner(tool, logger, opts...),
diff --git a/pkg/lib/certs/certs.go b/pkg/lib/certs/certs.go
diff --git a/pkg/lib/indexer/indexer.go b/pkg/lib/indexer/indexer.go
diff --git a/pkg/lib/registry/registry.go b/pkg/lib/registry/registry.go
diff --git a/tools.go b/tools.go

Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,6 @@ func newIndexExportCmd() *cobra.Command {`
`46`	`46`	`}`
`47`	`47`	`indexCmd.Flags().StringP("download-folder", "f", "downloaded", "directory where downloaded operator bundle(s) will be stored")`
`48`	`48`	`indexCmd.Flags().StringP("container-tool", "c", "none", "tool to interact with container images (save, build, etc.). One of: [none, docker, podman]")`
`49`		`- indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling index")`
`50`	`49`	`if err := indexCmd.Flags().MarkHidden("debug"); err != nil {`
`51`	`50`	`logrus.Panic(err.Error())`
`52`	`51`	`}`
`@@ -76,13 +75,9 @@ func runIndexExportCmdFunc(cmd *cobra.Command, args []string) error {`
`76`	`75`	`return err`
`77`	`76`	`}`
`78`	`77`
`79`		`- var skipTLS *bool`
`80`		`- if cmd.Flags().Changed("skip-tls") {`
`81`		`- skipTLSVal, err := cmd.Flags().GetBool("skip-tls")`
`82`		`- if err != nil {`
`83`		`- return err`
`84`		`- }`
`85`		`- skipTLS = &skipTLSVal`
	`78`	`+ skipTLS, err := cmd.Flags().GetBool("skip-tls")`
	`79`	`+ if err != nil {`
	`80`	`+ return err`
`86`	`81`	`}`
`87`	`82`
`88`	`83`	`logger := logrus.WithFields(logrus.Fields{"index": index, "package": packageName})`
`@@ -96,7 +91,7 @@ func runIndexExportCmdFunc(cmd *cobra.Command, args []string) error {`
`96`	`91`	`Package: packageName,`
`97`	`92`	`DownloadPath: downloadPath,`
`98`	`93`	`ContainerTool: containertools.NewContainerTool(containerTool, containertools.NoneTool),`
`99`		`- SkipTLS: skipTLS,`
	`94`	`+ SkipTLS: skipTLS,`
`100`	`95`	`}`
`101`	`96`
`102`	`97`	`err = indexExporter.ExportFromIndex(request)`
Original file line number	Diff line number	Diff line change
`@@ -22,10 +22,6 @@ func (d DockerCommandFactory) BuildCommand(o BuildOptions) (exec.Cmd, error) {`
`22`	`22`	`args = append(args, "-t", tag)`
`23`	`23`	`}`
`24`	`24`
`25`		`- if o.secure {`
`26`		`- args = append(args, "--tls")`
`27`		`- }`
`28`		`-`
`29`	`25`	`if o.context == "" {`
`30`	`26`	`return nil, fmt.Errorf("context not provided")`
`31`	`27`	`}`
Original file line number	Diff line number	Diff line change
`@@ -24,10 +24,6 @@ func (p PodmanCommandFactory) BuildCommand(o BuildOptions) (exec.Cmd, error) {`
`24`	`24`	`args = append(args, "-t", tag)`
`25`	`25`	`}`
`26`	`26`
`27`		`- if !o.secure {`
`28`		`- args = append(args, "--tls-verify=false")`
`29`		`- }`
`30`		`-`
`31`	`27`	`if o.context == "" {`
`32`	`28`	`return nil, fmt.Errorf("context not provided")`
`33`	`29`	`}`