From 1c0dac77d7663d8938b728907d14371f9b668857 Mon Sep 17 00:00:00 2001 From: Sudarsan Reddy Date: Fri, 4 Dec 2020 11:06:13 +0000 Subject: [PATCH] TUN-3599: improved delete if credentials isnt found. Tunnel delete is successful even if we don't find the credentials file in the user's filesystem. We no longer "error" indicating this is a problem. This fix also enables chaining of the delete command by removing a pre-mature return if the credentials file is not found. --- cmd/cloudflared/tunnel/subcommand_context.go | 12 +- .../tunnel/subcommand_context_test.go | 139 ++++++++++++++++++ 2 files changed, 143 insertions(+), 8 deletions(-) diff --git a/cmd/cloudflared/tunnel/subcommand_context.go b/cmd/cloudflared/tunnel/subcommand_context.go index c5652ee1..3e2d572a 100644 --- a/cmd/cloudflared/tunnel/subcommand_context.go +++ b/cmd/cloudflared/tunnel/subcommand_context.go @@ -228,14 +228,10 @@ func (sc *subcommandContext) delete(tunnelIDs []uuid.UUID) error { } credFinder := sc.credentialFinder(id) - tunnelCredentialsPath, err := credFinder.Path() - if err != nil { - sc.logger.Infof("Cannot locate tunnel credentials to delete, error: %v. Please delete the file manually", err) - return nil - } - - if err = os.Remove(tunnelCredentialsPath); err != nil { - sc.logger.Infof("Cannot delete tunnel credentials, error: %v. Please delete the file manually", err) + if tunnelCredentialsPath, err := credFinder.Path(); err == nil { + if err = os.Remove(tunnelCredentialsPath); err != nil { + sc.logger.Infof("Tunnel %v was deleted, but we could not remove its credentials file %s: %s. Consider deleting this file manually.", id, tunnelCredentialsPath, err) + } } } return nil diff --git a/cmd/cloudflared/tunnel/subcommand_context_test.go b/cmd/cloudflared/tunnel/subcommand_context_test.go index b586e91b..6577b8e0 100644 --- a/cmd/cloudflared/tunnel/subcommand_context_test.go +++ b/cmd/cloudflared/tunnel/subcommand_context_test.go @@ -6,6 +6,7 @@ import ( "fmt" "reflect" "testing" + "time" "github.com/cloudflare/cloudflared/connection" "github.com/cloudflare/cloudflared/logger" @@ -213,3 +214,141 @@ func Test_subcommandContext_findCredentials(t *testing.T) { }) } } + +type deleteMockTunnelStore struct { + tunnelstore.Client + mockTunnels map[uuid.UUID]mockTunnelBehaviour + deletedTunnelIDs []uuid.UUID +} + +type mockTunnelBehaviour struct { + tunnel tunnelstore.Tunnel + deleteErr error + cleanupErr error +} + +func newDeleteMockTunnelStore(tunnels ...mockTunnelBehaviour) *deleteMockTunnelStore { + mockTunnels := make(map[uuid.UUID]mockTunnelBehaviour) + for _, tunnel := range tunnels { + mockTunnels[tunnel.tunnel.ID] = tunnel + } + return &deleteMockTunnelStore{ + mockTunnels: mockTunnels, + deletedTunnelIDs: make([]uuid.UUID, 0), + } +} + +func (d *deleteMockTunnelStore) GetTunnel(tunnelID uuid.UUID) (*tunnelstore.Tunnel, error) { + tunnel, ok := d.mockTunnels[tunnelID] + if !ok { + return nil, fmt.Errorf("Couldn't find tunnel: %v", tunnelID) + } + return &tunnel.tunnel, nil +} + +func (d *deleteMockTunnelStore) DeleteTunnel(tunnelID uuid.UUID) error { + tunnel, ok := d.mockTunnels[tunnelID] + if !ok { + return fmt.Errorf("Couldn't find tunnel: %v", tunnelID) + } + + if tunnel.deleteErr != nil { + return tunnel.deleteErr + } + + d.deletedTunnelIDs = append(d.deletedTunnelIDs, tunnelID) + tunnel.tunnel.DeletedAt = time.Now() + delete(d.mockTunnels, tunnelID) + return nil +} + +func (d *deleteMockTunnelStore) CleanupConnections(tunnelID uuid.UUID) error { + tunnel, ok := d.mockTunnels[tunnelID] + if !ok { + return fmt.Errorf("Couldn't find tunnel: %v", tunnelID) + } + return tunnel.cleanupErr +} + +func Test_subcommandContext_Delete(t *testing.T) { + type fields struct { + c *cli.Context + logger logger.Service + isUIEnabled bool + fs fileSystem + tunnelstoreClient *deleteMockTunnelStore + userCredential *userCredential + } + type args struct { + tunnelIDs []uuid.UUID + } + newCertPath := "new_cert.json" + tunnelID1 := uuid.MustParse("df5ed608-b8b4-4109-89f3-9f2cf199df64") + tunnelID2 := uuid.MustParse("af5ed608-b8b4-4109-89f3-9f2cf199df64") + logger, err := logger.New() + require.NoError(t, err) + + var tests = []struct { + name string + fields fields + args args + want []uuid.UUID + wantErr bool + }{ + { + name: "clean up continues if credentials are not found", + fields: fields{ + logger: logger, + fs: mockFileSystem{ + rf: func(filePath string) ([]byte, error) { + return nil, errors.New("file not found") + }, + vfp: func(string) bool { return true }, + }, + c: func() *cli.Context { + flagSet := flag.NewFlagSet("test0", flag.PanicOnError) + flagSet.String(CredFileFlag, newCertPath, "") + c := cli.NewContext(cli.NewApp(), flagSet, nil) + err = c.Set(CredFileFlag, newCertPath) + return c + }(), + tunnelstoreClient: newDeleteMockTunnelStore( + mockTunnelBehaviour{ + tunnel: tunnelstore.Tunnel{ID: tunnelID1}, + }, + mockTunnelBehaviour{ + tunnel: tunnelstore.Tunnel{ID: tunnelID2}, + }, + ), + }, + + args: args{ + tunnelIDs: []uuid.UUID{tunnelID1, tunnelID2}, + }, + want: []uuid.UUID{tunnelID1, tunnelID2}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sc := &subcommandContext{ + c: tt.fields.c, + logger: tt.fields.logger, + isUIEnabled: tt.fields.isUIEnabled, + fs: tt.fields.fs, + tunnelstoreClient: tt.fields.tunnelstoreClient, + userCredential: tt.fields.userCredential, + } + err := sc.delete(tt.args.tunnelIDs) + if (err != nil) != tt.wantErr { + t.Errorf("subcommandContext.findCredentials() error = %v, wantErr %v", err, tt.wantErr) + return + } + got := tt.fields.tunnelstoreClient.deletedTunnelIDs + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("subcommandContext.findCredentials() = %v, want %v", got, tt.want) + return + } + }) + } +}