Use node-controller cluster role for node-lifecycle and cloud-node-lifecycle controller #72764

andrewsykim · 2019-01-10T05:48:27Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
In #70344 we split node-lifecycle-controller into two separate controllers: node-lifecycle-controller and cloud-node-lifecycle controller (see the PR for reasons). As a follow up we should have separate controller roles. This also fixes a bug (#72499) where nodes that do not exist are not deleted in the API server if --use-service-account-credentials is set.

Which issue(s) this PR fixes:
Fixes # #72499

Special notes for your reviewer:
This change also removes the need for the node-controller service account & role. Is there a deprecation process for bootstrap policies or a mechanism to clean them up?

Does this PR introduce a user-facing change?:

Add bootstrap service account & cluster roles for node-lifecycle-controller, cloud-node-lifecycle-controller, and cloud-node-controller.

/assign @mtaufen @liggitt

liggitt · 2019-01-10T05:54:07Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

 		Rules: []rbacv1.PolicyRule{
-			rbacv1helpers.NewRule("get", "list", "update", "delete", "patch").Groups(legacyGroup).Resources("nodes").RuleOrDie(),


the non-cloud node controller never deletes node objects?

It used to, but in #70344 we split the node deletion logic into a separate controller so it can be easily ported into the cloud controller manager.

liggitt · 2019-01-10T05:54:32Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

@@ -206,16 +206,31 @@ func buildControllerRoles() ([]rbacv1.ClusterRole, []rbacv1.ClusterRoleBinding)
 		},
 	})
 	addControllerRole(&controllerRoles, &controllerRoleBindings, rbacv1.ClusterRole{
-		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "node-controller"},
+		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "node-lifecycle-controller"},


this will orphan the existing clusterrole on upgraded clusters. what is the reason for the rename?

Ah, was wondering what happens there. So my justification for this is:

this is confusing cause the controller was always called node-lifecycle and not just node.

node-controller has access to delete nodes, so leaving this would give it access to delete nodes even though it no longer needs it.

I'm not sure what the implications of orphaned cluster roles are so I'll defer to your judgment here (worse case we can keep the node-controller role). Any reason why we can't support a way to clean up orphaned controllers? Would that not work because of version skew between components?

Any reason why we can't support a way to clean up orphaned controllers? Would that not work because of version skew between components?

We take an extremely conservative position on permissions management and upgrades. Roles and permissions are never automatically removed or tightened by an upgrade, to ensure that no component that is currently working is ever broken by an upgrade removing its permission.

New clusters would only have the current set of default roles/bindings

Good to know! If orphaning the node-controller role is still an option I'd like to go down that path. Otherwise, we can continue to use the node-controller role here unchanged for the node lifecycle controller.

@liggitt do you have a strong preference here? Options are:

orphan node-controller clusterrole and replace with new role node-lifecycle-controller

keep node-controller but gives delete access to node lifecycle controller even though it doesn't need it.

Of these, I'd favor Option 2 so we don't leave an unused role floating around.

this is confusing cause the controller was always called node-lifecycle and not just node

For history's sake, it was actually called the node controller until @cheftako split things up in #57492

fedebongio · 2019-01-10T21:17:01Z

/cc @cheftako

mtaufen · 2019-01-10T23:32:05Z

Would it really be the end of the world to just use the node-controller role for both node-lifecycle-controller and cloud-node-lifecycle-controller?

mtaufen · 2019-01-10T23:33:46Z

It's already reused across controllers, e.g. see the ipam controller.

andrewsykim · 2019-01-11T00:45:28Z

@mtaufen no objections from me, updated PR accordingly :)

liggitt · 2019-01-14T19:17:58Z

/retest

liggitt · 2019-01-14T19:57:03Z

cmd/kube-controller-manager/app/core.go

