Add AWS cloud provider option for IAM role #59668
Conversation
k8s-ci-robot
added
release-note
|
/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 | |||
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) | |||
|
/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. |
k8s-ci-robot
added
lgtm
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
brycecarman
force-pushed
the
ccm-iam-role
branch
from
d5a4d86 to
e0676eb
Compare
k8s-github-robot
removed
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
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
force-pushed
the
ccm-iam-role
branch
from
e0676eb to
3b99e1b
Compare
|
/lgtm Thanks @brycecarman ! |
k8s-ci-robot
added
the
lgtm
|
[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 |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
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: