aws_workspaces_directory: Assign IP Group

[Tensho]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #434
Closes #14669

Release note for CHANGELOG:

aws_workspaces_directory: Assign IP group

Acceptance Test

$ make testacc TEST=./aws TESTARGS='-run=TestAccAwsWorkspacesDirectory_ipGroupIds'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsWorkspacesDirectory_ipGroupIds -timeout 120m
=== RUN   TestAccAwsWorkspacesDirectory_ipGroupIds
=== PAUSE TestAccAwsWorkspacesDirectory_ipGroupIds
=== CONT  TestAccAwsWorkspacesDirectory_ipGroupIds
--- PASS: TestAccAwsWorkspacesDirectory_ipGroupIds (903.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	906.724s

[DrFaust92]

Hey @Tensho, see comments below

[Tensho]

@DrFaust92 Thank you for review, I'll make appropriate changes shortly 🙇

[Tensho]

@DrFaust92 I caught one issue with workspace directory and IP group mutual dependency. If I remove IP group resource and it's reference in workspaces directory declaration, then I get an error like this:

Error: Error Deleting Workspaces IP Group: ResourceAssociatedException: Can not delete IP Group with GroupId - wsipg-5g88k3vby because its attached with these Directories d-9267047639

If I understand correctly that happens, because workspace directory depends on IP group implicitly referencing it in ip_group_ids, so Terraform deletes dependent resource (IP group) first. But IP group can't be deleted until it is associated with any directory. Sounds like I should declare workspace directory dependency for IP group, but that makes a cycle and violates acyclic graph constraint:

resource "aws_workspaces_ip_group" "test" {
  depends_on = ["${aws_workspaces_directory.test.id}"]
}

resource "aws_workspaces_directory" "test" {
  directory_id = "${aws_directory_service_directory.test.id}"
  ip_group_ids = ["${aws_workspaces_ip_group.test.id}"]
}

I have only one idea – disassociate IP group from directory, but it requires to call DescribeWorkspaceDirectories API action and find the directory associated with IP group at client side – DescribeIpGroups API action doesn't return back reference to directory. Feels not idiomatic. Could you provide an example for already implemented resources with similar behavior?

[DrFaust92]

There is something similar in API Gateway Usage plan resource, care to take a look for inspiration?
im guessing there isn't much to do to beside listing all directories and dissociating all directories from that ip group.

maybe it's worth splitting the association itself to a separate resource. and leave the ip_groups in this resource computed.

wdyt?

[Tensho]

TBH, I haven't found anything could help in aws_api_gateway_usage_plan resource. UpdateUsagePlan action handles all change together with PatchOperations attribute, which is designed to handle update operations in ordered manner.

---

I doubt aws_workspaces_ip_group_attachment makes any difference and solves the cycle issue.

resource "aws_workspaces_directory" "test" {
  directory_id = "${aws_directory_service_directory.test.id}"
}

resource "aws_workspaces_ip_group" "test" {}

resource "aws_workspaces_ip_group_attachment" "test" {
  directory_id = "${aws_directory_service_directory.test.id}"
  ip_group_ids = ["${aws_workspaces_ip_group.test.id}"]
}

As far as attachment refers to directory and ip group resources, they are destroyed first by Terraform. Nor directory, neither ip group can be destroyed until disassociation is completed.

---

Seems like we should go with dumb aws_workspaces_ip_group resource disassociation on Delete().

@DrFaust92 Thank you for the insight and please don't hesitate to drop any comment regarding my plan 🙇 I'm not a seasoned Terraform provider developer yet, so appreciate any feedback 🙂

[DrFaust92]

Take a look a the delete function in usage plan. It deletes the associated stages.

I think your plan seems fine to me, @gdavison want if you want to chime in on this.

[Tensho]

I've just bumped with another idea – reverse (inject) dependency between resources – drop aws_workspaces_directory.ip_group_ids attribute in favor of aws_workspaces_ip_group.directory_id one.

resource "aws_workspaces_directory" "test" {
  directory_id = "${aws_directory_service_directory.test.id}"
}

resource "aws_workspaces_ip_group" "a" {
  directory_id = "${aws_workspaces_directory.test.id}"
}

resource "aws_workspaces_ip_group" "b" {
  directory_id = "${aws_workspaces_directory.test.id}"
}

I know AWS Terraform provider tries to follow AWS API as close as possible, but this option looks more explicit than implicit group disassociation.

@DrFaust92 @bflad @gdavison What do you think?

[bensunny1]

Thanks for working on this @Tensho. I personally prefer the attachment resource option, this would be more intuitive and most similar to other resources

[Tensho]

@bensunny1 TBH, I can't grasp how attachment may solve the problem I mentioned above in this thread. If I remove IP group attachment and IP group from the configuration all together, then both resources will be marked as orphaned and deleted in undesired order (first IP group, then IP group attachment) by Terraform. I guess it's OK for other attachments and target resources, e.g. aws_iam_role_policy_attachment and aws_iam_role/aws_iam_policy, but it doesn't work for IP group API. Or maybe I don't understand how Terraform builds the graph nodes for destroy operations...

@apparentlymart @bflad Please help us to sort out this chicken-egg question 🙇

[bensunny1]

@Tensho You're right, #14451 (comment) i think is a pretty good idea and deals with the problem

[gdavison]

I think the best option here is to add the DisassociateIpGroups() call to the aws_workspaces_ip_group delete operation as well. There isn't currently a way to model these mutual relationships in Terraform, so we're stuck with brute-forcing it.

[Tensho]

@gdavison Got it. Added ip group disassociation from a directory on delete.

[Tensho]

@DrFaust92 Fixed review comments, please take a look now 🙇‍♂️

[Tensho]

$ make testacc TEST=./aws TESTARGS='-run=TestAccAwsWorkspacesDirectory_ipGroupIds'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsWorkspacesDirectory_ipGroupIds -timeout 120m
=== RUN   TestAccAwsWorkspacesDirectory_ipGroupIds
=== PAUSE TestAccAwsWorkspacesDirectory_ipGroupIds
=== CONT  TestAccAwsWorkspacesDirectory_ipGroupIds
--- PASS: TestAccAwsWorkspacesDirectory_ipGroupIds (64.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.881s

[gdavison]

LGTM 🚀

--- PASS: TestAccAwsWorkspacesIpGroup_disappears (65.56s)
--- PASS: TestAccAwsWorkspacesIpGroup_basic (113.13s)
--- PASS: TestAccAwsWorkspacesIpGroup_tags (128.86s)
--- PASS: TestAccAwsWorkspacesDirectory_subnetIds (727.54s)
--- PASS: TestAccAwsWorkspacesDirectory_disappears (747.15s)
--- PASS: TestAccAwsWorkspacesDirectory_workspaceCreationProperties (747.94s)
--- PASS: TestAccAwsWorkspacesDirectory_selfServicePermissions (749.95s)
--- PASS: TestAccAwsWorkspacesIpGroup_MultipleDirectories (761.30s)
--- PASS: TestAccAwsWorkspacesDirectory_basic (771.07s)
--- PASS: TestAccAwsWorkspacesDirectory_ipGroupIds (794.51s)
--- PASS: TestAccAwsWorkspacesDirectory_tags (818.72s)

[ghost]

This has been released in version 3.17.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!