@@ -126,6 +126,7 @@ func startNodeLifecycleController(ctx ControllerContext) (http.Handler, bool, er
 		ctx.InformerFactory.Core().V1().Pods(),
 		ctx.InformerFactory.Core().V1().Nodes(),
 		ctx.InformerFactory.Apps().V1().DaemonSets(),
+		// cloud node lifecycle controller uses existing cluster role from node-controller


is this comment correct? is this the cloud node lifecycle controller?

fixed, thanks!

…lifecycle controller

liggitt · 2019-01-14T21:03:54Z

/lgtm
/approve
/retest

k8s-ci-robot · 2019-01-14T21:06:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, liggitt

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:

~~cmd/cloud-controller-manager/OWNERS~~ [andrewsykim,liggitt]
~~cmd/kube-controller-manager/OWNERS~~ [liggitt]

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 added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 10, 2019

k8s-ci-robot assigned liggitt and mtaufen Jan 10, 2019

k8s-ci-robot requested review from krousey and mml January 10, 2019 05:49

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 10, 2019

liggitt reviewed Jan 10, 2019

View reviewed changes

andrewsykim mentioned this pull request Jan 10, 2019

Nodes are not removed after deleting VMs. #72499

Closed

k8s-ci-robot requested a review from cheftako January 10, 2019 21:17

andrewsykim force-pushed the cloud-node-lifecycle-controller-rbac branch from 126f9b3 to ea6a0fd Compare January 11, 2019 00:44

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2019

andrewsykim changed the title ~~Create separate controller roles for node controllers~~ Use node-controller cluster role for node-lifecycle and cloud-node-lifecycle controller Jan 11, 2019

liggitt reviewed Jan 14, 2019

View reviewed changes

andrewsykim force-pushed the cloud-node-lifecycle-controller-rbac branch from ea6a0fd to 8fd10d4 Compare January 14, 2019 19:59

use node-controller cluster role for node e-lifecycle and cloud-node-…

426714c

…lifecycle controller

andrewsykim force-pushed the cloud-node-lifecycle-controller-rbac branch from 8fd10d4 to 426714c Compare January 14, 2019 20:00

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 14, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2019

andrewsykim mentioned this pull request Jan 14, 2019

Validate/test usage of service account credentials in cloud controller manager #72897

Open

k8s-ci-robot merged commit 3b0b74f into kubernetes:master Jan 15, 2019

atosatto mentioned this pull request Oct 3, 2023

Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use node-controller cluster role for node-lifecycle and cloud-node-lifecycle controller #72764

Use node-controller cluster role for node-lifecycle and cloud-node-lifecycle controller #72764

andrewsykim commented Jan 10, 2019

liggitt Jan 10, 2019

andrewsykim Jan 10, 2019

liggitt Jan 10, 2019 •

edited

andrewsykim Jan 10, 2019

liggitt Jan 10, 2019

andrewsykim Jan 10, 2019

andrewsykim Jan 10, 2019 •

edited

mtaufen Jan 10, 2019

mtaufen Jan 10, 2019 •

edited

fedebongio commented Jan 10, 2019

mtaufen commented Jan 10, 2019

mtaufen commented Jan 10, 2019

andrewsykim commented Jan 11, 2019

liggitt commented Jan 14, 2019

liggitt Jan 14, 2019

andrewsykim Jan 14, 2019

liggitt commented Jan 14, 2019

k8s-ci-robot commented Jan 14, 2019

		Rules: []rbacv1.PolicyRule{
		rbacv1helpers.NewRule("get", "list", "update", "delete", "patch").Groups(legacyGroup).Resources("nodes").RuleOrDie(),

Use node-controller cluster role for node-lifecycle and cloud-node-lifecycle controller #72764

Use node-controller cluster role for node-lifecycle and cloud-node-lifecycle controller #72764

Conversation

andrewsykim commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jan 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Jan 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtaufen Jan 10, 2019 • edited

Choose a reason for hiding this comment

fedebongio commented Jan 10, 2019

mtaufen commented Jan 10, 2019

mtaufen commented Jan 10, 2019

andrewsykim commented Jan 11, 2019

liggitt commented Jan 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 14, 2019

k8s-ci-robot commented Jan 14, 2019

liggitt Jan 10, 2019 •

edited

andrewsykim Jan 10, 2019 •

edited

mtaufen Jan 10, 2019 •

edited