sidecar: filter service ports to VS ports #39067

ryan-smick · 2022-05-20T16:43:51Z

Please provide a description of this PR:

Fixes issue: #38725

When constructing the SidecarScope, if a sidecar listed a dependency
on a virtual service, istio will currently add a dependency for the
sidecar on each transitive dependency of that virtual service,
including all ports that each transitive dependency exposes. This
change now filters the ports on those transitive dependencies down
to only those to which the virtual service will route. This allows users
to have services that expose hundreds of ports, but set a specific
hostname for a specific subset of those ports using a virtual service,
and istio will only propagate the configuration needed to route to
that subset of ports.

@hzxuzhonghu @howardjohn
@yingzhuivy

Contins behavior changes for the propagation of envoy clusters to sidecars

When constructing the SidecarScope, if a sidecar listed a dependency on a virtual service, istio would previously add a dependency for the sidecar on each transitive dependency of that virtual service, including all ports that each transitive dependency exposes. This change now filters the ports on those transitive dependencies down to only those to which the virtual service will route. This allows users to have services that expose hundreds of ports, but set a specific hostname for a specific subset of those ports using a virtual service, and istio will only propagate the configuration needed to route to that subset of ports. Change-Id: Id138a252de7d042f84093164a504e3a934dc8cf5 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/2948 Reviewed-by: Ying Zhu <ying.zhu@airbnb.com> Reviewed-by: Weibo He <weibo.he@airbnb.com>

istio-policy-bot · 2022-05-20T16:43:54Z

😊 Welcome @ryan-smick! This is either your first contribution to the Istio istio repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

istio-testing · 2022-05-20T16:44:04Z

Hi @ryan-smick. Thanks for your PR.

I'm waiting for a istio 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.

howardjohn

Just skimmed and looks good overall, will leave to @hzxuzhonghu

howardjohn · 2022-05-20T19:41:51Z

pilot/pkg/model/sidecar.go

