From 9d8e37c8def14c71a5e55dd5167a9647900440d7 Mon Sep 17 00:00:00 2001 From: Bo Meng Date: Thu, 16 May 2024 09:35:44 +0800 Subject: [PATCH] validation mandatory value proxy-url in backplane config validate the PD API KEY in config file if login via PD --- cmd/ocm-backplane/console/console.go | 15 ++++----- cmd/ocm-backplane/login/login.go | 3 ++ cmd/ocm-backplane/login/login_test.go | 42 +++++++++++++++++++++++-- cmd/ocm-backplane/logout/logout_test.go | 1 + pkg/cli/config/config.go | 27 ++++++++++++---- pkg/cli/config/config_test.go | 39 ++++++++++++++++++++++- 6 files changed, 111 insertions(+), 16 deletions(-) diff --git a/cmd/ocm-backplane/console/console.go b/cmd/ocm-backplane/console/console.go index 3a0373d7..4e199480 100644 --- a/cmd/ocm-backplane/console/console.go +++ b/cmd/ocm-backplane/console/console.go @@ -34,10 +34,6 @@ import ( "github.com/Masterminds/semver" homedir "github.com/mitchellh/go-homedir" - "github.com/openshift/backplane-cli/pkg/cli/config" - "github.com/openshift/backplane-cli/pkg/info" - "github.com/openshift/backplane-cli/pkg/ocm" - "github.com/openshift/backplane-cli/pkg/utils" 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" @@ -49,6 +45,11 @@ import ( "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + + "github.com/openshift/backplane-cli/pkg/cli/config" + "github.com/openshift/backplane-cli/pkg/info" + "github.com/openshift/backplane-cli/pkg/ocm" + "github.com/openshift/backplane-cli/pkg/utils" ) type containerEngineInterface interface { @@ -1194,7 +1195,7 @@ func dockerRunMonitorPlugin(containerName string, consoleContainerName string, n } func (ce *dockerLinux) runMonitorPlugin(containerName string, consoleContainerName string, nginxConf string, pluginArgs []string) error { - configDirectory, err := config.GetConfigDirctory() + configDirectory, err := config.GetConfigDirectory() if err != nil { return err } @@ -1203,7 +1204,7 @@ func (ce *dockerLinux) runMonitorPlugin(containerName string, consoleContainerNa } func (ce *dockerMac) runMonitorPlugin(containerName string, consoleContainerName string, nginxConf string, pluginArgs []string) error { - configDirectory, err := config.GetConfigDirctory() + configDirectory, err := config.GetConfigDirectory() if err != nil { return err } @@ -1265,7 +1266,7 @@ func (ce *podmanMac) putFileToMount(filename string, content []byte) error { // filename should be name only, not a path func dockerPutFileToMount(filename string, content []byte) error { // for files in linux, we put them into the user's backplane config directory - configDirectory, err := config.GetConfigDirctory() + configDirectory, err := config.GetConfigDirectory() if err != nil { return err } diff --git a/cmd/ocm-backplane/login/login.go b/cmd/ocm-backplane/login/login.go index c7958841..9e111290 100644 --- a/cmd/ocm-backplane/login/login.go +++ b/cmd/ocm-backplane/login/login.go @@ -122,6 +122,9 @@ func runLogin(cmd *cobra.Command, argv []string) (err error) { logger.Debugf("Extracting Backplane Cluster ID") // Currently go-pagerduty pkg does not include incident id validation. if args.pd != "" { + if bpConfig.PagerDutyAPIKey == "" { + return fmt.Errorf("please make sure the PD API Key is configured correctly in the config file") + } pdClient, err := pagerduty.NewWithToken(bpConfig.PagerDutyAPIKey) if err != nil { return fmt.Errorf("could not initialize the client: %w", err) diff --git a/cmd/ocm-backplane/login/login_test.go b/cmd/ocm-backplane/login/login_test.go index a7c77bec..992fd4d4 100644 --- a/cmd/ocm-backplane/login/login_test.go +++ b/cmd/ocm-backplane/login/login_test.go @@ -14,6 +14,9 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" + "k8s.io/client-go/tools/clientcmd" + "k8s.io/client-go/tools/clientcmd/api" + "github.com/openshift/backplane-cli/pkg/backplaneapi" backplaneapiMock "github.com/openshift/backplane-cli/pkg/backplaneapi/mocks" "github.com/openshift/backplane-cli/pkg/cli/config" @@ -22,8 +25,6 @@ import ( "github.com/openshift/backplane-cli/pkg/ocm" ocmMock "github.com/openshift/backplane-cli/pkg/ocm/mocks" "github.com/openshift/backplane-cli/pkg/utils" - "k8s.io/client-go/tools/clientcmd" - "k8s.io/client-go/tools/clientcmd/api" ) func MakeIoReader(s string) io.ReadCloser { @@ -464,6 +465,43 @@ var _ = Describe("Login command", func() { Expect(err.Error()).Should(ContainSubstring("status code 401")) }) + It("should return error when trying to login via PD but the PD API Key is not configured", func() { + args.pd = truePagerDutyIncidentID + + err := utils.CreateTempKubeConfig(nil) + Expect(err).To(BeNil()) + + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Create a temporary JSON configuration file in the temp directory for testing purposes. + tempDir := os.TempDir() + bpConfigPath = filepath.Join(tempDir, "mock.json") + tempFile, err := os.Create(bpConfigPath) + Expect(err).To(BeNil()) + + testData := config.BackplaneConfiguration{ + URL: backplaneAPIURI, + ProxyURL: new(string), + SessionDirectory: "", + AssumeInitialArn: "", + PagerDutyAPIKey: falsePagerDutyAPITkn, + } + + // Marshal the testData into JSON format and write it to tempFile. + pdTestData := testData + pdTestData.PagerDutyAPIKey = "" + jsonData, err := json.Marshal(pdTestData) + Expect(err).To(BeNil()) + _, err = tempFile.Write(jsonData) + Expect(err).To(BeNil()) + + os.Setenv("BACKPLANE_CONFIG", bpConfigPath) + + err = runLogin(nil, nil) + + Expect(err.Error()).Should(ContainSubstring("please make sure the PD API Key is configured correctly")) + }) + It("should fail to find a non existent PD Incident and return HTTP status code 404 when the requested resource is not found", func() { args.pd = falsePagerDutyIncidentID diff --git a/cmd/ocm-backplane/logout/logout_test.go b/cmd/ocm-backplane/logout/logout_test.go index f77235ae..6d338d77 100644 --- a/cmd/ocm-backplane/logout/logout_test.go +++ b/cmd/ocm-backplane/logout/logout_test.go @@ -14,6 +14,7 @@ import ( "k8s.io/client-go/tools/clientcmd/api" cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" + "github.com/openshift/backplane-cli/cmd/ocm-backplane/login" "github.com/openshift/backplane-cli/pkg/backplaneapi" backplaneapiMock "github.com/openshift/backplane-cli/pkg/backplaneapi/mocks" diff --git a/pkg/cli/config/config.go b/pkg/cli/config/config.go index f635088e..f591be24 100644 --- a/pkg/cli/config/config.go +++ b/pkg/cli/config/config.go @@ -15,9 +15,10 @@ import ( "github.com/openshift/backplane-cli/pkg/ocm" ) +// Please update the validateConfig function if there is any required config key added type BackplaneConfiguration struct { URL string `json:"url"` - ProxyURL *string `json:"proxy-url"` // Optional + ProxyURL *string `json:"proxy-url"` SessionDirectory string `json:"session-dir"` AssumeInitialArn string `json:"assume-initial-arn"` PagerDutyAPIKey string `json:"pd-key"` @@ -61,6 +62,11 @@ func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) { } } + if err = validateConfig(); err != nil { + // FIXME: we should return instead of warning here, after the tests do not require external network access + logger.Warn(err) + } + // Check if user has explicitly defined proxy; it has higher precedence over the config file err = viper.BindEnv("proxy-url", info.BackplaneProxyEnvName) if err != nil { @@ -72,9 +78,10 @@ func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) { logger.Warn("Manual URL configuration is deprecated, please remove URL key from Backplane configuration") } - // Check if user has explicitly defined backplane URL via env; it has higher precedence over the ocm env URL + // Warn if user has explicitly defined backplane URL via env url, ok := getBackplaneEnv(info.BackplaneURLEnvName) if ok { + logger.Warn(fmt.Printf("Manual URL configuration is deprecated, please unset the environment %s", info.BackplaneURLEnvName)) bpConfig.URL = url } else { // Fetch backplane URL from ocm env @@ -88,8 +95,6 @@ func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) { proxyURL := bpConfig.getFirstWorkingProxyURL(proxyInConfigFile) if proxyURL != "" { bpConfig.ProxyURL = &proxyURL - } else { - logger.Warn("No proxy configuration available. This may result in failing commands as backplane-api is only available from select networks.") } bpConfig.SessionDirectory = viper.GetString("session-dir") @@ -102,7 +107,6 @@ func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) { } else { logger.Info("No PagerDuty API Key configuration available. This will result in failure of `ocm-backplane login --pd ` command.") } - return bpConfig, nil } @@ -145,7 +149,18 @@ func (config *BackplaneConfiguration) getFirstWorkingProxyURL(s []string) string return "" } -func GetConfigDirctory() (string, error) { +func validateConfig() error { + + // Validate the proxy url + if viper.GetStringSlice("proxy-url") == nil && os.Getenv(info.BackplaneProxyEnvName) == "" { + return fmt.Errorf("proxy-url must be set explicitly in either config file or via the environment HTTPS_PROXY") + } + + return nil +} + +// GetConfigDirectory returns the backplane config path +func GetConfigDirectory() (string, error) { bpConfigFilePath, err := GetConfigFilePath() if err != nil { return "", err diff --git a/pkg/cli/config/config_test.go b/pkg/cli/config/config_test.go index 951340d1..7cc78b98 100644 --- a/pkg/cli/config/config_test.go +++ b/pkg/cli/config/config_test.go @@ -3,7 +3,12 @@ package config import ( "net/http" "net/http/httptest" + "os" "testing" + + "github.com/spf13/viper" + + "github.com/openshift/backplane-cli/pkg/info" ) func TestGetBackplaneConfig(t *testing.T) { @@ -25,7 +30,6 @@ func TestGetBackplaneConfig(t *testing.T) { t.Errorf("expected to return the explicitly defined proxy %v instead of the default one %v", userDefinedProxy, config.ProxyURL) } }) - } func TestGetBackplaneConnection(t *testing.T) { @@ -121,3 +125,36 @@ func TestBackplaneConfiguration_getFirstWorkingProxyURL(t *testing.T) { }) } } + +func TestValidateConfig(t *testing.T) { + tests := []struct { + name string + proxyConfig []string + envProxy string + expectError bool + }{ + {"No proxy set", nil, "", true}, + {"Proxy set in config", []string{"http://proxy.example.com"}, "", false}, + {"Proxy set in environment", nil, "http://proxy.example.com", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set up viper configuration + viper.Set("proxy-url", tt.proxyConfig) + + // Set up environment variable + if tt.envProxy != "" { + os.Setenv(info.BackplaneProxyEnvName, tt.envProxy) + } else { + os.Unsetenv(info.BackplaneProxyEnvName) + } + + // Validate config + err := validateConfig() + if (err != nil) != tt.expectError { + t.Errorf("expected error: %v, got: %v", tt.expectError, err) + } + }) + } +}