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.

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

New Resource: aws_organizations_organization #903

Merged
merged 18 commits into from
Feb 25, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
3 changes: 3 additions & 0 deletions aws/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import (
"github.com/aws/aws-sdk-go/service/mediastore"
"github.com/aws/aws-sdk-go/service/mq"
"github.com/aws/aws-sdk-go/service/opsworks"
"github.com/aws/aws-sdk-go/service/organizations"
"github.com/aws/aws-sdk-go/service/rds"
"github.com/aws/aws-sdk-go/service/redshift"
"github.com/aws/aws-sdk-go/service/route53"
Expand Down Expand Up @@ -194,6 +195,7 @@ type AWSClient struct {
lightsailconn *lightsail.Lightsail
mqconn *mq.MQ
opsworksconn *opsworks.OpsWorks
organizationsconn *organizations.Organizations
glacierconn *glacier.Glacier
guarddutyconn *guardduty.GuardDuty
codebuildconn *codebuild.CodeBuild
Expand Down Expand Up @@ -444,6 +446,7 @@ func (c *Config) Client() (interface{}, error) {
client.lightsailconn = lightsail.New(sess)
client.mqconn = mq.New(sess)
client.opsworksconn = opsworks.New(sess)
client.organizationsconn = organizations.New(sess)
client.r53conn = route53.New(r53Sess)
client.rdsconn = rds.New(awsRdsSess)
client.redshiftconn = redshift.New(sess)
Expand Down
28 changes: 28 additions & 0 deletions aws/import_aws_organization_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package aws

import (
"testing"

"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAWSOrganizationImportBasic(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this to _importBasic please? Thanks!

resourceName := "aws_organization.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSOrganizationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSOrganizationConfig(),
},

{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ func Provider() terraform.ResourceProvider {
"aws_opsworks_user_profile": resourceAwsOpsworksUserProfile(),
"aws_opsworks_permission": resourceAwsOpsworksPermission(),
"aws_opsworks_rds_db_instance": resourceAwsOpsworksRdsDbInstance(),
"aws_organization": resourceAwsOrganization(),
Copy link
Contributor

@bflad bflad Feb 20, 2018

Choose a reason for hiding this comment

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

Its going to look ugly at first and Naming is Hard, but we should probably also rename this aws_organizations_organization for a few reasons:

  • The AWS Organizations service supports multiple resources we'll want to manage, e.g. aws_organizations_account, aws_organizations_organization_unit, aws_organizations_policy, this will keep everything bundled together nicely naming-wise
  • To prevent confusion with other AWS services that might use the terminology "organization"
  • If AWS ever creates a separate account organization management service/method

Ideally the resource function names will be updated to long form as well: resourceAwsOrganizationsOrganization, resourceAwsOrganizationsOrganizationRead, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I handled all the other feedback you provided. I'm not against doing this but it seemed like more work than I was willing to put in tonight.

"aws_placement_group": resourceAwsPlacementGroup(),
"aws_proxy_protocol_policy": resourceAwsProxyProtocolPolicy(),
"aws_rds_cluster": resourceAwsRDSCluster(),
Expand Down
109 changes: 109 additions & 0 deletions aws/resource_aws_organization.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
package aws

import (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: can you please structure the imports as:

import (
  // stdlib ones

  // others
)

FYI many folks here are using goimports to automatically add, remove, and format imports 😄

"fmt"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/organizations"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"log"
)

func resourceAwsOrganization() *schema.Resource {
return &schema.Resource{
Create: resourceAwsOrganizationCreate,
Read: resourceAwsOrganizationRead,
Update: resourceAwsOrganizationUpdate,
Copy link
Contributor

Choose a reason for hiding this comment

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

As written, this resource does not support updates. We should remove this line from the resource and its associated function.

Delete: resourceAwsOrganizationDelete,
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},

Schema: map[string]*schema.Schema{
"arn": {
Type: schema.TypeString,
Computed: true,
},
"master_account_arn": {
Type: schema.TypeString,
Computed: true,
},
"master_account_email": {
Type: schema.TypeString,
Computed: true,
},
"master_account_id": {
Type: schema.TypeString,
Computed: true,
},
"feature_set": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set ForceNew: true on this attribute as updates are not implemented

Type: schema.TypeString,
Optional: true,
Default: "ALL",
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: we should use the SDK constant here too: organizations.OrganizationFeatureSetAll

ValidateFunc: validation.StringInSlice([]string{organizations.OrganizationFeatureSetAll, organizations.OrganizationFeatureSetConsolidatedBilling}, true),
},
},
}
}

func resourceAwsOrganizationCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).organizationsconn

createOpts := &organizations.CreateOrganizationInput{
FeatureSet: aws.String(d.Get("feature_set").(string)),
}
log.Printf("[DEBUG] Creating Organization: %#v", createOpts)

resp, err := conn.CreateOrganization(createOpts)
if err != nil {
return fmt.Errorf("Error creating organization: %s", err)
}

org := resp.Organization
d.SetId(*org.Id)
log.Printf("[INFO] Organization ID: %s", d.Id())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove this line or set it to DEBUG? 👍


return resourceAwsOrganizationUpdate(d, meta)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go right to calling resourceAwsOrganizationRead(d, meta)

}

func resourceAwsOrganizationRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).organizationsconn

