provider/aws: Added the ability to import aws_iam_role's

[bigkraig]

Not much to it, needed this to build a tf configuration for our core configs.

[stack72]

Hi @bigkraig

Any reason you are ignoring the assume_role_policy?

Paul

[bigkraig]

@stack72 I looked around at other import resources and it seems to be done the same way, without the ignore the test will fail since the assume_role_policy text doesn't get imported.

[stack72]

Hey @bigkraig

I think the issue here is that the AssumeRolePolicy isn't actually set back to state in the Read func. I think rather than ignoring it from state, we should add it there. The nice thing about the import work is that it has shown up some areas where we are not setting the state in the Read func :)

P.

[bigkraig]

@stack72 OK, I need to url decode the string when it comes back from Amazon and the response doesn't have an array for a couple of the values, but this passes the tests now.

[stack72]

@bigkraig

What happens if that assume_role_policy is a HEREDOC?

P.

[bigkraig]

@stack72 it shouldn't matter, the import is done from the API right, so its however AWS is serving it up.

[stack72]

This should be TestAccAWSRole_importBasic - then it will get run with the other IAM Role tests :)

[stack72]

ok, just running all the tests now

[stack72]

After making the small change to the test name, I can run all of the AWS IAM Role tests:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRole_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRole_ -timeout 120m
=== RUN   TestAccAWSRole_importBasic
--- PASS: TestAccAWSRole_importBasic (20.17s)
=== RUN   TestAccAWSRole_basic
--- PASS: TestAccAWSRole_basic (17.96s)
=== RUN   TestAccAWSRole_namePrefix
--- FAIL: TestAccAWSRole_namePrefix (17.77s)
    testing.go:264: Step 0 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        UPDATE: aws_iam_role.role
          assume_role_policy: "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"ec2.amazonaws.com\"},\"Action\":\"sts:AssumeRole\"}]}" => "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"Service\":[\"ec2.amazonaws.com\"]},\"Action\":[\"sts:AssumeRole\"]}]}"

        STATE:

        aws_iam_role.role:
          ID = test-role-rnexfxnf2varldpy5hps3mvp4m
          arn = arn:aws:iam::881237884953:role/test-role-rnexfxnf2varldpy5hps3mvp4m
          assume_role_policy = {"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ec2.amazonaws.com"},"Action":"sts:AssumeRole"}]}
          name = test-role-rnexfxnf2varldpy5hps3mvp4m
          name_prefix = test-role-
          path = /
          unique_id = AROAJRWZLRPOSID3VDDLA
=== RUN   TestAccAWSRole_testNameChange
--- FAIL: TestAccAWSRole_testNameChange (18.80s)
    testing.go:264: Step 0 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        UPDATE: aws_iam_role.role_update_test
          assume_role_policy: "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"ec2.amazonaws.com\"},\"Action\":\"sts:AssumeRole\"}]}" => "{\n  \"Version\": \"2012-10-17\",\n  \"Statement\": [\n    {\n      \"Action\": \"sts:AssumeRole\",\n      \"Principal\": {\n        \"Service\": \"ec2.amazonaws.com\"\n      },\n      \"Effect\": \"Allow\",\n      \"Sid\": \"\"\n    }\n  ]\n}\n"

        STATE:

        aws_iam_instance_profile.role_update_test:
          ID = role_update_test
          arn = arn:aws:iam::881237884953:instance-profile/test/role_update_test
          name = role_update_test
          path = /test/
          roles.# = 1
          roles.1207061191 = tf_old_name

          Dependencies:
            aws_iam_role.role_update_test
        aws_iam_role.role_update_test:
          ID = tf_old_name
          arn = arn:aws:iam::881237884953:role/test/tf_old_name
          assume_role_policy = {"Version":"2012-10-17","Statement":[{"Sid":"","Effect":"Allow","Principal":{"Service":"ec2.amazonaws.com"},"Action":"sts:AssumeRole"}]}
          name = tf_old_name
          path = /test/
          unique_id = AROAJGEO4CMAXFNDJ6FSA
        aws_iam_role_policy.role_update_test:
          ID = tf_old_name:role_update_test
          name = role_update_test
          policy = {
          "Version": "2012-10-17",
          "Statement": [
            {
              "Effect": "Allow",
              "Action": [
                "s3:GetBucketLocation",
                "s3:ListAllMyBuckets"
              ],
              "Resource": "arn:aws:s3:::*"
            }
          ]
        }

          role = tf_old_name

          Dependencies:
            aws_iam_role.role_update_test
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    74.733s
make: *** [testacc] Error 1

[bigkraig]

I've renamed it

[stack72]

Hi @bigkraig

The tests still fail here, I'm afraid

P.

[bigkraig]

@stack72 this one is tough. since we weren't reading in the policy before the test cases are littered with different spacing for the policy and updates are causing diffs everywhere. Since the policy is compared as a generic string and not the data structure, it breaks a lot of things.

we could go and update assume_role_policy strings all over the place to make them consistent, but i wonder if this was solved in a better way somewhere else already?

[bigkraig]

@stack72 OK, here are the files that I've modified and verified these tests are passing

builtin/providers/aws/resource_aws_iam_role_policy_attachment_test.go:
  TestAccAWSRolePolicyAttachment_

builtin/providers/aws/resource_aws_iam_role_test.go:
  TestAccAWSRole_

builtin/providers/aws/resource_aws_iam_role_policy_test.go:
  TestAccAWSIAMRolePolicy_

builtin/providers/aws/resource_aws_iam_instance_profile_test.go:
  TestAccAWSIAMInstanceProfile_

builtin/providers/aws/resource_aws_iam_policy_attachment_test.go:
  TestAccAWSPolicyAttachment_

[bigkraig]

@stack72 ping! =)

[bigkraig]

@stack72 hey Paul, can you check this out?

[stack72]

Hi @bigkraig

Apologies for taking so long to get back to you on this - tests look solid now :)

Thanks so much

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRole_'
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/10 12:13:39 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRole_ -timeout 120m
=== RUN   TestAccAWSRole_importBasic
--- PASS: TestAccAWSRole_importBasic (25.28s)
=== RUN   TestAccAWSRole_basic
--- PASS: TestAccAWSRole_basic (24.26s)
=== RUN   TestAccAWSRole_namePrefix
--- PASS: TestAccAWSRole_namePrefix (25.59s)
=== RUN   TestAccAWSRole_testNameChange
--- PASS: TestAccAWSRole_testNameChange (54.44s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    129.594s

Paul

[catsby]

Hello friends –

I reverted this PR in #8112 after noticing that our nightly test suite took a nose dive into a sea of bad times:

• https://travis-ci.org/hashicorp/terraform/builds/151113132

After another review and some git bisect I found this PR to be the cause, specifically this:

+   policy, _ := url.QueryUnescape(*role.AssumeRolePolicyDocument)
+   if err := d.Set("assume_role_policy", aws.String(policy)); err != nil {
+       return err
+   }

and this:

resource "aws_iam_role" "role" {
-    name = "test-role"
-     assume_role_policy = <<EOF
-{
-  "Version": "2012-10-17",
-  "Statement": [
-    {
-      "Action": "sts:AssumeRole",
-      "Principal": {
-        "Service": "ec2.amazonaws.com"
-      },
-      "Effect": "Allow",
-      "Sid": ""
-    }
-  ]
-}
-EOF
+  name = "test-role"
+  assume_role_policy = "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"ec2.amazonaws.com\"},\"Action\":\"sts:AssumeRole\"}]}"
  }

What's happened here is users can no longer use the heredoc format for assume_role_policy, which is a common pattern for Terraform configuration files (though not without it's own problems).

It seems we ran into this (comment above) but to address it we just didn't use heredoc, which I'm afraid isn't going to be acceptable for the foreseeable future. While the aws_iam tests passed, all the other suites that use iam_role and the heredoc format started failing.

We need to work into this PR a way to allow the heredoc behavior (ideally restoring some of the test configs) before we can move forward there

[tomwilkie]

Heredoc problems have been addressed in #9398

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.