IAM Support: Roles, Role Policies and Instance Profiles #343

Closed
wants to merge 9 commits into
from

Projects

None yet

3 participants

@Fodoj
Contributor
Fodoj commented Sep 23, 2015

Support for IAM resources.

  • Roles
  • Instance Profiles
  • RolePolicies
@Fodoj Fodoj referenced this pull request Sep 23, 2015
Open

New Resource: IAM Role #224

@Fodoj
Contributor
Fodoj commented Sep 23, 2015

Implements #224

@Fodoj
Contributor
Fodoj commented Sep 23, 2015

Added role policies support.

@tyler-ball any feedback so far?

@tyler-ball tyler-ball commented on the diff Sep 24, 2015
lib/chef/provisioning/aws_driver.rb
@@ -17,7 +20,6 @@
require "chef/resource/aws_load_balancer"
require "chef/resource/aws_network_acl"
require "chef/resource/aws_network_interface"
-require "chef/resource/aws_route_table"
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

It looks like you need to rebase off master

@Fodoj
Fodoj Sep 24, 2015 Contributor

for some reason it's required twice in this file, see line 25.

@tyler-ball
tyler-ball Oct 1, 2015 Contributor

good catch - thanks!

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/resource/aws_iam_instance_profile.rb
@@ -0,0 +1,33 @@
+require 'chef/provisioning/aws_driver/aws_resource'
+
+#
+# An AWS IAM instance profile, a container for an IAM role that you can use to
+# pass role information to an EC2 instance when the instance starts..
+#
+# `name` is unique for an AWS account.
+#
+# API documentation for the AWS Ruby SDK for IAM instance profiles (and the object returned from `aws_object`) can be found here:
+#
+# - http://docs.aws.amazon.com/sdkforruby/api/Aws/IAM/InstanceProfile.html
+#
+class Chef::Resource::AwsIamInstanceProfile < Chef::Provisioning::AWSDriver::AWSResource
+ aws_sdk_type ::Aws::IAM::InstanceProfile, id: :name
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

:name is the default id so you don't need to specify that unless you want to

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/resource/aws_iam_instance_profile.rb
+
+#
+# An AWS IAM instance profile, a container for an IAM role that you can use to
+# pass role information to an EC2 instance when the instance starts..
+#
+# `name` is unique for an AWS account.
+#
+# API documentation for the AWS Ruby SDK for IAM instance profiles (and the object returned from `aws_object`) can be found here:
+#
+# - http://docs.aws.amazon.com/sdkforruby/api/Aws/IAM/InstanceProfile.html
+#
+class Chef::Resource::AwsIamInstanceProfile < Chef::Provisioning::AWSDriver::AWSResource
+ aws_sdk_type ::Aws::IAM::InstanceProfile, id: :name
+
+ #
+ # The name of the role to create.
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

The name of the instance profile to create

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/resource/aws_iam_role_policy.rb
+# API documentation for the AWS Ruby SDK for IAM role policies (and the object returned from `aws_object`) can be found here:
+#
+# - http://docs.aws.amazon.com/sdkforruby/api/Aws/IAM/RolePolicy.html
+#
+class Chef::Resource::AwsIamRolePolicy < Chef::Provisioning::AWSDriver::AWSResource
+ aws_sdk_type ::Aws::IAM::RolePolicy
+
+ #
+ # The name of the policy to create
+ #
+ attribute :name, kind_of: String, name_attribute: true
+
+ #
+ # The name of the role policy belongs to
+ #
+ attribute :role, kind_of: String, required: true
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

This should also accept kind_of: [String, AwsIamRole, ::Aws::IAM::Role] like in the instance profile. Then the aws_object logic will need to change since it is no longer guaranteed a string.

