add support for Organizational Units #4207

afeld · 2018-04-14T05:10:43Z

Part of #571. Builds on #4229 - diff.

This pull request will add support for Organizational Units. ~~Still needs work - mostly wanted to submit to let folks know I'm working on it.~~

Get basic acceptance tests passing
Add update logic
Add the ability to specify a parent OU
Rebase

afeld · 2018-04-18T06:27:31Z

Ready for review! I'll rebase after/if #4229 is merged to reduce the size of the pull request. Thanks!

afeld · 2018-04-18T06:50:47Z

$ make testacc TEST=./aws TESTARGS="-run TestAccAWSOrganizations/Unit"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run TestAccAWSOrganizations/Unit -timeout 120m
=== RUN   TestAccAWSOrganizations
=== RUN   TestAccAWSOrganizations/Unit
=== RUN   TestAccAWSOrganizations/Unit/basic
=== RUN   TestAccAWSOrganizations/Unit/importBasic
=== RUN   TestAccAWSOrganizations/Unit/update
--- PASS: TestAccAWSOrganizations (49.83s)
    --- PASS: TestAccAWSOrganizations/Unit (49.83s)
        --- PASS: TestAccAWSOrganizations/Unit/basic (15.74s)
        --- PASS: TestAccAWSOrganizations/Unit/importBasic (13.32s)
        --- PASS: TestAccAWSOrganizations/Unit/update (20.76s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	64.820s

aws/resource_aws_organizations_unit.go

bflad · 2019-03-29T02:09:09Z

To provide an update, the maintainers will be circling back to looking through the various open Organizations data source pull requests to review them all at once after we determine which make the most sense for expected use cases.

@bflad You saw #4229? That's the data source on its own - I built this pull request on that one so that I could use the data source in the tests. I was thinking I would rebase this PR once the other is merged. That work?

Yes, I saw that pull request. 😄 My point is more centered around that data sources should not be required to provision infrastructure. That type of configuration within Terraform is discouraged where possible and the acceptance testing (analogous to real world configurations) should be able to stand up an Organization with Organizational Units from the ground up with resources only.

In this case, it seems like we should be able to add the Root ID(s) in the aws_organizations_organization resource to remove the data source requirement. Once this is done, the resource testing here references only resources and the data source testing references this resource to match the testing in the rest of the provider.

Hope this makes sense.

bryanlalexander · 2019-04-05T16:01:14Z

@bflad Is your contention that the PR from @afeld requires the data source in order to call the resource, and the solution is to return the values from a call to organizations:ListRoots in the Read and Create from aws_organizations_organizations?

bflad · 2019-04-05T18:24:48Z

@bryanlalexander that is spot on. 💯 If there isn't any movement on that implementation in the next few days I can likely get that submitted. Apologies this process has been dragging out for so long.

midacts · 2019-04-30T15:28:08Z

What are the next steps to get this added? : o

bflad · 2019-05-06T19:26:07Z

The aws_organizations_organization resource now has a roots attribute, which we can leverage in our acceptance testing and in the real world via "${aws_organizations_organization.XXX.roots.0.id}" 👍

@afeld please let us know if you can update this pull request to just the resource and we can provide quick turnaround on subsequent reviews. Thanks so much.

afeld · 2019-05-07T16:06:32Z

Sorry for the slow response, folks. I will try and get to it later this weekend, but can't promise. Also, haven't been working with Go or AWS Organizations for a while, so I'll be a bit rusty. If anyone else wants to pick it up and run with it in the meantime, you are more than welcome. Just reach out and I'll make you a committer on my fork.

…rce dependency

…st.go

bryanlalexander · 2019-05-08T04:08:38Z

@bflad we should be good to go now.

bflad · 2019-05-08T17:46:18Z

@afeld thank you so much for the heads up. We are all really appreciative of your work that went into this. Apologies again this was held up for so long.

@bryanlalexander 💯 🥇 thanks for those updates! Taking a deeper look now, but overall looks to be in pretty good shape.

bflad

Thanks @afeld and @bryanlalexander! This pull request was in great shape. There were a few minor feedback items noted below, but in the effort to save you additional work after having done so much to get this in its current state and allow this to be released tomorrow, I added one followup commit to all these (4238f78), which addresses these items including the more annoying/pedantic resource and function renaming. I hope you don't mind. 😅

Output from acceptance testing:

        --- PASS: TestAccAWSOrganizations/OrganizationalUnit (41.90s)
            --- PASS: TestAccAWSOrganizations/OrganizationalUnit/basic (17.42s)
            --- PASS: TestAccAWSOrganizations/OrganizationalUnit/Name (24.48s)

bflad · 2019-05-08T17:12:08Z

aws/resource_aws_organizations_test.go

@@ -14,6 +14,11 @@ func TestAccAWSOrganizations(t *testing.T) {
 		"Account": {
 			"basic": testAccAwsOrganizationsAccount_basic,
 		},
+		"Unit": {
+			"basic":       testAccAwsOrganizationsUnit_basic,
+			"importBasic": testAccAwsOrganizationsUnit_importBasic,


Nit: We are now preferring to add ImportState: true testing to all relevant resources tests instead of creating a separate test to cover additional configurations and remove mostly duplicate testing time/infrastructure. 👍 I don't want this to hold up this pull request though so just leaving this as a note here for future contributions!

bflad · 2019-05-08T17:20:00Z

aws/resource_aws_organizations_unit.go

+	input := &organizations.ListParentsInput{
+		ChildId: aws.String(childId),
+	}
+	resp, err := conn.ListParents(input)


The API documentation for this call notes:

Note

Always check the NextToken response parameter for a null value when calling a List* operation. These operations can occasionally return an empty set of results even when there are more results available. The NextToken response parameter value is null only when there are no more results to display.

It looks like the AWS Go SDK has a paginated function available here, ListParentsPages, which is likely the quickest option here.

bflad · 2019-05-08T17:23:24Z

aws/resource_aws_organizations_unit_test.go

+				Config: testAccAwsOrganizationsUnitConfig(name),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAwsOrganizationsUnitExists(resourceName, &unit),
+					resource.TestCheckResourceAttrSet(resourceName, "arn"),


Nit: We can use our helper function testAccMatchResourceAttrRegionalARN() here to verify the ARN contains a value like we are expecting here, e.g.

Suggested change

resource.TestCheckResourceAttrSet(resourceName, "arn"),

testAccMatchResourceAttrGlobalARN(resourceName, "arn", regexp.MustCompile(`ou/o-.+/ou-.+`)),

bflad · 2019-05-08T17:25:59Z

aws/resource_aws_organizations_unit_test.go

+			continue
+		}
+
+		exists, err := existsOrganization(conn)


I'm wondering if we could use just catch and ignore ErrCodeAWSOrganizationsNotInUseException with the DescribeOrganizationalUnit call below instead of making the additional API call here. 👍

bflad · 2019-05-08T17:27:17Z

aws/resource_aws_organizations_unit_test.go

+
+		if err != nil {
+			if isAWSErr(err, organizations.ErrCodeOrganizationalUnitNotFoundException, "") {
+				return nil


We should use continue here to ensure that if the test state contains multiple aws_organizations_organizational_unit resources that they are all checked appropriately. 😄

Suggested change

return nil

continue

bflad · 2019-05-08T17:33:14Z

aws/provider.go

@@ -592,6 +592,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_organizations_account":                               resourceAwsOrganizationsAccount(),
 			"aws_organizations_policy":                                resourceAwsOrganizationsPolicy(),
 			"aws_organizations_policy_attachment":                     resourceAwsOrganizationsPolicyAttachment(),
+			"aws_organizations_unit":                                  resourceAwsOrganizationsUnit(),


We have been cautiously including full naming for AWS service "objects", even if the naming becomes a little redundant to prevent any future issues where other similarly named "objects" are added. In this case, we would prefer aws_organizations_organizational_unit to match the Organizations service and service API concept of "Organizational Unit", in case the Organizations service ever releases other types of "Unit".

Suggested change

"aws_organizations_unit": resourceAwsOrganizationsUnit(),

"aws_organizations_organizational_unit": resourceAwsOrganizationsOrganizationalUnit(),

bflad · 2019-05-08T17:34:24Z

website/docs/d/organizations_unit.html.markdown

+    Provides details about an organizational unit
+---
+
+# Data Source: aws_organizations_unit


Leftover documentation from previous iteration of pull request. 👍

bflad · 2019-05-08T17:36:50Z

website/docs/r/organizations_policy_attachment.html.markdown

 resource "aws_organizations_policy_attachment" "root" {
  policy_id = "${aws_organizations_policy.example.id}"
-  target_id = "r-12345678"
+  target_id = "${data.aws_organizations_unit.root.id}"


This change is unrelated to this pull request and probably a leftover from when it was initially implemented with the data source, but I believe our intention here will be to show the new aws_organizations_organization roots attribute 😄

Suggested change

target_id = "${data.aws_organizations_unit.root.id}"

target_id = "${aws_organizations_unit.example.roots.0.id}"

bflad · 2019-05-08T17:37:47Z

website/docs/r/organizations_unit.html.markdown

+}
+
+resource "aws_organizations_unit" "tenants" {
+  parent_id = "${aws_organizations_organization.roots.0.id}"


Missing resource name 😉

Suggested change

parent_id = "${aws_organizations_organization.roots.0.id}"

parent_id = "${aws_organizations_organization.org.roots.0.id}"

bflad · 2019-05-08T17:38:50Z

website/docs/r/organizations_unit.html.markdown

+
+## Import
+
+The AWS organization can be imported by using the `id`, e.g.


Suggested change

The AWS organization can be imported by using the `id`, e.g.

AWS Organizations Organizational Units can be imported by using the `id`, e.g.

Output from acceptance testing: ``` --- PASS: TestAccAWSOrganizations/OrganizationalUnit (41.90s) --- PASS: TestAccAWSOrganizations/OrganizationalUnit/basic (17.42s) --- PASS: TestAccAWSOrganizations/OrganizationalUnit/Name (24.48s) ```

bflad · 2019-05-10T00:37:27Z

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

ghost · 2020-03-30T17:22:41Z

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!

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 14, 2018

afeld force-pushed the ou branch from 9149c11 to b8c7162 Compare April 17, 2018 06:26

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018

afeld force-pushed the ou branch from b8c7162 to 7f58533 Compare April 17, 2018 07:15

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018

afeld mentioned this pull request Apr 17, 2018

add data source for retrieving the root Organizational Unit #4229

Merged

bflad added new-resource Introduces a new resource. new-data-source Introduces a new data source. service/organizations Issues and PRs that pertain to the organizations service. labels Apr 17, 2018

afeld force-pushed the ou branch from 7f58533 to 22b1ba0 Compare April 18, 2018 05:34

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 18, 2018

afeld force-pushed the ou branch from 22b1ba0 to afb07b7 Compare April 18, 2018 05:45

ghost added size/L Managed by automation to categorize the size of a PR. size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Apr 18, 2018

afeld changed the title ~~[WIP] add support for Organizational Units~~ add support for Organizational Units Apr 18, 2018

ghost added the size/XL Managed by automation to categorize the size of a PR. label Apr 18, 2018

breser mentioned this pull request Apr 28, 2018

[WIP] Add an aws_organizations_account_ids data source #4384

Closed

afeld force-pushed the ou branch from 6c9e1d1 to 85d4121 Compare May 1, 2018 01:53

ghost added the size/XL Managed by automation to categorize the size of a PR. label May 1, 2018

afeld force-pushed the ou branch from 85d4121 to d98e5a6 Compare May 1, 2018 02:14

ghost added the size/XL Managed by automation to categorize the size of a PR. label May 1, 2018

afeld force-pushed the ou branch from d98e5a6 to c61a06c Compare May 1, 2018 05:38

ghost added the size/XL Managed by automation to categorize the size of a PR. label May 1, 2018

afeld commented May 1, 2018

View reviewed changes

aws/resource_aws_organizations_unit.go Show resolved Hide resolved

afeld mentioned this pull request May 1, 2018

[WIP] support setting the parent for organization accounts #4405

Closed

afeld mentioned this pull request May 9, 2018

Feature request: AWS Organizations support #571

Closed

bflad mentioned this pull request May 30, 2018

[WIP] New Data Sources: aws_organizations_* #4688

Closed

This was referenced Apr 21, 2019

[WIP] adds roots attribute to the aws_organizations_organization resource #8398

Closed

adds roots attribute to the aws_organizations_organization resource #8399

Merged

murraypete mentioned this pull request Apr 24, 2019

AWS Organizations Move Account support #8281

Closed

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label May 6, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label May 7, 2019

removes data source dependency for aws_organizations_unit resource

68165cd

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 May 8, 2019

bryanlalexander added 2 commits May 7, 2019 22:23

updates aws_organizations_unit resource on website, removing data sou…

206cd66

…rce dependency

Merge branch 'master' into ou

cf2ecb2

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 May 8, 2019

bryanlalexander added 2 commits May 7, 2019 22:45

resolves two pointer derefs inside resource_aws_organizations_unit_te…

fd0b5c2

…st.go

adds additional error check for OU not found

6ab939b

bflad added this to the v2.10.0 milestone May 8, 2019

bflad reviewed May 8, 2019

View reviewed changes

bflad merged commit 6ab939b into hashicorp:master May 8, 2019

bflad added a commit that referenced this pull request May 8, 2019

Update CHANGELOG for #4207

ab093ee

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for Organizational Units #4207

add support for Organizational Units #4207

afeld commented Apr 14, 2018 •

edited

Loading

afeld commented Apr 18, 2018 •

edited

Loading

afeld commented Apr 18, 2018

bflad commented Mar 29, 2019

bryanlalexander commented Apr 5, 2019

bflad commented Apr 5, 2019

midacts commented Apr 30, 2019

bflad commented May 6, 2019

afeld commented May 7, 2019

bryanlalexander commented May 8, 2019

bflad commented May 8, 2019

bflad left a comment

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad May 8, 2019

bflad commented May 10, 2019

ghost commented Mar 30, 2020

	resource.TestCheckResourceAttrSet(resourceName, "arn"),
	testAccMatchResourceAttrGlobalARN(resourceName, "arn", regexp.MustCompile(`ou/o-.+/ou-.+`)),

	"aws_organizations_unit": resourceAwsOrganizationsUnit(),
	"aws_organizations_organizational_unit": resourceAwsOrganizationsOrganizationalUnit(),

	target_id = "${data.aws_organizations_unit.root.id}"
	target_id = "${aws_organizations_unit.example.roots.0.id}"

	parent_id = "${aws_organizations_organization.roots.0.id}"
	parent_id = "${aws_organizations_organization.org.roots.0.id}"


		## Import

		The AWS organization can be imported by using the `id`, e.g.

	The AWS organization can be imported by using the `id`, e.g.
	AWS Organizations Organizational Units can be imported by using the `id`, e.g.

add support for Organizational Units #4207

add support for Organizational Units #4207

Conversation

afeld commented Apr 14, 2018 • edited Loading

afeld commented Apr 18, 2018 • edited Loading

afeld commented Apr 18, 2018

bflad commented Mar 29, 2019

bryanlalexander commented Apr 5, 2019

bflad commented Apr 5, 2019

midacts commented Apr 30, 2019

bflad commented May 6, 2019

afeld commented May 7, 2019

bryanlalexander commented May 8, 2019

bflad commented May 8, 2019

bflad 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

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

bflad commented May 10, 2019

ghost commented Mar 30, 2020

afeld commented Apr 14, 2018 •

edited

Loading

afeld commented Apr 18, 2018 •

edited

Loading