Support crd discovery selector #36639

hzxuzhonghu · 2021-12-27T07:59:57Z

Please provide a description of this PR:

howardjohn

This is a breaking change. The API explicitly only applies to discovery, so making it do more will be incompatible.

It would be good to revisit the design doc. I recall we explicitly decided to make it only discovery. If we are changing our minds there we ought to understand why our decisions have changed.

hzxuzhonghu · 2021-12-28T01:42:00Z

The api does not explicitly say it does not limit CR, and also it does not explicitly say it works on it. Will search the design

hzxuzhonghu · 2021-12-28T01:47:19Z

Unfortunately, there is even no mention of crd in the design https://docs.google.com/document/d/1S3GPCUBmTmvh09NmpuMCG3vwDTNkVeRhGT3lbLyA4qs/edit?resourcekey=0-a8iMutRwjQElYN5dlgEDdw#heading=h.h3lxcxfhqndp

SO I think there is no compatibility issue, this can be treated as a feature request

howardjohn · 2021-12-28T03:49:45Z

Regardless of if it's in the doc dramatically changing the functionality of an existing API is incompatible.

Let's take a step back here. I haven't seen any discussion of the motivations for this change. We already have a lot of 'scoping' mechanisms. What problems are we trying to solve here?

hzxuzhonghu · 2021-12-28T06:08:11Z

#35092

hzxuzhonghu · 2022-01-10T03:15:00Z

@howardjohn @ramaraochavali What do i need to promote this feature?

howardjohn · 2022-01-10T03:16:57Z

I still don't understand the use case. The issue you linked does not explain why they want this.

hzxuzhonghu · 2022-01-10T03:31:30Z

An special case i can think of is in multi tenant case, multi istio can be deployed in a single cluster. We donot want the crd{CR} to interference each other.

hzxuzhonghu · 2022-01-12T06:12:48Z

@howardjohn #31115

howardjohn · 2022-01-12T15:41:21Z

Doesn't Sidecar scoping handle this?

In multitenant situations I would imagine its typical to not actually have permission to read from all namespaces?

howardjohn · 2022-01-12T15:42:15Z

I think this is a pretty important topic and should probably have a proper design. At the very least, we cannot change the behavior of the existing field or there will be incompatible behavior change when users of discoverySelector upgrade to this commit

hzxuzhonghu · 2022-01-13T01:40:53Z

I donot think there are many users of discoverySelector who have CR created in namespaces that are not selected.

For sake, I am fine to add a knob for this. We can also propose a discussion about it.

howardjohn · 2022-01-14T15:49:50Z

I haven't tested this, but the doc for sidecar workload selector (this is what you mean, right?) does not mention scoping for all CRs but only a handful? Does scoping work on all CRs?

All the CRs it does not scope are already scoped by namespace. For example, if you have a PeerAuthentication if foobar namespace, and have no proxies in foobar, it does nothing.

emike922 · 2022-01-21T18:06:19Z

For us (non-vanilla) this might be a convenient way to avoid configuration leakage between multiple tenants/istiod control planes. Tenant boundaries could be defined in terms of immutable kubernetes.io/metadata.name labels, allowing to dynamically extend or shrink the scope of each control plane without too much hassle or risk.

For vanilla users, this might be useful to include or exclude namespaces for configuration discovery, for example to ignore "less trusted" namespaces when operating in a shared environment.
It could also be useful to enable testing of revisions with incompatible or maybe just experimental (globally-scoped) configurations without affecting the productive instance.

kfaseela · 2022-01-25T09:44:31Z

Doesn't Sidecar scoping handle this?

In multitenant situations I would imagine its typical to not actually have permission to read from all namespaces?

@howardjohn sidecar scoping solves part of the issue, but what about the gateways?

howardjohn · 2022-01-25T15:19:31Z

@kfaseela Gateways have namespace scoping options already: PILOT_SCOPE_GATEWAY_TO_NAMESPACE or use new Kubernetes Gateway API which has this as default

kfaseela · 2022-01-25T16:10:42Z

@kfaseela Gateways have namespace scoping options already: PILOT_SCOPE_GATEWAY_TO_NAMESPACE or use new Kubernetes Gateway API which has this as default

Would try this end to end along with sidecar scoping and get back. But wouldn't it simplify the configuration a lot when we have this supported out of the box, while using discovery selectors? Ideally we don't want to configure additional sidecar config to limit the CRD scope.

emike922 · 2022-01-25T16:43:00Z