@tyler-ball tyler-ball commented on the diff Sep 24, 2015
spec/integration/aws_iam_role_spec.rb
+describe Chef::Resource::AwsIamRole do
+ extend AWSSupport
+
+ when_the_chef_12_server "exists", organization: "foo", server_scope: :context do
+ with_aws "when connected to AWS" do
+
+ context "Basic IAM role creation" do
+ role_name = mk_role_name
+
+ it "aws_iam_role '#{role_name}' creates an IAM role" do
+
+ expect_recipe {
+ aws_iam_role role_name do
+ assume_role_policy_document ec2_role_policy
+ end
+ }.to create_an_aws_iam_role(role_name).and be_idempotent
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

You should verify the policy_document here too

@Fodoj
Fodoj Sep 24, 2015 Contributor

which policy_document?

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
spec/integration/aws_iam_role_spec.rb
+ expect_recipe {
+ aws_iam_role role_name do
+ assume_role_policy_document ec2_role_policy
+ end
+ }.to create_an_aws_iam_role(role_name).and be_idempotent
+ end
+
+ end
+
+ context "create role with instance profile" do
+ role_name = mk_role_name
+
+ it "aws_iam_instance_profile '#{role_name}' creates an instance profile with role" do
+ expect_recipe {
+
+ aws_iam_role role_name do
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

You should move this resource out of the it example block and into the wrapping context block - this ensures it is created before the example and destroyed after the example, and you don't need the create_an_aws_iam_role matcher in the example.

Then you can check the instance profile with .to create_an_aws_iam_instance_profile

@tyler-ball tyler-ball commented on the diff Sep 24, 2015
spec/integration/aws_iam_role_spec.rb
+ }.to create_an_aws_iam_role(role_name).and be_idempotent
+
+ role = driver.iam_resource.role(role_name)
+
+ expect(role.instance_profiles.count).to eq 1
+ expect(role.instance_profiles.first.name).to eq(role_name)
+ end
+ end
+
+ context "create role with role policy profile" do
+ role_name = mk_role_name
+
+ it "aws_iam_role_policy 'rds_full_access' creates a role policy for role" do
+ expect_recipe {
+
+ aws_iam_role role_name do
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

This resource should also be declared in the context, and the matcher should be checking the aws_iam_role_policy.

@Fodoj
Fodoj Sep 24, 2015 Contributor

It won't work. aws_support helper is too simple: it calls destroy action on resources without passing any additional attributes. aws_iam_role_policy requires role in order to get aws_object. My first attempt was to implement role policy as managed entry resource. It quickly became a huge mess and it was super hard to make it work.

At that point I realised that I wrote huge pile of code only to be able to automatically delete role policy inside tests using this helpers. After realising that there are no other benefits of doing so, I rolled back managed entry changes and kept as simple as it is now. In this case role polices are deleted when role is deleted.

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_role.rb
@@ -0,0 +1,37 @@
+require 'chef/provisioning/aws_driver/aws_provider'
+
+class Chef::Provider::AwsIamRole < Chef::Provisioning::AWSDriver::AWSProvider
+ provides :aws_iam_role
+
+ def action_create
+ iam_role = super
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

This method is unedded because it doesn't do anything besides call super

@tyler-ball tyler-ball and 1 other commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_role.rb
+ path: new_resource.path,
+ role_name: new_resource.name,
+ assume_role_policy_document: new_resource.assume_role_policy_document
+ })
+ end
+ end
+
+ def update_aws_object(iam_role)
+ end
+
+ def destroy_aws_object(iam_role)
+ converge_by "delete #{iam_role.name}" do
+ iam_role.instance_profiles.each do |profile|
+ profile.remove_role(role_name: iam_role.name)
+ end
+ iam_role.policies.map(&:delete)
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

Can the RolePolicies exist without the role? Does AWS raise an error if you try to delete the Role while it still has RolePolicies?

Instead of doing this I think its better to do

iam_role.policies.each do |policy|
  Cheffish.inline_resource(self, action) do
    aws_iam_role_policy policy do
      action :destroy
    end
  end
end
@Fodoj
Fodoj Sep 24, 2015 Contributor

