Fix Sidecarscope service and destinationrule map

[hzxuzhonghu]

Please provide a description of this PR:

Added namespace key for service and dr map, without namespace, istio can ignore some configs if there are many for same hostname but reside in different namespace

Fix #40952

To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

• [ ] Ambient
• [ ] Configuration Infrastructure
• [ ] Docs
• [ ] Installation
• [ ] Networking
• [ ] Performance and Scalability
• [ ] Policies and Telemetry
• [ ] Security
• [ ] Test and Release
• [ ] User Experience
• [ ] Developer Infrastructure

Please check any characteristics that apply to this pull request.

• [ ] Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

[ramaraochavali]

How will this fix https://github.com/istio/istio/issues/40952 ? How did it work earlier? That seems to be caused by some regression?

[hzxuzhonghu]

I have tested with https://github.com/istio/istio/issues/40952#issuecomment-1246698194

What regression do you mean?

[ramaraochavali]

I have tested with https://github.com/istio/istio/issues/40952#issuecomment-1246698194

What regression do you mean?

OK. Wondering how did it work on 1.13.2 but not on 1.14.3.? So thought some thing regressed. This change is not there in 1.13.2 also

[hzxuzhonghu]

@ramaraochavali see this comment https://github.com/istio/istio/issues/40952#issuecomment-1247866691 i figured out

[ramaraochavali]

see this comment https://github.com/istio/istio/issues/40952#issuecomment-1247866691 i figured out

Got it.

[howardjohn]

The intent is that sidecar scope only sees 1 Service per hostname. It's not clear to me why that invariant needs to be violated here?

On Tue, Sep 20, 2022, 6:32 PM Zhonghu Xu @.***> wrote:

@.**** commented on this pull request.

In pilot/pkg/model/push_context.go https://github.com/istio/istio/pull/41062#discussion_r975955595:

func (ps *PushContext) ServiceForHostname(proxy *Proxy, hostname host.Name) *Service { if proxy != nil && proxy.SidecarScope != nil {

• return proxy.SidecarScope.servicesByHostname[hostname]
  

• // first return the service from proxy namespace
  

• if proxy.SidecarScope.servicesByHostname[hostname][proxy.ConfigNamespace] != nil {
  

• 	return proxy.SidecarScope.servicesByHostname[hostname][proxy.ConfigNamespace]
  

• }
  

• // return arbitrary service
  

• for _, svc := range proxy.SidecarScope.servicesByHostname[hostname] {
  

@howardjohn https://github.com/howardjohn updated comment, actually it is return a service of hostname from other namespaces, but it is also exported to this proxy.

— Reply to this email directly, view it on GitHub https://github.com/istio/istio/pull/41062#discussion_r975955595, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXJZCEONCM4HXKHDFG3V7JQS5ANCNFSM6AAAAAAQQZACKA . You are receiving this because you were mentioned.Message ID: @.***>

[hzxuzhonghu]

The intent is that sidecar scope only sees 1 Service per hostname.

Why, in the issue case, it should see two services with same hostname but in different nses

[howardjohn]

The intent is that sidecar scope only sees 1 Service per hostname.

Why, in the issue case, it should see two services with same hostname but in different nses

Or see 1 Service with 2 destination rules? It seems like perhaps we are attaching the DR hostname to a specific Service too early, before it's filtered by sidecarscope.

Since this is a regression, and we historically used 1 Service in SidecarScope, it seems we should be able to keep that model?

[hzxuzhonghu]

DR selection is very related to proxy namespace and service namespace, I am concerned if changing this could introduce other regresssions

[hzxuzhonghu]

@howardjohn WDYT about dr selection？

[howardjohn]

Having SidecarScope have 2 services for 1 hostname is not acceptable imo, it violates the design. We previously had it working, so seems there must be another way

[hzxuzhonghu]

Previously, we donot use sidecar scope for gateway, should we make gateway behave differently with sidecar?

[ramaraochavali]

Previously, we donot use sidecar scope for gateway, should we make gateway behave differently with sidecar?

Is that how it worked in 1.13.2? I thought we have been using sidecar scope for gateway much before that?

[hzxuzhonghu]

Is that how it worked in 1.13.2? I thought we have been using sidecar scope for gateway much before that?

Yes, I am super sure, because I tested different commits manually. And figured it out

[hzxuzhonghu]

@howardjohn @GregHanson We need to come to a consensus on this, otherwise, my work is a waste.

[howardjohn]

SidecarScope should have 1 service per hostname. If we can keep that constraint I am open to other options, including reverting the PR that originally broke it (I believe it was mine and was meant to just be a refactoring)

[hzxuzhonghu]

TBH, it is also weird for a gateway to see multi services with same hostname in different namespaces. With that, user define a vs bound to the gateway, and traffic can go to any backend service instances across namespaces. The target can be random.

From the security view, if a namespace is owned by a different tenant, the isolation could be very weak. So I doubt how we we should do all that kind of work

[zhlsunshine]

Hi all, as mentioned in issue#41319 servicesByHostname map[host.Name]*Service can cause issue, so is it okay if consider that make the map key host.Name should be the longest hostname of the service? For example, simpleserver should not be the key, but simpleserver.default.svc.cluster.local can be. does it make sense?

[hzxuzhonghu]

Currently host.Name is a FQDN

[istio-testing]

@hzxuzhonghu: PR needs rebase.

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.