Adding NamespacedRules and handling to GR/GRB

[JonCrowther]

Issue: #42215

New Feature

Adds NamespacedRules to the GlobalRole field. Details of the field can be found in the issue, but in summation:

• When a global role (GR) is created, a role is created in each namespace with the listed rules
• When a global role binding (GRB) is created, a role binding is created in each namespace that binds to the namespaced roles
• When the role/role binding is modified, it checks if it's owned by a GR/GRB and then enqueues it to be verified

The work is split in the following commits:

1. Add the NamespacedRules field
2. Add GRB support
3. Add GR support
4. Add enqueueing

Testing

1. Create a GlobalRole with the new NamespacedRules field
   • It creates Roles based on the NamespacedRules
2. Create a GlobalRoleBinding with the above GlobalRole
   • It creates RoleBindings based on the NamespacedRules of the GR
3. Delete GR/GRB
   • Removes all Roles/RoleBindings

Engineering Testing

Manual Testing

Automated Testing

• Test types added/modified:
  • Unit
  • Integration (Go Framework) TODO
  • Integration (v2prov Framework) TODO

Summary: Unit tests for all new code and integration tests (TODO)

QA Testing Considerations

Regressions Considerations

This is a new field, so there shouldn't be any regressions. While the update/create functions for GR/GRBs got modified, the new check only occurs when the field is populated.

Existing / newly added automated tests that provide evidence there are no regressions:

• The existing RBAC integration tests prove that the previous RBAC functionality has not been modified

Security Considerations

Need a security assessment to ensure this doesn't introduce CVEs

[JonCrowther]

I added this test to get 100% code coverage of enqueue.go. I didn't make or modify this function

[JonCrowther]

Integration tests are in progress, opening for review to get feedback on functionality.

[pjbgf]

two small nits:

[MbolotSuse]

Couple of standout issues:

• We need to use name.SafeConcatName on the label values
• The purge methods should delete roles/bindings in the same namespace that claim ownership by a GR/GRB but aren't actually owned by that GR/GRB
• We can't update the roleRef on a binding, so you need to delete/create for the binding case instead of update

[JonCrowther]

Newest commit adds the following small reworks:

• Use of wrangler.SafeConcatName for the label
• Moved the purge unit tests into the larger reconcile unit tests
• More unit tests that had been missed
• More comments
• Improved explanation of how * is not supported
• Through manual testing, realized I needed DeleteNamespaced to delete Roles and RoleBindings, not just Delete
• Ran go mod tidy to make the CI happy

Large rework:

• Instead of searching for incorrectly made Role and RoleBindings, I create a list of all the correct items (using UID) while creating/updating all the Role/Bindings. After I select all Role/Bindings with the owner label and delete any that are not in my list of UIDs to keep. That way it should be more comprehensive in deleting Role/Bindings and avoid edge cases

[pjbgf]

Pointed a few minor nits regarding clarity - feel free to ignore.

[MbolotSuse]

Couple of comments on missing uses of safeConcatName, as well as some efficiency improvements.

[tomleb]

Some comments

[JonCrowther]

New commit changes:

• Added functionality for handling deleting/creating namespaces in the NamespacedRules field
  • Added an enqueuer to enqueue a GR when a namespace is modified
  • If namespace doesn't exist, warn the user but don't re-enqueue the reconcile
  • If namespace is terminating, don't attempt to create Roles/RoleBindings
  • Tested this manually (as well as unit tests). If the namespace gets created the Roles/RoleBindings get created too.
• Improved the efficiency of purging invalid Roles/RoleBindings by using maps
• Added and improved comments
• Reduced nested ifs by using continues
• Improved unit tests
• Used safeConcatName more consistently throughout and used indexers in the enqueuers to get GR/GRBs based on the new safeConcatName versions of the label

[MbolotSuse]

A couple of nits, but nothing that I think needs to be fixed for this to go in. I'm ok to approve the PR once the CI passes.

[MbolotSuse]

Nit: I highly recommend not creating tests cases like this which rely on call order to function. This highly couples your test case to the internal implementation of the underlying code, and can create flaky tests which fail when a specific order occurs.

In this case, I would recommend pre-computing the name of each role that might be retrieved, and then return a specific response for the specific roles which are being called.

[JonCrowther]

So in this specific test case I could get rid of the counter since I'm trying to get into the error path. However, I kept a similar pattern for the test at line 224. I've added more comments to explain the reason, and changed the counter to be modified based on when Create gets called, not just on consecutive Get calls.

I considered removing the test at 224, but I think it's important functionality to test. The difficulty is that the Get function gets called with the same input twice, but the expected behaviour changes depending on when it happens. I personally think modifying the counter to indicate before and after Create makes it less flaky, but if you feel it shouldn't exist I won't argue much.

[tomleb]

No changes since last approval so LGTM. Just added a nit.

[JonCrowther]

Rebased and pointed to release/v2.9

[MbolotSuse]

LGTM - don't see any issue due to rebase changes.