Yes, it complains if role with role policies is being removed

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_role.rb
+ iam = new_resource.driver.iam_resource
+
+ converge_by "create IAM role #{new_resource.name}" do
+ iam.create_role({
+ path: new_resource.path,
+ role_name: new_resource.name,
+ assume_role_policy_document: new_resource.assume_role_policy_document
+ })
+ end
+ end
+
+ def update_aws_object(iam_role)
+ end
+
+ def destroy_aws_object(iam_role)
+ converge_by "delete #{iam_role.name}" do
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

"delete IAM Role #{iam_role.name}"

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_role_policy.rb
@@ -0,0 +1,36 @@
+require 'chef/provisioning/aws_driver/aws_provider'
+
+class Chef::Provider::AwsIamRolePolicy < Chef::Provisioning::AWSDriver::AWSProvider
+ provides :aws_iam_role_policy
+
+ def action_create
+ iam_role_policy = super
@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_role_policy.rb
+ iam = new_resource.driver.iam_client
+
+ converge_by "create IAM role policy #{new_resource.name}" do
+ puts iam.put_role_policy({
+ role_name: new_resource.role,
+ policy_name: new_resource.name,
+ policy_document: new_resource.policy_document
+ }).inspect
+ end
+ end
+
+ def update_aws_object(iam_role_policy)
+ end
+
+ def destroy_aws_object(iam_role_policy)
+ converge_by "delete #{iam_role_policy.name}" do
@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_role_policy.rb
+
+ protected
+
+ def create_aws_object
+ iam = new_resource.driver.iam_client
+
+ converge_by "create IAM role policy #{new_resource.name}" do
+ puts iam.put_role_policy({
+ role_name: new_resource.role,
+ policy_name: new_resource.name,
+ policy_document: new_resource.policy_document
+ }).inspect
+ end
+ end
+
+ def update_aws_object(iam_role_policy)
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

It looks like you can update the inline policy document with http://docs.aws.amazon.com/sdkforruby/api/Aws/IAM/RolePolicy.html#put-instance_method

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_instance_profile.rb
+ iam_instance_profile = super
+
+ if new_resource.role && !iam_instance_profile.roles.map(&:name).include?(new_resource.role)
+ converge_by "associating role #{new_resource.role} with instance profile #{new_resource.name}" do
+ # Despite having collection methods for roles, instance profile can only have single role associated
+ clear_roles(iam_instance_profile)
+ iam_instance_profile.add_role({
+ role_name: new_resource.role
+ })
+ end
+ end
+ end
+
+ protected
+
+ def clear_roles(iam_instance_profile)
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

I think detach_roles is a more informative name

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_instance_profile.rb
@@ -0,0 +1,49 @@
+require 'chef/provisioning/aws_driver/aws_provider'
+
+class Chef::Provider::AwsInstanceProfile < Chef::Provisioning::AWSDriver::AWSProvider
+ provides :aws_iam_instance_profile
+
+ def action_create
+ iam_instance_profile = super
+
+ if new_resource.role && !iam_instance_profile.roles.map(&:name).include?(new_resource.role)
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

I'm fairly certain that this logic will need to be embedded into a update_attached_role(iam_instance_profile) method and called from both action_create and update_aws_object.