+	// A value of 0 in vsDestPorts is used as a sentinel to indicate a dependency
+	// on every port of the service.
+	if len(service.Ports) == 1 || len(vsDestPorts) == 0 || vsDestPorts.Contains(0) {
+		return service.DeepCopy()


Suggested change

return service.DeepCopy()

return service

Can we either not copy or comment why we need to copy? Its not clear to me its needed/why

howardjohn · 2022-05-20T19:42:50Z

pilot/pkg/model/sidecar.go

+func serviceMatchingVirtualServicePorts(service *Service, vsDestPorts sets.IntSet) *Service {
+	// A value of 0 in vsDestPorts is used as a sentinel to indicate a dependency
+	// on every port of the service.
+	if len(service.Ports) == 1 || len(vsDestPorts) == 0 || vsDestPorts.Contains(0) {


I don't think it matters much in practice, but is len(service.Ports) == 1 actually correct?

Imagine I have a Svc with port=1234 and a VS pointing to svc:5678. I think this would select the service?

Yes, this would select the service in that case. You're right, I think this is probably incorrect, and should be fine to remove. Will update

howardjohn · 2022-05-20T19:43:39Z

pilot/pkg/model/sidecar.go

+	}
+
+	if len(foundPorts) > 0 {
+		sc := service.DeepCopy()


nit: we could probably reduce a copy when we end up with len(foundPorts) == len(sc.Ports)

hzxuzhonghu · 2022-05-23T01:47:09Z

/ok-to-test

hzxuzhonghu · 2022-05-23T02:01:39Z

/test ?

istio-testing · 2022-05-23T02:01:40Z

@hzxuzhonghu: The following commands are available to trigger required jobs:

/test analyze-tests_istio
/test gencheck_istio
/test integ-cni_istio
/test integ-distroless_istio
/test integ-helm_istio
/test integ-ipv6_istio
/test integ-operator-controller_istio
/test integ-pilot-istiodremote-mc_istio
/test integ-pilot-istiodremote_istio
/test integ-pilot-multicluster_istio
/test integ-pilot_istio
/test integ-security-istiodremote_istio
/test integ-security-multicluster_istio
/test integ-security_istio
/test integ-telemetry-istiodremote_istio
/test integ-telemetry-mc_istio
/test integ-telemetry_istio
/test lint_istio
/test release-notes_istio
/test release-test_istio
/test unit-tests_istio

The following commands are available to trigger optional jobs:

/test benchmark_istio
/test integ-assertion_istio

Use /test all to run the following jobs that were automatically triggered:

analyze-tests_istio
gencheck_istio
integ-cni_istio
integ-distroless_istio
integ-helm_istio
integ-ipv6_istio
integ-operator-controller_istio
integ-pilot-istiodremote-mc_istio
integ-pilot-istiodremote_istio
integ-pilot-multicluster_istio
integ-pilot_istio
integ-security-istiodremote_istio
integ-security-multicluster_istio
integ-security_istio
integ-telemetry-istiodremote_istio
integ-telemetry-mc_istio
integ-telemetry_istio
lint_istio
release-notes_istio
release-test_istio
unit-tests_istio

In response to this:

/test ?

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.

hzxuzhonghu · 2022-05-23T02:02:09Z

/test benchmark_istio

ericvn · 2022-05-23T17:48:43Z

/retest

Remove extra unnecessary service copies when the ports we want to include for a service match all ports exposed on the service. Additionally, correct logic around selecting a service with a single port where that port may not match the VS destination port. Change-Id: I104646508a2ba6416a6a7fbd92f285d0173c93db Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3017 Reviewed-by: Douglas Jordan <douglas.jordan@airbnb.com> Reviewed-by: Ying Zhu <ying.zhu@airbnb.com>

hzxuzhonghu · 2022-05-25T02:27:26Z

/test integ-security-multicluster_istio

* sidecar: filter service ports to VS ports When constructing the SidecarScope, if a sidecar listed a dependency on a virtual service, istio would previously add a dependency for the sidecar on each transitive dependency of that virtual service, including all ports that each transitive dependency exposes. This change now filters the ports on those transitive dependencies down to only those to which the virtual service will route. This allows users to have services that expose hundreds of ports, but set a specific hostname for a specific subset of those ports using a virtual service, and istio will only propagate the configuration needed to route to that subset of ports. * sidecar: remove extraneous copies Remove extra unnecessary service copies when the ports we want to include for a service match all ports exposed on the service. Additionally, correct logic around selecting a service with a single port where that port may not match the VS destination port. Change-Id: Id98b49ece7c7bbb698d90dfcac07d8f0d95fcfd8 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3026 Reviewed-by: Ryan Smick <ryan.smick@airbnb.com>

There was a small bug in cherry-picking istio#39067. This CL correctly consumes the sets pkg. Note that sets was moved to the top level pkg in istio 1.14. Change-Id: I812a3baa2b7baed9f48eeeeb2990d0ecb47cf855 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3073 Reviewed-by: Ryan Smick <ryan.smick@airbnb.com>

* pilot: correctly consume sets pkg There was a small bug in cherry-picking istio#39067. This CL correctly consumes the sets pkg. Note that sets was moved to the top level pkg in istio 1.14. * sidecar: filter service ports to VS ports When constructing the SidecarScope, if a sidecar listed a dependency on a virtual service, istio would previously add a dependency for the sidecar on each transitive dependency of that virtual service, including all ports that each transitive dependency exposes. This change now filters the ports on those transitive dependencies down to only those to which the virtual service will route. This allows users to have services that expose hundreds of ports, but set a specific hostname for a specific subset of those ports using a virtual service, and istio will only propagate the configuration needed to route to that subset of ports. * sidecar: remove extraneous copies Remove extra unnecessary service copies when the ports we want to include for a service match all ports exposed on the service. Additionally, correct logic around selecting a service with a single port where that port may not match the VS destination port. Change-Id: I083a7addebd20c8aa4b98ba26b55454e15f8d74e Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3133 Reviewed-by: Douglas Jordan <douglas.jordan@airbnb.com>

This CL patches commit 37ce3a5 from upstream istio into air-release-1.14.3 to fix the redis ACA issue. Original PR can be found here: istio#39067. The description from that PR is reproduced below. When constructing the SidecarScope, if a sidecar listed a dependency on a virtual service, istio will currently add a dependency for the sidecar on each transitive dependency of that virtual service, including all ports that each transitive dependency exposes. This change now filters the ports on those transitive dependencies down to only those to which the virtual service will route. This allows users to have services that expose hundreds of ports, but set a specific hostname for a specific subset of those ports using a virtual service, and istio will only propagate the configuration needed to route to that subset of ports. Change-Id: I9965a8a4c2d2921382310b721de545b4972b1aca Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3506 Reviewed-by: Weibo He <weibo.he@airbnb.com> Reviewed-by: Ying Zhu <ying.zhu@airbnb.com>

Apply the following list of patches to istio 1.14.4: * sidecar: filter service ports to VS ports (istio#39067) * istio: register init push context metric (istio#40049) * istio: add metric for debouncing (istio#40523) * istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719) * istio: support inline multi-values header in authz header match (https://gerrit.musta.ch/c/public/istio/+/3622, not yet merged upstream) Change-Id: I69b861d884608dabad44db1fee03f66eb4b25ab2 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3695 Reviewed-by: Ying Zhu <ying.zhu@airbnb.com> Reviewed-by: Weibo He <weibo.he@airbnb.com>

Apply the following list of patches to istio 1.14.5: * sidecar: filter service ports to VS ports (istio#39067) * istio: register init push context metric (istio#40049) * istio: add metric for debouncing (istio#40523) * istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719) * istio: support inline multi-values header in authz header match (https://gerrit.musta.ch/c/public/istio/+/3622, not yet merged upstream) * istio: improve deep copy for ServiceAttribute (istio#40966) * avoid unnecessary copy virtual services for sidecar scope calculation (istio#41101) Change-Id: Ia4c9bfd619a0eb38c1a829bff2efbd21fd3b9cb2 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3883 Reviewed-by: Ying Zhu <ying.zhu@airbnb.com> Reviewed-by: Weibo He <weibo.he@airbnb.com>

ryan-smick requested review from a team, costinm, linsun, howardjohn, nmittler and ericvn as code owners May 20, 2022 16:43

istio-policy-bot added area/networking area/perf and scalability release-notes-none Indicates a PR that does not require release notes. labels May 20, 2022

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels May 20, 2022

howardjohn reviewed May 20, 2022

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels May 23, 2022

hzxuzhonghu approved these changes May 24, 2022

View reviewed changes

istio-testing merged commit 37ce3a5 into istio:master May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

sidecar: filter service ports to VS ports #39067

sidecar: filter service ports to VS ports #39067

Uh oh!

ryan-smick commented May 20, 2022 •

edited

Loading

Uh oh!

istio-policy-bot commented May 20, 2022

Uh oh!

istio-testing commented May 20, 2022

Uh oh!

howardjohn left a comment

Uh oh!

howardjohn May 20, 2022

Uh oh!

howardjohn May 20, 2022

Uh oh!

ryan-smick May 23, 2022

Uh oh!

howardjohn May 20, 2022

Uh oh!

hzxuzhonghu commented May 23, 2022

Uh oh!

hzxuzhonghu commented May 23, 2022

Uh oh!

istio-testing commented May 23, 2022

Uh oh!

hzxuzhonghu commented May 23, 2022

Uh oh!

ericvn commented May 23, 2022

Uh oh!

hzxuzhonghu commented May 25, 2022

Uh oh!

Uh oh!

sidecar: filter service ports to VS ports #39067

sidecar: filter service ports to VS ports #39067

Uh oh!

Conversation

ryan-smick commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-policy-bot commented May 20, 2022

Uh oh!

istio-testing commented May 20, 2022

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn May 20, 2022

Choose a reason for hiding this comment

Uh oh!

howardjohn May 20, 2022

Choose a reason for hiding this comment

Uh oh!

ryan-smick May 23, 2022

Choose a reason for hiding this comment

Uh oh!

howardjohn May 20, 2022

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented May 23, 2022

Uh oh!

hzxuzhonghu commented May 23, 2022

Uh oh!

istio-testing commented May 23, 2022

Uh oh!

hzxuzhonghu commented May 23, 2022

Uh oh!

ericvn commented May 23, 2022

Uh oh!

hzxuzhonghu commented May 25, 2022

Uh oh!

Uh oh!

ryan-smick commented May 20, 2022 •

edited

Loading