log.Printf("[INFO] Reading Organization: %s", d.Id())
org, err := conn.DescribeOrganization(&organizations.DescribeOrganizationInput{})
if err != nil {
if orgerr, ok := err.(awserr.Error); ok && orgerr.Code() == "AWSOrganizationsNotInUseException" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: We can simplify this with if isAWSErr(err, organizations.ErrCodeAWSOrganizationsNotInUseException, "") {

log.Printf("[WARN] Organization does not exist, removing from state: %s", d.Id())
d.SetId("")
return nil
}
return err
}

d.Set("arn", org.Organization.Arn)
d.Set("feature_set", org.Organization.FeatureSet)
d.Set("master_account_arn", org.Organization.MasterAccountArn)
d.Set("master_account_email", org.Organization.MasterAccountEmail)
d.Set("master_account_id", org.Organization.MasterAccountId)
return nil
}

func resourceAwsOrganizationUpdate(d *schema.ResourceData, meta interface{}) error {
return resourceAwsOrganizationRead(d, meta)
}

func resourceAwsOrganizationDelete(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).organizationsconn

log.Printf("[INFO] Deleting Organization: %s", d.Id())

_, err := conn.DeleteOrganization(&organizations.DeleteOrganizationInput{})
if err != nil {
return fmt.Errorf("Error deleting Organization: %s", err)
}

d.SetId("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: While its still around in a bunch of the codebase, this is actually not necessary on deletion 👍


return nil
}
122 changes: 122 additions & 0 deletions aws/resource_aws_organization_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
package aws

import (
"fmt"
"testing"

"github.com/aws/aws-sdk-go/service/organizations"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSOrganization_basic(t *testing.T) {
var organization organizations.Organization

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSOrganizationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSOrganizationConfig(),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSOrganizationExists("aws_organization.test", &organization),
resource.TestCheckResourceAttr("aws_organization.test", "feature_set", organizations.OrganizationFeatureSetAll),
resource.TestCheckResourceAttrSet("aws_organization.test", "arn"),
resource.TestCheckResourceAttrSet("aws_organization.test", "master_account_arn"),
resource.TestCheckResourceAttrSet("aws_organization.test", "master_account_email"),
resource.TestCheckResourceAttrSet("aws_organization.test", "feature_set"),
),
},
},
})
}

func TestAccAWSOrganization_consolidatedBilling(t *testing.T) {
var organization organizations.Organization

feature_set := organizations.OrganizationFeatureSetConsolidatedBilling

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSOrganizationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSOrganizationConfigConsolidatedBilling(feature_set),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSOrganizationExists("aws_organization.test", &organization),
resource.TestCheckResourceAttr("aws_organization.test", "feature_set", feature_set),
),
},
},
})
}

func testAccCheckAWSOrganizationDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).organizationsconn

for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_organization" {
continue
}

params := &organizations.DescribeOrganizationInput{}

resp, err := conn.DescribeOrganization(params)

if err != nil || resp == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be checking and returning errors:

if err != nil {
  if isAWSErr(err, organizations.ErrCodeAWSOrganizationsNotInUseException, "") {
    return nil
  }
  return err
}

if resp != nil && resp.Organization != nil {
  return fmt.Errorf("Organization still exists: %q", rs.Primary.ID)
}

return nil
}

if resp.Organization != nil {
return fmt.Errorf("Bad: Organization still exists: %q", rs.Primary.ID)
}
}

return nil
}

func testAccCheckAWSOrganizationExists(n string, a *organizations.Organization) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
if !ok {
return fmt.Errorf("Not found: %s", n)
}

if rs.Primary.ID == "" {
return fmt.Errorf("Organization ID not set")
}

conn := testAccProvider.Meta().(*AWSClient).organizationsconn
params := &organizations.DescribeOrganizationInput{}

resp, err := conn.DescribeOrganization(params)

if err != nil || resp == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be returning errors:

if err != nil {
  return err
}

if resp == nil || resp.Organization == nil {
  return fmt.Errorf("Organization %q does not exist", rs.Primary.ID)
}

return nil
}

if resp.Organization == nil {
return fmt.Errorf("Organization %q does not exist", rs.Primary.ID)
}

a = resp.Organization

return nil
}
}

func testAccAWSOrganizationConfig() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this can just be a const

return fmt.Sprintf(`
resource "aws_organization" "test" {}
`)
}

func testAccAWSOrganizationConfigConsolidatedBilling(feature_set string) string {
return fmt.Sprintf(`
resource "aws_organization" "test" {
feature_set = "%s"
}
`, feature_set)
}
10 changes: 10 additions & 0 deletions website/aws.erb
Original file line number Diff line number Diff line change
Expand Up @@ -1359,6 +1359,16 @@
</ul>
</li>

<li<%= sidebar_current("docs-aws-resource-organization") %>>
<a href="#">Organization Resources</a>
<ul class="nav nav-visible">

<li<%= sidebar_current("docs-aws-resource-organization") %>>
<a href="/docs/providers/aws/r/organization.html">aws_organization</a>
</li>
</ul>
</li>

<li<%= sidebar_current("docs-aws-resource-(db|rds)") %>>
<a href="#">RDS Resources</a>
<ul class="nav nav-visible">
Expand Down
33 changes: 33 additions & 0 deletions website/docs/r/organization.html.markdown
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
---
layout: "aws"
page_title: "AWS: aws_organization
sidebar_current: "docs-aws-resource-organization|"
description: |-
Provides a resource to create an organization.
---

# aws\_organization
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Backslashes are no longer required for documentation titles


Provides a resource to create an organization.

## Example Usage:

```hcl
resource "aws_organization" "org" {
feature_set = "ALL"
}
```

## Argument Reference

The following arguments are supported:

* `feature_set` - (Optional) Specify "ALL" (default) or "CONSOLIDATED_BILLING".

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add all the other attributes here? e.g.

## Attributes Reference

The following additional attributes are exported:

* `arn` - ARN of the organization
* `id` - Identifier of the organization
* `master_account_arn` - ...
...

## Import

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

```
$ terraform import aws_organization.my_org 111111111111
Copy link
Contributor

Choose a reason for hiding this comment

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

While the read function will currently work with anything here at the moment, we should be encouraging importing by the organization ID (o-XXXXXXX) for consistent usage with the documentation above and potentially to future proof this resource.

```