From 4841f38814bc50d303029a359d43f53d7fecaacc Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Sun, 25 Nov 2018 23:56:35 -0500 Subject: [PATCH 1/2] tests/resource/aws_organizations_organization: Refactor and enhance existing testing Changes: * tests/resource/aws_organizations_organization: Perform stricter attribute checking * tests/resource/aws_organizations_organization: Perform import testing in all acceptance tests Output from acceptance testing: ``` --- PASS: TestAccAWSOrganizations (26.87s) --- PASS: TestAccAWSOrganizations/Organization (26.87s) --- PASS: TestAccAWSOrganizations/Organization/basic (12.68s) --- PASS: TestAccAWSOrganizations/Organization/FeatureSet (14.18s) ``` --- aws/provider_test.go | 29 ++++++++- ...rce_aws_organizations_organization_test.go | 61 ++++++++----------- aws/resource_aws_organizations_test.go | 5 +- 3 files changed, 56 insertions(+), 39 deletions(-) diff --git a/aws/provider_test.go b/aws/provider_test.go index 927fbdb1bd4..95e712fa318 100644 --- a/aws/provider_test.go +++ b/aws/provider_test.go @@ -99,6 +99,13 @@ func testAccAwsProviderAccountID(provider *schema.Provider) string { return client.accountid } +// testAccCheckResourceAttrAccountID ensures the Terraform state exactly matches the account ID +func testAccCheckResourceAttrAccountID(resourceName, attributeName string) resource.TestCheckFunc { + return func(s *terraform.State) error { + return resource.TestCheckResourceAttr(resourceName, attributeName, testAccGetAccountID())(s) + } +} + // testAccCheckResourceAttrRegionalARN ensures the Terraform state exactly matches a formatted ARN with region func testAccCheckResourceAttrRegionalARN(resourceName, attributeName, arnService, arnResource string) resource.TestCheckFunc { return func(s *terraform.State) error { @@ -113,7 +120,7 @@ func testAccCheckResourceAttrRegionalARN(resourceName, attributeName, arnService } } -// testAccCheckResourceAttrRegionalARN ensures the Terraform state regexp matches a formatted ARN with region +// testAccMatchResourceAttrRegionalARN ensures the Terraform state regexp matches a formatted ARN with region func testAccMatchResourceAttrRegionalARN(resourceName, attributeName, arnService string, arnResourceRegexp *regexp.Regexp) resource.TestCheckFunc { return func(s *terraform.State) error { arnRegexp := arn.ARN{ @@ -147,6 +154,26 @@ func testAccCheckResourceAttrGlobalARN(resourceName, attributeName, arnService, } } +// testAccMatchResourceAttrGlobalARN ensures the Terraform state regexp matches a formatted ARN without region +func testAccMatchResourceAttrGlobalARN(resourceName, attributeName, arnService string, arnResourceRegexp *regexp.Regexp) resource.TestCheckFunc { + return func(s *terraform.State) error { + arnRegexp := arn.ARN{ + AccountID: testAccGetAccountID(), + Partition: testAccGetPartition(), + Resource: arnResourceRegexp.String(), + Service: arnService, + }.String() + + attributeMatch, err := regexp.Compile(arnRegexp) + + if err != nil { + return fmt.Errorf("Unable to compile ARN regexp (%s): %s", arnRegexp, err) + } + + return resource.TestMatchResourceAttr(resourceName, attributeName, attributeMatch)(s) + } +} + // testAccGetAccountID returns the account ID of testAccProvider // Must be used returned within a resource.TestCheckFunc func testAccGetAccountID() string { diff --git a/aws/resource_aws_organizations_organization_test.go b/aws/resource_aws_organizations_organization_test.go index 5ab92da3540..8eb4f566033 100644 --- a/aws/resource_aws_organizations_organization_test.go +++ b/aws/resource_aws_organizations_organization_test.go @@ -2,6 +2,7 @@ package aws import ( "fmt" + "regexp" "testing" "github.com/aws/aws-sdk-go/service/organizations" @@ -9,7 +10,8 @@ import ( "github.com/hashicorp/terraform/terraform" ) -func testAccAwsOrganizationsOrganization_importBasic(t *testing.T) { +func testAccAwsOrganizationsOrganization_basic(t *testing.T) { + var organization organizations.Organization resourceName := "aws_organizations_organization.test" resource.Test(t, resource.TestCase{ @@ -19,8 +21,15 @@ func testAccAwsOrganizationsOrganization_importBasic(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccAwsOrganizationsOrganizationConfig, + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsOrganizationsOrganizationExists(resourceName, &organization), + testAccMatchResourceAttrGlobalARN(resourceName, "arn", "organizations", regexp.MustCompile(`organization/o-.+`)), + resource.TestCheckResourceAttr(resourceName, "feature_set", organizations.OrganizationFeatureSetAll), + testAccMatchResourceAttrGlobalARN(resourceName, "master_account_arn", "organizations", regexp.MustCompile(`account/o-.+/.+`)), + resource.TestMatchResourceAttr(resourceName, "master_account_email", regexp.MustCompile(`.+@.+`)), + testAccCheckResourceAttrAccountID(resourceName, "master_account_id"), + ), }, - { ResourceName: resourceName, ImportState: true, @@ -30,8 +39,9 @@ func testAccAwsOrganizationsOrganization_importBasic(t *testing.T) { }) } -func testAccAwsOrganizationsOrganization_basic(t *testing.T) { +func testAccAwsOrganizationsOrganization_FeatureSet(t *testing.T) { var organization organizations.Organization + resourceName := "aws_organizations_organization.test" resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t); testAccOrganizationsAccountPreCheck(t) }, @@ -39,36 +49,16 @@ func testAccAwsOrganizationsOrganization_basic(t *testing.T) { CheckDestroy: testAccCheckAwsOrganizationsOrganizationDestroy, Steps: []resource.TestStep{ { - Config: testAccAwsOrganizationsOrganizationConfig, + Config: testAccAwsOrganizationsOrganizationConfigFeatureSet(organizations.OrganizationFeatureSetConsolidatedBilling), Check: resource.ComposeTestCheckFunc( - testAccCheckAwsOrganizationsOrganizationExists("aws_organizations_organization.test", &organization), - resource.TestCheckResourceAttr("aws_organizations_organization.test", "feature_set", organizations.OrganizationFeatureSetAll), - resource.TestCheckResourceAttrSet("aws_organizations_organization.test", "arn"), - resource.TestCheckResourceAttrSet("aws_organizations_organization.test", "master_account_arn"), - resource.TestCheckResourceAttrSet("aws_organizations_organization.test", "master_account_email"), - resource.TestCheckResourceAttrSet("aws_organizations_organization.test", "feature_set"), + testAccCheckAwsOrganizationsOrganizationExists(resourceName, &organization), + resource.TestCheckResourceAttr(resourceName, "feature_set", organizations.OrganizationFeatureSetConsolidatedBilling), ), }, - }, - }) -} - -func testAccAwsOrganizationsOrganization_consolidatedBilling(t *testing.T) { - var organization organizations.Organization - - feature_set := organizations.OrganizationFeatureSetConsolidatedBilling - - resource.Test(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t); testAccOrganizationsAccountPreCheck(t) }, - Providers: testAccProviders, - CheckDestroy: testAccCheckAwsOrganizationsOrganizationDestroy, - Steps: []resource.TestStep{ { - Config: testAccAwsOrganizationsOrganizationConfigConsolidatedBilling(feature_set), - Check: resource.ComposeTestCheckFunc( - testAccCheckAwsOrganizationsOrganizationExists("aws_organizations_organization.test", &organization), - resource.TestCheckResourceAttr("aws_organizations_organization.test", "feature_set", feature_set), - ), + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, }, }, }) @@ -86,10 +76,11 @@ func testAccCheckAwsOrganizationsOrganizationDestroy(s *terraform.State) error { resp, err := conn.DescribeOrganization(params) + if isAWSErr(err, organizations.ErrCodeAWSOrganizationsNotInUseException, "") { + return nil + } + if err != nil { - if isAWSErr(err, organizations.ErrCodeAWSOrganizationsNotInUseException, "") { - return nil - } return err } @@ -133,10 +124,10 @@ func testAccCheckAwsOrganizationsOrganizationExists(n string, a *organizations.O const testAccAwsOrganizationsOrganizationConfig = "resource \"aws_organizations_organization\" \"test\" {}" -func testAccAwsOrganizationsOrganizationConfigConsolidatedBilling(feature_set string) string { +func testAccAwsOrganizationsOrganizationConfigFeatureSet(featureSet string) string { return fmt.Sprintf(` resource "aws_organizations_organization" "test" { - feature_set = "%s" + feature_set = %q } -`, feature_set) +`, featureSet) } diff --git a/aws/resource_aws_organizations_test.go b/aws/resource_aws_organizations_test.go index 5804aeb009f..ebe6626a624 100644 --- a/aws/resource_aws_organizations_test.go +++ b/aws/resource_aws_organizations_test.go @@ -7,9 +7,8 @@ import ( func TestAccAWSOrganizations(t *testing.T) { testCases := map[string]map[string]func(t *testing.T){ "Organization": { - "basic": testAccAwsOrganizationsOrganization_basic, - "importBasic": testAccAwsOrganizationsOrganization_importBasic, - "consolidatedBilling": testAccAwsOrganizationsOrganization_consolidatedBilling, + "basic": testAccAwsOrganizationsOrganization_basic, + "FeatureSet": testAccAwsOrganizationsOrganization_FeatureSet, }, "Account": { "basic": testAccAwsOrganizationsAccount_basic, From 1e33be85c4d03b7c10d4033b6d427f70a2a22b09 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 26 Nov 2018 01:10:48 -0500 Subject: [PATCH 2/2] resource/aws_organizations_organization: Support managing AWS service access principals Changes: * resource/aws_organizations_organization: Add `aws_service_access_principals` argument Output from acceptance testing: ``` --- PASS: TestAccAWSOrganizations (58.72s) --- PASS: TestAccAWSOrganizations/Organization (58.72s) --- PASS: TestAccAWSOrganizations/Organization/basic (16.86s) --- PASS: TestAccAWSOrganizations/Organization/AwsServiceAccessPrincipals (29.94s) --- PASS: TestAccAWSOrganizations/Organization/FeatureSet (11.93s) ``` --- ...resource_aws_organizations_organization.go | 96 +++++++++++++++++-- ...rce_aws_organizations_organization_test.go | 60 ++++++++++++ aws/resource_aws_organizations_test.go | 5 +- .../organizations_organization.html.markdown | 6 ++ 4 files changed, 159 insertions(+), 8 deletions(-) diff --git a/aws/resource_aws_organizations_organization.go b/aws/resource_aws_organizations_organization.go index 7b4fb8311ee..0e45120b027 100644 --- a/aws/resource_aws_organizations_organization.go +++ b/aws/resource_aws_organizations_organization.go @@ -14,6 +14,7 @@ func resourceAwsOrganizationsOrganization() *schema.Resource { return &schema.Resource{ Create: resourceAwsOrganizationsOrganizationCreate, Read: resourceAwsOrganizationsOrganizationRead, + Update: resourceAwsOrganizationsOrganizationUpdate, Delete: resourceAwsOrganizationsOrganizationDelete, Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, @@ -36,6 +37,11 @@ func resourceAwsOrganizationsOrganization() *schema.Resource { Type: schema.TypeString, Computed: true, }, + "aws_service_access_principals": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, "feature_set": { Type: schema.TypeString, Optional: true, @@ -66,6 +72,21 @@ func resourceAwsOrganizationsOrganizationCreate(d *schema.ResourceData, meta int org := resp.Organization d.SetId(*org.Id) + awsServiceAccessPrincipals := d.Get("aws_service_access_principals").(*schema.Set).List() + for _, principalRaw := range awsServiceAccessPrincipals { + principal := principalRaw.(string) + input := &organizations.EnableAWSServiceAccessInput{ + ServicePrincipal: aws.String(principal), + } + + log.Printf("[DEBUG] Enabling AWS Service Access in Organization: %s", input) + _, err := conn.EnableAWSServiceAccess(input) + + if err != nil { + return fmt.Errorf("error enabling AWS Service Access (%s) in Organization: %s", principal, err) + } + } + return resourceAwsOrganizationsOrganizationRead(d, meta) } @@ -74,13 +95,15 @@ func resourceAwsOrganizationsOrganizationRead(d *schema.ResourceData, meta inter log.Printf("[INFO] Reading Organization: %s", d.Id()) org, err := conn.DescribeOrganization(&organizations.DescribeOrganizationInput{}) + + if isAWSErr(err, organizations.ErrCodeAWSOrganizationsNotInUseException, "") { + log.Printf("[WARN] Organization does not exist, removing from state: %s", d.Id()) + d.SetId("") + return nil + } + if err != nil { - if isAWSErr(err, organizations.ErrCodeAWSOrganizationsNotInUseException, "") { - log.Printf("[WARN] Organization does not exist, removing from state: %s", d.Id()) - d.SetId("") - return nil - } - return err + return fmt.Errorf("error describing Organization: %s", err) } d.Set("arn", org.Organization.Arn) @@ -88,9 +111,70 @@ func resourceAwsOrganizationsOrganizationRead(d *schema.ResourceData, meta inter d.Set("master_account_arn", org.Organization.MasterAccountArn) d.Set("master_account_email", org.Organization.MasterAccountEmail) d.Set("master_account_id", org.Organization.MasterAccountId) + + awsServiceAccessPrincipals := make([]string, 0) + + // ConstraintViolationException: The request failed because the organization does not have all features enabled. Please enable all features in your organization and then retry. + if aws.StringValue(org.Organization.FeatureSet) == organizations.OrganizationFeatureSetAll { + err = conn.ListAWSServiceAccessForOrganizationPages(&organizations.ListAWSServiceAccessForOrganizationInput{}, func(page *organizations.ListAWSServiceAccessForOrganizationOutput, lastPage bool) bool { + for _, enabledServicePrincipal := range page.EnabledServicePrincipals { + awsServiceAccessPrincipals = append(awsServiceAccessPrincipals, aws.StringValue(enabledServicePrincipal.ServicePrincipal)) + } + return !lastPage + }) + + if err != nil { + return fmt.Errorf("error listing AWS Service Access for Organization (%s): %s", d.Id(), err) + } + } + + if err := d.Set("aws_service_access_principals", awsServiceAccessPrincipals); err != nil { + return fmt.Errorf("error setting aws_service_access_principals: %s", err) + } + return nil } +func resourceAwsOrganizationsOrganizationUpdate(d *schema.ResourceData, meta interface{}) error { + conn := meta.(*AWSClient).organizationsconn + + if d.HasChange("aws_service_access_principals") { + oldRaw, newRaw := d.GetChange("aws_service_access_principals") + oldSet := oldRaw.(*schema.Set) + newSet := newRaw.(*schema.Set) + + for _, disablePrincipalRaw := range oldSet.Difference(newSet).List() { + principal := disablePrincipalRaw.(string) + input := &organizations.DisableAWSServiceAccessInput{ + ServicePrincipal: aws.String(principal), + } + + log.Printf("[DEBUG] Disabling AWS Service Access in Organization: %s", input) + _, err := conn.DisableAWSServiceAccess(input) + + if err != nil { + return fmt.Errorf("error disabling AWS Service Access (%s) in Organization: %s", principal, err) + } + } + + for _, enablePrincipalRaw := range newSet.Difference(oldSet).List() { + principal := enablePrincipalRaw.(string) + input := &organizations.EnableAWSServiceAccessInput{ + ServicePrincipal: aws.String(principal), + } + + log.Printf("[DEBUG] Enabling AWS Service Access in Organization: %s", input) + _, err := conn.EnableAWSServiceAccess(input) + + if err != nil { + return fmt.Errorf("error enabling AWS Service Access (%s) in Organization: %s", principal, err) + } + } + } + + return resourceAwsOrganizationsOrganizationRead(d, meta) +} + func resourceAwsOrganizationsOrganizationDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).organizationsconn diff --git a/aws/resource_aws_organizations_organization_test.go b/aws/resource_aws_organizations_organization_test.go index 8eb4f566033..959bfedbb6f 100644 --- a/aws/resource_aws_organizations_organization_test.go +++ b/aws/resource_aws_organizations_organization_test.go @@ -24,6 +24,7 @@ func testAccAwsOrganizationsOrganization_basic(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckAwsOrganizationsOrganizationExists(resourceName, &organization), testAccMatchResourceAttrGlobalARN(resourceName, "arn", "organizations", regexp.MustCompile(`organization/o-.+`)), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.#", "0"), resource.TestCheckResourceAttr(resourceName, "feature_set", organizations.OrganizationFeatureSetAll), testAccMatchResourceAttrGlobalARN(resourceName, "master_account_arn", "organizations", regexp.MustCompile(`account/o-.+/.+`)), resource.TestMatchResourceAttr(resourceName, "master_account_email", regexp.MustCompile(`.+@.+`)), @@ -39,6 +40,49 @@ func testAccAwsOrganizationsOrganization_basic(t *testing.T) { }) } +func testAccAwsOrganizationsOrganization_AwsServiceAccessPrincipals(t *testing.T) { + var organization organizations.Organization + resourceName := "aws_organizations_organization.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccOrganizationsAccountPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsOrganizationsOrganizationDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAwsOrganizationsOrganizationConfigAwsServiceAccessPrincipals1("config.amazonaws.com"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsOrganizationsOrganizationExists(resourceName, &organization), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.#", "1"), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.553690328", "config.amazonaws.com"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccAwsOrganizationsOrganizationConfigAwsServiceAccessPrincipals2("config.amazonaws.com", "ds.amazonaws.com"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsOrganizationsOrganizationExists(resourceName, &organization), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.#", "2"), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.553690328", "config.amazonaws.com"), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.3567899500", "ds.amazonaws.com"), + ), + }, + { + Config: testAccAwsOrganizationsOrganizationConfigAwsServiceAccessPrincipals1("fms.amazonaws.com"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsOrganizationsOrganizationExists(resourceName, &organization), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.#", "1"), + resource.TestCheckResourceAttr(resourceName, "aws_service_access_principals.4066123156", "fms.amazonaws.com"), + ), + }, + }, + }) +} + func testAccAwsOrganizationsOrganization_FeatureSet(t *testing.T) { var organization organizations.Organization resourceName := "aws_organizations_organization.test" @@ -124,6 +168,22 @@ func testAccCheckAwsOrganizationsOrganizationExists(n string, a *organizations.O const testAccAwsOrganizationsOrganizationConfig = "resource \"aws_organizations_organization\" \"test\" {}" +func testAccAwsOrganizationsOrganizationConfigAwsServiceAccessPrincipals1(principal1 string) string { + return fmt.Sprintf(` +resource "aws_organizations_organization" "test" { + aws_service_access_principals = [%q] +} +`, principal1) +} + +func testAccAwsOrganizationsOrganizationConfigAwsServiceAccessPrincipals2(principal1, principal2 string) string { + return fmt.Sprintf(` +resource "aws_organizations_organization" "test" { + aws_service_access_principals = [%q, %q] +} +`, principal1, principal2) +} + func testAccAwsOrganizationsOrganizationConfigFeatureSet(featureSet string) string { return fmt.Sprintf(` resource "aws_organizations_organization" "test" { diff --git a/aws/resource_aws_organizations_test.go b/aws/resource_aws_organizations_test.go index ebe6626a624..a437b76bb99 100644 --- a/aws/resource_aws_organizations_test.go +++ b/aws/resource_aws_organizations_test.go @@ -7,8 +7,9 @@ import ( func TestAccAWSOrganizations(t *testing.T) { testCases := map[string]map[string]func(t *testing.T){ "Organization": { - "basic": testAccAwsOrganizationsOrganization_basic, - "FeatureSet": testAccAwsOrganizationsOrganization_FeatureSet, + "basic": testAccAwsOrganizationsOrganization_basic, + "AwsServiceAccessPrincipals": testAccAwsOrganizationsOrganization_AwsServiceAccessPrincipals, + "FeatureSet": testAccAwsOrganizationsOrganization_FeatureSet, }, "Account": { "basic": testAccAwsOrganizationsAccount_basic, diff --git a/website/docs/r/organizations_organization.html.markdown b/website/docs/r/organizations_organization.html.markdown index ce9cbeb9937..dc2b4c173ca 100644 --- a/website/docs/r/organizations_organization.html.markdown +++ b/website/docs/r/organizations_organization.html.markdown @@ -14,6 +14,11 @@ Provides a resource to create an organization. ```hcl resource "aws_organizations_organization" "org" { + aws_service_access_principals = [ + "cloudtrail.amazonaws.com", + "config.amazonaws.com", + ] + feature_set = "ALL" } ``` @@ -22,6 +27,7 @@ resource "aws_organizations_organization" "org" { The following arguments are supported: +* `aws_service_access_principals` - (Optional) List of AWS service principal names for which you want to enable integration with your organization. This is typically in the form of a URL, such as service-abbreviation.amazonaws.com. Organization must have `feature_set` set to `ALL`. For additional information, see the [AWS Organizations User Guide](https://docs.aws.amazon.com/organizations/latest/userguide/orgs_integrate_services.html). * `feature_set` - (Optional) Specify "ALL" (default) or "CONSOLIDATED_BILLING". ## Attributes Reference