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

Conversation

asedge
Copy link
Contributor

@asedge asedge commented Jun 19, 2017

A resource to help implement AWS Organizations: #571.

Originally attempted a PR at: hashicorp/terraform#13831. Noticed the providers had been split so I'm trying again to get this merged.

Copy link
Contributor

@tamsky tamsky left a comment

Choose a reason for hiding this comment

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

suggested style fixes


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.

missing trailing-"

}

return nil

Copy link
Contributor

Choose a reason for hiding this comment

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

rm extra NL

@asedge
Copy link
Contributor Author

asedge commented Jun 27, 2017

@tamsky Thanks for the review. Submitted a fix for the issues you pointed out.

@asedge
Copy link
Contributor Author

asedge commented Jun 28, 2017

Thanks again @MrGossett!

@tpoindessous
Copy link
Contributor

Hi

any idea this PR will be merged ?

Thanks !

@Ninir
Copy link
Contributor

Ninir commented Jul 11, 2017

Hey @asedge, everyone,

This looks good to me. I am going to add some fixes and the missing upgrade from CONSOLIDATED_BILLING to ALL (http://docs.aws.amazon.com/organizations/latest/APIReference/API_EnableAllFeatures.html).

Will keep you informed when everything sounds ok.

Thanks!

@hashicorp hashicorp deleted a comment from tamsky Jul 11, 2017
@asedge
Copy link
Contributor Author

asedge commented Jul 11, 2017

@Ninir, fine by me. Thanks!

@asedge
Copy link
Contributor Author

asedge commented Jul 14, 2017

@Ninir, do you think anything else is needed? I'm willing to make changes if necessary.

@Ninir
Copy link
Contributor

Ninir commented Jul 14, 2017

Hey @asedge

Sorry for keeping you not up-to-date on this.
I'm struggling with something a bit tricky at the moment, regarding the feature_set.

As you know, an AWS Organization can be created in 2 modes: CONSOLIDATED_BILLING or ALL.

There are 2 cases right now:

  1.   If the option is set to ALL or not set, you cannot switch back to CONSOLIDATED BILLING (I added a validation error in my local code)
    
  2.   If the option is CONSOLIDATED_BILLING and you want to switch to ALL, the [EnableAllFeatures](http://docs.aws.amazon.com/organizations/latest/APIReference/API_EnableAllFeatures.html) call is made.
    

EnableAllFeatures for AWS Organizations needs unanimity from its members, thus everyone receives an email to either accept or decline the change.
As soon as everyone accepted the enabling of the ALL mode, a new email is received by the Organization Owner, and the owner needs to go to the console, validating the change manually.
The action can take at min 1min (you’re alone in the org), at max 15 days (expiration).

There are 2 ways to proceed there:

  1. Let the user change the value from one to the other and throw an error or make the EnableCall
  2. Remove the ability to change this specific attribute by adding the ForceNew flag

In case 1, the template configuration could then be updated, the API Call (EnableAllFeatures) would be made, but Terraform would ask still for the change until you finalize the process manually in the console
In case 2, if you already had members, you would need to remove them from it.

We are currently discussing the UX perspective with @catsby on this.
What are your thougths there? :)

@asedge
Copy link
Contributor Author

asedge commented Jul 15, 2017

@Ninir Is it possible to store some information about the Handshake in Terraform's state? The presence of a Handshake would indicate there's no need to call EnableAllFeatures again plus the Handshake includes the expiration date: Handshake structure - which we could use to time out the presence of the Handshake. Perhaps a bit too over complicated.

There's a complication with case 2 as well. If any of the Organization member accounts have been created from Organization master account they cannot be removed from the Organization without completing some manual steps: Removing a Member Account From Your Organization. I have another PR for creating aws_organization_account and some of the discussion around this happened here: hashicorp/terraform#12337

There don't seem to be many good options.

@Ninir
Copy link
Contributor

Ninir commented Jul 26, 2017

Cases requiring a validation are always tough to handle. We try to avoid these complex stuff by either not allowing, either finding a way to do so.
Here, storing the handshake could bring a few drawbacks, since organisations are pretty new.
We started a talk with @catsby on that, will get back to you soon!

Thanks for being patient on this topic :)

@Ninir Ninir requested a review from catsby August 17, 2017 19:45
@barundel
Copy link

any update? would really like to see this functionality ;)

@sionsmith
Copy link

Can someone resolve this conflict so we can get this into GA.

@ellerbrock
Copy link

just started to play for the first time with aws and terraform and would love to start the whole account setup with terraform. so when i understand it right its not yet merged and currently not possible to do the overall:

  • create organisiation
  • create subaccounts
  • add organisation policies to subaccounts ...

yet, right?

cheers maik

@asedge
Copy link
Contributor Author

asedge commented Aug 25, 2017

@ellerbrock You are correct, it's not yet possible. I have aws_organization_account ready to file a PR but was hoping to get this merged first. Both resources depend on adding some new libs from aws-sdk-go. I had both PRs filed when providers were still tightly coupled with Terraform but ended up just constantly fixing merge conflicts in vendor/vendor.json.

@radeksimko radeksimko requested review from radeksimko and removed request for catsby August 29, 2017 13:12
@ellerbrock
Copy link

ellerbrock commented Sep 4, 2017

any news if stuff get merged?
i have to setup a aws multi account and would love to do all with one awesome tool called terraform :)

@JoshiiSinfield
Copy link

+1.

@bcwilsondotcom
Copy link

+1 Seems like there's a lot of us that would love to have this merged.

@radeksimko
Copy link
Member

Hi folks,
we do appreciate the interest and +1's if these don't generate notifications. 😅

We'll get to this PR 🔜 .
I'll lock this thread temporarily, until we do so, to avoid cluttering everyone's notification feed.

Thanks.

@Ninir
Copy link
Contributor

Ninir commented Feb 20, 2018

@bflad we certainly need to address #903 (comment) in some way as it is sure that people will encounter this issue a day or the other. As discussed, going no-op with warnings or just ignoring...

aws/provider.go Outdated
@@ -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.

@radeksimko radeksimko removed their request for review February 20, 2018 15:21
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 21, 2018
@asedge
Copy link
Contributor Author

asedge commented Feb 23, 2018

@bflad I believe I've taken care of everything you've given me feedback on.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thanks for all the hard work here @asedge!

make testacc TEST=./aws TESTARGS='-run=TestAccAWSOrganizations'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSOrganizations -timeout 120m
=== RUN   TestAccAWSOrganizations
=== RUN   TestAccAWSOrganizations/Organization
=== RUN   TestAccAWSOrganizations/Organization/basic
=== RUN   TestAccAWSOrganizations/Organization/importBasic
=== RUN   TestAccAWSOrganizations/Organization/consolidatedBilling
--- PASS: TestAccAWSOrganizations (56.39s)
    --- PASS: TestAccAWSOrganizations/Organization (56.39s)
        --- PASS: TestAccAWSOrganizations/Organization/basic (18.38s)
        --- PASS: TestAccAWSOrganizations/Organization/importBasic (21.86s)
        --- PASS: TestAccAWSOrganizations/Organization/consolidatedBilling (16.15s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	56.435s

@bflad bflad modified the milestones: v1.13.0, v1.11.0 Feb 25, 2018
@bflad
Copy link
Contributor

bflad commented Feb 25, 2018

@Ninir I think we are okay for now with #903 (comment) since we are no longer allowing updates in the resource (ForceNew: true on feature_set). We can and certainly should address those two scenarios if or when updates are added to the resource. Please correct me if I'm wrong and I'll address ASAP. Thanks!

@bflad bflad changed the title New Resource: aws_organization New Resource: aws_organizations_organization Feb 25, 2018
@bflad bflad merged commit 385a2a3 into hashicorp:master Feb 25, 2018
bflad added a commit that referenced this pull request Feb 25, 2018
@bflad
Copy link
Contributor

bflad commented Mar 9, 2018

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

@asedge
Copy link
Contributor Author

asedge commented Mar 9, 2018

@bflad It looks like I screwed up the documentation page: https://www.terraform.io/docs/providers/aws/r/organizations_organization.html

@ghost
Copy link

ghost commented Apr 7, 2020

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 ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-resource Introduces a new resource. service/organizations Issues and PRs that pertain to the organizations service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.