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

GCE: Create cloud-provider roles and bindings via addons #59686

Merged
merged 1 commit into from Feb 17, 2018

Conversation

@nicksardo
Member

nicksardo commented Feb 10, 2018

What this PR does / why we need it:
This removes the cloud-provider role and role binding from the rbac boostrapper and replaces it with a policy applied via addon mgr. This also creates a new clusterrole allowing the service account to create events for any namespace.

Special notes for your reviewer:
/assign @bowei @timstclair
/cc timstclair

Release note:

GCE: A role and clusterrole will now be provided with GCE/GKE for allowing the cloud-provider to post warning events on all services and watching configmaps in the kube-system namespace.
@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 10, 2018

/cc @MrHohn

@k8s-ci-robot k8s-ci-robot requested a review from MrHohn Feb 10, 2018

@bowei

This comment has been minimized.

Member

bowei commented Feb 10, 2018

/approve no-issue

- patch
- update
- list
- watch

This comment has been minimized.

@jhorwit2

jhorwit2 Feb 11, 2018

Member

Why did the privileges change?

This comment has been minimized.

@nicksardo

nicksardo Feb 12, 2018

Member

I think there may be a case in the future when we need patch/update on the configmap being used for our cluster id.

This comment has been minimized.

@nicksardo

nicksardo Feb 12, 2018

Member

Our ingress controller already updates this config map but currently uses the unauthenticated port.

@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 12, 2018

Outstanding question:
The kube-controller-manager usually starts prior to the addon manager applying these policies. As a result, the configmap watcher in the GCE cloudprovider errors out for tens of seconds. Meanwhile, the service controller will attempt to sync every loadbalancer at start. Since the clusterid hasn't been retrieved, the cloudprovider's EnsureLoadBalancer will error and cause a K8s event.

Need to test this PR more...

/hold

@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 12, 2018

Actually, the policies don't exist for new clusters, which means services also don't exist.
/unhold

@MrHohn

Looks good, some minor comments.

rbac.authorization.kubernetes.io/autoupdate: "true"
labels:
addonmanager.kubernetes.io/mode: Reconcile
kubernetes.io/cluster-service: "true"

This comment has been minimized.

@MrHohn

MrHohn Feb 12, 2018

Member

cluster-service label is no longer required for addons, except for those services that need to show up via kubectl cluster-info.

This comment has been minimized.

@nicksardo

nicksardo Feb 13, 2018

Member

Removing

