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

Jump to bottom

r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

Merged
merged 1 commit into from
Feb 23, 2020
Merged

r/aws_workspaces_directory: Fix panic if previously registered directory is no longer registered #11837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 14 additions & 8 deletions aws/resource_aws_workspaces_directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,17 +141,22 @@ func resourceAwsWorkspacesDirectoryCreate(d *schema.ResourceData, meta interface
func resourceAwsWorkspacesDirectoryRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).workspacesconn

resp, err := conn.DescribeWorkspaceDirectories(&workspaces.DescribeWorkspaceDirectoriesInput{
DirectoryIds: []*string{aws.String(d.Id())},
})
raw, state, err := workspacesDirectoryRefreshStateFunc(conn, d.Id())()
if err != nil {
return err
return fmt.Errorf("error getting workspaces directory (%s): %s", d.Id(), err)
}
if state == workspaces.WorkspaceDirectoryStateDeregistered {
log.Printf("[WARN] workspaces directory (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}
dir := resp.Directories[0]

dir := raw.(*workspaces.WorkspaceDirectory)
d.Set("directory_id", dir.DirectoryId)
d.Set("subnet_ids", dir.SubnetIds)
d.Set("self_service_permissions", flattenSelfServicePermissions(dir.SelfservicePermissions))
if err := d.Set("self_service_permissions", flattenSelfServicePermissions(dir.SelfservicePermissions)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, could you explain what are the circumstances when Set() may return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the type of the data passed to Set() isn't compatible with the schema type declared for the attribute it's possible to get an error (e.g. passing a map[string]interface{} when a *schema.Set is expected.
To be safe, anytime a "complex" type is being set we should really check errors (although there are many places in the code that isn't done).
Silent failure can lead to attributes not being set correctly in state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thank you 🙇

return fmt.Errorf("error setting self_service_permissions: %s", err)
}

tags, err := keyvaluetags.WorkspacesListTags(conn, d.Id())
if err != nil {
Expand Down Expand Up @@ -205,7 +210,7 @@ func resourceAwsWorkspacesDirectoryDelete(d *schema.ResourceData, meta interface

err := workspacesDirectoryDelete(d.Id(), conn)
if err != nil {
return err
return fmt.Errorf("error deleting workspaces directory (%s): %s", d.Id(), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}
log.Printf("[DEBUG] Workspaces directory %q is deregistered", d.Id())

Expand Down Expand Up @@ -255,7 +260,8 @@ func workspacesDirectoryRefreshStateFunc(conn *workspaces.WorkSpaces, directoryI
if len(resp.Directories) == 0 {
return resp, workspaces.WorkspaceDirectoryStateDeregistered, nil
}
return resp, *resp.Directories[0].State, nil
directory := resp.Directories[0]
return directory, aws.StringValue(directory.State), nil
}
}

Expand Down
51 changes: 42 additions & 9 deletions aws/resource_aws_workspaces_directory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ func testSweepWorkspacesDirectories(region string) error {
// These tests need to be serialized, because they all rely on the IAM Role `workspaces_DefaultRole`.
func TestAccAwsWorkspacesDirectory(t *testing.T) {
testCases := map[string]func(t *testing.T){
"basic": testAccAwsWorkspacesDirectory_basic,
"subnetIds": testAccAwsWorkspacesDirectory_subnetIds,
"basic": testAccAwsWorkspacesDirectory_basic,
"disappears": testAccAwsWorkspacesDirectory_disappears,
"subnetIds": testAccAwsWorkspacesDirectory_subnetIds,
}
for name, tc := range testCases {
tc := tc
Expand All @@ -66,6 +67,7 @@ func TestAccAwsWorkspacesDirectory(t *testing.T) {
}

func testAccAwsWorkspacesDirectory_basic(t *testing.T) {
var v workspaces.WorkspaceDirectory
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point 👍 I guess this should be added in a separate PR with the actual usage of v in checks. I know it's a good pattern to test TF state along with the actual resource one, however, it doesn't make sense without those tests.

booster := acctest.RandString(8)
resourceName := "aws_workspaces_directory.main"

Expand All @@ -77,7 +79,7 @@ func testAccAwsWorkspacesDirectory_basic(t *testing.T) {
{
Config: testAccWorkspacesDirectoryConfigA(booster),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAwsWorkspacesDirectoryExists(resourceName),
testAccCheckAwsWorkspacesDirectoryExists(resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "subnet_ids.#", "2"),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.#", "1"),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.0.change_compute_type", "false"),
Expand All @@ -94,7 +96,7 @@ func testAccAwsWorkspacesDirectory_basic(t *testing.T) {
{
Config: testAccWorkspacesDirectoryConfigB(booster),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAwsWorkspacesDirectoryExists(resourceName),
testAccCheckAwsWorkspacesDirectoryExists(resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.#", "1"),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.0.change_compute_type", "false"),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.0.increase_volume_size", "true"),
Expand All @@ -109,7 +111,7 @@ func testAccAwsWorkspacesDirectory_basic(t *testing.T) {
{
Config: testAccWorkspacesDirectoryConfigC(booster),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAwsWorkspacesDirectoryExists(resourceName),
testAccCheckAwsWorkspacesDirectoryExists(resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.#", "1"),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.0.change_compute_type", "true"),
resource.TestCheckResourceAttr(resourceName, "self_service_permissions.0.increase_volume_size", "false"),
Expand All @@ -128,7 +130,30 @@ func testAccAwsWorkspacesDirectory_basic(t *testing.T) {
})
}

func testAccAwsWorkspacesDirectory_disappears(t *testing.T) {
var v workspaces.WorkspaceDirectory
booster := acctest.RandString(8)
resourceName := "aws_workspaces_directory.main"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsWorkspacesDirectoryDestroy,
Steps: []resource.TestStep{
{
Config: testAccWorkspacesDirectoryConfigA(booster),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAwsWorkspacesDirectoryExists(resourceName, &v),
testAccCheckAwsWorkspacesDirectoryDisappears(&v),
),
ExpectNonEmptyPlan: true,
},
},
})
}

func testAccAwsWorkspacesDirectory_subnetIds(t *testing.T) {
var v workspaces.WorkspaceDirectory
booster := acctest.RandString(8)
resourceName := "aws_workspaces_directory.main"

Expand All @@ -140,7 +165,7 @@ func testAccAwsWorkspacesDirectory_subnetIds(t *testing.T) {
{
Config: testAccWorkspacesDirectoryConfig_subnetIds(booster),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsWorkspacesDirectoryExists(resourceName),
testAccCheckAwsWorkspacesDirectoryExists(resourceName, &v),
resource.TestCheckResourceAttr(resourceName, "subnet_ids.#", "2"),
),
},
Expand Down Expand Up @@ -181,7 +206,13 @@ func testAccCheckAwsWorkspacesDirectoryDestroy(s *terraform.State) error {
return nil
}

func testAccCheckAwsWorkspacesDirectoryExists(n string) resource.TestCheckFunc {
func testAccCheckAwsWorkspacesDirectoryDisappears(v *workspaces.WorkspaceDirectory) resource.TestCheckFunc {
return func(s *terraform.State) error {
return workspacesDirectoryDelete(aws.StringValue(v.DirectoryId), testAccProvider.Meta().(*AWSClient).workspacesconn)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear from the test helper name that it just deletes the resource under the test and doesn't check anything actually. But the framework expects TestCheckFunc type, so nothing better to do here 🤷‍♂

}
}

func testAccCheckAwsWorkspacesDirectoryExists(n string, v *workspaces.WorkspaceDirectory) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
if !ok {
Expand All @@ -201,6 +232,8 @@ func testAccCheckAwsWorkspacesDirectoryExists(n string) resource.TestCheckFunc {
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth to check the length here too, isn't it?

if *resp.Directories[0].DirectoryId == rs.Primary.ID {
*v = *resp.Directories[0]

return nil
}

Expand Down Expand Up @@ -310,7 +343,7 @@ locals {
Name = "tf-testacc-workspaces-directory-%s"
}
}

resource "aws_subnet" "primary" {
vpc_id = "${aws_vpc.main.id}"
availability_zone_id = "${local.workspaces_az_ids[0]}"
Expand All @@ -320,7 +353,7 @@ locals {
Name = "tf-testacc-workspaces-directory-%s-primary"
}
}

resource "aws_subnet" "secondary" {
vpc_id = "${aws_vpc.main.id}"
availability_zone_id = "${local.workspaces_az_ids[1]}"
Expand Down