Split Gateway and Knative roles

[rainest]

What this PR does / why we need it:
Splits Gateway RBAC permissions into a separate ClusterRole. This lets non-cluster-admin users create the core role despite not having permissions to create roles with permissions to Gateway resources.

Splits Knative out of the generated controllers by copying its current controller and removing Knative-specific template code. controller-gen limitations require this live in a separate directory to generate a separate role.

Fixes some linter errors that were previously hidden because they were in generated code.

Special notes for your reviewer:
See also Kong/charts#595

This still adds the new role to the single manifests. AFAIK, this is fine and we do not need to make it completely optional and only available in a separate file: if you cannot create those roles, only those resources will fail to apply, whereas the others will apply normally.

controller-gen doesn't provide tools to separate roles by annotation, but it does let you generate roles from a selection of paths. Separating roles thus requires that those controllers live in their own directory. The alternative is maintaining these roles manually.

E2E run tests actual manifests with Gateway. There isn't any E2E Knative test, so relying on manual review/similarity to the change for Gateway to vet that.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

• the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

[shaneutt]

Overall I didn't see anything that looked wrong, but I was wondering about tests. The most important implication of this change would seem to be the possibility that we would break existing permissions.

Perhaps now is a good time to do the following:

• add an SA with the specific RBAC permissions we need for integration tests, instead of relying on the default kubeconfig SA (for reference, the upcoming Gateway Operator does this)
• add a nightly E2E test that does an upgrade from the previous minor release to the current main and runs a series of tests on the upgraded system (theoretically, the integration tests).

Let me know your thoughts? 🤔

[rainest]

@shaneutt do you know how we can make KTF provide a kubeconfig for a specific user? Briefly it looks like https://github.com/Kong/kubernetes-testing-framework/blob/e4bbf5856625040ecfcdc5a0b0ff5c9bf8cb94cb/pkg/clusters/utils.go#L120 would need to accept an optional user-provided restConfig, where we could somehow fill out https://github.com/Kong/kubernetes-testing-framework/blob/e4bbf5856625040ecfcdc5a0b0ff5c9bf8cb94cb/pkg/utils/kubernetes/generators/kubeconfig.go#L29-L31 with the appropriate creds.

The gateway-operator approach doesn't quite fit within our existing KIC tests since we invoke the CLI rather than starting the manager ourselves, which is probably desirable for testing everything that leads up to manager creation. That doesn't leave anywhere I can really inject the new client function unless I hackily add a cluster.NewClientFunc to our manager.config, only set it from tests (for obvious reasons, you can't populate it with viper), and add code to use it if present in manager.Run().

E2E tests are already checking upgrades, though that shouldn't be particularly relevant here--we're mostly concerned with the permissions granted on any individual version. The difference is that E2E won't test as many individual permissions as integration. E2E is more testing whether the roles get properly assigned at all (we essentially assume that if HTTPRoute and Ingress work that the rest of those permissions are present), not that they contain all the permissions we actually need.

[shaneutt]

@shaneutt do you know how we can make KTF provide a kubeconfig for a specific user? Briefly it looks like https://github.com/Kong/kubernetes-testing-framework/blob/e4bbf5856625040ecfcdc5a0b0ff5c9bf8cb94cb/pkg/clusters/utils.go#L120 would need to accept an optional user-provided restConfig, where we could somehow fill out https://github.com/Kong/kubernetes-testing-framework/blob/e4bbf5856625040ecfcdc5a0b0ff5c9bf8cb94cb/pkg/utils/kubernetes/generators/kubeconfig.go#L29-L31 with the appropriate creds.

One option might be to inject -n <namespace> --as system:serviceaccount:<namespace>:<sa> so that you can use impersonation instead of having to generate a full kubeconfig for the account, otherwise if you still actually want to make the kubeconfig file I believe you can do something like this.

The gateway-operator approach doesn't quite fit within our existing KIC tests since we invoke the CLI rather than starting the manager ourselves, which is probably desirable for testing everything that leads up to manager creation. That doesn't leave anywhere I can really inject the new client function unless I hackily add a cluster.NewClientFunc to our manager.config, only set it from tests (for obvious reasons, you can't populate it with viper), and add code to use it if present in manager.Run().

I guess I see where you're coming from that in KIC we do more in the CLI so it's good to have test coverage there. Perhaps we can test that in different ways so we can free ourselves up to invoke the manager more directly? I get the sense that testing those CLI mechanisms more directly and not having to worry about them in the integration tests could be freeing. Alternatively we could officially add impersonation to the ingress controller, e.g. the cli could take an --as <sa> and the ENV could too and then it would be less hacky, no?

[rainest]

--as seems to break separation of concerns since you normally expect the Deployment ServiceAccount to actually be, well, that.

I think I'll go with a new Run() overrides argument for non-config inputs, and have viper always pass an empty one. That'll preserve the integrity of the existing config while being simple to implement.

[shaneutt]

Checking in on this one?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[rainest]

@shaneutt never got around to writing a new role override system for KIC's tests and it's lowish priority since lacking permission to create those roles can maybe fall within the set of requirements we don't intend to support in these, only in the chart.

Would you be open to merging this with conflicts resolved and whatever existing role testing we get from E2E (less than integration, but at least some coverage for both Ingress and Gateway, none for Knative), and a separate general issue for adding integration tests using proper roles?

[shaneutt]

@shaneutt never got around to writing a new role override system for KIC's tests and it's lowish priority since lacking permission to create those roles can maybe fall within the set of requirements we don't intend to support in these, only in the chart.

Would you be open to merging this with conflicts resolved and whatever existing role testing we get from E2E (less than integration, but at least some coverage for both Ingress and Gateway, none for Knative), and a separate general issue for adding integration tests using proper roles?

I'm not going to say I'm completely closed to it, but my biggest hesitation is that we're talking about changing the RBAC permissions in a way that helps reduce permissions scope for Gateway API, but also significantly changes how permissions are applied and that gives me pause. Perhaps we could zoom on this one real quick?

[rainest]

Rebased to fix conflicts and incorporate changes since this was originally written:

• Split Knative controller disables IngressClass watches when that controller is disabled.
• Split Knative controller uses the new log thing from the new controller-runtime version.

https://github.com/Kong/kubernetes-ingress-controller/actions/runs/2750356088 is an E2E test for the new tip of the branch. If that or the integration tests fail, I screwed up the merge/update and need to do that again. If not, review as usual.