Node admission/authorization by liggitt · Pull Request #14227 · openshift/origin

liggitt · 2017-05-17T05:05:55Z

Enables the node authorizer and NodeRestriction admission plugins

liggitt · 2017-07-12T23:46:18Z

Will wait for the 1.7 rebase and drop all the upstream commits

openshift-ci-robot · 2017-09-07T20:18:00Z

@liggitt: Your pull request title starts with "WIP", so the do-not-merge/work-in-progress label will be added.

Details

This label will ensure that your pull request will not be merged. Remove the prefix from your pull request title to trigger the removal of the label and allow for your pull request to be merged.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

liggitt · 2017-09-12T18:07:09Z

cc @openshift/sig-security

simo5 · 2017-09-12T18:37:11Z

pkg/cmd/server/bootstrappolicy/policy.go

+				Kind:     "ClusterRole",
+				Name:     NodeRoleName,
+			},
+		},


please use newOriginClusterBinding() or even better rbac.newClusterBinding() if you do not have to maintain a name difference between the role and the binding.

that helper does not allow empty subjects, which we want here.

very annoying, what is the purpose of a binding with no subjects ?

so that tightening reconciliation removes the grant to the system:nodes group

simo5 · 2017-09-12T18:38:46Z

pkg/cmd/server/bootstrappolicy/policy_test.go

+		for _, r := range miss {
+			t.Logf("\t%s", r.CompactString())
+		}
+	}


Why is this needed ?

to ensure there were no differences between our node role and the upstream node role as we converge

deads2k · 2017-09-12T18:56:57Z

pkg/cmd/server/origin/master_config.go

 }

-func newAuthorizer(kubeAuthorizer kauthorizer.Authorizer, kubeSubjectLocator rbacauthorizer.SubjectLocator, clusterRoleGetter rbaclisters.ClusterRoleLister, projectRequestDenyMessage string) (kauthorizer.Authorizer, authorizer.SubjectLocator) {
+func newAuthorizer(kubeAuthorizer kauthorizer.Authorizer, kubeSubjectLocator rbacauthorizer.SubjectLocator, informers InformerAccess, projectRequestDenyMessage string) (kauthorizer.Authorizer, authorizer.SubjectLocator) {


We're trying to kill the big one. Plumb through the ones you want or make this a method on the config type or make a Complete method and we can try to start converging.

explained why change can't be done

liggitt · 2017-09-12T20:40:47Z

/retest

liggitt · 2017-09-12T21:49:59Z

Router flake
/retest

liggitt · 2017-09-14T03:41:31Z

any other comments?

enj

Minor issue.

enj · 2017-09-15T19:02:11Z

pkg/cmd/server/bootstrappolicy/policy.go

-		newOriginClusterBinding(NodeRoleBindingName, NodeRoleName).
-			Groups(NodesGroup).
-			BindingOrDie(),
+		// Preserve the empty node binding with no subjects for tightening reconciliation


Seems like we should have a GetDeadClusterRoleBindings.

I wouldn't want to chance someone calling this and missing calling that

We already trust callers to use GetBootstrapClusterRoles which gives them GetDeadClusterRoles. I would expect GetBootstrapClusterRoleBindings to have the same logic.

created GetDeadClusterRoleBindings

liggitt · 2017-09-15T21:01:18Z

updated, and added an integration test that replicates the checks in https://github.com/kubernetes/kubernetes/blob/release-1.7/test/integration/auth/node_test.go against our wired API server to ensure our default authz/admission set up protects us.

enj · 2017-09-15T21:32:09Z

vendor/k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

-		// If an admin wants to grant the system:node role (which cannot partition Node API access), they will need to create their own clusterrolebinding.
-		// TODO: Remove the subjects from this binding in 1.8 (leave the empty binding for tightening reconciliation), and remove AddClusterRoleBindingFilter()
-		rbac.NewClusterBinding(systemNodeRoleName).Groups(user.NodesGroup).BindingOrDie(),
+		// This default binding of the system:node role to the system:nodes group is deprecated in 1.7 with the availability of the Node authorizer.


Uhh upstream commit?

Yes, pushed too fast. Will fix

…:nodes group

liggitt · 2017-09-17T18:34:40Z

comments addressed, PTAL

enj · 2017-09-18T13:05:04Z

/lgtm

openshift-merge-robot · 2017-09-18T13:05:13Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-09-18T13:52:39Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-09-18T14:05:54Z

@liggitt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/cmd	`7fa775c`	link	`/test cmd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-merge-robot · 2017-09-18T14:49:38Z

Automatic merge from submit-queue (batch tested with PRs 16224, 14227)

liggitt changed the title ~~Node admission/authorization~~ WIP - Node admission/authorization May 17, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2017

liggitt closed this Jul 12, 2017

liggitt reopened this Jul 12, 2017

openshift-merge-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 24, 2017

openshift-merge-robot assigned deads2k and mfojtik Jul 25, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2017

liggitt modified the milestone: 3.7.0 Aug 24, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2017

stevekuznetsov changed the title ~~WIP - Node admission/authorization~~ WIP - Node admission/authorization Sep 7, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2017

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2017

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 12, 2017

liggitt changed the title ~~WIP - Node admission/authorization~~ Node admission/authorization Sep 12, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2017

openshift deleted a comment from openshift-bot Sep 12, 2017

liggitt assigned enj and unassigned mfojtik Sep 12, 2017

simo5 previously requested changes Sep 12, 2017

View reviewed changes

deads2k reviewed Sep 12, 2017

View reviewed changes

enj suggested changes Sep 15, 2017

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 15, 2017

enj reviewed Sep 15, 2017

View reviewed changes

liggitt added 3 commits September 16, 2017 21:13

UPSTREAM: 49638: Remove default binding of system:node role to system…

a4e5c04

…:nodes group

Enable NodeRestriction admission

a5b2890

Wire node authorizer

7fa775c

enj approved these changes Sep 18, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2017

openshift-merge-robot merged commit 09d04de into openshift:master Sep 18, 2017

liggitt deleted the node-authz branch September 21, 2017 21:46

Conversation

liggitt commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Jul 12, 2017

Uh oh!

openshift-ci-robot commented Sep 7, 2017

Uh oh!

liggitt commented Sep 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Sep 12, 2017

Uh oh!

liggitt commented Sep 12, 2017

Uh oh!

liggitt commented Sep 14, 2017

Uh oh!

enj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Sep 17, 2017

Uh oh!

enj commented Sep 18, 2017

Uh oh!

openshift-merge-robot commented Sep 18, 2017

Uh oh!

openshift-merge-robot commented Sep 18, 2017

Uh oh!

openshift-ci-robot commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-robot commented Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

liggitt commented May 17, 2017 •

edited

Loading

openshift-ci-robot commented Sep 18, 2017 •

edited

Loading