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

Move cloud-specific roles out of RBAC bootstrap #66635

Merged

Conversation

@wgliang
Copy link
Member

commented Jul 26, 2018

What this PR does / why we need it:
The system:aws-cloud-provider ClusterRole(Binding)s should be removed from the bootstrap policy, and instead configured by the provisioning tools.

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

Special notes for your reviewer:
@tallclair

/sig aws
/sig cloud-provider
/sig auth

Release note:

The `system:aws-cloud-provider` cluster role, deprecated in v1.13, is no longer auto-created. Deployments using the AWS cloud provider should grant required permissions to the `aws-cloud-provider` service account in the `kube-system` namespace as part of deployment. 
@tallclair

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

Deleting the roles is the easy part. The hard part is coordinating with the whole provisioning ecosystem to avoid unexpected breakage. How do we know when this is safe to merge?

@micahhausler

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

@tallclair I'm not familiar with where what component this gets run in, is it part of the APIServer?

@tallclair

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

@micahhausler yeah, it's part of the RBAC plugin, so whichever apiserver that's loaded in. Typically it's the kube-aggregator server, with extension servers using delegation, but I think it's also possible to run RBAC in each extension apiserver.

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

/retest

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2018

/test pull-kubernetes-bazel-test

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2018

@wgliang I think the expected output needs to be updated.

duglin added a commit to duglin/kubernetes that referenced this pull request Sep 10, 2018
Missed some files in kubernetes#66635
Signed-off-by: Doug Davis <dug@us.ibm.com>
duglin added a commit to duglin/kubernetes that referenced this pull request Sep 10, 2018
Missed some files in kubernetes#66635
Signed-off-by: Doug Davis <dug@us.ibm.com>
@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@wgliang will you have time to finish this one?

@wgliang

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

Yeah, I'll will update it.

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

@wgliang thanks. If it helps, I created this: master...duglin:pr66635 if case you didn't have time. There's another "aws" line I think might need to be removed too.

@wgliang wgliang force-pushed the wgliang:feature/remove-aws-cloud-provider branch from f329c87 to c5875e7 Sep 11, 2018

@wgliang

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

@duglin
Thanks for your review. I have updated it.

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

/lgtm

@duglin

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

with the thaw... any chance of moving this one along now?

@tallclair

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

This LGTM, but I'd like to get approval from someone from @kubernetes/sig-aws-misc
Maybe @justinsb ?

@duglin

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

ping @justinsb @kubernetes/sig-aws-misc

@tallclair

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

/cc @lavalamp - FYI

@k8s-ci-robot k8s-ci-robot requested a review from lavalamp Oct 10, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

How did this get in there in the first place? We really shouldn't have any cloud provider specific code like this that's not hidden behind an interface in a cloud provider package.

@gnufied

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

@lavalamp it got in here via #56709 and at that time there used to be a generic system:cloud-provider service account which had similar access to modify resources. There is a long history of comments how it got there(if you read the PR). but GCE cloudprovider has similar access. I originally proposed AWS cloud-provider to use same system:cloud-provider service account that GCE uses.

@gnufied

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

BTW - is there any reason we can't deprecate this RBAC policy first and remove it in next release? That at least will give time for installation tools to catch up and will be consistent with how other cloudprovider policies are being removed. cc @justinsb

@liggitt

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

agree with announcing removal prior to removing

@liggitt

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@wgliang can you make sure a deprecation notice makes it into the 1.13 release notes, and is announced to the relevant sigs (looks like aws, in this case), along with the required action for deployers? do we want to copy this role out to a yaml manifest that deployers can use as they transition to granting this permission externally?

@wgliang

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2018

@liggitt No problem. I will follow up with 1.13 release note and communicate with the relevant personnel.

@liggitt

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

added to 1.13 release notes as deprecated

/hold
for 1.15

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

/hold cancel

master is open for 1.15 dev now, can you rebase this?

@wgliang wgliang force-pushed the wgliang:feature/remove-aws-cloud-provider branch from c5875e7 to 128fd88 Apr 2, 2019

@wgliang

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

@liggitt Done.

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

/retest
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, liggitt, wgliang

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-ci-robot k8s-ci-robot merged commit 4e397d9 into kubernetes:master Apr 2, 2019

17 checks passed

cla/linuxfoundation wgliang authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
justinsb added a commit to justinsb/kops that referenced this pull request May 9, 2019
Include aws-cloud-provider roles in 1.15
We consider it part of the storage configuration for AWS now.

Upstream change: kubernetes/kubernetes#66635
justinsb added a commit to justinsb/kops that referenced this pull request May 9, 2019
Include aws-cloud-provider roles in 1.15
We consider it part of the storage configuration for AWS now.

Upstream change: kubernetes/kubernetes#66635
rjosephwright added a commit to cloudboss/keights that referenced this pull request Jun 25, 2019
Update to support Kubernetes 1.15
* Update kubeadm init and join configs with new versions and
  options.
* This includes switching to cgroupfs cgroup driver in kubelet
  due to kubernetes/kubernetes#76531.
* Add AWS RBAC to keights-system Ansible role since
  kubernetes/kubernetes#66635 removes
  those permissions by default.
* Update CI assets and README build status badges.
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.