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: r/aws_fms_admin_account #4310

Merged
merged 1 commit into from
Aug 1, 2019

Conversation

gazoakley
Copy link
Contributor

@gazoakley gazoakley commented Apr 22, 2018

Closes #4057

New Resources:

  • aws_fms_admin_account
$ make testacc TESTARGS='-run=TestAccFmsAdminAccount_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccFmsAdminAccount_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccFmsAdminAccount_basic
--- PASS: TestAccFmsAdminAccount_basic (43.60s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	43.643s

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 22, 2018
@gazoakley
Copy link
Contributor Author

Seems like the call to AssociateAdminAccount returns success without actually doing anything if an organization has only recently been created - running the tests with a pre-existing organization passes. Might need to retry creating the association and waiting until GetAdminAccount returns the expected details.

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 23, 2018
@gazoakley gazoakley changed the title [WIP] New resource: r/aws_fms_admin_account New resource: r/aws_fms_admin_account Apr 23, 2018
@gazoakley gazoakley changed the title New resource: r/aws_fms_admin_account New Resource: r/aws_fms_admin_account Apr 23, 2018
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.

Thanks for this @gazoakley! Left some initial comments below. Please take a look and let us know if you have any questions.

Schema: map[string]*schema.Schema{
"account_id": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead make this Optional: true and use meta.(*AWSClient).accountid if its not provided? e.g.

if v, ok := d.GetOk("account_id"); ok && v != "" {
  accountID := v.(string)
} else {
  accountID := meta.(*AWSClient).accountid
}

return fmt.Errorf("Error waiting for firewall manager admin account association (%s): %s", accountId, sterr)
}

d.SetId("fms-admin-account")
Copy link
Contributor

Choose a reason for hiding this comment

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

While we don't necessarily need to reference it during read/delete at the moment, can you set this to accountId still? It'll make the ID available if we need it in the future and the import a little more obvious:

# Currently:
terraform import aws_fms_admin_account.example fms-admin-account
# Proposed:
terraform import aws_fms_admin_account.example 123456789012

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bflad I adopted this approach from aws_iam_account_password_policy (aws_spot_datafeed_subscription does this too) since only one FMS admin account can be associated to an account. My concern is that the user can disassociate/associate a different admin account outside Terraform - it might be confusing/misleading to have the resource ID as an account ID different to associated admin account ID. I'm guessing it wouldn't be safe to call SetId during a read operation if the associated admin account has changed? Any suggestions?

Copy link
Contributor

@bflad bflad Apr 23, 2018

Choose a reason for hiding this comment

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

We can check d.Id() != aws.StringValue(res.AdminAccount) to trigger recreation:

if d.Id() != aws.StringValue(res.AdminAccount) {
  log.Printf("[WARN] FMS Admin Account does not match, removing from state", d.Id())
  d.SetId("")
  return nil
}

Read: resourceAwsFmsAdminAccountRead,
Delete: resourceAwsFmsAdminAccountDelete,

Importer: &schema.ResourceImporter{
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing ## Import documentation section

// but does not define this in its error codes
if isAWSErr(err, "AccessDeniedException", "is not currently delegated by AWS FM") {
log.Printf("[WARN] No associated firewall manager admin account found, removing from state: %s", d.Id())
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.

We no longer need d.SetId("") during deletion functions. See also #4191

return fmt.Errorf("Error disassociating firewall manager admin account: %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.

We no longer need d.SetId("") during deletion functions. See also #4191

conn := testAccProvider.Meta().(*AWSClient).fmsconn

for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_config_authorization" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy paste typo 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops! 🤣

@bflad bflad added new-resource Introduces a new resource. service/fms Issues and PRs that pertain to the fms service. labels Apr 23, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 23, 2018
@jonrau1
Copy link

jonrau1 commented Apr 16, 2019

Any update for resolving conflicts and getting Firewall Manager merged?

@robh007
Copy link
Contributor

robh007 commented Jun 24, 2019

Hi @gazoakley is there any appetite to finish this PR? Cheers.

@aeschright aeschright requested a review from a team June 25, 2019 21:35
@bflad bflad added this to the v2.22.0 milestone Aug 1, 2019
@bflad bflad merged commit 68d9d7d into hashicorp:master Aug 1, 2019
@bflad
Copy link
Contributor

bflad commented Aug 1, 2019

Hi folks 👋 Apologies for the delays here. We went ahead and rebased this pull request, tidied it up, and merged it in. Thanks @gazoakley for the contribution. 🚀

bflad added a commit that referenced this pull request Aug 1, 2019
@ghost
Copy link

ghost commented Aug 1, 2019

This has been released in version 2.22.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
Copy link

ghost commented Nov 2, 2019

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 Nov 2, 2019
@gazoakley gazoakley deleted the f-firewall-admin-account branch April 7, 2020 16:24
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/fms Issues and PRs that pertain to the fms 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.

Feature Request: Support Firewall Manager Administrator Account Association
4 participants