Skip to content

Commit

Permalink
Fix bugs in credentials directory management and connection checking (#…
Browse files Browse the repository at this point in the history 
…1287)

* Change handling of credsDir / serverlessDir

* Broaden errors considered as 'not connected'
  • Loading branch information
@joshuaauerbachwatson
joshuaauerbachwatson authored Oct 20, 2022
1 parent 0c91e24 commit e3bb55e
Show file tree
Hide file tree
Showing 9 changed files with 45 additions and 46 deletions.
8 changes: 4 additions & 4 deletions commands/activations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion commands/functions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion commands/namespaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)")
}
Expand Down
2 changes: 1 addition & 1 deletion commands/namespaces_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
10 changes: 5 additions & 5 deletions commands/serverless.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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.")
Expand All @@ -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")
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
34 changes: 16 additions & 18 deletions commands/serverless_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{}{
Expand Down Expand Up @@ -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{}{
Expand All @@ -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",
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down
6 changes: 3 additions & 3 deletions commands/serverless_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
8 changes: 4 additions & 4 deletions do/mocks/ServerlessService.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 10 additions & 9 deletions do/serverless.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -357,15 +357,15 @@ 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
}
if !serverlessUptodate(s.serverlessDir) {
return ErrServerlessNeedsUpgrade
}
if !isServerlessConnected(leafCredsDir, s.serverlessDir) {
if !isServerlessConnected(s.credsDir) {
return ErrServerlessNotConnected
}
return nil
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit e3bb55e

Please sign in to comment.