#872 Add certificates and issuers to aggregated RBAC

[fuel-wlightning]

What this PR does / why we need it:
This adds two permissions to the user facing RBAC roles. This allows a user with permissions to their own namespace to be able to manage their own certificates and issuers.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #872

Special notes for your reviewer:
I was unable to figure out where to put tests for the chart. I'm happy to fix with some pointers. Manual test steps are below.

Manual Testing & Verification
View Access:

# Create Namespace:
kubectl create ns test-view
# Setup Service Account:
kubectl --namespace=test-view create sa test-user
# Setup Role Binding:
kubectl --namespace=test-view create rolebinding test-rb --clusterrole=view --serviceaccount=test-view:test-user
# Wait a few seconds for RBAC to process above changes
sleep 5
# Read (should succeed)
kubectl --namespace=test-view --as=system:serviceaccount:test-view:test-user auth can-i get certificates
# Write (should fail)
kubectl --namespace=test-view --as=system:serviceaccount:test-view:test-user auth can-i create certificates
# Remove namespace
kubectl delete ns test-view

Edit Access:

# Create Namespace:
kubectl create ns test-edit
# Setup Service Account:
kubectl --namespace=test-edit create sa test-user
# Setup Role Binding:
kubectl --namespace=test-edit create rolebinding test-rb --clusterrole=edit --serviceaccount=test-edit:test-user
# Wait a few seconds for RBAC to process above changes
sleep 5
# Read (should succeed)
kubectl --namespace=test-edit --as=system:serviceaccount:test-edit:test-user auth can-i get certificates
# Write (should succeed)
kubectl --namespace=test-edit --as=system:serviceaccount:test-edit:test-user auth can-i create certificates
# Remove namespace
kubectl delete ns test-edit

Admin Access:

# Create Namespace:
kubectl create ns test-admin
# Setup Service Account:
kubectl --namespace=test-admin create sa test-user
# Setup Role Binding:
kubectl --namespace=test-admin create rolebinding test-rb --clusterrole=admin --serviceaccount=test-admin:test-user
# Wait a few seconds for RBAC to process above changes
sleep 5
# Read (should succeed)
kubectl --namespace=test-admin --as=system:serviceaccount:test-admin:test-user auth can-i get certificates
# Write (should succeed)
kubectl --namespace=test-admin --as=system:serviceaccount:test-admin:test-user auth can-i create certificates
# Remove namespace
kubectl delete ns test-admin

Release note:

Added RBAC permissions for user facing roles to access Certificates and Issuers.

[munnerz]

/ok-to-test

[fuel-wlightning]

/retest

[fuel-wlightning]

This now has the deploy manifests updated, removed the relavent note from the top description.

[munnerz]

Thanks for the detailed steps to test this - if we want to automate checks here, we'll need to actually write the test into part of our e2e pipeline.

Feel free to give this a go - I think we do provide provisions to just run kubectl using our e2e framework. See ./test/e2e for some examples.

That said, I am happy to accept this as is for now if you would like to follow up with tests later!

[fuel-wlightning]

I'd like to go ahead and give writing an e2e test for it a go. So I'll work on that today.

[munnerz]

Thanks very much for putting those tests together! This looks good to me 😄

I will add lgtm and approved labels, but also add 'hold' just in case there's anything else you had meant to add.

You can merge this when you are ready with /hold cancel 😄

/lgtm
/approve
/hold

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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:

• OWNERS [munnerz]

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

[fuel-wlightning]

@munnerz Glad to get feedback. I plan to expand these beyond certificates, to cover issuers.

I did have questions regarding the following.

1. With rbacClusterRoleHasAccessToResource being used by multiple test files, I'm thinking ofmoving it to framework/util.go does this sound good?
2. Do you want a negative suite of tests on cluster issuer.
3. I am a little concerned about adding time to the e2e tests, my tests just for certificate have added 5 minutes to the full run. Should I try and limit my testing to avoid this?

[fuel-wlightning]

Okay,
This looks good to me now.

1. I went ahead and put it in /test/e2e/framework/utils.go. It seems to fit.
2. Due to the amount of time that's taken by the tests and ClusterIssuers being out of scope of the change, I decided not to implement the negative tests.
3. Total increase of e2e testing time is: 6min 33seconds. Not quite as much as I feared. The only change I could think of is to try decreasing the delay for waiting for RBAC to take effect, which is only a potential savings of 24 seconds (assuming half a second works). I feel like that might be better fitting a new a PR which can address e2e optimizations in general.

Canceling do-not-merge hold, as I'm happy if the reviewers are now.
/hold cancel

[munnerz]

Thanks very much for the work here!

Yes there's a number of still to-do optimisations needed around e2e's (notably, we run most of our tests in parallel...)

/lgtm