is istio-pilot endpoint really needed?

It would only be needed if we need to support multicluster with the pre-istiod deployment model.

BTW - I think the istiod & istio-pilot endpoints will also be created as the result of pilot.enabled=true. so do we still need to create this again?

istio-pilot shouldn't be needed in 1.6+. I kept it because it would make backporting to 1.5 a bit easier. If someone was using the legacy charts and separate services in 1.5 they wouldn't be enabling pilot in the remote cluster.

isn't enabling pilot the only way to enable istiod? That is what the remote profile has today.

istiod runs in each clsuter and we need the istiod-remote service to avoid conflicting the "real" istiod service in the cluster.

When istiod=false (separate services) pilot doesn't need to run in the remote cluster. The istio-pilot service in the remote cluster points to the pilot in the main cluster. This is how things worked in <= 1.4.

We probably don't need to support the istiod=false case for multicluster

I agree for not needing this in master. What about 1.5?

@ayj I'd recommend sorting out what this should look like for 1.5 in master. Then follow up with another PR after we release 1.5.1.

Master doesn't have the legacy helm charts. We'll need to manually cherry-pick this PR anyway.

This doesn't do anything in master, it is reading from mesh config directly. 1.5 its needed though

can you remove after this merges?

Yup - #22050

We probably don't need to support the istiod=false case for multicluster

defining variables like this doesn't actually work inside if statements. this is fixed on 1.5 but somehow broken on master, I have a fix in https://github.com/istio/istio/pull/21788/files

Yup. Looks like your PR and my change below are similar except for the user of the DNS name instead of IP address.

interesting, thanks!

should this be grpc-istiod? Or tcp

I assume tcp, it is used everywhere for port 15012

Changed to tcp-istiod

can you remove after this merges?

nit: I think this should be a warning. Its not an internal error, its just sometimes not possible from the metadata provided by the client

Actually I didn't notice you aren't logging this. We should not return an error here, I think. Was there a reason you think we should?

I went back and forth on changing this. I was seeing a lot of these messages in the logs. It didn't seem like an error/warning when using multiple control planes. The check on line 541 will fail because Cluster A's registry won't have any pods for Cluster B. And getProxyServiceInstancesFromMetadata will fail because the remote proxy's cluster_id doesn't match the cluster's ID. It should still be printed but as a debug statement by the aggregate.

Some other alternatives:

• modify getProxyServiceInstancesFromMetadata to not return an error when cluster ID's didn't match.
• modify the aggregate controller to only call GetProxyServiceInstances() on the controller whose cluster_id matches the target proxy.

I am fine with making changes so this logs nothing in multicluster case - that makes complete sense. but if we return an error here, won't the aggregate controller bubble that error up and ultimately we will return an error? So it will always be an error for MC case? I am surprised this works at all, am I missing some step?

AFAICT the aggregate controller will log the error but not bubble it up if another controller returned one or more instances (see

How about the following? cc @nmittler

diff --git a/pilot/pkg/serviceregistry/aggregate/controller.go b/pilot/pkg/serviceregistry/aggregate/controller.go
index 1741a762e..6964364a4 100644
--- a/pilot/pkg/serviceregistry/aggregate/controller.go
+++ b/pilot/pkg/serviceregistry/aggregate/controller.go
@@ -228,6 +228,9 @@ func (c *Controller) GetProxyServiceInstances(node *model.Proxy) ([]*model.Servi
        // It doesn't make sense for a single proxy to be found in more than one registry.
        // TODO: if otherwise, warning or else what to do about it.
        for _, r := range c.GetRegistries() {
+               if r.Cluster() != "" && node.Metadata != nil && node.Metadata.ClusterID != "" && node.Metadata.ClusterID != r.Cluster() {
+                       continue
+               }
                instances, err := r.GetProxyServiceInstances(node)
                if err != nil {
                        errs = multierror.Append(errs, err)

Ah thanks... I looked right at that code and missed it. I think that makes sense, probably means things up a bit in general

Updated to filter in the aggregate controller. We need to special case the default cluster which is always name Kubernetes. I started to clean things up so the default registry uses the provided ClusterName but decided defer that for another PR. This is large enough as-is.

So if r.Cluster == Kubernetes and node.Cluster == MyOtherCluster, this will NOT be skipped? Isn't that what we were trying to avoid?

Yes, that is one of the cases we were trying to avoid. ClusterID is overloaded to mean both the registry type (k8s, consul) and the name (which cluster). I didn't want to fix that issue holistically in this PR (see / #22093). On further thought though we could filter 100% of the cases if we had some special handling above that is less intrusive (see skipSearchingRegistryForProxy above).

Why this change? This means we can only get endpoints from one registry? Which will break all the other registries like ServiceEntry?

See the for loop comment - "It doesn't make sense for a single proxy to be found in more than one registry". out only ever has at most instances from one registry. The code breaks out of the loop two lines below on line 251.

I don't think that comment is accurate? I am surprised all tests pass, I think you should be able to have ServiceEntry + Kube.

@rshriram ?

I've removed the change from this PR. We can address it in a follow-up.