From e3bb55e2a3fad730e8ac308859ea490c29b92469 Mon Sep 17 00:00:00 2001 From: Joshua Auerbach Date: Thu, 20 Oct 2022 10:46:32 -0400 Subject: [PATCH] Fix bugs in credentials directory management and connection checking (#1287) * Change handling of credsDir / serverlessDir * Broaden errors considered as 'not connected' --- commands/activations_test.go | 8 ++++---- commands/functions_test.go | 2 +- commands/namespaces.go | 2 +- commands/namespaces_test.go | 2 +- commands/serverless.go | 10 +++++----- commands/serverless_test.go | 34 ++++++++++++++++------------------ commands/serverless_util.go | 6 +++--- do/mocks/ServerlessService.go | 8 ++++---- do/serverless.go | 19 ++++++++++--------- 9 files changed, 45 insertions(+), 46 deletions(-) diff --git a/commands/activations_test.go b/commands/activations_test.go index 53fb9a81f..8987a5f6a 100644 --- a/commands/activations_test.go +++ b/commands/activations_test.go @@ -95,7 +95,7 @@ func TestActivationsGet(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("activation/get", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{}, nil) @@ -175,7 +175,7 @@ func TestActivationsList(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("activation/list", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{}, nil) @@ -257,7 +257,7 @@ func TestActivationsLogs(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) if tt.expectStream { expectedArgs := append([]string{"activation/logs"}, tt.expectedNimArgs...) tm.serverless.EXPECT().Cmd("nocapture", expectedArgs).Return(fakeCmd, nil) @@ -333,7 +333,7 @@ func TestActivationsResult(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("activation/result", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{}, nil) diff --git a/commands/functions_test.go b/commands/functions_test.go index 1b05a025d..1d2d98d2d 100644 --- a/commands/functions_test.go +++ b/commands/functions_test.go @@ -308,7 +308,7 @@ func TestFunctionsList(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("action/list", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{}, nil) diff --git a/commands/namespaces.go b/commands/namespaces.go index 7b91fc3b7..a496e7796 100644 --- a/commands/namespaces.go +++ b/commands/namespaces.go @@ -95,7 +95,7 @@ func RunNamespacesCreate(c *CmdConfig) error { if !uniq { return fmt.Errorf("you are using label '%s' for another namespace; labels should be unique", label) } - if !skipConnect && ss.CheckServerlessStatus(hashAccessToken(c)) == do.ErrServerlessNotInstalled { + if !skipConnect && ss.CheckServerlessStatus() == do.ErrServerlessNotInstalled { skipConnect = true fmt.Fprintln(c.Out, "Warning: namespace will be created but not connected (serverless software is not installed)") } diff --git a/commands/namespaces_test.go b/commands/namespaces_test.go index 08ef7427f..46deccaee 100644 --- a/commands/namespaces_test.go +++ b/commands/namespaces_test.go @@ -114,7 +114,7 @@ func TestNamespacesCreate(t *testing.T) { tm.serverless.EXPECT().ListNamespaces(ctx).Return(initialList, nil) } if tt.willConnect { - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().Return(nil) creds := do.ServerlessCredentials{Namespace: "hello", APIHost: "https://api.example.com"} tm.serverless.EXPECT().WriteCredentials(creds).Return(nil) } diff --git a/commands/serverless.go b/commands/serverless.go index 134e5acc4..fae997869 100644 --- a/commands/serverless.go +++ b/commands/serverless.go @@ -142,7 +142,7 @@ func RunServerlessInstall(c *CmdConfig) error { } credsLeafDir = hashAccessToken(c) serverless = c.Serverless() - status = serverless.CheckServerlessStatus(credsLeafDir) + status = serverless.CheckServerlessStatus() } switch status { case nil: @@ -164,7 +164,7 @@ func RunServerlessInstall(c *CmdConfig) error { func RunServerlessUpgrade(c *CmdConfig) error { credsLeafDir := hashAccessToken(c) serverless := c.Serverless() - status := serverless.CheckServerlessStatus(credsLeafDir) + status := serverless.CheckServerlessStatus() switch status { case nil: fmt.Fprintln(c.Out, "Serverless support is already installed at an appropriate version. No action needed.") @@ -182,7 +182,7 @@ func RunServerlessUpgrade(c *CmdConfig) error { // RunServerlessUninstall removes the serverless support and any stored credentials func RunServerlessUninstall(c *CmdConfig) error { - err := c.Serverless().CheckServerlessStatus(hashAccessToken(c)) + err := c.Serverless().CheckServerlessStatus() if err == do.ErrServerlessNotInstalled { return errors.New("Nothing to uninstall: no serverless support was found") } @@ -202,7 +202,7 @@ func RunServerlessConnect(c *CmdConfig) error { sls := c.Serverless() // Non-standard check for the connect command (only): it's ok to not be connected. - err = sls.CheckServerlessStatus(hashAccessToken(c)) + err = sls.CheckServerlessStatus() if err != nil && err != do.ErrServerlessNotConnected { return err } @@ -298,7 +298,7 @@ func finishConnecting(sls do.ServerlessService, creds do.ServerlessCredentials, // RunServerlessStatus gives a report on the status of the serverless (installed, up to date, connected) func RunServerlessStatus(c *CmdConfig) error { - status := c.Serverless().CheckServerlessStatus(hashAccessToken(c)) + status := c.Serverless().CheckServerlessStatus() if status == do.ErrServerlessNotInstalled { return status } diff --git a/commands/serverless_test.go b/commands/serverless_test.go index d1f9fe654..80f66412a 100644 --- a/commands/serverless_test.go +++ b/commands/serverless_test.go @@ -100,7 +100,7 @@ func TestServerlessConnect(t *testing.T) { nsResponse := do.NamespaceListResponse{Namespaces: tt.namespaceList} creds := do.ServerlessCredentials{Namespace: "ns1", APIHost: "https://api.example.com"} - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).Return(do.ErrServerlessNotConnected) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNotConnected) ctx := context.TODO() tm.serverless.EXPECT().ListNamespaces(ctx).Return(nsResponse, nil) if tt.expectedError == nil { @@ -131,7 +131,7 @@ func TestServerlessStatusWhenConnected(t *testing.T) { Stdout: config.Out, } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("auth/current", []string{"--apihost", "--name"}).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{ Entity: map[string]interface{}{ @@ -183,7 +183,7 @@ func TestServerlessStatusWithLanguages(t *testing.T) { go:1.22 (go:default) ` - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("auth/current", []string{"--apihost", "--name"}).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{ Entity: map[string]interface{}{ @@ -204,7 +204,7 @@ func TestServerlessStatusWhenNotConnected(t *testing.T) { Stdout: config.Out, } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("auth/current", []string{"--apihost", "--name"}).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{ Error: "403", @@ -218,7 +218,7 @@ func TestServerlessStatusWhenNotConnected(t *testing.T) { func TestServerlessStatusWhenNotInstalled(t *testing.T) { withTestClient(t, func(config *CmdConfig, tm *tcMocks) { - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).Return(do.ErrServerlessNotInstalled) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNotInstalled) err := RunServerlessStatus(config) @@ -229,7 +229,7 @@ func TestServerlessStatusWhenNotInstalled(t *testing.T) { func TestServerlessStatusWhenNotUpToDate(t *testing.T) { withTestClient(t, func(config *CmdConfig, tm *tcMocks) { - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).Return(do.ErrServerlessNeedsUpgrade) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNeedsUpgrade) err := RunServerlessStatus(config) @@ -244,7 +244,7 @@ func TestServerlessInstallFromScratch(t *testing.T) { config.Out = buf credsToken := hashAccessToken(config) - tm.serverless.EXPECT().CheckServerlessStatus(credsToken).Return(do.ErrServerlessNotInstalled) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNotInstalled) tm.serverless.EXPECT().InstallServerless(credsToken, false).Return(nil) err := RunServerlessInstall(config) @@ -257,8 +257,7 @@ func TestServerlessInstallWhenInstalledNotCurrent(t *testing.T) { buf := &bytes.Buffer{} config.Out = buf - credsToken := hashAccessToken(config) - tm.serverless.EXPECT().CheckServerlessStatus(credsToken).Return(do.ErrServerlessNeedsUpgrade) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNeedsUpgrade) err := RunServerlessInstall(config) require.NoError(t, err) @@ -271,7 +270,7 @@ func TestServerlessInstallWhenInstalledAndCurrent(t *testing.T) { buf := &bytes.Buffer{} config.Out = buf - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().Return(nil) err := RunServerlessInstall(config) require.NoError(t, err) @@ -284,8 +283,7 @@ func TestServerlessUpgradeWhenNotInstalled(t *testing.T) { buf := &bytes.Buffer{} config.Out = buf - credsToken := hashAccessToken(config) - tm.serverless.EXPECT().CheckServerlessStatus(credsToken).Return(do.ErrServerlessNotInstalled) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNotInstalled) err := RunServerlessUpgrade(config) require.NoError(t, err) @@ -298,7 +296,7 @@ func TestServerlessUpgradeWhenInstalledAndCurrent(t *testing.T) { buf := &bytes.Buffer{} config.Out = buf - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().Return(nil) err := RunServerlessUpgrade(config) require.NoError(t, err) @@ -312,7 +310,7 @@ func TestServerlessUpgradeWhenInstalledAndNotCurrent(t *testing.T) { config.Out = buf credsToken := hashAccessToken(config) - tm.serverless.EXPECT().CheckServerlessStatus(credsToken).Return(do.ErrServerlessNeedsUpgrade) + tm.serverless.EXPECT().CheckServerlessStatus().Return(do.ErrServerlessNeedsUpgrade) tm.serverless.EXPECT().InstallServerless(credsToken, true).Return(nil) err := RunServerlessUpgrade(config) @@ -373,7 +371,7 @@ func TestServerlessInit(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("project/create", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{ Entity: tt.out, @@ -437,7 +435,7 @@ func TestServerlessDeploy(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("project/deploy", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Exec(fakeCmd).Return(do.ServerlessOutput{}, nil) @@ -573,7 +571,7 @@ func TestServerlessUndeploy(t *testing.T) { } if tt.expectedError == nil && len(tt.expectedNimCmds) > 0 { - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) } if tt.expectTriggerList { tm.serverless.EXPECT().ListTriggers(context.TODO(), "").Return(cannedTriggerList, nil) @@ -633,7 +631,7 @@ func TestServerlessWatch(t *testing.T) { } } - tm.serverless.EXPECT().CheckServerlessStatus(hashAccessToken(config)).MinTimes(1).Return(nil) + tm.serverless.EXPECT().CheckServerlessStatus().MinTimes(1).Return(nil) tm.serverless.EXPECT().Cmd("nocapture", tt.expectedNimArgs).Return(fakeCmd, nil) tm.serverless.EXPECT().Stream(fakeCmd).Return(nil) diff --git a/commands/serverless_util.go b/commands/serverless_util.go index b0869e5b8..716df037d 100644 --- a/commands/serverless_util.go +++ b/commands/serverless_util.go @@ -32,7 +32,7 @@ const ( // ServerlessExec executes a serverless command func ServerlessExec(c *CmdConfig, command string, args ...string) (do.ServerlessOutput, error) { serverless := c.Serverless() - err := serverless.CheckServerlessStatus(hashAccessToken(c)) + err := serverless.CheckServerlessStatus() if err != nil { return do.ServerlessOutput{}, err } @@ -51,7 +51,7 @@ func serverlessExecNoCheck(serverless do.ServerlessService, command string, args // Sets up the arguments and (especially) the flags for the actual call func RunServerlessExec(command string, c *CmdConfig, booleanFlags []string, stringFlags []string) (do.ServerlessOutput, error) { serverless := c.Serverless() - err := serverless.CheckServerlessStatus(hashAccessToken(c)) + err := serverless.CheckServerlessStatus() if err != nil { return do.ServerlessOutput{}, err } @@ -68,7 +68,7 @@ func RunServerlessExec(command string, c *CmdConfig, booleanFlags []string, stri // RunServerlessExecStreaming is like RunServerlessExec but assumes that output will not be captured and can be streamed. func RunServerlessExecStreaming(command string, c *CmdConfig, booleanFlags []string, stringFlags []string) error { serverless := c.Serverless() - err := serverless.CheckServerlessStatus(hashAccessToken(c)) + err := serverless.CheckServerlessStatus() if err != nil { return err } diff --git a/do/mocks/ServerlessService.go b/do/mocks/ServerlessService.go index 9926b7f3c..4c02b965b 100644 --- a/do/mocks/ServerlessService.go +++ b/do/mocks/ServerlessService.go @@ -38,17 +38,17 @@ func (m *MockServerlessService) EXPECT() *MockServerlessServiceMockRecorder { } // CheckServerlessStatus mocks base method. -func (m *MockServerlessService) CheckServerlessStatus(arg0 string) error { +func (m *MockServerlessService) CheckServerlessStatus() error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CheckServerlessStatus", arg0) + ret := m.ctrl.Call(m, "CheckServerlessStatus") ret0, _ := ret[0].(error) return ret0 } // CheckServerlessStatus indicates an expected call of CheckServerlessStatus. -func (mr *MockServerlessServiceMockRecorder) CheckServerlessStatus(arg0 interface{}) *gomock.Call { +func (mr *MockServerlessServiceMockRecorder) CheckServerlessStatus() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckServerlessStatus", reflect.TypeOf((*MockServerlessService)(nil).CheckServerlessStatus), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckServerlessStatus", reflect.TypeOf((*MockServerlessService)(nil).CheckServerlessStatus)) } // Cmd mocks base method. diff --git a/do/serverless.go b/do/serverless.go index 0bb42a8a4..1fb8ecb02 100644 --- a/do/serverless.go +++ b/do/serverless.go @@ -214,7 +214,7 @@ type ServerlessService interface { WriteCredentials(ServerlessCredentials) error ReadCredentials() (ServerlessCredentials, error) GetHostInfo(string) (ServerlessHostInfo, error) - CheckServerlessStatus(string) error + CheckServerlessStatus() error InstallServerless(string, bool) error GetFunction(string, bool) (whisk.Action, []FunctionParameter, error) InvokeFunction(string, interface{}, bool, bool) (map[string]interface{}, error) @@ -357,7 +357,7 @@ func initWhisk(s *serverlessService) error { return nil } -func (s *serverlessService) CheckServerlessStatus(leafCredsDir string) error { +func (s *serverlessService) CheckServerlessStatus() error { _, err := os.Stat(s.serverlessDir) if os.IsNotExist(err) { return ErrServerlessNotInstalled @@ -365,7 +365,7 @@ func (s *serverlessService) CheckServerlessStatus(leafCredsDir string) error { if !serverlessUptodate(s.serverlessDir) { return ErrServerlessNeedsUpgrade } - if !isServerlessConnected(leafCredsDir, s.serverlessDir) { + if !isServerlessConnected(s.credsDir) { return ErrServerlessNotConnected } return nil @@ -788,7 +788,7 @@ func (s *serverlessService) WriteProject(project ServerlessProject) (string, err // of that function are listed. If 'fcn' is empty all triggers are listed. func (s *serverlessService) ListTriggers(ctx context.Context, fcn string) ([]ServerlessTrigger, error) { empty := []ServerlessTrigger{} - err := s.CheckServerlessStatus(HashAccessToken(s.accessToken)) + err := s.CheckServerlessStatus() if err != nil { return empty, err } @@ -842,7 +842,7 @@ func fixBaseDate(trigger ServerlessTrigger) ServerlessTrigger { // GetTrigger gets the contents of a trigger for display func (s *serverlessService) GetTrigger(ctx context.Context, name string) (ServerlessTrigger, error) { empty := ServerlessTrigger{} - err := s.CheckServerlessStatus(HashAccessToken(s.accessToken)) + err := s.CheckServerlessStatus() if err != nil { return empty, err } @@ -1007,11 +1007,12 @@ func (s *serverlessService) ReadCredentials() (ServerlessCredentials, error) { // asking the plugin to validate credentials), so the test is not foolproof. // It merely tests whether a credentials directory has been created for the // current doctl access token and appears to have a credentials.json in it. -func isServerlessConnected(leafCredsDir string, serverlessDir string) bool { - creds := GetCredentialDirectory(leafCredsDir, serverlessDir) - credsFile := filepath.Join(creds, CredentialsFile) +func isServerlessConnected(credsDir string) bool { + credsFile := filepath.Join(credsDir, CredentialsFile) _, err := os.Stat(credsFile) - return !os.IsNotExist(err) + // We used to test specifically for "not found" here but in fact any error is enough to + // prevent connections from working. + return err == nil } // serverlessUptodate answers whether the installed version of the serverlessUptodate is at least