function start-lb-controller {
setup-addon-manifests "addons" "loadbalancing"

This comment has been minimized.

@MrHohn

MrHohn Feb 12, 2018

Member

minor: having both cluster-loadbalancing/ and loadbalancing/ seems confusing. Should we make a cluster-loadbalancing/rbac folder or something similar?

This comment has been minimized.

@nicksardo

nicksardo Feb 12, 2018

Member

The readme says to avoid naming conflicts. Are subdirs merged correctly?

This comment has been minimized.

@MrHohn

MrHohn Feb 12, 2018

Member

Sorry, didn't notice this is under cluster/gce. Now it seems good :)

This comment has been minimized.

@nicksardo

nicksardo Feb 13, 2018

Member

I'm open to better naming. Thankfully, it can easily be changed later when/if we add more manifests.

@@ -85,13 +85,6 @@ func init() {
rbac.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("secrets").RuleOrDie(),
},
})
addNamespaceRole(metav1.NamespaceSystem, rbac.Role{
// role for the cloud providers to access/create kube-system configmaps
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "cloud-provider"},

This comment has been minimized.

@MrHohn

MrHohn Feb 12, 2018

Member

Just to make sure, is GCE the only consumer of this cloud-provider role?

This comment has been minimized.

@nicksardo

nicksardo Feb 12, 2018

Member

I checked kubernetes core and didn't see any other users. Naturally, that doesn't stop out-of-tree providers from depending on it. Sooner we can get this in and let people discover the change, the better.

This comment has been minimized.

@tallclair

tallclair Feb 14, 2018

Member

I think this may need to follow the deprecation policy, probably covered by rule #7:

Rule #7: Deprecated behaviors must function for no less than 1 year after their announced deprecation.

Has this removal been announced?

This comment has been minimized.

@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 14, 2018

/retest

@tallclair

This needs a release note.

@@ -85,13 +85,6 @@ func init() {
rbac.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("secrets").RuleOrDie(),
},
})
addNamespaceRole(metav1.NamespaceSystem, rbac.Role{
// role for the cloud providers to access/create kube-system configmaps
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "cloud-provider"},

This comment has been minimized.

@tallclair

tallclair Feb 14, 2018

Member

I think this may need to follow the deprecation policy, probably covered by rule #7:

Rule #7: Deprecated behaviors must function for no less than 1 year after their announced deprecation.

Has this removal been announced?

@@ -85,13 +85,6 @@ func init() {
rbac.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("secrets").RuleOrDie(),
},
})
addNamespaceRole(metav1.NamespaceSystem, rbac.Role{
// role for the cloud providers to access/create kube-system configmaps
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "cloud-provider"},

This comment has been minimized.

@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 14, 2018

Thanks for the review @tallclair.

How about I create a separate PR which adds a deprecation comment to namespace_policy.go and I set the action required release note there?

apiVersion: v1
kind: ServiceAccount
metadata:
name: cloud-provider

This comment has been minimized.

@nicksardo

nicksardo Feb 15, 2018

Member

@tallclair Is it necessary to create the service account?

This comment has been minimized.

@tallclair

tallclair Feb 15, 2018

Member

Hmm, I think something is already creating it, but I'm not sure what.

kind: ClusterRoleBinding
metadata:
annotations:
rbac.authorization.kubernetes.io/autoupdate: "true"

This comment has been minimized.

@liggitt

liggitt Feb 15, 2018

Member

remove this annotation, it's not meaningful to the add-on manager

kind: RoleBinding
metadata:
annotations:
rbac.authorization.kubernetes.io/autoupdate: "true"

This comment has been minimized.

@liggitt

liggitt Feb 15, 2018

Member

remove this annotation, it's not meaningful to the add-on manager

@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 15, 2018

Removed rbac.authorization.kubernetes.io/autoupdate: "true" from every resource.

@tallclair

This comment has been minimized.

Member

tallclair commented Feb 15, 2018

Do you want to add a deprecation notice of the built-in role to the release note?

@nicksardo

This comment has been minimized.

Member

nicksardo commented Feb 15, 2018

@tallclair Created #59949 for that purpose. WDYT?

@tallclair

This comment has been minimized.

Member

tallclair commented Feb 16, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, nicksardo, tallclair

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 17, 2018

Automatic merge from submit-queue (batch tested with PRs 59683, 59964, 59841, 59936, 59686). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 3a60b0b into kubernetes:master Feb 17, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation nicksardo 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 Job succeeded.
Details
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

k8s-merge-robot added a commit that referenced this pull request Feb 22, 2018

Merge pull request #59949 from nicksardo/deprecate-cloud-provider
Automatic merge from submit-queue (batch tested with PRs 59052, 59157, 59428, 59949, 60151). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Start deprecation of role for `cloud-provider` service account in rbac boostrap

**What this PR does / why we need it**:
See #59686 for reference

**Special notes for your reviewer**:
/assign @tallclair 

**Release note**:
```release-note
Action Required: The boostrapped RBAC role and rolebinding for the `cloud-provider` service account is now deprecated. If you're currently using this service account, you must create and apply your own RBAC policy for new clusters.
```

k8s-merge-robot added a commit that referenced this pull request Sep 1, 2018

Merge pull request #67224 from grayluck/namespace-cloudprovider-rbac
Automatic merge from submit-queue (batch tested with PRs 65251, 67255, 67224, 67297, 68105). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Add namespace for (cluster)role(binding) cloud-provider.

**What this PR does / why we need it**:
Add namespace for (cluster)role(binding) cloud-provider.
Change the addonmanager mode to be from reconcile to EnsureExists.

Needs to be cherrypicked together with #59686.

**Special notes for your reviewer**:
/assign @bowei @tallclair 
/sig auth

**Release note**:

```release-note
Role, ClusterRole and their bindings for cloud-provider is put under system namespace. Their addonmanager mode switches to EnsureExists.
```

Manual tested. Cluster can be created succesfully using kube-up.sh with desired (cluster)role(binding)s.

k8s-ci-robot added a commit that referenced this pull request Sep 18, 2018

Merge pull request #66872 from grayluck/automated-cherry-pick-of-#59686
…-upstream-release-1.9

Cherry pick of #59686 #67224: Add cloud-provider policies to be applied via addon mgr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment