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

Implement a subset of IAM resources #939

Merged
merged 4 commits into from
May 5, 2015
Merged

Implement a subset of IAM resources #939

merged 4 commits into from
May 5, 2015

Conversation

bitglue
Copy link

@bitglue bitglue commented Feb 6, 2015

Implements support for IAM users, user policies, and access keys. This
is only a subset of what IAM can do (notably missing: roles and instance
profiles and associated policies), but it's a start.

Makes a dent in #28.

The user policies and access keys are implemented as their own resources, rather than being sub-trees in the aws_iam_user, to support use cases where one module defines a user, and other modules add policies or keys.

@mitchellh
Copy link
Contributor

This actually all looks really good. Can you add acceptance tests? Once that is in, I'm happy to merge. Thanks for doing docs, as well.

@bitglue
Copy link
Author

bitglue commented Feb 17, 2015

I'll give it a shot. Might be some days or weeks.

@radeksimko
Copy link
Member

👍
Except acceptance tests, it may also need links in the website sidebar:

diff --git a/website/source/layouts/aws.erb b/website/source/layouts/aws.erb
index 030192d..e14ba79 100644
--- a/website/source/layouts/aws.erb
+++ b/website/source/layouts/aws.erb
@@ -41,6 +41,18 @@
                                        <a href="/docs/providers/aws/r/elb.html">aws_elb</a>
                     </li>

+                    <li<%= sidebar_current("docs-aws-resource-iam=access-key") %>>
+                    <a href="/docs/providers/aws/r/iam_access_key.html">aws_iam_access_key</a>
+                    </li>
+
+                    <li<%= sidebar_current("docs-aws-resource-iam-user") %>>
+                    <a href="/docs/providers/aws/r/iam_user.html">aws_iam_user</a>
+                    </li>
+
+                    <li<%= sidebar_current("docs-aws-resource-iam-user-policy") %>>
+                    <a href="/docs/providers/aws/r/iam_user_policy.html">aws_iam_user_policy</a>
+                    </li>
+
                     <li<%= sidebar_current("docs-aws-resource-instance") %>>
                                        <a href="/docs/providers/aws/r/instance.html">aws_instance</a>
                                        </li>

and it would be nice to have aws_iam_role resource as well.
http://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateRole.html

Most importantly some more significant changes will need to be done to make it work with the new aws-sdk-go library which is now being used for almost all AWS resources (no more goamz).

@bitglue
Copy link
Author

bitglue commented Mar 6, 2015

Agreed, @radeksimko. Thanks for the feedback. I will tackle those issues eventually. Currently I'm focused on delivering an internal demo and proof of concept, but when that's done (and successful, I hope), I'll be making a finishing pass over this and many other things. Still some ways out on my priority list, however.

@bitglue
Copy link
Author

bitglue commented Mar 23, 2015

I've updated the website as @radeksimko suggested, and updated to use aws-sdk-go. Still needs some work on the rough edges, and acceptance tests.

@bitglue
Copy link
Author

bitglue commented Apr 9, 2015

@radeksimko roles and instance profiles are implemented now.

I did some horrible copy&paste to implement groups/users/roles, which really are almost exactly the same file. I'll have to do some thinking on how to clean that up.

Also, I didn't implement IAM managed policies because the vendored AWS SDK didn't support them. But I see now that the current version from aws-labs which does support policies is in use in places -- is migrating to that the goal?

@radeksimko
Copy link
Member

But I see now that the current version from aws-labs which does support policies is in use in places -- is migrating to that the goal?

I believe it is, @catsby started migrating all existing AWS resources over back to the vendored AWS SDK. I would probably hold on until all the resources are migrated and "workarounds" (like import aliases) removed, then I'd update the code so it uses the vendored version.

@catsby
Copy link
Contributor

catsby commented Apr 10, 2015

I'm in the process of migrating to the upstream awslabs version of aws-sdk-go, and retiring our fork. It is a bit of an as-I-can process, while I try to stay on top of bug fixes in the AWS provider as well.

@bitglue to my knowledge, RDS is the only resource currently using the IAM connection I added in #1292. If it's helpful I can convert RDS next, so you can convert/update this PR to use the upstream library and carry on with IAM managed policies.

@ocxo
Copy link

ocxo commented Apr 10, 2015

@catsby is there anything you could use some help with in the awslabs migration?

@catsby
Copy link
Contributor

