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 watch-namespace automation #420

Merged
merged 2 commits into from
Jul 27, 2021
Merged

[kong] add watch-namespace automation #420

merged 2 commits into from
Jul 27, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 27, 2021

What this PR does / why we need it:

Automates role creation for controllers that watch a specific set of namespaces only. When ingressController.watchNamespaces is set to an empty array, watches all namespaces and uses a ClusterRole to access resources, same as we've done in the past. When it is set to a list of namespaces, specifies them in --watch-namespace and creates a Role/RoleBinding for each individual namespace.

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#1503

Special notes for your reviewer:

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])

Add a watchNamespaces setting to the controller. When set to a list of
namespaces, sets CONTROLLER_WATCH_NAMESPACE to the list of namespaces
and creates Role to access resources in every watched namespace.

When set to an empty list (the default), watches all namespaces and
creates a ClusterRole.
@rainest rainest requested a review from a team as a code owner July 27, 2021 19:39
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.

No blockers, but the failing CI tests seem to indicate we've broken something?

@rainest
Copy link
Contributor Author

rainest commented Jul 27, 2021

I think I failed to heed my own advice: the automation does require KIC 2.x, but we don't test with that yet. CI for the new feature would require a 2.x instance to run successfully, so until then we can only test the default, which uses existing functionality with a slightly changed template.

Will probably just remove that part of the change. Poking at it a bit since it failed in a weird way (the namespace was just completely empty, which is... odd), and I should do a manual test that does use 2.x.

Q: is it feasible to automate the manual tests ?

@shaneutt
Copy link
Member

I think I failed to heed my own advice: the automation does require KIC 2.x, but we don't test with that yet. CI for the new feature would require a 2.x instance to run successfully, so until then we can only test the default, which uses existing functionality with a slightly changed template.

Will probably just remove that part of the change. Poking at it a bit since it failed in a weird way (the namespace was just completely empty, which is... odd), and I should do a manual test that does use 2.x.

Sounds good. Once you have done manual testing, and have some corrections for the tests I'm still ✔️

- Fix an incorrectly-placed namespace field in a RoleBinding template.
- Revert changes to ci/test2-values.yaml. Do not test specific
  namespaces as it requires KIC 2.x and the namespaces in question. We
  can re-add this test once we default to KIC 2, though we'll need to
  use a namespace that exists in the test environment (so probably watch
  default only).
- Extend local namespace role for 2.x. Role now includes get permissions
  to Services and Endpoints, to allow controllers without cluster
  permissions to see the proxy Service for publish service info.
- Change the RBAC API versions. We were using a deprecated version, and
  do not need changes other than the API version itself to use a
  supported version.
- When watching specific namespaces, disable the KongClusterPlugin
  controller. That controller does not work without cluster permissions.
@rainest
Copy link
Contributor Author

rainest commented Jul 27, 2021

Post manual testing updates in a096df3. Requesting review again since the changes are large enough to warrant another look IMO.

2.x will now start with a limited set of namespaces without permissions errors.

We actually won't be able to completely test watchNamespaces even with 2.x, since you can't use namespaces that don't exist and can't create new namespaces for CI. The workaround in #421 is the only option. Still gets most of what we want to test (templates are correct and the controller doesn't break completely with that option) even though it's not a realistic configuration.

RBAC version bump wasn't necessary for this, but we should do it anyway because v1beta1 is deprecated, and we intended to change it per Kong/kubernetes-ingress-controller#801. That's handled upstream but wasn't handled here because I only overwrote rules when making these changes.

@rainest rainest requested a review from shaneutt July 27, 2021 21:13
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.

LGTM 👍

Just one comment about cluster plugin, and then when that's resolved and CI is happy, I'm happy 🖖

@rainest rainest merged commit 5c27eb7 into next Jul 27, 2021
@rainest rainest deleted the feat/limit-namespaces branch July 27, 2021 22:24
@shaneutt shaneutt mentioned this pull request Aug 23, 2021
2 tasks
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.

None yet

2 participants