Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Add [optional] explicit IAM role specification to NodeDrainer #1123

Merged
merged 1 commit into from
Feb 6, 2018
Merged

Add [optional] explicit IAM role specification to NodeDrainer #1123

merged 1 commit into from
Feb 6, 2018

Conversation

ivanilves
Copy link
Contributor

Add [optional] explicit IAM role specification to NodeDrainer.

If you are happy kube2iam user depending on your setup you may want no AWS credentials being available on pods having no IAM role specifications. At least I believe default access is an evil thing to be avoided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2018
@ivanilves
Copy link
Contributor Author

Maybe, not sure, but specifying explicit IAM role could help with #1105

@mumoshu
Copy link
Contributor

mumoshu commented Jan 30, 2018

@ivanilves Thanks for the fix. Yes, I began to believe we'd need to do this everywhere!

Btw, would you mind naming it iamRole.arn? So that we could enhance it to understand e.g. iamRole.create: true to let kube-aws create the role for you in the future.

@ivanilves ivanilves changed the title Add [optional] explicit IAM role specification to NodeDrainer [WIP] Add [optional] explicit IAM role specification to NodeDrainer Jan 30, 2018
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2018
@codecov-io
Copy link

codecov-io commented Jan 30, 2018

Codecov Report

Merging #1123 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1123      +/-   ##
==========================================
+ Coverage   35.23%   35.25%   +0.01%     
==========================================
  Files          63       63              
  Lines        3735     3736       +1     
==========================================
+ Hits         1316     1317       +1     
  Misses       2207     2207              
  Partials      212      212
Impacted Files Coverage Δ
model/node_drainer.go 100% <ø> (ø) ⬆️
core/controlplane/config/config.go 61.58% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 890ac66...7876cfb. Read the comment docs.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 30, 2018
@ivanilves ivanilves changed the title [WIP] Add [optional] explicit IAM role specification to NodeDrainer Add [optional] explicit IAM role specification to NodeDrainer Jan 31, 2018
@ivanilves
Copy link
Contributor Author

ivanilves commented Jan 31, 2018

Hey @mumoshu I've followed your advice.

Could you please re-review and comment, maybe something more needs to be changed?

@mumoshu
Copy link
Contributor

mumoshu commented Feb 6, 2018

@ivanilves Thanks a lot for taking your time! LGTM.

@mumoshu mumoshu merged commit dd20514 into kubernetes-retired:master Feb 6, 2018
@mumoshu mumoshu added this to the v0.9.10.rc-1 milestone Feb 6, 2018
@ivanilves
Copy link
Contributor Author

Thank you! ☺️ 👏

kylehodgetts pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Mar 27, 2018
…r-iam-role

Add [optional] explicit IAM role specification to NodeDrainer
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants