Support watch multiple namespaces matched witch namespace selector #7472

zryfish · 2021-08-10T11:59:28Z

What this PR does / why we need it:

The watch-namespace flag only supports a single string namespace value or an empty value that maps to all namespaces.
This PR implements watching multiple namespaces with a labelSelector. There is an issue with more details #7090

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

fixes #7090

How Has This Been Tested?

make test
make e2e-test

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2021-08-10T11:59:36Z

@zryfish: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-08-10T11:59:37Z

Welcome @zryfish!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-08-10T11:59:37Z

Hi @zryfish. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

zryfish · 2021-08-16T06:00:06Z

/kind feature

rikatz · 2021-08-21T14:59:02Z

/ok-to-test
/assign

For v1.0.1

rikatz · 2021-10-10T19:03:18Z

internal/ingress/controller/store/store.go

 	resyncPeriod time.Duration,
 	client clientset.Interface,
 	updateCh *channels.RingChannel,
 	disableCatchAll bool,
 	icConfig *ingressclass.IngressClassConfiguration) Storer {

+	namespaceSelector, _ := labels.Parse(watchNamespaceSelector)


I would probably complain here about ignoring the error (even if you do that earlier, in the main.go).

I think we already waited too much for this change (sorry, been busy with a lot of stuff), but I'm here thinking if we cannot change the new function to receive a labels.Selector (change the nginx.Configuration structure) so we can "skip" this here.

Yeah, good suggestion, changed as suggested

rikatz · 2021-10-10T19:03:45Z

internal/ingress/controller/store/store.go

@@ -315,6 +329,35 @@ func New(
 	store.informers.Service = infFactory.Core().V1().Services().Informer()
 	store.listers.Service.Store = store.informers.Service.GetStore()

+	// avoid caching namespaces at cluster scope when watching single namespace


test/e2e/framework/deployment.go

rikatz · 2021-10-10T19:10:50Z

@zryfish overall it looks really good to me, thanks!

I have left one concern about the helm vs defaults (probably my mess) and some small nits/typos to be fixed.

Targeting to release a new version on tuesday, 12 (holiday here, will have some time), if we can ship this until there it would be great.

Thanks

zryfish · 2021-10-12T06:30:38Z

@rikatz Typo should be fixed now.

rikatz · 2021-10-12T20:47:35Z

@zryfish I will cut v1.0.4 now and will for sure ship this in v1.0.5 :)

Just want to make a final pass but need to solve an openssl bug here

Thanks!

rikatz · 2021-10-24T21:24:05Z

@zryfish one last thing (promise!) now that I was doing a final pass:

store.informers.Endpoint.AddEventHandler(epEventHandler)
store.informers.Secret.AddEventHandler(secrEventHandler)
store.informers.Service.AddEventHandler(serviceHandler)

On the handlers (inside the function) wouldn't be good to also limit the watch Namespace? This way you wouldn't populate the informers and caches with unnecessary stuff :)

zryfish · 2021-10-25T01:42:45Z

@zryfish one last thing (promise!) now that I was doing a final pass:
store.informers.Endpoint.AddEventHandler(epEventHandler)
store.informers.Secret.AddEventHandler(secrEventHandler)
store.informers.Service.AddEventHandler(serviceHandler)
On the handlers (inside the function) wouldn't be good to also limit the watch Namespace? This way you wouldn't populate the informers and caches with unnecessary stuff :)

Yeah, I agree. But I think we already limit the namespace during initializing the informer factories if the scope is enabled.

	// create informers factory, enable and assign required informers
	infFactory := informers.NewSharedInformerFactoryWithOptions(client, resyncPeriod,
		informers.WithNamespace(namespace),
	)
	store.informers.Ingress = infFactory.Networking().V1().Ingresses().Informer()
	store.listers.Ingress.Store = store.informers.Ingress.GetStore()

I couldn't think of a better way to limit namespaces when the namespace selector is specified because newly created namespaces could match with the namespace selector, we need to watch those namespaces too. Namespace selector is much like watching the cluster namespaces because the watching namespaces may change during execution, cluster level resources caching is needed.

skip caching namespaces at cluster scope if only watching single namespace add --watch-namespace-selector in user guide add e2e test

