feature: Support Dynamic namespaces using Labels #3299

ciarams87 · 2022-11-23T14:38:51Z

Proposed changes

Provide a new CLI arg watch-namespace-label which supports watching for a namespace label instead of for discrete namespace names - this allows the list of watched namespaces to change dynamically based on a selector. The IC will watch for namespaces with the label selector and will dynamically add or remove watched namespaces as namespaces have this label added or removed.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

codecov-commenter · 2022-11-25T13:28:24Z

Codecov Report

Merging #3299 (ee0a427) into main (5d607dd) will decrease coverage by 1.22%.
The diff coverage is 0.39%.

❗ Current head ee0a427 differs from pull request most recent head 6112640. Consider uploading reports for the commit 6112640 to get more accurate results

@@            Coverage Diff             @@
##             main    #3299      +/-   ##
==========================================
- Coverage   52.86%   51.64%   -1.23%     
==========================================
  Files          59       59              
  Lines       16137    16520     +383     
==========================================
  Hits         8531     8531              
- Misses       7318     7701     +383     
  Partials      288      288

Impacted Files	Coverage Δ
cmd/nginx-ingress/flags.go	`31.03% <0.00%> (-1.95%)`	⬇️
cmd/nginx-ingress/main.go	`0.00% <0.00%> (ø)`
internal/configs/configurator.go	`36.00% <0.00%> (-1.06%)`	⬇️
internal/externaldns/controller.go	`4.93% <0.00%> (-1.27%)`	⬇️
internal/k8s/controller.go	`11.90% <0.00%> (-0.96%)`	⬇️
internal/k8s/handlers.go	`6.49% <0.00%> (-0.34%)`	⬇️
internal/k8s/task_queue.go	`0.00% <0.00%> (ø)`
internal/certmanager/cm_controller.go	`12.12% <3.92%> (-3.15%)`	⬇️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/nginx-ingress/main.go

deployments/helm-chart/values.yaml

hafe

Why not have a fixed name namespace label? Reduces complexity of the change

ciarams87 · 2022-11-28T10:53:25Z

Why not have a fixed name namespace label? Reduces complexity of the change

@hafe I'm not sure what you mean here?

hafe · 2022-11-28T10:57:25Z

Why not have a fixed name namespace label? Reduces complexity of the change

@hafe I'm not sure what you mean here?

That you decide and use a hardcoded label name.

What is the purpose of being able to change the name on the command line?

ciarams87 · 2022-11-28T11:13:21Z

Why not have a fixed name namespace label? Reduces complexity of the change

@hafe I'm not sure what you mean here?

That you decide and use a hardcoded label name.

What is the purpose of being able to change the name on the command line?

Thanks for the feedback @hafe. The purpose here is so that the user can configure the Ingress Controller to watch whatever label makes sense for their environment. It cannot be changed during runtime, so it is effectively hardcoded in the deployment spec. If the label needs to be changed, a redeployment will be required to do so.

hafe · 2022-11-28T11:25:45Z

I was thinking that freedom to change the name is perhaps not needed

brianehlert · 2022-11-29T17:09:18Z

I think that one thing we were cognizant of when we were designing this, is that a customer could have multiple NGINX Ingress Controller deployments running within any cluster at the same time, and thus we could not have only one hard coded label as the two deployments would clash.
This way each deployment can have a unique label, just like they might have unique namespaces.

hafe · 2022-11-29T17:47:17Z

I think that one thing we were cognizant of when we were designing this, is that a customer could have multiple NGINX Ingress Controller deployments running within any cluster at the same time, and thus we could not have only one hard coded label as the two deployments would clash.
This way each deployment can have a unique label, just like they might have unique namespaces.

Of course, thanks for the explanation!

shaun-nx

👏

vepatel

Great stuff 👏🏼

for more information, see https://pre-commit.ci

ciarams87 force-pushed the dynamic-namespaces branch 2 times, most recently from 3f42285 to 22fbff7 Compare November 23, 2022 14:41

github-actions bot added the tests Pull requests that update tests label Nov 24, 2022

ciarams87 force-pushed the dynamic-namespaces branch from 147fd08 to 8416484 Compare November 24, 2022 15:19

ciarams87 added enhancement Pull requests for new features/feature enhancements and removed tests Pull requests that update tests labels Nov 24, 2022

github-actions bot added tests Pull requests that update tests and removed enhancement Pull requests for new features/feature enhancements labels Nov 25, 2022

shaun-nx reviewed Nov 25, 2022

View reviewed changes

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved

github-actions bot added documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart labels Nov 25, 2022

ciarams87 added the enhancement Pull requests for new features/feature enhancements label Nov 25, 2022

