NetworkPolicy RBAC fixes #17549

danwinship · 2017-11-30T23:49:05Z

As part of the k8s 1.8 rebase, the NetworkPolicy code was changed to use networking.NetworkPolicy rather than extensions.NetworkPolicy, but the roles weren't updated to have the right permissions.. (This wasn't caught because only extended-networking-minimal gets run on PRs by default, and that only tests multitenant.)

Fixes test_branch_origin_extended_networking

(kubernetes/kubernetes#56650 hasn't actually merged yet.)

danwinship · 2017-11-30T23:49:24Z

/test extended_networking

danwinship · 2017-11-30T23:51:03Z

cc @soltysh for overlap/conflict with #17491

danwinship · 2017-12-01T13:30:25Z

flake #17519
/retest

soltysh · 2017-12-01T15:08:42Z

cc @soltysh for overlap/conflict with #17491

Aha, I was right, I was missing these bits when I was going through policies. But since I haven't seen them in k8s I assumed them not needed in origin.

soltysh

/lgtm

simo5 · 2017-12-01T16:26:59Z

@enj PTAL

enj · 2017-12-01T16:38:13Z

/hold

Upstream does not seem to have a decision on who should be able to manipulate these objects by default. I am not sure even an admin for a project should be able to mess with this (do not have enough familiarly with network policy to definitely say).

enj · 2017-12-01T16:44:56Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -326,6 +326,7 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {
 				rbac.NewRule(readWrite...).Groups(buildGroup, legacyBuildGroup).Resources("buildlogs").RuleOrDie(),
 				rbac.NewRule(read...).Groups(kapiGroup).Resources("resourcequotausages").RuleOrDie(),
 				rbac.NewRule("create").Groups(authzGroup, legacyAuthzGroup).Resources("resourceaccessreviews", "subjectaccessreviews").RuleOrDie(),
+				rbac.NewRule(readWrite...).Groups(networkingGroup).Resources("networkpolicies").RuleOrDie(),


Not sure how this is backwards compatibility.

oops, just jumped to the bottom of the block and didn't see that comment

enj · 2017-12-01T16:45:15Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -720,6 +721,7 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {
 				rbac.NewRule(read...).Groups(networkGroup, legacyNetworkGroup).Resources("egressnetworkpolicies", "hostsubnets", "netnamespaces").RuleOrDie(),
 				rbac.NewRule(read...).Groups(kapiGroup).Resources("nodes", "namespaces").RuleOrDie(),
 				rbac.NewRule(read...).Groups(extensionsGroup).Resources("networkpolicies").RuleOrDie(),
+				rbac.NewRule(read...).Groups(networkingGroup).Resources("networkpolicies").RuleOrDie(),


This one seems fine.

enj · 2017-12-01T16:47:27Z

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

+					"replicasets", "replicasets/scale", "replicationcontrollers/scale",
+					"networkpolicies").RuleOrDie(),
+
+				rbac.NewRule(ReadWrite...).Groups(networkingGroup).Resources("networkpolicies").RuleOrDie(),


Seems a bit powerful for edit?

The description says that "admin" is just "edit plus the power to grant permissions to other users", and the two have identical rules other than the two under "// additional admin powers" in "admin"

this is in vendor/ shouldn't it go via a backport from upstream PR anyway ?

enj · 2017-12-01T16:48:33Z

@danwinship I am also unsure how changing the RBAC admin/edit/view upstream does anything for us (we have our own admin/edit/view)?

danwinship · 2017-12-01T17:37:30Z

I am also unsure how changing the RBAC admin/edit/view upstream does anything for us (we have our own admin/edit/view)?

oh, hm, I was assuming there was some kind of merging between ours and theirs. It's actually the change to SDNReaderRoleName in the second commit that fixes the tests. I guess I can drop the UPSTREAM commit then?

danwinship · 2017-12-01T17:41:55Z

/test extended_networking

danwinship · 2017-12-01T19:30:48Z

/retest

soltysh · 2017-12-06T16:33:39Z

@enj with the upstream bits dropped are you ok with this getting in?

liggitt · 2017-12-06T17:36:37Z

I want sign-off on the upstream change (that namespace constrained users are expected to be able to modify network policy). There's also the question of whether write access would be limited to the admin role

danwinship · 2017-12-11T14:38:26Z

This rule is also for the apps group, so don’t add network policies to this one. Make a separate extensions only one

Ah... I just copied from the "admin" policies, but it looks like it's wrong there too now. (Got broken in the kube 1.8.1 rebase). Fixed for both "admin" and "edit" now.

liggitt · 2017-12-11T15:06:46Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -322,6 +322,9 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {

 				rbac.NewRule(readWrite...).Groups(templateGroup, legacyTemplateGroup).Resources("templates", "templateconfigs", "processedtemplates", "templateinstances").RuleOrDie(),

+				rbac.NewRule(readWrite...).Groups(extensionsGroup).Resources("networkpolicies").RuleOrDie(),


can make these a single rule: Groups(appsGroup, extensionsGroup)

liggitt · 2017-12-11T15:06:52Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -381,6 +384,9 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {

 				rbac.NewRule(readWrite...).Groups(templateGroup, legacyTemplateGroup).Resources("templates", "templateconfigs", "processedtemplates", "templateinstances").RuleOrDie(),

+				rbac.NewRule(readWrite...).Groups(extensionsGroup).Resources("networkpolicies").RuleOrDie(),


here as well

And fix some inconsistencies with the existing NetworkPolicy roles.

danwinship · 2017-12-11T16:53:20Z

/retest

danwinship · 2017-12-11T16:58:41Z

/test extended_conformance_install

danwinship · 2017-12-12T17:59:42Z

tests pass. @liggitt ? (needs a "/hold cancel" too)

liggitt · 2017-12-12T18:54:45Z

/lgtm
/hold cancel

openshift-ci-robot · 2017-12-12T18:54:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, liggitt, soltysh

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

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

~~pkg/cmd/OWNERS~~ [liggitt]
~~test/testdata/OWNERS~~ [danwinship,liggitt]

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

deads2k · 2017-12-12T19:11:55Z

/hold

This will conflict with the rebase. Please merge after that.

deads2k · 2017-12-13T19:33:02Z

rebase landed

/hold cancel

/test all

openshift-bot · 2017-12-14T06:53:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

soltysh · 2017-12-14T08:38:47Z

/retest

danwinship · 2017-12-14T13:31:27Z

/retest

danwinship · 2017-12-14T14:29:42Z

/retest

danwinship · 2017-12-14T19:46:55Z

/retest

openshift-bot · 2017-12-15T15:10:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-12-15T17:09:56Z

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

openshift-ci-robot · 2017-12-15T18:01:54Z

@danwinship: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/cmd	`98b52bf`	link	`/test cmd`
ci/openshift-jenkins/extended_conformance_gce	`98b52bf`	link	`/test extended_conformance_gce`

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.

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-12-15T18:15:46Z

Automatic merge from submit-queue (batch tested with PRs 17549, 17785).

danwinship added component/networking kind/bug Categorizes issue or PR as related to a bug. sig/networking labels Nov 30, 2017

danwinship added this to the 3.8.0 milestone Nov 30, 2017

openshift-ci-robot requested review from php-coder and simo5 November 30, 2017 23:49

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Nov 30, 2017

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 30, 2017

danwinship assigned liggitt Nov 30, 2017

soltysh approved these changes Dec 1, 2017

View reviewed changes

openshift-ci-robot assigned soltysh Dec 1, 2017

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2017

enj reviewed Dec 1, 2017

View reviewed changes

danwinship force-pushed the networkpolicy-rbac branch from 4f74e08 to 2461351 Compare December 1, 2017 17:39

openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. vendor-update Touching vendor dir or related files labels Dec 1, 2017

soltysh mentioned this pull request Dec 6, 2017

Clean policy #17491

Closed

liggitt reviewed Dec 11, 2017

View reviewed changes

Update OpenShift roles for networking.k8s.io

98b52bf

And fix some inconsistencies with the existing NetworkPolicy roles.

danwinship force-pushed the networkpolicy-rbac branch from 7c5c6ba to 98b52bf Compare December 11, 2017 15:28

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

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 12, 2017

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2017

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2017

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2017

openshift-merge-robot merged commit bfc48b1 into openshift:master Dec 15, 2017

danwinship deleted the networkpolicy-rbac branch March 7, 2018 14:28

		@@ -322,6 +322,9 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {

		rbac.NewRule(readWrite...).Groups(templateGroup, legacyTemplateGroup).Resources("templates", "templateconfigs", "processedtemplates", "templateinstances").RuleOrDie(),

		rbac.NewRule(readWrite...).Groups(extensionsGroup).Resources("networkpolicies").RuleOrDie(),

		@@ -381,6 +384,9 @@ func GetOpenshiftBootstrapClusterRoles() []rbac.ClusterRole {

		rbac.NewRule(readWrite...).Groups(templateGroup, legacyTemplateGroup).Resources("templates", "templateconfigs", "processedtemplates", "templateinstances").RuleOrDie(),

		rbac.NewRule(readWrite...).Groups(extensionsGroup).Resources("networkpolicies").RuleOrDie(),

NetworkPolicy RBAC fixes #17549

NetworkPolicy RBAC fixes #17549

Conversation

danwinship commented Nov 30, 2017

danwinship commented Nov 30, 2017

danwinship commented Nov 30, 2017

danwinship commented Dec 1, 2017

soltysh commented Dec 1, 2017

soltysh left a comment

Choose a reason for hiding this comment

simo5 commented Dec 1, 2017

enj commented Dec 1, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Dec 1, 2017

danwinship commented Dec 1, 2017

danwinship commented Dec 1, 2017

danwinship commented Dec 1, 2017

soltysh commented Dec 6, 2017

liggitt commented Dec 6, 2017 • edited Loading

danwinship commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Dec 11, 2017

danwinship commented Dec 11, 2017

danwinship commented Dec 12, 2017

liggitt commented Dec 12, 2017

openshift-ci-robot commented Dec 12, 2017

deads2k commented Dec 12, 2017

deads2k commented Dec 13, 2017

openshift-bot commented Dec 14, 2017

soltysh commented Dec 14, 2017

danwinship commented Dec 14, 2017

danwinship commented Dec 14, 2017

danwinship commented Dec 14, 2017

openshift-bot commented Dec 15, 2017

openshift-merge-robot commented Dec 15, 2017

openshift-ci-robot commented Dec 15, 2017 • edited Loading

openshift-merge-robot commented Dec 15, 2017

liggitt commented Dec 6, 2017 •

edited

Loading

openshift-ci-robot commented Dec 15, 2017 •

edited

Loading