Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add safe-delete workspace API, and org setting to restrict force-delete #539

Merged
merged 10 commits into from
Oct 27, 2022
Merged

Add safe-delete workspace API, and org setting to restrict force-delete #539

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7e7faed
Add safe-delete workspace API, and org setting to control if workspac…
mpminardi Sep 21, 2022
8730b72
Add a comment to clarify that AllowForceDelete will be false for old …
JarrettSpiker Oct 11, 2022
51341b4
Generate mocks for SafeDelete
JarrettSpiker Oct 11, 2022
16d153b
Remove duplicate assertion
JarrettSpiker Oct 14, 2022
2213c1e
Add changelog entry
JarrettSpiker Oct 14, 2022
e8b91ef
Update tests to exit early if there is a setup error
JarrettSpiker Oct 21, 2022
01e5267
Attempt cleanup on workspaces which should be deleted
JarrettSpiker Oct 24, 2022
dc48cae
Cleanup workspaces by ID not name
JarrettSpiker Oct 25, 2022
d86cb66
Exit test early if workspace permissions nil to avoid panic
JarrettSpiker Oct 25, 2022
f3a30d5
Prevent test panic
JarrettSpiker Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add safe-delete workspace API, and org setting to control if workspac… 
…e admins can force delete
  • Loading branch information
@mpminardi @JarrettSpiker
mpminardi authored and JarrettSpiker committed Oct 21, 2022
commit 7e7faed32908585af12288f0c7c11ac390f9f895
7 changes: 7 additions & 0 deletions organization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ type Organization struct {
TrialExpiresAt time.Time `jsonapi:"attr,trial-expires-at,iso8601"`
TwoFactorConformant bool `jsonapi:"attr,two-factor-conformant"`
SendPassingStatusesForUntriggeredSpeculativePlans bool `jsonapi:"attr,send-passing-statuses-for-untriggered-speculative-plans"`
AllowForceDeleteWorkspaces bool `jsonapi:"attr,allow-force-delete-workspaces"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this property is missing for older TFE versions, the default will be false. And then, of course, all deletes will be force deletes because safe delete isn't implemented. I think we are OK with this contradiction but I wanted to point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a really good point...I am also ok with the contradiction, it seems less confusing than making this a *bool or something when it isn't strictly necessary and we use regular bools are used for everything else in this struct. I also cant think of a less confusing name...

I will add a comment though to make it clear that this will be false for older TFE versions, even though all deletes are force deletes in that case

}

// Capacity represents the current run capacity of an organization.
Expand Down Expand Up @@ -166,6 +167,9 @@ type OrganizationCreateOptions struct {

// Optional: SendPassingStatusesForUntriggeredSpeculativePlans toggles behavior of untriggered speculative plans to send status updates to version control systems like GitHub.
SendPassingStatusesForUntriggeredSpeculativePlans *bool `jsonapi:"attr,send-passing-statuses-for-untriggered-speculative-plans,omitempty"`

// Optional: AllowForceDeleteWorkspaces toggles behavior of allowing workspace admins to delete workspaces with resources under management.
AllowForceDeleteWorkspaces *bool `jsonapi:"attr,allow-force-delete-workspaces,omitempty"`
}

// OrganizationUpdateOptions represents the options for updating an organization.
Expand Down Expand Up @@ -202,6 +206,9 @@ type OrganizationUpdateOptions struct {

// SendPassingStatusesForUntriggeredSpeculativePlans toggles behavior of untriggered speculative plans to send status updates to version control systems like GitHub.
SendPassingStatusesForUntriggeredSpeculativePlans *bool `jsonapi:"attr,send-passing-statuses-for-untriggered-speculative-plans,omitempty"`

// Optional: AllowForceDeleteWorkspaces toggles behavior of allowing workspace admins to delete workspaces with resources under management.
AllowForceDeleteWorkspaces *bool `jsonapi:"attr,allow-force-delete-workspaces,omitempty"`
}

// ReadRunQueueOptions represents the options for showing the queue.
Expand Down
34 changes: 34 additions & 0 deletions organization_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,41 @@ func TestOrganizationsReadRunTasksEntitlement(t *testing.T) {
assert.NotEmpty(t, entitlements.ID)
assert.True(t, entitlements.RunTasks)
})
}

func TestOrganizationsAllowForceDeleteSetting(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our docs should be much more explicit, but we've recently introduced test splitting in our CI and we're requiring all top-level tests to call skipIfNotCINode(t) as the first check. This will ensure the test runs only once on the correct test node instead of on every single node. You do not need to do this for subtests.

client := testClient(t)
ctx := context.Background()

t.Run("creates and updates allow force delete", func(t *testing.T) {
options := OrganizationCreateOptions{
Name: String(randomString(t)),
Email: String(randomString(t) + "@tfe.local"),
AllowForceDeleteWorkspaces: Bool(true),
}

org, err := client.Organizations.Create(ctx, options)
assert.Nil(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be require.NoError(t, error) -- reason being is the assert package will not halt the execution of the test if it fails, leading to more cascading errors and more noise. Make sure to change all error checks to use require 👍


t.Cleanup(func() {
err := client.Organizations.Delete(ctx, org.Name)
if err != nil {
t.Logf("error deleting organization (%s): %s", org.Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we t.Errorf() when cleanup fails.

}
})

assert.Equal(t, *options.Name, org.Name)
assert.Equal(t, *options.Email, org.Email)
assert.True(t, org.AllowForceDeleteWorkspaces)

org, err = client.Organizations.Update(ctx, org.Name, OrganizationUpdateOptions{AllowForceDeleteWorkspaces: Bool(false)})
assert.Nil(t, err)
assert.False(t, org.AllowForceDeleteWorkspaces)

org, err = client.Organizations.Read(ctx, org.Name)
assert.Nil(t, err)
assert.False(t, org.AllowForceDeleteWorkspaces)
})
}

func orgItemsContainsName(items []*Organization, name string) bool {
Expand Down
66 changes: 55 additions & 11 deletions workspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ type Workspaces interface {
// DeleteByID deletes a workspace by its ID.
DeleteByID(ctx context.Context, workspaceID string) error

// SafeDelete a workspace by its name.
SafeDelete(ctx context.Context, organization string, workspace string) error

// SafeDeleteByID deletes a workspace by its ID.
SafeDeleteByID(ctx context.Context, workspaceID string) error

// RemoveVCSConnection from a workspace.
RemoveVCSConnection(ctx context.Context, organization, workspace string) (*Workspace, error)

Expand Down Expand Up @@ -194,17 +200,18 @@ type WorkspaceActions struct {

// WorkspacePermissions represents the workspace permissions.
type WorkspacePermissions struct {
CanDestroy bool `jsonapi:"attr,can-destroy"`
CanForceUnlock bool `jsonapi:"attr,can-force-unlock"`
CanLock bool `jsonapi:"attr,can-lock"`
CanManageRunTasks bool `jsonapi:"attr,can-manage-run-tasks"`
CanQueueApply bool `jsonapi:"attr,can-queue-apply"`
CanQueueDestroy bool `jsonapi:"attr,can-queue-destroy"`
CanQueueRun bool `jsonapi:"attr,can-queue-run"`
CanReadSettings bool `jsonapi:"attr,can-read-settings"`
CanUnlock bool `jsonapi:"attr,can-unlock"`
CanUpdate bool `jsonapi:"attr,can-update"`
CanUpdateVariable bool `jsonapi:"attr,can-update-variable"`
CanDestroy bool `jsonapi:"attr,can-destroy"`
CanForceUnlock bool `jsonapi:"attr,can-force-unlock"`
CanLock bool `jsonapi:"attr,can-lock"`
CanManageRunTasks bool `jsonapi:"attr,can-manage-run-tasks"`
CanQueueApply bool `jsonapi:"attr,can-queue-apply"`
CanQueueDestroy bool `jsonapi:"attr,can-queue-destroy"`
CanQueueRun bool `jsonapi:"attr,can-queue-run"`
CanReadSettings bool `jsonapi:"attr,can-read-settings"`
CanUnlock bool `jsonapi:"attr,can-unlock"`
CanUpdate bool `jsonapi:"attr,can-update"`
CanUpdateVariable bool `jsonapi:"attr,can-update-variable"`
CanForceDelete *bool `jsonapi:"attr,can-force-delete"` // pointer b/c it will be useful to check if this property exists, as opposed to having it default to false
}

// WSIncludeOpt represents the available options for include query params.
Expand Down Expand Up @@ -770,6 +777,43 @@ func (s *workspaces) DeleteByID(ctx context.Context, workspaceID string) error {
return req.Do(ctx, nil)
}

// SafeDelete a workspace by its name.
func (s *workspaces) SafeDelete(ctx context.Context, organization, workspace string) error {
if !validStringID(&organization) {
return ErrInvalidOrg
}
if !validStringID(&workspace) {
return ErrInvalidWorkspaceValue
}

u := fmt.Sprintf(
"organizations/%s/workspaces/%s/actions/safe-delete",
url.QueryEscape(organization),
url.QueryEscape(workspace),
)
req, err := s.client.NewRequest("POST", u, nil)
if err != nil {
return err
}

return req.Do(ctx, nil)
}

// SafeDeleteByID safely deletes a workspace by its ID.
func (s *workspaces) SafeDeleteByID(ctx context.Context, workspaceID string) error {
if !validStringID(&workspaceID) {
return ErrInvalidWorkspaceID
}

u := fmt.Sprintf("workspaces/%s/actions/safe-delete", url.QueryEscape(workspaceID))
req, err := s.client.NewRequest("POST", u, nil)
if err != nil {
return err
}

return req.Do(ctx, nil)
}

// RemoveVCSConnection from a workspace.
func (s *workspaces) RemoveVCSConnection(ctx context.Context, organization, workspace string) (*Workspace, error) {
if !validStringID(&organization) {
Expand Down
120 changes: 120 additions & 0 deletions workspace_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,126 @@ func TestWorkspacesDeleteByID(t *testing.T) {
})
}

func TestCanForceDeletePermission(t *testing.T) {
client := testClient(t)
ctx := context.Background()

orgTest, orgTestCleanup := createOrganization(t, client)
defer orgTestCleanup()

wTest, _ := createWorkspace(t, client, orgTest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, Is there a reason we don't clean this workspace up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I was copying from the update tests in this file, which also ignore the cleanup

Im not sure why though...I will try to update all the tests to do the cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I remember. Because the the workspace gets removed in the process of the test, and then the cleanup call throws an errors. Or, in some other tests (and also this one, I would guess) the org also gets cleaned up first, which deletes the workspace and causes a defered workspace cleanup to fail

E.g. if you try to defer the cleanup in the test I linked above you get

=== RUN   TestWorkspacesUpdate
=== CONT  TestWorkspacesUpdate
    helper_test.go:1602: Error destroying workspace! WARNING: Dangling resources
        may exist! The full error is shown below.
        
        Workspace: 0874350d-2197-1355-74c7-4ee437bbc010
        Error: resource not found
--- FAIL: TestWorkspacesUpdate (5.32s)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels like it should be a solve-able problem. I will update the workspace cleanup function not to error on 404s, and then update the other tests in this file to cleanup the workspaces which they create


t.Run("workspace permission set includes can-force-delete", func(t *testing.T) {
w, err := client.Workspaces.ReadByID(ctx, wTest.ID)
require.NoError(t, err)
assert.Equal(t, wTest, w)
assert.Equal(t, wTest, w)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate assertion here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thanks!

assert.NotNil(t, w.Permissions.CanForceDelete)
assert.True(t, *w.Permissions.CanForceDelete)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like your test is panicking:

=== FAIL: . TestCanForceDeletePermission/workspace_permission_set_includes_can-force-delete (0.13s)
    workspace_integration_test.go:1237: 
        	Error Trace:	/home/runner/work/go-tfe/go-tfe/workspace_integration_test.go:1237
        	Error:      	Expected value not to be nil.
        	Test:       	TestCanForceDeletePermission/workspace_permission_set_includes_can-force-delete
    --- FAIL: TestCanForceDeletePermission/workspace_permission_set_includes_can-force-delete (0.13s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x7f0a47]

goroutine 2995 [running]:
testing.tRunner.func1.2({0x861640, 0xbf9470})
	/opt/hostedtoolcache/go/1.19.2/x64/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.19.2/x64/src/testing/testing.go:1399 +0x39f
panic({0x861640, 0xbf9470})
	/opt/hostedtoolcache/go/1.19.2/x64/src/runtime/panic.go:884 +0x212
github.com/hashicorp/go-tfe.TestCanForceDeletePermission.func1(0x1?)
	/home/runner/work/go-tfe/go-tfe/workspace_integration_test.go:1238 +0xe7
testing.tRunner(0xc0004b56c0, 0xc0006b2780)
	/opt/hostedtoolcache/go/1.19.2/x64/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.19.2/x64/src/testing/testing.go:1493 +0x35f
exit status 2

Two things:

  • This very well may be an issue with our CI instance not having the latest API changes. In which case we'd need to fix this.
  • We tend to require the existence of a relationship struct, i.e require.NotNil(t, w.Permissions), before we check said relationship's fields (otherwise we risk a panic).

Copy link
Contributor Author

@JarrettSpiker JarrettSpiker Oct 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This very well may be an issue with our CI instance not having the latest API changes. In which case we'd need to fix this.

I think that this is at least partially it...where can I find the instance that the acceptance tests run against? I can check out if it has all the api changes expected

Edit: Found it, and it looks like it has at least some of the expected changes...I will dig into why the tests are failing like this!

Edit edit: I was wrong in the previous edit. I was looking at tfelocal-cloud (which has the correct changes) not tflocal-go-tfe which does not. @sebasslash can you confirm that the integration tests are running against tflocal-go-tfe, and help me figure out how to update it?

})
}

func TestWorkspacesSafeDelete(t *testing.T) {
client := testClient(t)
ctx := context.Background()

orgTest, orgTestCleanup := createOrganization(t, client)
defer orgTestCleanup()

wTest, _ := createWorkspace(t, client, orgTest)

t.Run("with valid options", func(t *testing.T) {
err := client.Workspaces.SafeDelete(ctx, orgTest.Name, wTest.Name)
require.NoError(t, err)

// Try loading the workspace - it should fail.
_, err = client.Workspaces.Read(ctx, orgTest.Name, wTest.Name)
assert.Equal(t, ErrResourceNotFound, err)
})

t.Run("when organization is invalid", func(t *testing.T) {
err := client.Workspaces.SafeDelete(ctx, badIdentifier, wTest.Name)
assert.EqualError(t, err, ErrInvalidOrg.Error())
})

t.Run("when workspace is invalid", func(t *testing.T) {
err := client.Workspaces.SafeDelete(ctx, orgTest.Name, badIdentifier)
assert.EqualError(t, err, ErrInvalidWorkspaceValue.Error())
})

t.Run("when workspace is locked", func(t *testing.T) {
wTest, workspaceCleanup := createWorkspace(t, client, orgTest)
defer workspaceCleanup()
w, err := client.Workspaces.Lock(ctx, wTest.ID, WorkspaceLockOptions{})
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed one require.NoError(t, err), I'd even say we should also require.True(t, w.Locked) since the rest of your test depends on this being true. If this fails, there's no point in continuing to run the rest of the test.

assert.True(t, w.Locked)

err = client.Workspaces.SafeDelete(ctx, orgTest.Name, wTest.Name)
assert.Contains(t, err.Error(), "conflict")
assert.Contains(t, err.Error(), "currently locked")
})

t.Run("when workspace has resources under management", func(t *testing.T) {
wTest, workspaceCleanup := createWorkspace(t, client, orgTest)
defer workspaceCleanup()
_, svTestCleanup := createStateVersion(t, client, 0, wTest)
t.Cleanup(svTestCleanup)

err := client.Workspaces.SafeDelete(ctx, orgTest.Name, wTest.Name)
// cant verify the exact error here because it is timing dependent on the backend
// based on whether the state version has been processed yet
assert.Contains(t, err.Error(), "conflict")
})
}

func TestWorkspacesSafeDeleteByID(t *testing.T) {
client := testClient(t)
ctx := context.Background()

orgTest, orgTestCleanup := createOrganization(t, client)
defer orgTestCleanup()

wTest, _ := createWorkspace(t, client, orgTest)

t.Run("with valid options", func(t *testing.T) {
err := client.Workspaces.SafeDeleteByID(ctx, wTest.ID)
require.NoError(t, err)

// Try loading the workspace - it should fail.
_, err = client.Workspaces.ReadByID(ctx, wTest.ID)
assert.Equal(t, ErrResourceNotFound, err)
})

t.Run("without a valid workspace ID", func(t *testing.T) {
err := client.Workspaces.SafeDeleteByID(ctx, badIdentifier)
assert.EqualError(t, err, ErrInvalidWorkspaceID.Error())
})

t.Run("when workspace is locked", func(t *testing.T) {
wTest, workspaceCleanup := createWorkspace(t, client, orgTest)
defer workspaceCleanup()
w, err := client.Workspaces.Lock(ctx, wTest.ID, WorkspaceLockOptions{})
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

assert.True(t, w.Locked)

err = client.Workspaces.SafeDeleteByID(ctx, wTest.ID)
assert.Contains(t, err.Error(), "conflict")
assert.Contains(t, err.Error(), "currently locked")
})

t.Run("when workspace has resources under management", func(t *testing.T) {
wTest, workspaceCleanup := createWorkspace(t, client, orgTest)
defer workspaceCleanup()
_, svTestCleanup := createStateVersion(t, client, 0, wTest)
t.Cleanup(svTestCleanup)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocker: Not a huge deal to mix the two, but the preference here is for t.Cleanup()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that it is preferred to do t.Cleanup(workspaceCleanup) instead of defer workspaceCleanup() directly?


err := client.Workspaces.SafeDeleteByID(ctx, wTest.ID)
// cant verify the exact error here because it is timing dependent on the backend
// based on whether the state version has been processed yet
assert.Contains(t, err.Error(), "conflict")
})
}

func TestWorkspacesRemoveVCSConnection(t *testing.T) {
skipIfNotCINode(t)

Expand Down