diff --git a/cmd/ocm-backplane/console/console.go b/cmd/ocm-backplane/console/console.go index 0683c788..b855c954 100644 --- a/cmd/ocm-backplane/console/console.go +++ b/cmd/ocm-backplane/console/console.go @@ -33,7 +33,7 @@ import ( "time" "github.com/Masterminds/semver" - homedir "github.com/mitchellh/go-homedir" + "github.com/mitchellh/go-homedir" consolev1typedclient "github.com/openshift/client-go/console/clientset/versioned/typed/console/v1" consolev1alpha1typedclient "github.com/openshift/client-go/console/clientset/versioned/typed/console/v1alpha1" operatorv1typedclient "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1" @@ -56,8 +56,7 @@ type containerEngineInterface interface { pullImage(imageName string) error putFileToMount(filename string, content []byte) error stopContainer(containerName string) error - // some container engines have special handlings for different types of containers - runConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error + runConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error runMonitorPlugin(containerName string, consoleContainerName string, nginxConf string, pluginArgs []string) error containerIsExist(containerName string) (bool, error) } @@ -86,6 +85,8 @@ type consoleOptions struct { monitorPluginPort string monitorPluginImage string terminationFunction execActionOnTermInterface + ctx context.Context + cancel context.CancelFunc } // envVar for environment variable passing to container @@ -101,7 +102,7 @@ const ( PODMAN = "podman" // Linux name in runtime.GOOS LINUX = "linux" - // MacOS name in runtime.GOOS + // MACOS name in runtime.GOOS MACOS = "darwin" // Environment variable that indicates if open by browser is set as default @@ -132,21 +133,25 @@ const ( var ( validContainerEngines = []string{PODMAN, DOCKER} // For mocking - createClientSet = func(c *rest.Config) (kubernetes.Interface, error) { return kubernetes.NewForConfig(c) } - createCommand = exec.Command + createClientSet = func(c *rest.Config) (kubernetes.Interface, error) { return kubernetes.NewForConfig(c) } + createCommand = exec.Command + createCommandContext = exec.CommandContext // Pull Secret saving directory pullSecretConfigDirectory string - - ctx, cancel = context.WithCancel(context.Background()) ) -func newConsoleOptions() *consoleOptions { - return &consoleOptions{terminationFunction: &execActionOnTermStruct{}} +func newConsoleOptions(ctx context.Context) *consoleOptions { + opts := &consoleOptions{ + terminationFunction: &execActionOnTermStruct{}, + } + opts.ctx, opts.cancel = context.WithCancel(ctx) + + return opts } func NewConsoleCmd() *cobra.Command { - ops := newConsoleOptions() + ops := newConsoleOptions(context.Background()) consoleCmd := &cobra.Command{ Use: "console", Short: "Launch OpenShift web console for the current cluster", @@ -157,7 +162,9 @@ func NewConsoleCmd() *cobra.Command { You can specify container engine with -c. If not specified, it will lookup the PATH in the order of podman and docker. `, SilenceUsage: true, - RunE: ops.run, + RunE: func(cmd *cobra.Command, args []string) error { + return ops.run() + }, } flags := consoleCmd.Flags() @@ -205,7 +212,7 @@ func NewConsoleCmd() *cobra.Command { return consoleCmd } -func (o *consoleOptions) run(cmd *cobra.Command, argv []string) error { +func (o *consoleOptions) run() error { err := o.determineOpenBrowser() if err != nil { return err @@ -222,7 +229,6 @@ func (o *consoleOptions) run(cmd *cobra.Command, argv []string) error { if err != nil { return err } - // finialize the console image url err = o.determineImage(kubeconfig) if err != nil { return err @@ -374,7 +380,6 @@ func (o *consoleOptions) getContainerEngineImpl() (containerEngineInterface, err } else if runtime.GOOS == LINUX && containerEngine == DOCKER { containerEngineImpl = &dockerLinux{} } else if runtime.GOOS == MACOS && containerEngine == DOCKER { - logger.Warnln("Docker on MacOS is not tested") containerEngineImpl = &dockerMac{} } return containerEngineImpl, nil @@ -555,7 +560,7 @@ func (o *consoleOptions) runConsoleContainer(ce containerEngineInterface) error bridgeListen := fmt.Sprintf("http://0.0.0.0:%s", o.port) - envVars := []envVar{} + var envVars []envVar // Set proxy URL to the container proxyURL, err := getProxyURL() if err != nil { @@ -592,7 +597,7 @@ func (o *consoleOptions) runConsoleContainer(ce containerEngineInterface) error "-listen", bridgeListen, } - return ce.runConsoleContainer(consoleContainerName, o.port, containerArgs, envVars) + return ce.runConsoleContainer(o.ctx, o.cancel, consoleContainerName, o.port, containerArgs, envVars) } func (o *consoleOptions) runMonitorPlugin(ce containerEngineInterface) error { @@ -683,7 +688,7 @@ func (o *consoleOptions) cleanUp(ce containerEngineInterface) error { return err } - containersToCleanUp := []string{} + var containersToCleanUp []string // forcing order of removal as the order is not deterministic between container engines if o.needMonitorPlugin { @@ -698,7 +703,7 @@ func (o *consoleOptions) cleanUp(ce containerEngineInterface) error { o.terminationFunction = &execActionOnTermStruct{} } - <-ctx.Done() + <-o.ctx.Done() err = o.terminationFunction.execActionOnTerminationFunction(func() error { for _, c := range containersToCleanUp { exist, err := ce.containerIsExist(c) @@ -911,9 +916,6 @@ func GetConfigDirectory() (string, error) { // Update config directory default path pullSecretConfigDirectory = filepath.Join(home, ".kube/ocm-pull-secret") - if err != nil { - return "", fmt.Errorf("can't modify config directory. Error: %s", err.Error()) - } } return pullSecretConfigDirectory, nil @@ -1070,8 +1072,8 @@ func (ce *dockerMac) pullImage(imageName string) error { return generalDockerPullImage(imageName) } -// the shared function for podman to run console container for both linux and macos -func podmanRunConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error { +// the shared function for podman to run console container for both linux and macOS +func podmanRunConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error { _, authFilename, err := fetchPullSecretIfNotExist() if err != nil { return err @@ -1093,27 +1095,24 @@ func podmanRunConsoleContainer(containerName string, port string, consoleArgs [] engRunArgs = append(engRunArgs, consoleArgs...) logger.WithField("Command", fmt.Sprintf("`%s %s`", PODMAN, strings.Join(engRunArgs, " "))).Infoln("Running container") - runCmd := exec.CommandContext(ctx, PODMAN, engRunArgs...) + runCmd := createCommandContext(ctx, PODMAN, engRunArgs...) defer cancel() runCmd.Stderr = os.Stderr runCmd.Stdout = nil - err = runCmd.Run() - if err != nil { - return err - } - return nil + + return runCmd.Run() } -func (ce *podmanMac) runConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error { - return podmanRunConsoleContainer(containerName, port, consoleArgs, envVars) +func (ce *podmanMac) runConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error { + return podmanRunConsoleContainer(ctx, cancel, containerName, port, consoleArgs, envVars) } -func (ce *podmanLinux) runConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error { - return podmanRunConsoleContainer(containerName, port, consoleArgs, envVars) +func (ce *podmanLinux) runConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error { + return podmanRunConsoleContainer(ctx, cancel, containerName, port, consoleArgs, envVars) } -// the shared function for docker to run console container for both linux and macos -func dockerRunConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error { +// the shared function for docker to run console container for both linux and macOS +func dockerRunConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error { configDirectory, _, err := fetchPullSecretIfNotExist() if err != nil { return err @@ -1138,26 +1137,24 @@ func dockerRunConsoleContainer(containerName string, port string, consoleArgs [] } engRunArgs = append(engRunArgs, consoleArgs...) logger.WithField("Command", fmt.Sprintf("`%s %s`", DOCKER, strings.Join(engRunArgs, " "))).Infoln("Running container") - runCmd := createCommand(DOCKER, engRunArgs...) + + runCmd := createCommandContext(ctx, DOCKER, engRunArgs...) + defer cancel() runCmd.Stderr = os.Stderr runCmd.Stdout = nil - err = runCmd.Run() - if err != nil { - return err - } - return nil + return runCmd.Run() } -func (ce *dockerMac) runConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error { - return dockerRunConsoleContainer(containerName, port, consoleArgs, envVars) +func (ce *dockerMac) runConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error { + return dockerRunConsoleContainer(ctx, cancel, containerName, port, consoleArgs, envVars) } -func (ce *dockerLinux) runConsoleContainer(containerName string, port string, consoleArgs []string, envVars []envVar) error { - return dockerRunConsoleContainer(containerName, port, consoleArgs, envVars) +func (ce *dockerLinux) runConsoleContainer(ctx context.Context, cancel context.CancelFunc, containerName string, port string, consoleArgs []string, envVars []envVar) error { + return dockerRunConsoleContainer(ctx, cancel, containerName, port, consoleArgs, envVars) } -// the shared function for podman to run monitoring plugin for both linux and macos +// the shared function for podman to run monitoring plugin for both linux and macOS func podmanRunMonitorPlugin(containerName string, consoleContainerName string, nginxConfPath string, pluginArgs []string) error { _, authFilename, err := fetchPullSecretIfNotExist() if err != nil { @@ -1197,8 +1194,8 @@ func (ce *podmanLinux) runMonitorPlugin(containerName string, consoleContainerNa return podmanRunMonitorPlugin(containerName, consoleContainerName, nginxConfPath, pluginArgs) } -// the shared function for docker to run monitoring plugin for both linux and macos -func dockerRunMonitorPlugin(containerName string, consoleContainerName string, nginxConfPath string, pluginArgs []string) error { +// the shared function for docker to run monitoring plugin for both linux and macOS +func dockerRunMonitorPlugin(containerName string, _ string, nginxConfPath string, pluginArgs []string) error { configDirectory, _, err := fetchPullSecretIfNotExist() if err != nil { return err @@ -1385,7 +1382,7 @@ func (ce *podmanLinux) stopContainer(containerName string) error { return generalStopContainer(PODMAN, containerName) } -// podman-stop for MacOS +// podman-stop for macOS func (ce *podmanMac) stopContainer(containerName string) error { return generalStopContainer(PODMAN, containerName) } @@ -1395,7 +1392,7 @@ func (ce *dockerLinux) stopContainer(containerName string) error { return generalStopContainer(DOCKER, containerName) } -// docker-stop for MacOS +// docker-stop for macOS func (ce *dockerMac) stopContainer(containerName string) error { return generalStopContainer(DOCKER, containerName) } @@ -1405,7 +1402,7 @@ func (ce *podmanLinux) containerIsExist(containerName string) (bool, error) { return generalContainerIsExist(PODMAN, containerName) } -// podman-exist for MacOS +// podman-exist for macOS func (ce *podmanMac) containerIsExist(containerName string) (bool, error) { return generalContainerIsExist(PODMAN, containerName) } @@ -1415,7 +1412,7 @@ func (ce *dockerLinux) containerIsExist(containerName string) (bool, error) { return generalContainerIsExist(DOCKER, containerName) } -// docker-exist for MacOS +// docker-exist for macOS func (ce *dockerMac) containerIsExist(containerName string) (bool, error) { return generalContainerIsExist(DOCKER, containerName) } diff --git a/cmd/ocm-backplane/console/console_test.go b/cmd/ocm-backplane/console/console_test.go index a995bfe0..5f9f1e99 100644 --- a/cmd/ocm-backplane/console/console_test.go +++ b/cmd/ocm-backplane/console/console_test.go @@ -1,7 +1,9 @@ package console import ( + "context" "os" + "os/exec" "reflect" "runtime" @@ -18,8 +20,6 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd/api" - "os/exec" - cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" "github.com/openshift/backplane-cli/pkg/info" "github.com/openshift/backplane-cli/pkg/ocm" @@ -57,6 +57,14 @@ var _ = Describe("console command", func() { return exec.Command("true") } + createCommandContext = func(ctx context.Context, prog string, args ...string) *exec.Cmd { + command := []string{prog} + command = append(command, args...) + capturedCommands = append(capturedCommands, command) + + return exec.Command("true") + } + pullSecret = "testpullsecret" clusterID = "cluster123" proxyURL = "https://my.proxy:443" @@ -117,7 +125,7 @@ var _ = Describe("console command", func() { It("Should not return an error if no pods are found", func() { setupConfig() createPathPodman() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) ce, err := o.getContainerEngineImpl() Expect(err).To(BeNil()) err = o.beforeStartCleanUp(ce) @@ -128,7 +136,7 @@ var _ = Describe("console command", func() { setupConfig() createPathPodman() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(nil, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) ce, err := o.getContainerEngineImpl() Expect(err).To(BeNil()) capturedCommands = nil @@ -148,7 +156,7 @@ var _ = Describe("console command", func() { It("should read the openbrowser variable from environment variables and it is true", func() { setupConfig() os.Setenv(EnvBrowserDefault, "true") - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineOpenBrowser() os.Setenv(EnvBrowserDefault, "") Expect(err).To(BeNil()) @@ -158,7 +166,7 @@ var _ = Describe("console command", func() { It("should read the openbrowser variable from environment variables and it is false", func() { setupConfig() os.Setenv(EnvBrowserDefault, "false") - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineOpenBrowser() os.Setenv(EnvBrowserDefault, "") Expect(err).To(BeNil()) @@ -168,7 +176,7 @@ var _ = Describe("console command", func() { It("should read the openbrowser variable from environment variables and we it is undefined", func() { setupConfig() os.Setenv(EnvBrowserDefault, "") - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineOpenBrowser() Expect(err).To(MatchError(ContainSubstring("unable to parse boolean value from environment variable"))) }) @@ -183,7 +191,7 @@ var _ = Describe("console command", func() { It("should pick a random port for listen if not specified", func() { setupConfig() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineListenPort() Expect(err).To(BeNil()) Expect(len(o.port)).ToNot(Equal(0)) @@ -217,7 +225,7 @@ var _ = Describe("console command", func() { }}}), nil } setupConfig() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) // for testing, we don't need a real rest.Config err := o.determineImage(nil) Expect(err).To(BeNil()) @@ -236,7 +244,7 @@ var _ = Describe("console command", func() { It("should not assgin a port for monitoring plugin", func() { setupConfig() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(clusterInfo, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineNeedMonitorPlugin() Expect(err).To(BeNil()) err = o.determineMonitorPluginPort() @@ -247,7 +255,7 @@ var _ = Describe("console command", func() { It("should not lookup the monitoring plugin image", func() { setupConfig() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(clusterInfo, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineNeedMonitorPlugin() Expect(err).To(BeNil()) err = o.determineMonitorPluginImage(nil) @@ -258,7 +266,7 @@ var _ = Describe("console command", func() { It("should not add monitoring plugin to console arguments", func() { setupConfig() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(clusterInfo, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineNeedMonitorPlugin() Expect(err).To(BeNil()) plugins, err := o.getPlugins() @@ -278,7 +286,7 @@ var _ = Describe("console command", func() { It("should assgin a port for monitoring plugin", func() { setupConfig() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(clusterInfo, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineNeedMonitorPlugin() Expect(err).To(BeNil()) err = o.determineMonitorPluginPort() @@ -307,7 +315,7 @@ var _ = Describe("console command", func() { } setupConfig() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(clusterInfo, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineNeedMonitorPlugin() Expect(err).To(BeNil()) err = o.determineMonitorPluginImage(nil) @@ -318,7 +326,7 @@ var _ = Describe("console command", func() { It("should add monitoring plugin to console arguments", func() { setupConfig() mockOcmInterface.EXPECT().GetClusterInfoByID(clusterID).Return(clusterInfo, nil).AnyTimes() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) err := o.determineNeedMonitorPlugin() Expect(err).To(BeNil()) plugins, err := o.getPlugins() @@ -359,7 +367,7 @@ var _ = Describe("console command", func() { Context("An container is created to run the console, prior to doing that we need to check if container distro is supported", func() { It("In the case we explicitly specify Podman, the code should return support for Podman", func() { oldpath := createPathPodman() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) o.containerEngineFlag = PODMAN cei, err := o.getContainerEngineImpl() Expect(err).To(BeNil()) @@ -376,7 +384,7 @@ var _ = Describe("console command", func() { It("In the case we explicitly specify Docker, the code should return support for Docker", func() { oldpath := createPathDocker() - o := consoleOptions{} + o := newConsoleOptions(context.Background()) o.containerEngineFlag = DOCKER cei1, err1 := o.getContainerEngineImpl() Expect(err1).To(BeNil()) @@ -391,7 +399,7 @@ var _ = Describe("console command", func() { }) It("Test the situation where the environment variable is not a supported value", func() { - o := consoleOptions{} + o := newConsoleOptions(context.Background()) o.containerEngineFlag = "FOO" _, err4 := o.getContainerEngineImpl() Expect(err4).To(MatchError(ContainSubstring("container engine can only be one of podman|docker"))) @@ -450,7 +458,7 @@ var _ = Describe("console command", func() { ocmEnvironment, _ := cmv1.NewEnvironment().BackplaneURL("fakeBackPlaneUrl").Build() // Tell mockOCM interface to return ocnEnvironment mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnvironment, nil).AnyTimes() - o := consoleOptions{terminationFunction: &execActionOnTermMockStruct{}} + o := newConsoleOptions(context.Background()) o.port = "1337" o.url = "http://127.0.0.2:1447" o.openBrowser = false