catsby commented Apr 10, 2015

@fromonesrc I created #1488 to track the migrations

@bitglue
Copy link
Author

bitglue commented Apr 11, 2015

On Apr 10, 2015 9:57 AM, "Clint" notifications@github.com wrote:

@bitglue to my knowledge, RDS is the only resource currently using the
IAM connection I added in #1292. If it's helpful I can convert RDS next, so
you can convert/update this PR to use the upstream library and carry on
with IAM managed policies.

That would be super great.

@catsby
Copy link
Contributor

catsby commented Apr 13, 2015

@bitglue I opened #1510 for converting RDS, and upgrading the IAM connection. I suppose the latter wasn't strictly needed, but I wanted to rid the resource of hashicorp/aws-sdk-go. Only RDS instance is converted there; parameter group and subnet to follow. UPDATE I bundled Parameter Group, Subnet, and Security Group in that PR as well

@bitglue
Copy link
Author

bitglue commented Apr 17, 2015

@catsby Thanks for doing that. I've rebased to the current master so this should now work with the awslabs SDK.

@johnrengelman
Copy link
Contributor

This is the biggest missing piece for me right now with managing our environment. +1 for the feature.

@johnrengelman
Copy link
Contributor

I've added a number of basic acceptance tests on top of these commits here: #1591. Hopefully this gets us to a point where we can merge this in.

@bitglue
Copy link
Author

bitglue commented Apr 19, 2015

If we can wait a day or two, I'm adding a resource for managed policies. And I do have a clean account, so I'll give @johnrengelman's tests a run.

@ravenac95
Copy link

I found some issues in testing this on my own aws account. When creating everything from an aws_iam_role, aws_iam_role_policy, aws_iam_instance_profile, and an aws_instance terraform operates too quickly for changes to propagate. So what seems to happen is that aws_iam_instance_profile gets "created" but when aws_instance attempts to spin up an EC2 instance the aws_iam_instance_profile resource is not yet ready.

Essentially I see the following error:

Error applying plan:

1 error(s) occurred:

* 1 error(s) occurred:

* 1 error(s) occurred:

* 1 error(s) occurred:

* Error launching source instance: InvalidParameterValue: Value (test-server-profile) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

I may have some time to look at this over the course of this week, but I just wanted to make sure that this problem was known. Just in case someone had some ideas off the top of their head.

@bitglue
Copy link
Author

bitglue commented Apr 28, 2015

@ravenac95 I've never had that happen to me. Do you have a repro case you can share?

@ravenac95
Copy link

@bitglue here's what I have that consistently breaks. (There's a step by step at the bottom)

In test.tf:

variable "aws_access_key" {}
variable "aws_secret_key" {}

variable "ec2_ami" {
    default = "ami-d05e75b8"
}
variable "ec2_key" {}

variable "ec2_instance_type" {
    default = "m3.medium"
}

variable "server_name" {
    default = "foo-server"
}
variable "server_type" {
    default = "foo-server"
}
variable "server_foo_volume_size" {
    default = 1024
}
variable "server_foo_volume_type" {
    default = "standard"
}

provider "aws" {
    access_key = "${var.aws_access_key}"
    secret_key = "${var.aws_secret_key}"
    region = "us-east-1"
}

resource "aws_iam_role" "foo_server" {
    name = "${ var.server_type }-role"

    assume_role_policy = <<EOF
{
    "Version": "2012-10-17",
    "Statement": [{
        "Effect": "Allow",
        "Principal": { "Service": "ec2.amazonaws.com" },
        "Action": "sts:AssumeRole"
    }]
}
EOF
}

resource "aws_iam_role_policy" "foo_server" {
    name = "${ var.server_type }-role-policy"
    role = "${ aws_iam_role.foo_server.name }"
    policy = <<EOF
{
    "Version": "2012-10-17",
    "Statement": [{
        "Action": [
            "ec2:*"
        ],
        "Effect": "Allow",
        "Resource": "*"
    }]
}
EOF
}

resource "aws_iam_instance_profile" "foo_server" {
    name = "${ var.server_type }-profile"
    roles = ["${ aws_iam_role.foo_server.name }"]
}

resource "aws_security_group" "foo_server" {
    name = "${ var.server_name }"
    description = "Security group for foo server"

    ingress {
        from_port = 22
        to_port = 22
        protocol = "tcp"
        cidr_blocks = ["0.0.0.0/0"]
    }
}

