🐛 Further deduplication of RBAC rules #620

Porges · 2021-09-20T04:37:00Z

Closes #612.

Check if rules are rendered irrelevant by other rules and remove them if so.

Don't fail 5 times when failing, and don't chdir.

k8s-ci-robot · 2021-09-20T04:37:08Z

Hi @Porges. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2021-09-20T04:37:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Porges
To complete the pull request process, please assign joelanford after the PR has been reviewed.
You can assign the PR to them by writing /assign @joelanford in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Porges · 2021-09-20T04:38:11Z

pkg/rbac/parser_test.go

+}
+
+func Test_SubsumesIsReflexive(t *testing.T) {
+	g := gomega.NewWithT(t)


use some kind of property testing for this and following test?

What do you mean with property testing?

See updated tests; in this case using the testing/quick package. We generate arbitrary Rules and then make assertions about them.

Porges · 2021-09-20T04:39:16Z

pkg/rbac/testdata/controller.go

 // +kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=watch;watch
 // +kubebuilder:rbac:groups=art,resources=jobs,verbs=get,namespace=park
-// +kubebuilder:rbac:groups=batch.io,resources=cronjobs,resourceNames=foo;bar;baz,verbs=get;watch
+// +kubebuilder:rbac:groups=batch.io,resources=cronjobs2,resourceNames=foo;bar;baz,verbs=get;watch


renamed to cronjobs2 or else it is subsumed by the kubebuilder:rbac:groups=batch.io,resources=cronjobs,verbs=get;watch;create line.

pkg/rbac/parser.go

vincepri · 2021-09-20T15:22:55Z

/ok-to-test

k8s-triage-robot · 2021-12-19T15:58:38Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-01-18T16:02:21Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

Porges · 2022-01-19T20:06:36Z

/remove-lifecycle rotten

it's not rotten, it's floundering

sbueringer

Not a maintainer, but I try to help out :)

A few suggestions

pkg/rbac/stringset.go

sbueringer · 2022-02-17T12:35:56Z

pkg/rbac/parser.go

+			result := []*NormalizedRule{it}
+			for _, d := range dest {
+				result = insertRule(result, d)
+			}


Shouldn't this omit "other"?

It could, but since it.Subsumes(other) and we start the new list with it being contained, then other will be omitted by the previous check above. Explicitly skipping it makes the code a bit messier.

What you are doing here is both harder to read and less eficient than:

func remove(s []int, i int) []int { s[i] = s[len(s)-1] return s[:len(s)-1] } return append(remove(dest, idx), it)

@alvaroaleman that doesn’t result in the quite same outcome; if Rule X is superset of Rule Y in the list, it could also be a superset of Rule Z (and others), so we still need to go through and check the remainder of the list.

pkg/rbac/parser_test.go

sbueringer · 2022-02-17T12:43:09Z

pkg/rbac/parser_test.go

+}
+
+func Test_SubsumesIsReflexive(t *testing.T) {
+	g := gomega.NewWithT(t)


What do you mean with property testing?

pkg/rbac/testdata/controller.go

alvaroaleman

Hi @Porges

Thanks for submitting this change. There are a couple of issues with it IMHO, most notably that is too invasive and not properly tested, with the result that it is incorrect.

From what I can tell by looking at #612, all of it can be solved by by removing rules that have a supset. As a result, I would expect the change to the existing code to look something like this:

--- a/pkg/rbac/parser.go
+++ b/pkg/rbac/parser.go
@@ -194,6 +194,8 @@ func GenerateRoles(ctx *genall.GenerationContext, roleName string) ([]interface{
                        ruleMap[key].addVerbs(rule.Verbs)
                }
 
+               ruleMap = removeRulesThatHaveSuperset(ruleMap)
+
                // sort the Rules in rules according to their ruleKeys
                keys := make([]ruleKey, 0, len(ruleMap))
                for key := range ruleMap {

As a general rule of thumb for making any changes:

Keep the change as small as possible
Do not make any unrelated stylistic changes
Keep the change scoped, do not mix refactorings (change of existing code that does not intend to change behavior) with behavioural changes

alvaroaleman · 2022-04-30T15:48:38Z

pkg/rbac/parser.go

+// meaning that the other is unnecessary.
+// Remember that Kubernetes RBAC rules are purely additive, there
+// are no deny rules.
+func (nr *NormalizedRule) Subsumes(other *NormalizedRule) bool {


I can directly tell you that this is wrong:

Empty groups does not mean all groups, it means no group

Same for Resources

Resources support the * operator to indicate all resources

Also, if we introduce something like that it needs thorough unit testing. This is not an area where we can introduce correctness issues.

Have updated this versus the documentation on the PolicyRule struct. Thanks, this is the kind of feedback I wanted on the correctness. 👍

I have been building out the unit tests (given that there were none to start with) but definitely could do with more.

pkg/rbac/parser.go

alvaroaleman · 2022-04-30T16:04:00Z

pkg/rbac/parser.go

+			result := []*NormalizedRule{it}
+			for _, d := range dest {
+				result = insertRule(result, d)
+			}


What you are doing here is both harder to read and less eficient than:

func remove(s []int, i int) []int { s[i] = s[len(s)-1] return s[:len(s)-1] } return append(remove(dest, idx), it)

alvaroaleman · 2022-04-30T16:18:43Z

pkg/rbac/parser.go

+	// if two different rules have the same comparison key then they can have their verbs merged
+	// this key should comprise all the fields below except Verbs and Namespace (since rules
+	// are partitioned by namespace before merging)
+	ComparisonKey string


Sorry, I don't understand why this is needed. If one rule is a superset of another rule, the subet rule can be removed. Why is any additional merging needed?

This is to handle verb list merging (a part of the existing codebase as well); if two rules match exactly the same things then they can be unified and their verb lists merged.

alvaroaleman · 2022-04-30T16:39:01Z

pkg/rbac/parser.go

 	return result
 }

+// Subsumes indicates if one rule entirely determines another,


This might be due to the fact that I am not a native English speaker, but I find a name like IsSuperset way more obvious.

Porges · 2022-05-01T22:40:42Z

Ideally we'd also rely on the k8s code to help enforce requirements on the PolicyRule (see PolicyRuleBuilder) but importing this requires a bunch of replaces added into go.mod. Is this viable/okay to proceed with?

alvaroaleman · 2022-05-04T15:19:56Z

he k8s code to help enforce requirements on the PolicyRule (see PolicyRuleBuilder) but importing this requires a bunch of replaces added into go.mod. Is this viable/okay to proceed with?

Unfortunately not, because that will break go install.

In general, k8s itself states you must never import k8s/kubernetes (I can't find the reference for that off-hand, but it exists). The only way to get us to use upstream code would be to talk to upstream sig-apimachinery if they are open to move this to staging (i.E. below the staging directory in the repo). Then it gets published to repos tthat can be imported without replacfe. On a quick look, it seems to be that this might be possible for this package, as it only seems to use external api. But I might be missing context.

k8s-triage-robot · 2022-08-02T15:50:07Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-09-01T16:04:57Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-10-01T16:48:03Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-10-01T16:48:07Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

Porges added 4 commits September 20, 2021 14:27

Reorganize code a bit

1c4589c

Add go:generate annotation for ease of dev

a515f77

Improve test

d35d7be

Don't fail 5 times when failing, and don't chdir.

Rework RBAC rule merging

924d970

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 20, 2021

k8s-ci-robot requested review from droot and mengqiy September 20, 2021 04:37

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 20, 2021

Porges commented Sep 20, 2021

View reviewed changes

pkg/rbac/parser.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 20, 2021

camilamacedo86 mentioned this pull request Dec 14, 2021

🐛 don't generate markers if resource is not created kubernetes-sigs/kubebuilder#2430

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 18, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 19, 2022

sbueringer reviewed Feb 17, 2022

View reviewed changes

Porges added 3 commits April 28, 2022 22:28

Merge remote-tracking branch 'upstream/master' into dedupe-rules

88b9485

Get rid of init()

dfde166

Switch code to use apimachinery sets

bd34e96

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 28, 2022

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2022

Add samples with URLs

2c19ffb

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2022

Porges added 3 commits April 28, 2022 23:09

Fix test

f950de3

Rename sortkey

739ede5

Add more examples

cd2c4fc

alvaroaleman reviewed Apr 30, 2022

View reviewed changes

Porges added 5 commits May 1, 2022 20:59

Address a few comments

dc3fec1

Update behaviour to be correct vs documentation on PolicyRule struct

5d3f27c

Simplify star in sets where appropriate

976e106

Fix comment

5cecdc9

Add some property-based tests

57e6c71

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 2, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 1, 2022

k8s-ci-robot closed this Oct 1, 2022

🐛 Further deduplication of RBAC rules #620

🐛 Further deduplication of RBAC rules #620

Uh oh!

Conversation

Porges commented Sep 20, 2021

Uh oh!

k8s-ci-robot commented Sep 20, 2021

Uh oh!

k8s-ci-robot commented Sep 20, 2021

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!

Uh oh!

vincepri commented Sep 20, 2021

Uh oh!

k8s-triage-robot commented Dec 19, 2021

Uh oh!

k8s-triage-robot commented Jan 18, 2022

Uh oh!

Porges commented Jan 19, 2022

Uh oh!

sbueringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alvaroaleman 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!

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!

Porges commented May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alvaroaleman commented May 4, 2022

Uh oh!

k8s-triage-robot commented Aug 2, 2022

Uh oh!

k8s-triage-robot commented Sep 1, 2022

Uh oh!

k8s-triage-robot commented Oct 1, 2022

Uh oh!

k8s-ci-robot commented Oct 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

sbueringer left a comment •

edited

Loading

Porges commented May 1, 2022 •

edited

Loading