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] add RBAC resources for controller 2.x #364

Closed
wants to merge 1 commit into from
Closed

Conversation

rainest
Copy link
Contributor

@rainest rainest commented May 26, 2021

What this PR does / why we need it:

From https:/Kong/kubernetes-ingress-controller/tree/a556daddd55f164bd1d327611c541e8af35f2251, take kustomize build output from railgun/config/rbac, replace names, roleRefs, and subjects with template values used by the chart, and append that to the controller RBAC template.

Which issue this PR fixes

Related to Kong/kubernetes-ingress-controller#1352

Special notes for your reviewer:

  • Omitted the metrics Service that kustomize generates. This requires some additional work to add to the chart properly. It's not required for controller resource interactions.
  • Added alongside existing resources with no additional values gate for simplicity. The 2.x roles use different names than the existing roles and don't conflict with them, though they do add permissions we didn't have previously. We may want to revisit this before an actual release.

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
  • Title of the PR and commit headers start with chart name (e.g. [kong])

Copy link
Contributor

@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.

This LGTM and I don't have any blockers, but I do want to point out one consideration: can/should we temporarily hide these behind a flag (like a feature gate) in the next branch until we officially commit to KIC 2.x support in the coming weeks?

@shaneutt
Copy link
Contributor

Remind me if you would please, what are we blocked on here?

@rainest
Copy link
Contributor Author

rainest commented Jul 22, 2021

Originally it was finishing up Kong/kubernetes-ingress-controller#1215 to determine the actual permissions needed. I'd further wanted to finish Kong/kubernetes-ingress-controller#1256 to pull RBAC content from the complete manifests instead of directly from the kubebuilder-generated stuff.

https:/Kong/kubernetes-ingress-controller/blob/next/railgun/config/rbac/leader_election_role.yaml and the ClusterRole in https:/Kong/kubernetes-ingress-controller/blob/next/railgun/config/rbac/role.yaml (alternate version: https:/Kong/kubernetes-ingress-controller/blob/9813575912655554c4d5d823281f428464034a62/deploy/single-v2/all-in-one-dbless.yaml#L1005-L1250) are now correct. However, they don't include the finalizer permissions that alpha.2 expects; they'll only work with a next image.

We should be good to go ahead and add in the new RBAC resources and do another pass to compare them against 1.x--not sure if we'd want to try and replace the original roles entirely or do what I'd originally done here, which is to install both roles. They should be closer (if not identical) now.

@rainest
Copy link
Contributor Author

rainest commented Jul 23, 2021

Did some manual review of the new resources now that we've adjusted permissions and manifest script is ready. As best I can tell, the 2.x permissions are a superset of the 1.x permissions, and as such we can simply replace the 1.x permissions in the chart entirely. I don't think the additional permissions requested (mostly status permissions and new CRDs) are likely to be a concern for security-conscious users.

There are a few caveats, however. I had to perform this comparison manually, as the permission generator for 2.x generates a rule per resource, and the 1.x permissions grouped them. For 1.x, you'll have something like this:

- apiGroups:
  - configuration.konghq.com
  resources:
  - kongplugins
  - kongclusterplugins
  verbs:
  - get

whereas 2.x will have:

- apiGroups:
  - configuration.konghq.com
  resources:
  - kongclusterplugins
  verbs:
  - get
- apiGroups:
  - configuration.konghq.com
  resources:
  - kongplugins
  verbs:
  - get

So they're functionally identical, but you can't diff them. I don't know of a tool that will calculate effective permissions, but the list is small enough that it was easy enough to walk. There were some other differences:

  • 1.x uses extensions for the old Ingress resource's group, whereas 2.x uses apiextensions.k8s.io. Are these indeed equivalent?
  • 1.x still includes kongcredentials (removed in KIC 1.0) in the flat manifests' ClusterRoles. I'm not concerned with backwards compatibility since we'd already removed this from the permission set we distribute with the chart.
  • 1.x uses different names in the flat manifests. The chart overrides these names anyway.

@shaneutt
Copy link
Contributor

shaneutt commented Jul 23, 2021

1.x uses extensions for the old Ingress resource's group, whereas 2.x uses apiextensions.k8s.io. Are these indeed equivalent?

I have searched around and I was unable to verify whether these are equivalent on initial inspection, we'll need to double check this.

1.x still includes kongcredentials (removed in KIC 1.0) in the flat manifests' ClusterRoles. I'm not concerned with backwards compatibility since we'd already removed this from the permission set we distribute with the chart.

Sounds good to me 👍

1.x uses different names in the flat manifests. The chart overrides these names anyway.

This does indicate a specific compatibility issue with kustomize deployments though doesn't it?

@rainest rainest mentioned this pull request Jul 23, 2021
3 tasks
@rainest
Copy link
Contributor Author

rainest commented Jul 23, 2021

Closing in favor of #419, which was written from scratch with a different approach and updated permissions. It does not add new role templates, and instead only replaces role permissions.

This does indicate a specific compatibility issue with kustomize deployments though doesn't it?

Not sure I follow the exact issue. The difference in names is basically arbitrary--we set manager-role in the Make targets for v2, and probably actually should change it to kong-ingress-clusterrole (same as v1), if only to have something that reflects the specific application in the role name.

The chart can't use the original roles verbatim because it templates them to include the release name. It edited the names for v1 roles and such also, and should continue to do so with v2.

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

Successfully merging this pull request may close these issues.

2 participants