This might also be useful as an option to lock down certain CR discovery settings that cannot be overridden by the user (default mesh-wide sidecar w/o workload selector in root namespace is easily overridden by another sidecar if I understand correctly)

SpecialYang · 2022-02-10T07:17:59Z

We have implemented this feature in production but by little dirty way. We can deploy multiple istiod instances in one K8s cluster for tenant case. These istiod instances can not only locate at individual namespaces, but also locate at the same namespace isolated by labels of CR.

Part of code changes like below.

Server.

if alifeatures.WatchResourcesByNamespaceForPrimaryCluster != "" {
	informerOptions := kubelib.InformerOptions{
		WatchedNamespace: alifeatures.WatchResourcesByNamespaceForPrimaryCluster,
		Label:            alifeatures.WatchResourcesByLabelForPrimaryCluster,
	}
	s.kubeClient, err = kubelib.NewClientWithInformerOptions(kubelib.NewClientConfigForRestConfig(kubeRestConfig), informerOptions)
} else {
	s.kubeClient, err = kubelib.NewClient(kubelib.NewClientConfigForRestConfig(kubeRestConfig))
}

KubeClient.

func NewClientWithInformerOptions(clientConfig clientcmd.ClientConfig, options InformerOptions) (*client, error) {
	var c client
	var err error
	clientFactory := newClientFactory(clientConfig)
	c.clientFactory = clientFactory
	c.config, err = clientFactory.ToRESTConfig()
	if err != nil {
		return nil, err
	}
	c.restClient, err = clientFactory.RESTClient()
	if err != nil {
		return nil, err
	}
	c.discoveryClient, err = clientFactory.ToDiscoveryClient()
	if err != nil {
		return nil, err
	}
	c.mapper = restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(c.discoveryClient))
	c.Interface, err = kubernetes.NewForConfig(c.config)
	c.kube = c.Interface
	if err != nil {
		return nil, err
	}
	c.kubeInformer = informers.NewSharedInformerFactoryWithOptions(c.Interface, resyncInterval, informers.WithNamespace(options.WatchedNamespace))
	c.metadata, err = metadata.NewForConfig(c.config)
	if err != nil {
		return nil, err
	}
	c.metadataInformer = metadatainformer.NewFilteredSharedInformerFactory(c.metadata, resyncInterval, options.WatchedNamespace, nil)
	c.dynamic, err = dynamic.NewForConfig(c.config)
	if err != nil {
		return nil, err
	}
	c.dynamicInformer = dynamicinformer.NewFilteredDynamicSharedInformerFactory(c.dynamic, resyncInterval, options.WatchedNamespace, nil)
	c.istio, err = istioclient.NewForConfig(c.config)
	if err != nil {
		return nil, err
	}
	if options.Label != "" {
		c.istioInformer = istioinformer.NewSharedInformerFactoryWithOptions(c.istio, resyncInterval, istioinformer.WithNamespace(options.WatchedNamespace), istioinformer.WithTweakListOptions(func(listOpts *metav1.ListOptions) {
			listOpts.LabelSelector = options.Label
		}))
	} else {
		c.istioInformer = istioinformer.NewSharedInformerFactoryWithOptions(c.istio, resyncInterval, istioinformer.WithNamespace(options.WatchedNamespace))
	}
	c.gatewayapi, err = gatewayapiclient.NewForConfig(c.config)
	if err != nil {
		return nil, err
	}
	if options.Label != "" {
		c.gatewayapiInformer = gatewayapiinformer.NewSharedInformerFactoryWithOptions(c.gatewayapi, resyncInterval, gatewayapiinformer.WithNamespace(options.WatchedNamespace), gatewayapiinformer.WithTweakListOptions(func(listOpts *metav1.ListOptions) {
			listOpts.LabelSelector = options.Label
		}))
	} else {
		c.gatewayapiInformer = gatewayapiinformer.NewSharedInformerFactoryWithOptions(c.gatewayapi, resyncInterval, gatewayapiinformer.WithNamespace(options.WatchedNamespace))
	}
	c.mcsapis, err = mcsapisClient.NewForConfig(c.config)
	if err != nil {
		return nil, err
	}
	if options.Label != "" {
		c.mcsapisInformers = mcsapisInformer.NewSharedInformerFactoryWithOptions(c.mcsapis, resyncInterval, mcsapisInformer.WithNamespace(options.WatchedNamespace), mcsapisInformer.WithTweakListOptions(func(listOpts *metav1.ListOptions) {
			listOpts.LabelSelector = options.Label
		}))
	} else {
		c.mcsapisInformers = mcsapisInformer.NewSharedInformerFactoryWithOptions(c.mcsapis, resyncInterval, mcsapisInformer.WithNamespace(options.WatchedNamespace))
	}
	c.extSet, err = kubeExtClient.NewForConfig(c.config)
	if err != nil {
		return nil, err
	}
	return &c, nil
}

howardjohn · 2022-09-26T21:41:30Z

pilot/pkg/config/kube/crdclient/cache_handler.go

-			return i.Lister()
-		}
-		return i.Lister().ByNamespace(namespace)
+		informer: informer.NewFilteredSharedIndexInformer(cl.namespacesFilter, i.Informer()),
 	}


lister: cache.NewGenericLister(h.informer.GetIndexer(), schema.Resource().GroupVersionResource().GroupResource())

can we do this?

Definitely we can implement it in theory, but I think currently it is enough to use

NewGenericLister does not provide much benefit to our case

Why not? It makes the code much simpler as we do not need to do the weird indexer operations we have replaced it with, and is standard k8s practice

As you can see with generic lister for clusterscoped resource, we need to handle differently. however with underlying indexer, we do not need to care about it, that is why I believe this way is more simple

Actually, all the indexer related code are in pkg/kube, the fiteredInformer does only provide Get List interface.

I am not sure how it's simpler. The lister requires 1 single if statement here - the indexer approach has like 50 lines of added complexity throughout the PR of more complex error handling, type casting, and still has the if for namespaces but at each List call instead of in one single place

Listers are the Kubernetes standard for his to list objects, I don't see a compelling reason to move away from that. It also seems unrelated to the rest of the PR.

GenericLister returns runtime.Object, which also need type cast, error handling. I do not have strong preference though

yes but we don't need this: https://github.com/istio/istio/pull/36639/files#diff-eff3d408ac506e9395b51f480c999a588f9fa408e392ce77506f25ceae00d6a1R328

and we now have to do:

item, _, err := h.informer.GetIndexer().GetByKey(KeyFunc(namespace, name)) if item == nil || err != nil {

instead of just

obj, err := h.lister(namespace).Get(name) if err != nil {

howardjohn

overall it's looking right but it's changing a lot of unrelated things which I think add complexity

howardjohn · 2022-09-28T01:38:37Z

pilot/pkg/config/kube/ingress/controller.go

 		ingressLister:   ingressInformer.Lister(),
 		classes:         classes,
 		serviceInformer: serviceInformer.Informer(),
 		serviceLister:   serviceInformer.Lister(),
 	}

+	if options.DiscoveryNamespacesFilter != nil {
+		c.filteredIngressInformer = informer.NewFilteredSharedIndexInformer(options.DiscoveryNamespacesFilter.Filter, ingressInformer.Informer())


nit: no need for if, the result is the same

still applicable but not blocking

howardjohn · 2022-09-28T01:44:11Z

pkg/kube/namespace/filter.go

@@ -74,6 +74,10 @@ func (d *discoveryNamespacesFilter) Filter(obj any) bool {
 		return true
 	}

+	if ns, ok := obj.(string); ok {


what is this for?

This is to filter out namespaces, which is useful for namespace controller

Its not used in this PR, right? Just the future?

In the namespace controllre PR we discussed making a FilterObject(obj Object) and FilterNamespace(namespace string). Instead of doing all the type casting, etc. This is more explicit API

howardjohn

lgtm, we can followup on the rest

howardjohn · 2022-09-28T23:29:51Z

pilot/pkg/config/kube/ingress/controller.go

 		ingressLister:   ingressInformer.Lister(),
 		classes:         classes,
 		serviceInformer: serviceInformer.Informer(),
 		serviceLister:   serviceInformer.Lister(),
 	}

+	if options.DiscoveryNamespacesFilter != nil {
+		c.filteredIngressInformer = informer.NewFilteredSharedIndexInformer(options.DiscoveryNamespacesFilter.Filter, ingressInformer.Informer())


still applicable but not blocking

howardjohn · 2022-09-28T23:30:56Z

pilot/pkg/serviceregistry/kube/controller/controller.go

@@ -802,7 +803,7 @@ func (c *Controller) syncNodes() error {

 func (c *Controller) syncServices() error {
 	var err *multierror.Error
-	services := c.serviceInformer.GetIndexer().List()
+	services, _ := c.serviceInformer.List("")


nit: use metav1.NamespaceAll

howardjohn · 2022-09-28T23:31:36Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcache.go

+		if c.opts.DiscoveryNamespacesFilter != nil {
+			ec.filteredInformer = informer.NewFilteredSharedIndexInformer(c.opts.DiscoveryNamespacesFilter.Filter, dInformer.Informer())
+		} else {
+			ec.filteredInformer = informer.NewFilteredSharedIndexInformer(nil, dInformer.Informer())


same comment about reducing if

c.opts.DiscoveryNamespacesFilter can be nil, this can be optimized later, because we need to filter namespace string as well

howardjohn · 2022-09-28T23:37:28Z

pkg/kube/multicluster/secretcontroller.go

+	nsInformer := kubeclientset.KubeInformer().Core().V1().Namespaces().Informer()
+	_ = nsInformer.SetTransform(kube.StripUnusedFields)
+	nsLister := kubeclientset.KubeInformer().Core().V1().Namespaces().Lister()
+	controller.DiscoveryNamespacesFilter = filter.NewDiscoveryNamespacesFilter(nsLister, meshWatcher.Mesh().GetDiscoverySelectors())


do we want to protect this with the feature flag?

Address some followup comments

hzxuzhonghu requested review from a team as code owners December 27, 2021 07:59

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2021

istio-policy-bot added area/networking release-notes-none Indicates a PR that does not require release notes. labels Dec 27, 2021

howardjohn reviewed Dec 27, 2021

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 25, 2022

hzxuzhonghu force-pushed the crd-filter branch from 2387605 to 1819ba4 Compare January 26, 2022 02:34

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 26, 2022

emike922 mentioned this pull request Mar 11, 2022

add injectionSelectors istio/api#2212

Closed

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Mar 12, 2022

hzxuzhonghu added 8 commits September 24, 2022 17:24

Fix

165ded3

fix

90b1cb1

ingress controller update

cb3b316

update

ffb0784

remove cb

ae65c2a

fix build

6ffaa38

Fix lint

9c20088

If no filter specified, use the k8s informer directly

e63c660

howardjohn reviewed Sep 26, 2022

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 26, 2022

Merge branch 'master' into crd-filter

b6704d1

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 27, 2022

kfaseela mentioned this pull request Sep 27, 2022

create istio-ca-root-cert cm in namespaces selected by discoverySelectors #41198

Merged

howardjohn reviewed Sep 28, 2022

View reviewed changes

hzxuzhonghu added 6 commits September 28, 2022 11:44

Address comments

0f789a7

Address comments

ca634cf

Address comments

c1dd905

Filter service import/export

1779d06

Fix build

eed0015

lint

80b33e8

howardjohn approved these changes Sep 28, 2022

View reviewed changes

hzxuzhonghu added 2 commits September 29, 2022 10:31

Address comments

fa47a92

lint

663b570

istio-testing merged commit ffe8a3f into istio:master Sep 29, 2022

hzxuzhonghu deleted the crd-filter branch September 29, 2022 04:04

howardjohn added a commit to howardjohn/istio that referenced this pull request Oct 3, 2022

Minor refactoring after istio#36639

7c24f85

Address some followup comments

kfaseela mentioned this pull request Oct 18, 2022

istiod only deal with the stable namespace istio crd，such as istio-system #40316

Closed

kfaseela mentioned this pull request Nov 9, 2022

Add 1.16.0 release notes istio/istio.io#12201

Merged

10 tasks

kfaseela mentioned this pull request Dec 23, 2022

Doc: multiple control planes using revisions and discoverySelectors istio/istio.io#12415

Merged

10 tasks

Support crd discovery selector #36639

Support crd discovery selector #36639

Conversation

hzxuzhonghu commented Dec 27, 2021 • edited by istio-policy-bot

howardjohn left a comment

Choose a reason for hiding this comment

hzxuzhonghu commented Dec 28, 2021

hzxuzhonghu commented Dec 28, 2021

howardjohn commented Dec 28, 2021

hzxuzhonghu commented Dec 28, 2021

hzxuzhonghu commented Jan 10, 2022

howardjohn commented Jan 10, 2022

hzxuzhonghu commented Jan 10, 2022

hzxuzhonghu commented Jan 12, 2022

howardjohn commented Jan 12, 2022

howardjohn commented Jan 12, 2022

hzxuzhonghu commented Jan 13, 2022

howardjohn commented Jan 14, 2022

emike922 commented Jan 21, 2022

kfaseela commented Jan 25, 2022 • edited

howardjohn commented Jan 25, 2022

kfaseela commented Jan 25, 2022

emike922 commented Jan 25, 2022

SpecialYang commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Dec 27, 2021 •

edited by istio-policy-bot

kfaseela commented Jan 25, 2022 •

edited