@tyler-ball tyler-ball commented on an outdated diff Sep 24, 2015
lib/chef/provider/aws_iam_instance_profile.rb
+ end
+
+ protected
+
+ def clear_roles(iam_instance_profile)
+ iam_instance_profile.roles.each do |r|
+ iam_instance_profile.remove_role(role_name: r.name)
+ end
+ end
+
+ def create_aws_object
+ iam = new_resource.driver.iam_resource
+
+ converge_by "create IAM instance profile #{new_resource.name}" do
+ iam.create_instance_profile({
+ path: new_resource.path,
@tyler-ball
tyler-ball Sep 24, 2015 Contributor

I like that this isn't defaulted in the resource (because it means it doesn't need to be specified in the :destroy action) but I think we should default it here to "/". Ditto to the path attribute in roles.

@tyler-ball
Contributor

Hey @Fodoj I think this is a great start on these resources/providers. I made a bunch of inline notes but they're mostly minor notes or implementation details. Thanks!

I have 3 high level comments:

  1. Lets restrict this PR to only these 3 resources. We can continue the rest of the IAM resources in later PRs, but its much easier to review if it is smaller.
  2. These 3 resources will need full test coverage. According to our CONTRIBUTING documentation we should test an
    1. :create action which fully populates all available attributes
    2. :create action against an existing resource which updates all available attributes
    3. :destroy action which shows the resource can be destroyed. It is really nice to make all attributes optional so that nothing needs to be set on the resource for the :destroy action besides the name. This means defaulting attributes in the provider.
  3. I have a design discussion question. I am reading about inline vs managed policies on AWS. Rather than creating a specific aws_iam_role_policy resource I think we could expose an inline_policies attribute on the aws_iam_role resource that allows users to specify the policies they want on that role. What do you think about that? I think it will be more clear to users the difference between inline policies and managed policies once we have both of them.

Cookbook using roles, policies and role policies in current code base:

aws_iam_policy "my_managed_policy" do
  policy_document "..."
  # maybe other attributes"
end

aws_iam_role "my_role" do
  path "/"
  assume_role_policy_document "..."
  managed_policies ["my_managed_policy"]
end

aws_iam_role_policy "my_role_policy" do
  role "my_role"
  policy_document "..."
end

But if we allow inline_policies I think the recipe gets easier to understand:

aws_iam_policy "my_managed_policy" do
  policy_document "..."
  # maybe other attributes"
end

aws_iam_role "my_role" do
  path "/"
  assume_role_policy_document "..."
  inline_policies {policy1: "...", policy2: "..."}
  managed_policies ["my_managed_policy"]
end

# Later, when we have users & groups

aws_iam_group "my_group" do
  # required attributes
  inline_policies {policy3: "...", policy4: "..."}
  managed_policies ["my_managed_policy"]
end
@Fodoj
Contributor
Fodoj commented Sep 24, 2015

@tyler-ball Thanks for review.

  1. I agree, I will introduce fixes you proposed and will work on other resources in separate PR.
  2. Ok, will do more tests
  3. I was thinking about it and actually that was my first thought about how to implement it. In the end I decided to go with first approach, because it's lest decoupled from actual aws sdk and aws api. There is nothing like "inline policy" in aws api or sdk. RolePolicy is a separate (though super weird) entity. Using some hash for describing them would probably be a nice syntax at first, but it will lead to weird implementation, confusion with end users of library and probably be more error prone that simple CRUD as it is now. Does it make sense?
@tyler-ball
Contributor

@Fodoj I am okay with them being separate resources. Proceed, and I will look forward to the next commit!

@Fodoj
Contributor
Fodoj commented Sep 24, 2015

@tyler-ball all changes are in, tests are green:

@Fodoj Fodoj changed the title from [WIP] IAM Support to IAM Support: Roles, Role Policies and Instance Profiles Sep 24, 2015
@pronix
pronix commented Sep 30, 2015

@tyler-ball @randomcamel Hello
will this PR merged ?
there are a lot people who using aws IAM features, why not merge it ?
More security less bureaucracy!!!! 7 days bla bla bla!!!!

@tyler-ball
Contributor

@pronix I'm going to review it, just haven't had time. Its next on my todo list

@tyler-ball tyler-ball added this to the 1.5.0 milestone Oct 2, 2015
@Fodoj
Contributor
Fodoj commented Oct 5, 2015

@tyler-ball so it will be included in 1.5.0?

@tyler-ball
Contributor

Closing this in favor of #358 - please see my description there for explanation. Thanks for the work @Fodoj - this will get merged and released in 1.5.0 soon!

@tyler-ball tyler-ball closed this Oct 7, 2015
@tyler-ball tyler-ball removed the Developing label Oct 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment