bug: apisix-controller will watch all namespace when no namespaces match namespaceSelector

[nayihz]

Issue description

I set namespace_selector: ["apisix.ingress=watching"] in conf.yaml before I start the apisix-ingress-controller. And there are no namespaces can match this label. So I think what we want is that the controller would not watch any namespaces. But in fact it watch all the exsiting namesapces(default/kube-system/kube-public...).

I check the code and find that it will list all the exsiting namespaces and put them all in watchingNamespaces when watchingNamespaces is empty.

Is it really what we want?

I think it can put all existing namespaces in watchingNamespaces only when the namespace_selector is empty.

apisix-ingress-controller/pkg/ingress/compare.go

Lines 50 to 64 in 1159522

	if !validation.HasValueInSyncMap(c.watchingNamespaces) {
	opts := v1.ListOptions{}
	// list all namespaces
	nsList, err := c.kubeClient.Client.CoreV1().Namespaces().List(ctx, opts)
	if err != nil {
	log.Error(err.Error())
	ctx.Done()
	} else {
	wns := new(sync.Map)
	for _, v := range nsList.Items {
	wns.Store(v.Name, struct{}{})
	}
	c.watchingNamespaces = wns
	}
	}

Environment

• your apisix-ingress-controller version (output of apisix-ingress-controller version --long):
• your Kubernetes cluster version (output of kubectl version):
• if you run apisix-ingress-controller in Bare-metal environment, also show your OS version (uname -a):

Minimal test code / Steps to reproduce

1. set namespace_selector: ["apisix.ingress=watching"] which no namespaces can match this label.
2. start controller.
3. print all items in watchingNamespaces

Actual result

it can print all the exsiting namespaces' name.

Error log

no error log

Expected result

no namespaces in watchingNamespaces

[tao12345666333]

Thanks！ I think this issue has been discussed before.
cc @gxthrj @tokers @lingsamuel

[nayihz]

if !validation.HasValueInSyncMap(c.watchingNamespaces) && len(c.watchingLabels) == 0 {
     // list all namespaces and put them all in c.watchingNamespaces
}

Is this the right way?

[gxthrj]

I think it can put all existing namespaces in watchingNamespaces only when the namespace_selector is empty.

This way is more reasonable.

[tao12345666333]

ref:
bug: The newly created namespace will not be watched · Issue #741 · apache/apisix-ingress-controller

fix: ingress do not watching any namespace when namespaceSelector is empty by gxthrj · Pull Request #742 · apache/apisix-ingress-controller

feat: add label-selector for watching namespace by gxthrj · Pull Request #715 · apache/apisix-ingress-controller

[nayihz]

if !validation.HasValueInSyncMap(c.watchingNamespaces) && len(c.watchingLabels) == 0 {
     // list all namespaces and put them all in c.watchingNamespaces
}

Is this the right way?

I think this is the right way to solve this problem. We should put all exsiting namespaces in watchingNamespaces only when both c.watchingNamespaces and c.watchingLabels are empty.

[lingsamuel]

Agreed. When the selector is not empty, it is more semantic to watch nothing.

[nayihz]

if !validation.HasValueInSyncMap(c.watchingNamespaces) && len(c.watchingLabels) == 0 {
     // ...
}

It could solve this problem after add len(c.watchingLabels) == 0. I'd like to fix this If you think it's okay. @tao12345666333 @gxthrj @lingsamuel

[lingsamuel]

I assume that the new behavior is accepted. This could be a breaking change for some environments.
I have assigned this issue to @cmssczy

/cc @tao12345666333 @gxthrj

[tao12345666333]

Yes, this is a breaking change. But it's also valuable and I think we should make changes.

[tao12345666333]

if !validation.HasValueInSyncMap(c.watchingNamespaces) && len(c.watchingLabels) == 0 {

     // ...

}

It could solve this problem after add len(c.watchingLabels) == 0. I'd like to fix this If you think it's okay. @tao12345666333 @gxthrj @lingsamuel

@cmssczy This modification is required, and some comments or documentation may need to be modified. Also need to check other related sections

[nayihz]

This modification is required, and some comments or documentation may need to be modified. Also need to check other related sections

I will check it. Thanks for remind.

[nayihz]

what is the difference between app_namespaces and namespace_selector in config-default.yaml? It seems that app_namespaces is outdated.

[gxthrj]

what is the difference between app_namespaces and namespace_selector in config-default.yaml? It seems that app_namespaces is outdated.

Yes, app_namespaces is outdate.

[nayihz]

Yes, app_namespaces is outdate.

I will send a pr to fix it.

[nayihz]

Do we plan to delete app_namespaces paremeter in conf/config-default.yaml in next version? I'd like to fix this If so.

[tao12345666333]

Do we plan to delete app_namespaces paremeter in conf/config-default.yaml in next version? I'd like to fix this If so.

@cmssczy yes, please! Thanks