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

Add AWS cloud provider option for IAM role #59668

Merged
merged 1 commit into from Feb 16, 2018

Conversation

Projects
None yet
5 participants
@brycecarman
Copy link
Contributor

brycecarman commented Feb 9, 2018

What this PR does / why we need it:
Adds the option to provide an IAM role ARN in the AWS cloud provider config file that should be assumed when communicating with the AWS APIs.
For example, this allows running Controller Manager in a account separate from the worker nodes, but still allows all resources created to interact with the workers. ELBs created would be in the same account as the worker nodes for instance.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59526

Special notes for your reviewer:
None

Release note:

Add AWS cloud provider option to use an assumed IAM role 
@brycecarman

This comment has been minimized.

Copy link
Contributor Author

brycecarman commented Feb 9, 2018

/assign @justinsb

@@ -526,6 +528,9 @@ type CloudConfig struct {
// RouteTableID enables using a specific RouteTable
RouteTableID string

// The IAM role to assume when interacting with AWS

This comment has been minimized.

@justinsb

justinsb Feb 15, 2018

Member

Nit: I believe go comments are supposed to follow "RoleArn is the IAM role..."

Doc comments work best as complete sentences, which allow a wide variety of automated presentations. The first sentence should be a one-sentence summary that starts with the name being declared.

https://golang.org/doc/effective_go.html

I'm not that strict on it though :-)

glog.Infof("Using AWS assumed role %v", cfg.Global.RoleArn)
provider = &stscreds.AssumeRoleProvider{
Client: sts.New(sess),
RoleARN: cfg.Global.RoleArn,

This comment has been minimized.

@justinsb

justinsb Feb 15, 2018

Member

On RoleArn vs RoleARN - Go tends to prefer RoleARN. https://github.com/golang/go/wiki/CodeReviewComments#initialisms

But this is actually bound to a config file. It looks like you can override the mapping with a gcfg tag, but we haven't done that yet. Maybe just be sure that you want the config option in the file to be RoleArn (i.e. is that consistent with usage elsewhere in AWS configuration files, outside of kubernetes) and then I think consistency trumps golang naming conventions.

This comment has been minimized.

@brycecarman

brycecarman Feb 15, 2018

Author Contributor

Agreed. It is RoleARN in AWS SDK so there is no reason to deviate from that.

@@ -389,7 +396,7 @@ func TestGetRegion(t *testing.T) {

func TestFindVPCID(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterId)
c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
c, err := newAWSCloud(CloudConfig{}, awsServices)

This comment has been minimized.

@justinsb

justinsb Feb 15, 2018

Member

This makes me happy :-)

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Feb 15, 2018

/ok-to-test

/lgtm

Maybe double-check if you want the config file option to be RoleArn or RoleARN, but I'll go with what you recommend.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 15, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@brycecarman

This comment has been minimized.

Copy link
Contributor Author

brycecarman commented Feb 15, 2018

/hold

@brycecarman brycecarman force-pushed the brycecarman:ccm-iam-role branch from d5a4d86 to e0676eb Feb 16, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Feb 16, 2018

@brycecarman

This comment has been minimized.

Copy link
Contributor Author

brycecarman commented Feb 16, 2018

/hold cancel

Add AWS cloud provider option for IAM role
Currently the AWS cloud provider uses the EC2 instance role when
interacting with AWS APIs. This change gives the option to provide and IAM
role that the cloud provider will assume before calling the APIs. All
resources created by the role will be owned by that account instead of
the account where the EC2 instance is running.

@brycecarman brycecarman force-pushed the brycecarman:ccm-iam-role branch from e0676eb to 3b99e1b Feb 16, 2018

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Feb 16, 2018

/lgtm

Thanks @brycecarman !

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 16, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycecarman, justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 16, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 16, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 1db96b0 into kubernetes:master Feb 16, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation brycecarman authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@wojtek-t wojtek-t referenced this pull request Mar 21, 2018

Closed

[test flakes] master-scalability suites #60589

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.