ciarams87 force-pushed the dynamic-namespaces branch from fda6ff0 to 05e68cd Compare November 25, 2022 15:10

github-actions bot removed the enhancement Pull requests for new features/feature enhancements label Nov 25, 2022

ciarams87 force-pushed the dynamic-namespaces branch from 05e68cd to 15a7ba7 Compare November 25, 2022 15:12

ciarams87 changed the title ~~feat: Support Dynamic namespaces using Labels~~ feature: Support Dynamic namespaces using Labels Nov 25, 2022

ciarams87 added the enhancement Pull requests for new features/feature enhancements label Nov 25, 2022

ciarams87 marked this pull request as ready for review November 25, 2022 15:13

ciarams87 requested a review from a team as a code owner November 25, 2022 15:13

ciarams87 requested review from haywoodsh, jjngx and shaun-nx November 25, 2022 15:13

shaun-nx reviewed Nov 25, 2022

View reviewed changes

deployments/helm-chart/values.yaml Outdated Show resolved Hide resolved

hafe reviewed Nov 27, 2022

View reviewed changes

ciarams87 force-pushed the dynamic-namespaces branch from 15a7ba7 to 7cdb409 Compare November 28, 2022 10:55

github-actions bot removed the enhancement Pull requests for new features/feature enhancements label Nov 28, 2022

jjngx approved these changes Nov 28, 2022

View reviewed changes

ciarams87 force-pushed the dynamic-namespaces branch 2 times, most recently from ee0a427 to 6112640 Compare November 29, 2022 12:49

shaun-nx approved these changes Nov 30, 2022

View reviewed changes

vepatel approved these changes Nov 30, 2022

View reviewed changes

ciarams87 force-pushed the dynamic-namespaces branch from 6112640 to 4826269 Compare November 30, 2022 13:11

ciarams87 added the enhancement Pull requests for new features/feature enhancements label Nov 30, 2022

ciarams87 and others added 11 commits November 30, 2022 14:18

Add labels flag and create namespace watcher when present

5a7874b

Create namespaced watchers dynamically for simple case

e5db4b1

Handle unwatched app protect resources

c93135d

Add dynamic watchers to externaldns and cert-manager controllers

ebfd289

Add initial python tests

9bef1be

Add app protect waf tests

2474c61

[pre-commit.ci] auto fixes from pre-commit.com hooks

6fd5b12

for more information, see https://pre-commit.ci

Add python tests for cert-manager and external-dns

c949925

Add helm config and docs

a9e421d

Update logging for new and deleted watched namespaces

b970037

[pre-commit.ci] auto fixes from pre-commit.com hooks

cb34ac7

for more information, see https://pre-commit.ci

ciarams87 force-pushed the dynamic-namespaces branch from 4826269 to cb34ac7 Compare November 30, 2022 14:18

github-actions bot removed the enhancement Pull requests for new features/feature enhancements label Nov 30, 2022

ciarams87 added the enhancement Pull requests for new features/feature enhancements label Nov 30, 2022

ciarams87 merged commit 363e697 into main Nov 30, 2022

ciarams87 deleted the dynamic-namespaces branch November 30, 2022 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feature: Support Dynamic namespaces using Labels #3299

feature: Support Dynamic namespaces using Labels #3299

Uh oh!

ciarams87 commented Nov 23, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 25, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

hafe left a comment

Uh oh!

ciarams87 commented Nov 28, 2022

Uh oh!

hafe commented Nov 28, 2022

Uh oh!

ciarams87 commented Nov 28, 2022

Uh oh!

hafe commented Nov 28, 2022

Uh oh!

brianehlert commented Nov 29, 2022

Uh oh!

hafe commented Nov 29, 2022

Uh oh!

shaun-nx left a comment

Uh oh!

vepatel left a comment

Uh oh!

Uh oh!

feature: Support Dynamic namespaces using Labels #3299

feature: Support Dynamic namespaces using Labels #3299

Uh oh!

Conversation

ciarams87 commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

codecov-commenter commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

hafe left a comment

Choose a reason for hiding this comment

Uh oh!

ciarams87 commented Nov 28, 2022

Uh oh!

hafe commented Nov 28, 2022

Uh oh!

ciarams87 commented Nov 28, 2022

Uh oh!

hafe commented Nov 28, 2022

Uh oh!

brianehlert commented Nov 29, 2022

Uh oh!

hafe commented Nov 29, 2022

Uh oh!

shaun-nx left a comment

Choose a reason for hiding this comment

Uh oh!

vepatel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ciarams87 commented Nov 23, 2022 •

edited

Loading

codecov-commenter commented Nov 25, 2022 •

edited

Loading