resource "aws_instance" "foo_server" {
    ami = "${ var.ec2_ami }"
    instance_type = "${ var.ec2_instance_type }"
    security_groups = ["${ aws_security_group.foo_server.name }"]
    key_name = "${ var.ec2_key }"

    tags {
        Name = "${ var.server_name }"
        Type = "${ var.server_type }"
    }

    ebs_block_device {
        device_name = "/dev/sdf"
        volume_size = "${ var.server_foo_volume_size }"
        volume_type = "${ var.server_foo_volume_type }"
        encrypted = true
        delete_on_termination = false
    }

    iam_instance_profile = "${ aws_iam_instance_profile.foo_server.name }"
}

output "ip" {
    value = "${aws_instance.foo_server.public_dns}"
}

Then in a test.tfvars file:

aws_access_key = "...key..."
aws_secret_key = "...key..."
ec2_key = "...key-name..."

First, let me know if I'm doing something silly here. Perhaps I just misconfigured something.

Step-by-Step Reproduction

  1. Run terraform apply -var-file test.tfvars
  2. That will fail with an error like the one from above except (foo-server-profile) instead of (test-server-profile)
  3. Run terraform apply -var-file test.tfvars again
  4. That should work!

@catsby
Copy link
Contributor

catsby commented May 4, 2015

Hey all – how do we feel about the state of this pull request? Are there questions/issues outstanding, or do you feel it's ready to be merged?

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label May 4, 2015
@johnrengelman
Copy link
Contributor

I think the errors being described above are likely AWS "eventual consistency" type things. So I think we should merge this to get it out there and then fix those types of errors as the come up.

@bitglue
Copy link
Author

bitglue commented May 4, 2015

Agree.

@bitglue
Copy link
Author

bitglue commented May 4, 2015

@ravenac95 thanks for the repro case, BTW. I can reproduce, but I don't have the bandwidth to fix it now. I thought maybe #1732 would be a way forward. If this gets merged, probably you should move the repro case to a new issue.

@ravenac95
Copy link

@bitglue No worries. I may poke around (terraform as a whole) to see if there's any way to bring a better solution to AWS's eventual consistency, but for the most part I think the problems encountered by my case are not indicative of a problem in this PR but represent a problem at a higher level. So ya, I agree this should be merged as well :)

@catsby
Copy link
Contributor

catsby commented May 5, 2015

Hey @bitglue – this looks awesome, thanks!

We do have at least one request, and that is to break out the changes you have made here to schema helper, with the schema.HashString method and friends, into a separate pull request. I realize some of your IAM resources use that method, so we'll be sure to merge it first, but it would help separate concerns here.

Let us know if you can work on that, and thanks again!

@bitglue
Copy link
Author

bitglue commented May 5, 2015

@catsby It's already a separate commit. If you want to merge it separately, I think just git merge 3756e425c370f65a8fdee7682efd43c3b5ba797c will do that, provided you've git fetched my fork first.

Phil Frost and others added 4 commits May 5, 2015 12:47
Sets of strings are pretty common. Let's not duplicate the function
necessary to create a set of strings in so many places.
- Users
- Groups
- Roles
- Inline policies for the above three
- Instance profiles
- Managed policies
- Access keys

This is most of the data types provided by IAM. There are a few things
missing, but the functionality here is probably sufficient for 95% of
the cases. Makes a dent in hashicorp#28.
@bitglue
Copy link
Author

bitglue commented May 5, 2015

Rebased to fix conflicts, so now that commit is 33183c0.

@johnrengelman
Copy link
Contributor

You should probably just create a new PR from that commit so they can merge it with a button click. Just git the button from here: master...bitglue:33183c078bfb820d32a0135c5ec928aa88d77c5c

@bitglue
Copy link
Author

bitglue commented May 5, 2015

No thanks. Then I'll have two PRs to keep coherent and free of conflicts.

@catsby
Copy link
Contributor

catsby commented May 5, 2015

Pulling this in, thanks.

catsby added a commit that referenced this pull request May 5, 2015
Implement a subset of IAM resources
@catsby catsby merged commit 18b43b7 into hashicorp:master May 5, 2015
@bitglue bitglue deleted the iam branch July 10, 2015 16:36
@ghost
Copy link

ghost commented May 1, 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 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.

@ghost ghost locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants