Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kong] update RBAC for KIC v2 #419

Merged
merged 2 commits into from
Jul 23, 2021
Merged

[kong] update RBAC for KIC v2 #419

merged 2 commits into from
Jul 23, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 23, 2021

What this PR does / why we need it:

Updates the RBAC role permissions for KIC v2. Supersedes #364. New PR since the upstream permissions have changed a ton since #364 started and because I used a different update strategy.

Added the v2 leader election permissions alongside the v1 permissions in the same role, as v2 uses a new leader election system built into controller-runtime. There is some overlap, but trying to consolidate them in the chart will make any future updates harder. If they remain separated, you can just replace whichever block changed with the new version from upstream.

Replaced the entire ClusterRole permission set (which granted access to Ingresses and such) from v1 with the v2 permissions. The v2 permissions are a superset of the v1 permissions, so there doesn't appear to be any reason to maintain separate versions. Note that the v2 permissions do have a slightly different structure. They do not group resources in the same API group and equal permission sets together; see the example at #364 (comment)

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)
Related to Kong/kubernetes-ingress-controller#1352

Special notes for your reviewer:

This includes the change from Kong/kubernetes-ingress-controller#1584. That should be merged imminently.

This does not include permissions necessary to add finalizers. KIC 2.0.0-alpha.2 still attempts to add those, and will fail without them. We have removed finalizers from next but have not yet released alpha.3.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not main
  • New or modified sections of values.yaml are documented in the README.md
  • Title of the PR and commit headers start with chart name (e.g. [kong])

Update the RBAC roles for KIC v2:
- Replace the ClusterRole permissions (for interacting with
  Ingress-related configuration) with the permission set from v2. The v2
  permissions are a superset of the v1 permissions.
- Add the leader election Role permissions from v2 alongside the
  existing permissions. v2 uses a new leader election system (the
  election system built into controller-runtime). Some of these
  permissions overlap with the v1 permissions. They have not been
  consolidated to distinguish which are which clearly.
@rainest rainest requested a review from a team as a code owner July 23, 2021 18:37
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

My only thought is: what are we doing about testing this?

@rainest
Copy link
Contributor Author

rainest commented Jul 23, 2021

Was good aside from the part where I opened a copy of the role from the source tree where I didn't have KIC#1584 checked out, whoops.

Testing-wise we won't have have anything consistent until the functional tests are expanded to include all resources the controller can interact with (versus just Ingress, Service, and Endpoint so far with the basic httpbin test). Absent that it's our old friend "upgrade a release that has a bunch of config and see if it comes online with the expected config/check error logs", which worked, implies that it was able to read resources, and caught the still-inaccurate Ingress v1beta1 class.

@rainest rainest merged commit a1a6346 into next Jul 23, 2021
@rainest rainest deleted the feat/v2-rbac-2 branch July 23, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants