terraform-provider-alks icon indicating copy to clipboard operation
terraform-provider-alks copied to clipboard

alks_iamrole type change goes unnoticed

Open jantman opened this issue 7 years ago • 6 comments

We're using version 0.9.11 with Terraform 0.9.11.

Scenario

  1. We create an alks_iamrole of type = "Amazon EC2".
  2. Everything works right; some time passes.
  3. Someone (via a ticket) changes the Trust Policy on this role.
  4. Terraform plan shows no changes

Expected Behavior

Terraform plan shows either a different "type" for the role, or an unknown "type".

I don't know the ALKS API very well, but it feels like this violates the idempotent and authoritative nature of terraform.

jantman avatar Jan 05 '18 15:01 jantman

Thanks for reporting this, digging into it now! :)

brianantonelli avatar Jan 10 '18 19:01 brianantonelli

Alright, I've resolved the issue. I created a alks_iamrole with type set to Amazon EC2 and then changed the trust policy to set it to Lambda. Now when I run terraform plan it detects the change and forces a new resource:

brianantonelli in ~/Dev/go/src/github.com/Cox-Automotive/terraform-provider-alks/examples on develop*
🍔  terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

alks_iamrole.test_role: Refreshing state... (ID: aba-test-123456)
aws_iam_role_policy_attachment.sr-attach: Refreshing state... (ID: aba-test-123456-20180110192336719600000001)
aws_iam_role_policy.test_policy: Refreshing state... (ID: aba-test-123456:test_policy)

------------------------------------------------------------------------

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

-/+ alks_iamrole.test_role (new resource required)
      id:                       "aba-test-123456" => <computed> (forces new resource)
      arn:                      "arn:aws:iam::120678615247:role/acct-managed/aba-test-123456" => <computed>
      include_default_policies: "false" => "false"
      ip_arn:                   "arn:aws:iam::120678615247:instance-profile/acct-managed/aba-test-123456" => <computed>
      name:                     "aba-test-123456" => "aba-test-123456"
      role_added_to_ip:         "true" => <computed>
      type:                     "" => "Amazon EC2" (forces new resource)


Plan: 1 to add, 0 to change, 1 to destroy.

------------------------------------------------------------------------

Note: You didn't specify an "-out" parameter to save this plan, so Terraform
can't guarantee that exactly these actions will be performed if
"terraform apply" is subsequently run.

brianantonelli avatar Jan 10 '18 19:01 brianantonelli

Actually there's still an issue, the ALKS API isn't currently returning the role type as null so this issue can't be fixed until the API is updated. I'll cut a ticket with them to get it resolved..

brianantonelli avatar Jan 10 '18 19:01 brianantonelli

Ah ok, thanks! Admittedly this is a real edge case that theoretically can only be caused by human error, so I don't think there's a lot of pressure to fix it. But it's definitely not expected behavior, so I thought it best to open the issue.

Thanks so much for looking into this!

jantman avatar Jan 10 '18 21:01 jantman

This needs to have the role actually passed into the ALKS API, instead of the role name. Right now, ALKS doesn't support this, though there are stories on the backlog for doing this. See this issue specifically.

There was some discussion there around "looking up" the type of role, and returning it in the ALKS API - unfortunately, if any modifications were made to the role type, the lookup would fail.

The solution here is to allow the policy for the role to be passed into the API, and validate the policy meets our requirements. This would allow the ALKS Terraform provider to detect drift, and apply policy updates.

I'll bring this up with the team again, and ensure we have priority on the appropriate backlog stories. 👍

ekozlowski avatar May 24 '18 15:05 ekozlowski

Internal Tools team has had an initial discussion on this item - we agree it's still something that makes sense to look into. Some more digging will need to be done on the ALKS side to determine the level of effort getting a workable solution will take.

A Spike has been made in the Internal Tools backlog for investigating this in the near future.

aaron-seitz avatar Apr 05 '19 18:04 aaron-seitz