rikatz · 2021-11-12T19:39:17Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2021-11-12T19:39:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, zryfish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

skip caching namespaces at cluster scope if only watching single namespace add --watch-namespace-selector in user guide add e2e test

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 10, 2021

k8s-ci-robot requested a review from cpanato August 10, 2021 11:59

k8s-ci-robot requested a review from rikatz August 10, 2021 11:59

k8s-ci-robot added area/docs area/helm Issues or PRs related to helm charts needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 10, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 10, 2021

zryfish mentioned this pull request Aug 10, 2021

Support for watching multiple namespaces with a namespace labelSelector #7090

Closed

zryfish changed the title ~~[WIP] Support watch multiple namespaces labels matched witch namespace selector~~ [WIP] Support watch multiple namespaces matched witch namespace selector Aug 10, 2021

zryfish force-pushed the add_watch_multiple_namespaces_by_label_selector branch from 6802d0e to 43b30b0 Compare August 10, 2021 12:10

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2021

zryfish changed the title ~~[WIP] Support watch multiple namespaces matched witch namespace selector~~ Support watch multiple namespaces matched witch namespace selector Aug 11, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2021

zryfish force-pushed the add_watch_multiple_namespaces_by_label_selector branch from 5872057 to a4d782c Compare August 11, 2021 15:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 16, 2021

rikatz added this to the v1.0.1 milestone Aug 21, 2021

k8s-ci-robot assigned rikatz Aug 21, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 21, 2021

rikatz reviewed Oct 10, 2021

View reviewed changes

test/e2e/framework/deployment.go Outdated Show resolved Hide resolved

rikatz modified the milestones: v1.0.3, v1.0.4 Oct 10, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2021

zryfish force-pushed the add_watch_multiple_namespaces_by_label_selector branch from 3d1fd03 to cf0e5ed Compare October 12, 2021 05:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2021

rikatz modified the milestones: v1.0.4, v1.0.5 Oct 12, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2021

zryfish force-pushed the add_watch_multiple_namespaces_by_label_selector branch from cf0e5ed to 6d548ae Compare October 15, 2021 02:55

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2021

zryfish force-pushed the add_watch_multiple_namespaces_by_label_selector branch from 6d548ae to fd36b8c Compare November 8, 2021 02:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2021

support watch namespaces matched namespace selector

0b20c7f

skip caching namespaces at cluster scope if only watching single namespace add --watch-namespace-selector in user guide add e2e test

zryfish force-pushed the add_watch_multiple_namespaces_by_label_selector branch from fd36b8c to 0b20c7f Compare November 8, 2021 05:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2021

k8s-ci-robot merged commit 7203a0b into kubernetes:main Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support watch multiple namespaces matched witch namespace selector #7472

Support watch multiple namespaces matched witch namespace selector #7472

zryfish commented Aug 10, 2021 •

edited

Loading

k8s-ci-robot commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

zryfish commented Aug 16, 2021

rikatz commented Aug 21, 2021

rikatz Oct 10, 2021

zryfish Oct 12, 2021

rikatz Oct 10, 2021

rikatz commented Oct 10, 2021

zryfish commented Oct 12, 2021

rikatz commented Oct 12, 2021

rikatz commented Oct 24, 2021

zryfish commented Oct 25, 2021

rikatz commented Nov 12, 2021

k8s-ci-robot commented Nov 12, 2021

Support watch multiple namespaces matched witch namespace selector #7472

Support watch multiple namespaces matched witch namespace selector #7472

Conversation

zryfish commented Aug 10, 2021 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

zryfish commented Aug 16, 2021

rikatz commented Aug 21, 2021

rikatz Oct 10, 2021

Choose a reason for hiding this comment

zryfish Oct 12, 2021

Choose a reason for hiding this comment

rikatz Oct 10, 2021

Choose a reason for hiding this comment

rikatz commented Oct 10, 2021

zryfish commented Oct 12, 2021

rikatz commented Oct 12, 2021

rikatz commented Oct 24, 2021

zryfish commented Oct 25, 2021

rikatz commented Nov 12, 2021

k8s-ci-robot commented Nov 12, 2021

zryfish commented Aug 10, 2021 •

edited

Loading