New Resource: aws_workspaces_workspace #11608

Tensho · 2020-01-15T13:39:07Z

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

Release note for CHANGELOG:

New Data Source: aws_workspaces_workspace (#11608)

Example

data "aws_workspaces_directory" "main" {
  directory_id = "d-ten5h0y19"
}

data "aws_workspaces_bundle" "value_windows" {
  bundle_id = "b-ten5h0y19"
}

resource "aws_workspaces_workspace" "jhon.doe" {
  directory_id = "${data.aws_workspaces_directory.main.id}"
  bundle_id = "${data.aws_workspaces_bundle.value_windows.id}"
  user_name = "jhon.doe"

  root_volume_encryption_enabled = true
  user_volume_encryption_enabled = true
  volume_encryption_key = "aws/workspaces"

  workspace_properties {
    compute_type_name = "VALUE" 
    root_volume_size_gib = 80
    running_mode = "AUTO_STOP"
    running_mode_auto_stop_timeout_in_minutes = 120
    user_volume_size_gib = 10
  }
  
  tags = {
    Department = "IT"
  }
}

Acceptance Tests

$ make testacc TEST=./aws TESTARGS='-run=TestAccAwsWorkspacesWorkspace'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsWorkspacesWorkspace -timeout 120m
=== RUN   TestAccAwsWorkspacesWorkspace_basic
--- PASS: TestAccAwsWorkspacesWorkspace_basic (1483.87s)
=== RUN   TestAccAwsWorkspacesWorkspace_Tags
--- PASS: TestAccAwsWorkspacesWorkspace_Tags (1546.16s)
=== RUN   TestAccAwsWorkspacesWorkspace_WorkspaceProperties
--- PASS: TestAccAwsWorkspacesWorkspace_WorkspaceProperties (1667.00s)
=== RUN   TestAccAwsWorkspacesWorkspace_validateRootVolumeSize
--- PASS: TestAccAwsWorkspacesWorkspace_validateRootVolumeSize (2.48s)
=== RUN   TestAccAwsWorkspacesWorkspace_validateUserVolumeSize
--- PASS: TestAccAwsWorkspacesWorkspace_validateUserVolumeSize (3.28s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	4704.253s

Considerations

There is a warning which I'm not sure how I should address:

    The following problems may be the cause of any confusing errors from downstream operations:
      - .volume_encryption_key: was null, but now cty.StringVal("")

What is the general strategy for absent attributes?

References

Support for AWS Workspaces

website/aws.erb

aws/resource_aws_workspace.go

marcosdiez · 2020-03-13T17:36:45Z

Hey @Tensho, your PR now has a conflict (and fixing it is trivial). Could you please do so, to increase the chances of it getting merged? Thanks!

Tensho · 2020-03-13T22:49:11Z

Rebased, but still have to sort out defaults for attributes of the Set type. Asked more clarification in #142. Will resolve other issues after that.

gdavison

Thanks for the PR, @Tensho. I've gone through and added a number of changes that should be made for this.

aws/provider.go

aws/resource_aws_workspace.go

gdavison · 2020-03-13T21:47:41Z

aws/resource_aws_workspace_test.go

+	})
+}
+
+func testSweepWorkspaces(region string) error {


The aws_workspaces_directory sweeper will probably need to add a dependency on this sweeper

Would it? You can build aws_workspaces_directory without needing an aws_workspaces_workspace. Maybe I'm not understanding sweepers.

I guess @gdavison means opposite direction – aws_workspaces_workspace sweeper depends on aws_workspaces_directory one.

Yes, when we add a dependency to a sweeper, it means that the other sweeper needs to be run before this one, or it will return an error.

aws/resource_aws_workspace.go

gdavison · 2020-03-14T05:55:32Z

aws/resource_aws_workspace.go

+							Type:         schema.TypeInt,
+							Optional:     true,
+							Default:      80,
+							ValidateFunc: validation.IntInSlice([]int{80, 175}),


Based on the documentation at https://docs.aws.amazon.com/workspaces/latest/adminguide/modify-workspaces.html, it looks like the valid values for root_volume_size_gib are 80 or 175 to 2000.

If root_volume_size_gib is 80, then user_volume_size_gib is one of 10, 50, or 100.

If root_volume_size_gib is 175 to 2000, then user_volume_size_gib is 100 to 2000
Validation should be updated to allow the higher values.
Also, root and user volumes cannot change size at the same time. The code should deal with that case.

I don't' remember exactly why, but I failed to accomplish this. Will make another attempt and share the feedback. Thanks.

@gdavison Ah, I remembered, how can I refer to attribute A in the validation func of attribute B? Or should I handle this in resource Update func?

@gdavison The question about validation func for dependent attributes is still actual for me. Please could you answer the questions?

For the ValidateFuncs in the resource schema, you can use validation.Any() to chain together validation functions.

For root_volume_size_gib, this could be

ValidateFunc: validation.Any( validation.IntInSlice([]int{80}), validation.IntBetween(175, 2000), )

For final validation that the two fields are compatible, we'll have to let the AWS API handle it.

Done. I've added one acceptance for user volume size validation, but I doubt this is the right way to test all possible cases. Please could you refer me to the good validation unit test example?

aws/resource_aws_workspace_test.go

Tensho · 2020-03-15T07:09:28Z

@gdavison I'll do the changes according to your review this week. Thank you very much 🙇

aws/resource_aws_workspaces_workspace.go

Tensho · 2020-03-19T21:17:22Z

@gdavison @bflad @ewbankkit I iterate on Update function tests and it takes ages to wait for workspace launch. Could you suggest the way to import already prepared workspace to the state and test only the update phase in the subsequent test case without any destroy after all? I'd consider such a workaround only for debug purposes and remove it in favor of full configuration at the end. I'd expect something like this should work:

Steps: []resource.TestStep{
	{
		Destroy: false,
		ResourceName: "aws_workspaces_workspace.test",
		ImportState:   true,
		ImportStateId: "ws-h16p65ykp", // already prepeared workspace
	},
	{
		Destroy: false,
		Config: testAccWorkspacesWorkspaceConfig(booster),
		Check: resource.ComposeAggregateTestCheckFunc(
                   resource.TestCheckResourceAttr(resourceName, "directory_id", "d-99673d2890"),
                )
...
func testAccWorkspacesWorkspaceConfg() string {
	return fmt.Sprintf(`
resource "aws_workspaces_workspace" "test" {
  bundle_id = "wsb-98rvbl24y" # prepeared bundle
  directory_id = "d-99673d2890" # prepeared directory
  user_name = "andrii.babichev" # prepared user
}
`)
}

However, test framework still creates the resource on the second test case.

What is generally the best way to work on long-launching resources?

BTW, I switched workspace_properties type from TypeSet to TypeList and everything is OK, except the fact I still don't understand the proper case for the Set type 🙂

Tensho · 2020-03-26T22:39:21Z

@gdavison Good news, I'm on the finish line with this resource, hope to resolve some workspace properties modification snags with AWS Support and then tidy up tests to be self-sufficient. I guess I learned about Terraform providers here much more than from all other resources all together 🙂 Appreciate if you answer the questions left in the discussion 🙇

aws/resource_aws_workspaces_workspace.go

aws/resource_aws_workspaces_workspace_test.go

Tensho · 2020-05-04T10:06:14Z

@gdavison @DrFaust92 Thank you for the feedback 🙇 I've made required changes, please check out the current state.

DrFaust92 · 2020-05-04T13:31:06Z

@Tensho, tagging logic fix is handled by #13089. can you the currently generated functions (this will cause failure but it will be fixed across all workspaces resources with the aforementioned PR)
as seen in https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_workspaces_directory.go

DrFaust92 · 2020-05-04T13:31:40Z

alternatively #13089 needs to be merged before this

Tensho · 2020-05-04T13:39:00Z

@DrFaust92 I'm pretty fine with prior #13089 merge, just whatever makes this resource public appearance sooner.

Tensho · 2020-05-05T09:06:33Z

@bflad @gdavison Please could you take a look? 🙇

gdavison

It's looking great! A couple changes to make, and I'm going to merge #13089 first. This might create some merge conflicts

gdavison · 2020-05-01T23:17:52Z

aws/resource_aws_workspaces_directory.go

 		DirectoryId: aws.String(id),
 	}
+	err := conn.DescribeWorkspacesPages(input, func(resp *workspaces.DescribeWorkspacesOutput, _ bool) bool {


The delete function for a resource should only handle deleting itself. Terraform will use the resource dependency graph to handle the order. However, the sweeper for aws_workspaces_directory should add aws_workspaces_workspace to its Dependencies array.

You shared knowledge and examples about sweepers and now I understand the concept better. Thank you 🙇 Fixed workspaces_directory sweeper depend on worksapces_workspace.

gdavison · 2020-05-01T23:22:12Z

examples/workspaces/main.tf

@@ -33,6 +33,32 @@ resource "aws_workspaces_directory" "main" {
  subnet_ids   = ["${aws_subnet.private-a.id}", "${aws_subnet.private-b.id}"]
 }

+data "aws_workspaces_bundle" "value_windows" {
+  bundle_id = "b-ten5h0y19"


Examples should be runnable with minimal configuration by users. The bundle ID should be a variable, rather than hardcoded.

Fixed to the public "Value with Windows 10 (English)" bundle

gdavison · 2020-05-05T22:03:12Z

aws/resource_aws_workspaces_workspace_test.go

+# resource "aws_iam_role" "workspaces-default" {
+#   name               = "workspaces_DefaultRole"
+#   assume_role_policy = data.aws_iam_policy_document.workspaces.json
+# }
+# 
+# resource "aws_iam_role_policy_attachment" "workspaces-default-service-access" {
+#   role       = aws_iam_role.workspaces-default.name
+#   policy_arn = "arn:aws:iam::aws:policy/AmazonWorkSpacesServiceAccess"
+# }
+# 
+# resource "aws_iam_role_policy_attachment" "workspaces-default-self-service-access" {
+#   role       = aws_iam_role.workspaces-default.name
+#   policy_arn = "arn:aws:iam::aws:policy/AmazonWorkSpacesSelfServiceAccess"
+# }


If these aren't needed, we can just remove them

Sorry, that's leaked from git stash. I have to comment out this section every time I run acctests, because the target account already have this role in place. I've compared the current workspaces default role inline policies and found some changed since I started this PR:

AmazonWorkSpacesServiceAccess –> SkyLightServiceAccess

AmazonWorkSpacesSelfServiceAccess –> SkyLightSelfServiceAccess

However manged policies (starts with AmazonWorkSpace) are still in place and identical to the inline analogs.

Fixed.

If you are curious as me what does SkyLight mean – it's a AWS workspace agent written in Go 😸

Tensho · 2020-05-06T07:50:32Z

@gdavison I've rebased my branch to the lastest upstream master. Don't hesitate to ask for any other changes 🙇

gdavison · 2020-05-08T00:09:00Z

Hi @Tensho, it's looking great. I'm getting some errors in our acceptance test accounts. Something about subnets not being found.

I'll investigate those tomorrow, and we can get this merged!

gdavison

I've found the issue. It's related to the data source data "aws_subnet" "default" {...}. I'm going to make the fix needed and merge it in once it's done.
I should be able to reuse the set up configuration from https://github.com/terraform-providers/terraform-provider-aws/blob/6cf84739011aa60bb8aa0c501b08dfc280716af7/aws/resource_aws_workspaces_directory_test.go#L370
Thanks for contributing! 🚀

Tensho · 2020-05-08T20:43:36Z

@gdavison Please could you explain the issue with the default subnet? I intentionally switched to default VPC and subnets, because the test environment cleanup (destroy) constantly failed on VPC resources decommission.

gdavison · 2020-05-11T22:57:11Z

When the code was using the subnets data source, it was failing with a subnet not found error. Possibly because our account doesn't have a default subnet it all AZs in us-west-2.

It took me a while, but I finally tracked down the error when using a new VPC for the test. If the role workspaces_DefaultRole doesn't have the policy arn:aws:iam::aws:policy/AmazonWorkSpacesServiceAccess attached, it can't delete the ENI created for the Workspace. So I added

  depends_on = [
    aws_iam_role_policy_attachment.workspaces-default-service-access,
  ]

Tensho · 2020-05-12T07:59:45Z

@gdavison Thank you very much for your work and I'm so happy we can finally declare workspaces with Terraform 🙇 Have you found the bug with TF_LOG=DEBUG?

Let me check one moment with you. If I understood you right, workspaces directory and default role have a race due to the parallel destruction. The default role destruction is always faster workspaces directory one, that's why we need explicitly declare dependency. However, I'd expect workspace directory should depend onworkspaces_DefaultRole role instead of workspace depends on the required policy.

resource "aws_iam_role" "workspaces-default" {
  name               = "workspaces_DefaultRole"
  assume_role_policy = data.aws_iam_policy_document.workspaces.json
}

resource "aws_iam_role_policy_attachment" "workspaces-default-service-access" {
  role       = aws_iam_role.workspaces-default.name
  policy_arn = "arn:aws:iam::aws:policy/AmazonWorkSpacesServiceAccess"
}

GOOD
resource "aws_workspaces_workspace" "test" {
  ...
  depends_on = [
    aws_iam_role_policy_attachment.workspaces-default-service-access,
  ]
}

BETTER
resource "aws_workspaces_directory" "test" {
  ...
  depends_on = [
    aws_iam_role.workspaces-default,
  ]
}

What's your take on this?

BTW, I didn't know about composeConfig() func 👍

ghost · 2020-05-15T01:04:08Z

This has been released in version 2.62.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 · 2020-06-11T17:10:07Z

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!

Tensho requested a review from a team January 15, 2020 13:39

ewbankkit mentioned this pull request Jan 22, 2020

Refactor WorkSpaces resources to use keyvaluetags package #11645

Merged

ewbankkit reviewed Jan 25, 2020

View reviewed changes

website/aws.erb Outdated Show resolved Hide resolved

slapula suggested changes Jan 29, 2020

View reviewed changes

aws/resource_aws_workspace.go Outdated Show resolved Hide resolved

ewbankkit reviewed Feb 1, 2020

View reviewed changes

aws/resource_aws_workspace.go Outdated Show resolved Hide resolved

Tensho force-pushed the r-workspaces-workspace branch from 6735042 to 96e390f Compare February 3, 2020 18:01

Tensho force-pushed the r-workspaces-workspace branch from 4507ac4 to 6f7ab0c Compare March 13, 2020 20:23

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Mar 13, 2020

Tensho mentioned this pull request Mar 13, 2020

Default TypeList / TypeSet hashicorp/terraform-plugin-sdk#142

Open

gdavison requested changes Mar 14, 2020

View reviewed changes

ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Mar 16, 2020

Tensho commented Mar 17, 2020

View reviewed changes

aws/resource_aws_workspaces_workspace.go Show resolved Hide resolved

Tensho changed the title ~~New Resource: aws_workspace~~ New Resource: aws_workspaces_workspace Mar 17, 2020

thatderek mentioned this pull request Mar 21, 2020

Expand functionality of data.aws_workspaces_bundle to do local filtering #12496

Closed

jonjozwiak mentioned this pull request Mar 25, 2020

Add new check: Amazon WorkSpaces - Check encryption at rest bridgecrewio/checkov#141

Closed

Tensho force-pushed the r-workspaces-workspace branch from 9699935 to f0a5ea9 Compare March 26, 2020 22:30

Tensho force-pushed the r-workspaces-workspace branch from f0a5ea9 to f4251a8 Compare April 2, 2020 15:06

gdavison removed the needs-triage Waiting for first response or review from a maintainer. label May 1, 2020

DrFaust92 reviewed May 2, 2020

View reviewed changes

aws/resource_aws_workspaces_workspace.go Outdated Show resolved Hide resolved

DrFaust92 suggested changes May 2, 2020

View reviewed changes

aws/resource_aws_workspaces_workspace_test.go Show resolved Hide resolved

Tensho force-pushed the r-workspaces-workspace branch from 433a486 to 0c59e3a Compare May 4, 2020 07:50

Tensho force-pushed the r-workspaces-workspace branch from 34a430f to 595a9bc Compare May 5, 2020 09:04

gdavison requested changes May 5, 2020

View reviewed changes

Tensho force-pushed the r-workspaces-workspace branch from 595a9bc to 25cb6ae Compare May 6, 2020 07:42

Tensho added 2 commits May 6, 2020 10:45

New resource: aws_workspaces_workspace

b6d00a2

Add workspaces_directory sweeper dependency on workspaces_workspace

0762d2b

Tensho force-pushed the r-workspaces-workspace branch from 25cb6ae to 0762d2b Compare May 6, 2020 07:45

Fix workspaces default role

264edcc

gdavison approved these changes May 8, 2020

View reviewed changes

gdavison added this to the v2.62.0 milestone May 8, 2020

gdavison merged commit 264edcc into hashicorp:master May 12, 2020

gdavison added a commit that referenced this pull request May 12, 2020

Update CHANGELOG for #11608

4a53185

Tensho deleted the r-workspaces-workspace branch May 12, 2020 07:28

ewbankkit mentioned this pull request May 13, 2020

Fix aws_workspaces_workspace Examples Check linting error #13300

Merged

ghost locked and limited conversation to collaborators Jun 11, 2020

New Resource: aws_workspaces_workspace #11608

New Resource: aws_workspaces_workspace #11608

Conversation

Tensho commented Jan 15, 2020 • edited Loading

Community Note

Example

Acceptance Tests

Considerations

References

marcosdiez commented Mar 13, 2020

Tensho commented Mar 13, 2020

gdavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tensho Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tensho May 4, 2020 • edited Loading

Choose a reason for hiding this comment

Tensho commented Mar 15, 2020

Tensho commented Mar 19, 2020 • edited Loading

Tensho commented Mar 26, 2020 • edited Loading

Tensho commented May 4, 2020

DrFaust92 commented May 4, 2020

DrFaust92 commented May 4, 2020

Tensho commented May 4, 2020

Tensho commented May 5, 2020

gdavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tensho May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tensho May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Tensho May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Tensho commented May 6, 2020

gdavison commented May 8, 2020 • edited Loading

gdavison left a comment

Choose a reason for hiding this comment

Tensho commented May 8, 2020

gdavison commented May 11, 2020

Tensho commented May 12, 2020

ghost commented May 15, 2020

ghost commented Jun 11, 2020

Tensho commented Jan 15, 2020 •

edited

Loading

Tensho Mar 16, 2020 •

edited

Loading

Tensho May 4, 2020 •

edited

Loading

Tensho commented Mar 19, 2020 •

edited

Loading

Tensho commented Mar 26, 2020 •

edited

Loading

Tensho May 6, 2020 •

edited

Loading

Tensho May 6, 2020 •

edited

Loading

Tensho May 6, 2020 •

edited

Loading

gdavison commented May 8, 2020 •

edited

Loading