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

Support cluster-scoped resources when using watchNamespaces #611

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jun 11, 2022

What this PR does / why we need it:

When watchNamespaces is non-empty, create a ClusterRole that contains
cluster-scoped resources only.

Remove endpoints from the cluster resource template. They appear to have
been included by mistake.

Configure one of the CI values to use watchNamespaces.

Which issue this PR fixes

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

When watchNamespaces was originally developed, the only cluster-scoped resource we had was KongClusterPlugin, and KongPlugin could substitute for it with an acceptable loss of functionality. Since, CNCF has introduced IngressClass and GatewayClass, which have no namespace-scoped equivalent. We need to read these to properly implement the v1 Ingress and vAny Gateway specs.

Special notes for your reviewer:

Ready for review, but do not merge pending discussion with product and field team stakeholders in KUBE-61.

Checklist

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

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • Commits follow the Kong commit message guidelines

@rainest rainest requested a review from a team as a code owner June 11, 2022 00:00
@rainest rainest changed the title feat(rbac) support cluster-scoped always Support cluster-scoped resources when using watchNamespaces Jun 11, 2022
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.

👍

@shaneutt shaneutt self-requested a review June 13, 2022 18:10
shaneutt
shaneutt previously approved these changes Jun 13, 2022
@jasiek-zywczak
Copy link

the behaviour of charts is (almost) correct, when we are running kong in the cluster we should not require cluster role, the problem is that chart is not disable watching the ingressClass in that case
#648

@rainest
Copy link
Contributor Author

rainest commented Jul 19, 2022

@jasiek-zywczak while the controller should indeed disable that lookup when disabling the controller, the plan is to continue with this change (we haven't received objections from the stakeholders we reached out to), which should resolve your issue, correct? The install would have permissions for IngressClass, and the attempts to list them would not fail. Did you have any concerns with this approach?

@jasiek-zywczak
Copy link

jasiek-zywczak commented Jul 19, 2022

It is a matter of being consequent. Why disabling access to cluster plugins then? I mean, in case watch namespace is set?
And btw chart's docs precisely mention that access to cluster objects should be disabled.
When I set that I want to watch a particular namespace it means I don't want to watch, in some cases, the whole cluster.

When watchNamespaces is non-empty, create a ClusterRole that contains
cluster-scoped resources only.

Remove endpoints from the cluster resource template. They appear to have
been included by mistake.

Configure one of the CI values to use watchNamespaces.
@rainest
Copy link
Contributor Author

rainest commented Jul 19, 2022

Why disabling access to cluster plugins then? I mean, in case watch namespace is set?

That is a good catch--this change should indeed remove the KongClusterPlugin disable as well. I've updated this to do so and document the new behavior.

When I set that I want to watch a particular namespace it means I don't want to watch, in some cases, the whole cluster.

That will be the case for namespaced resources. The library we use to implement watchNamespaces has two modes: it can add cluster-level watches for everything or create separate watches for each namespace and a cluster-level watch for cluster-scoped resources only. It's not possible to watch cluster-scoped resources at anything other than the cluster level, so trying to impose a namespace-restricted model on them doesn't make sense.

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.

New installs of v2.8.2 missing RBAC permissions for IngressClass
3 participants