-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Allow importing of aws_iam_role, aws_iam_role_policy, aws_iam_policy and aws_iam_instance_profile. #9398
Conversation
…and aws_iam_instance_profile
HI @tomwilkie Thanks for the PR here - this is great! If possible, please can you add an import test to each of the resources? This would help us keep the import path code tested for each of the resources You can find an example of the code here Thanks again Paul |
Tests all work:
|
Thanks @tomwilkie :) P. |
As part of making this work, I can to make |
I'm not sure whether this is acceptable, so let me know if you can think of any workarounds. One idea would be to strip all whitespace from policy documents when they are read from the .tf files, but I don't know if terraform has machinery to do this yet. |
ok, we had a PR like this merged before and it caused issues everywhere. so we reverted it. I think we are going to have to shelve this PR for a while now while we investigate what the problems are Paul |
Do you have a link to said PR? I think we can work around this. |
Its seems a |
It seems @jen20 has already written everything we need! |
This is the PR #7617 :) |
Okay yes I've got a fix for the problems you hit there, just testing now... |
…_policy and aws_iam_policy.policy
Okay that fixed the whitespace / heredoc issue - they now work nicely. Just running the tests one last time but I don't see any problems. |
Yeah that works nicely:
|
ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tomwilkie
Sorry this has taken so long - I have left a few questions inline as to why some of the code paths have changed. Let me know your thoughts :)
Want to make sure we don't break any backwards compatibility for this.
Can you tell me if we use a template file and pass the json in or using HEREDOC syntax, will the diffSuppression still work as expected?
Paul
|
||
var testAccAwsIamPolicyConfig = ` | ||
resource "aws_iam_policy" "test" { | ||
name = "test_policy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to randomize this - if this fails then the test will fail the following night due to orphaned resources. This should be a func that accepts a random string as a name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckAWSPolicyDestroy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should jsut use the same Destroy method as defined in iam_user_test.go testAccCheckAWSUserDestroy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one destroys aws_iam_policy
, whereas the function you refer to destroys aws_iam_user
.
resource "aws_iam_role" "role" { | ||
name = "tf_test_role_test" | ||
path = "/" | ||
assume_role_policy = "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"ec2.amazonaws.com\"},\"Action\":\"sts:AssumeRole\"}]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a heredoc affect how the import works here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not; would you like me to make this a heredoc?
} | ||
|
||
resource "aws_iam_role_policy" "foo" { | ||
name = "tf_test_policy_test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to randomize the name :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
var testAccAwsIamRolePolicyConfig = ` | ||
resource "aws_iam_role" "role" { | ||
name = "tf_test_role_test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Randomize the name :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -81,7 +86,29 @@ func resourceAwsIamPolicyRead(d *schema.ResourceData, meta interface{}) error { | |||
return fmt.Errorf("Error reading IAM policy %s: %s", d.Id(), err) | |||
} | |||
|
|||
return readIamPolicy(d, response.Policy) | |||
getPolicyVersionRequest := &iam.GetPolicyVersionInput{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we looking for a specific policy version? This is a new code path that wasn't in use before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're looking for the DefaulVersionId from the response to GetPolicy
.
return fmt.Errorf("Error reading IAM policy version %s: %s", d.Id(), err) | ||
} | ||
|
||
policy, err := url.QueryUnescape(*getPolicyVersionResponse.PolicyVersion.Document) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why queryunescape? What is this doing for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS policies are return urlencoded: http://docs.aws.amazon.com/IAM/latest/APIReference/API_GetPolicyVersion.html
@@ -221,7 +248,8 @@ func readIamPolicy(d *schema.ResourceData, policy *iam.Policy) error { | |||
if err := d.Set("arn", *policy.Arn); err != nil { | |||
return err | |||
} | |||
// TODO: set policy | |||
|
|||
if err := d.Set("arn", *policy.Arn); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d.Set doesn't need to have a value dereferenced before being passed - it safely dereferences a pointer if it can :)
Thanks for feedback! Will get that addressed ASAP. |
@stack72 PTAL |
Hi @tomwilkie Finally getting around to looking at this right now - sorry it took so long! Paul |
Hi @tomwilkie I am going to merge this manually - I actually added support for the import of aws_iam_instance_profile recently :) So had some conflicts
|
Manually merged to master! |
Thanks Paul!
…On Thu, Dec 1, 2016 at 2:42 PM Paul Stack ***@***.***> wrote:
Closed #9398 <#9398>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9398 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAbGhVJoke1LQeaOsMJ4twWwFu6sk13Wks5rDtzWgaJpZM4KYhzV>
.
|
Totally perplexed : Terraform v0.7.13 % terraform import aws_iam_role.MY_Cool_Role My_Cool_Name_RoleError importing: 1 error(s) occurred:
|
Hi @clearly You may need to upgrade to a later version to use this feature Paul |
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. |
No description provided.