ambient: match multiple svc VIPs in waypoint #51939

jewertow · 2024-07-08T13:17:50Z

Please provide a description of this PR:

Istio will store all cluster VIPs for service entries, but these VIPs will be applied only to waypoint listeners for now - I will fix sidecars in a follow-up PR.

istio-testing · 2024-07-08T13:17:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jewertow · 2024-07-08T13:18:02Z

/test all

jewertow · 2024-07-08T13:18:26Z

/test integ-ambient-ipv6

jewertow · 2024-07-08T14:04:41Z

/test integ-ambient-ipv6

jewertow · 2024-07-08T14:16:57Z

/test integ-ambient

Signed-off-by: Jacek Ewertowski <[email protected]>

…ode enabled Signed-off-by: Jacek Ewertowski <[email protected]>

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow · 2024-07-08T14:30:10Z

/test all

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow · 2024-07-08T14:48:54Z

/test integ-ambient-ipv6

jewertow · 2024-07-08T15:21:56Z

/test integ-ambient-ipv6

howardjohn

LGTM. Lets just change to sort and this is good to go

howardjohn · 2024-07-08T16:51:34Z

pilot/pkg/serviceregistry/serviceentry/conversion.go

+		// between using DefaultAddress or ClusterVIPs[0] to create a listener.
+		notDefaultAddresses := sets.New[string](addresses...).Delete(ha.address)
+		addresses = []string{ha.address}
+		addresses = append(addresses, notDefaultAddresses.UnsortedList()...)


We should probably use a sorted list for stability. If we shuffle it everytime it can lead to constant XDS changes which is expensive. The list is virtually guaranteed to be short, so this should be acceptable performance

Ah, right. Thanks!

howardjohn · 2024-07-08T16:53:59Z

tests/integration/ambient/waypoint_test.go

@@ -381,20  381,26 @@ func TestWaypointDNS(t *testing.T) {
 				if src.Config().HasSidecar() {
 					t.Skip("TODO: sidecars don't properly handle use-waypoint")
 				}
+				address := "240.240.240.239"
+				if _, v6 := getSupportedIPFamilies(t); v6 {


I think we can unconditionally put both. Did you see issues?

I didn't try using both.

I think that putting both addresses can't work, because Address is used directly to build the URL, so it won't work with multiple addresses. I tried putting both to be sure that Go can't handle it and it was failing.

howardjohn · 2024-07-08T16:54:50Z

tests/integration/ambient/waypoint_test.go

@@ -381,20  381,26 @@ func TestWaypointDNS(t *testing.T) {
 				if src.Config().HasSidecar() {
 					t.Skip("TODO: sidecars don't properly handle use-waypoint")
 				}
+				address := "240.240.240.239"


We can also fixup baseline_test L1742 and L1913

I will look at those tests.

howardjohn

I'll approve, I put a hold for the sorting thing -- feel free to remove once addressed. Thanks!!

Signed-off-by: Jacek Ewertowski <[email protected]>

hzxuzhonghu · 2024-07-09T02:02:38Z

pilot/pkg/model/service.go

@@ -1275,7  1275,7 @@ func (s *Service) GetExtraAddressesForProxy(node *Proxy) []string {
 // GetAllAddressesForProxy returns a k8s service's extra addresses to the cluster where the node resides.
 // Especially for dual stack k8s service to get other IP family addresses.
 func (s *Service) GetAllAddressesForProxy(node *Proxy) []string {
-	if features.EnableDualStack && node.Metadata != nil && node.Metadata.ClusterID != "" {
+	if (features.EnableDualStack || features.EnableAmbient) && node.Metadata != nil && node.Metadata.ClusterID != "" {


features.EnableAmbient can violates EnableDualStack setting?

I would rather say that dual-stack is enabled by default in ambient mode.

It is a env, why cannot user turn it off

I don't know, I didn't add that condition.

howardjohn · 2024-07-09T09:12:12Z

1 to on by default in ambient (as we have done in other areas for dual stack)

…

On Tue, Jul 9, 2024, 4:14 AM Jacek Ewertowski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/model/service.go <#51939 (comment)>: > @@ -1275,7 1275,7 @@ func (s *Service) GetExtraAddressesForProxy(node *Proxy) []string { // GetAllAddressesForProxy returns a k8s service's extra addresses to the cluster where the node resides. // Especially for dual stack k8s service to get other IP family addresses. func (s *Service) GetAllAddressesForProxy(node *Proxy) []string { - if features.EnableDualStack && node.Metadata != nil && node.Metadata.ClusterID != "" { if (features.EnableDualStack || features.EnableAmbient) && node.Metadata != nil && node.Metadata.ClusterID != "" { I would rather say that dual-stack is enabled by default in ambient mode. — Reply to this email directly, view it on GitHub <#51939 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKAX65ZJO72H5AIAKDZLOLXXAVCNFSM6AAAAABKQ2QAWGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRVGQZDQMZRG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

Signed-off-by: Jacek Ewertowski <[email protected]>

…stener (#52283) * networking: match multiple VIPs in sidecar outbound listener (#51967) * networking: match multiple addresses in sidecar outbound listener Signed-off-by: Jacek Ewertowski <[email protected]> * Add unit tests for GetAllAddressesForProxy Signed-off-by: Jacek Ewertowski <[email protected]> * Refactor buildSidecarOutboundListener Signed-off-by: Jacek Ewertowski <[email protected]> * Add an integration test Signed-off-by: Jacek Ewertowski <[email protected]> * Add a release note Signed-off-by: Jacek Ewertowski <[email protected]> * Refactor GetAllAddressesForProxy Signed-off-by: Jacek Ewertowski <[email protected]> * Fix linter error Signed-off-by: Jacek Ewertowski <[email protected]> * Revert removal svcExtraListenAddresses variable Signed-off-by: Jacek Ewertowski <[email protected]> * Fix unit tests Signed-off-by: Jacek Ewertowski <[email protected]> * Refactoring Signed-off-by: Jacek Ewertowski <[email protected]> * Fix TestEDSOverlapping Signed-off-by: Jacek Ewertowski <[email protected]> * Skip testServiceEntryWithMultipleVIPs in ambient mode Signed-off-by: Jacek Ewertowski <[email protected]> * Fix TestEDSUnhealthyEndpoints Signed-off-by: Jacek Ewertowski <[email protected]> * Add test case for ServiceEntry with resolution NONE and multiple VIPs Signed-off-by: Jacek Ewertowski <[email protected]> * Fix lint error Signed-off-by: Jacek Ewertowski <[email protected]> * Fix linter errors Signed-off-by: Jacek Ewertowski <[email protected]> * Fix lint error Signed-off-by: Jacek Ewertowski <[email protected]> * Generate service with all ports Signed-off-by: Jacek Ewertowski <[email protected]> * do not create service instance for each hostname/address pair Signed-off-by: Jacek Ewertowski <[email protected]> * Refactor GetExtraAddressesForProxy and revert removal of its usage from buildSidecarOutboundListener Signed-off-by: Jacek Ewertowski <[email protected]> * Fix listenerBindings.Extra() Signed-off-by: Jacek Ewertowski <[email protected]> * Handle IPv6 prefix length Signed-off-by: Jacek Ewertowski <[email protected]> * Check address' family only if a proxy supports given family Signed-off-by: Jacek Ewertowski <[email protected]> * Update a comment Signed-off-by: Jacek Ewertowski <[email protected]> * Refactor getAllAddressesForProxy Signed-off-by: Jacek Ewertowski <[email protected]> --------- Signed-off-by: Jacek Ewertowski <[email protected]> * Apply missing changes from #51939 Signed-off-by: Jacek Ewertowski <[email protected]> * networking: fix DNS auto-allocation (#52147) * Fix returning auto-allocated address Signed-off-by: Jacek Ewertowski <[email protected]> * Add an integration test Signed-off-by: Jacek Ewertowski <[email protected]> * Add missing build tag and license to dns_auto_allocation_test.go Signed-off-by: Jacek Ewertowski <[email protected]> * Fix unit tests Signed-off-by: Jacek Ewertowski <[email protected]> * Add workload selector to ProxyConfig Signed-off-by: Jacek Ewertowski <[email protected]> * Trigger injection if ProxyConfig was not propagated Signed-off-by: Jacek Ewertowski <[email protected]> --------- Signed-off-by: Jacek Ewertowski <[email protected]> * Remove leftovers from conflict resolution Signed-off-by: Jacek Ewertowski <[email protected]> --------- Signed-off-by: Jacek Ewertowski <[email protected]>

* upstream/master: Concurrent xds cache benchmark (istio#51958) Fix TestController_ServiceWithChangingDiscoveryNamespaces flake (istio#51940) Allow setting seccomp profile type (e.g. `RuntimeDefault`) for waypoints (istio#51826) Automator: update proxy@master in istio/istio@master (istio#51931) istioctl - improve use of fancy icons (istio#51926) Automator: update common-files@master in istio/istio@master (istio#51961) Bump certifi from 2024.2.2 to 2024.7.4 in /samples/helloworld/src (istio#51949) Speedup isntall of multiple suites by up to 30s (istio#51948) ambient: add tests for components restarting (istio#51710) Enable dualStack cfg for istiod control plane in dualStack tests (istio#51844) Skip test setup for CNI skew test when we skip all tests (istio#51744) validation: add a few missed duration validations (istio#51946) Bump certifi in /samples/bookinfo/src/productpage (istio#51925) ambient: match multiple svc VIPs in waypoint (istio#51939) configure tls inspector initial buffer size (istio#51928) Be a bit more consistent with annotations in zt chart (istio#51692) update kiali addon to 1.87 (istio#51941) TestReachability: prune redundant test cases (istio#51924) Drop redundant test suite in security sds_ingress (istio#51923) Fix FuzzShallowcopyTrafficPolicy (istio#51916)

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 8, 2024

istio-policy-bot added area/dual-stack area/networking labels Jul 8, 2024

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 8, 2024

jewertow added 8 commits July 8, 2024 16:26

networking: match multiple IPs for one service in waypoint

72ac851

Signed-off-by: Jacek Ewertowski <[email protected]>

Fix unit tests

d62adb3

Signed-off-by: Jacek Ewertowski <[email protected]>

Always set cluster VIPs

32162fe

Signed-off-by: Jacek Ewertowski <[email protected]>

Refactor convertServices and remove buildServices

6f3d9c3

Signed-off-by: Jacek Ewertowski <[email protected]>

Return dual-stack addresses from GetAllAddressesForProxy if ambient m…

ff854eb

…ode enabled Signed-off-by: Jacek Ewertowski <[email protected]>

Enable TestWaypointDNS

9295120

Signed-off-by: Jacek Ewertowski <[email protected]>

Fix TestWaypointDNS

7f31c5e

Signed-off-by: Jacek Ewertowski <[email protected]>

Fix lint errors

391a4d1

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow force-pushed the match-multiple-svc-ips-in-waypoint branch from f162c2a to 391a4d1 Compare July 8, 2024 14:27

Add a release note

275e92b

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow added 2 commits July 8, 2024 16:31

Refactor TestWaypointDNS

7dd9af3

Signed-off-by: Jacek Ewertowski <[email protected]>

Fix the release note

b617fcd

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow changed the title ~~networking: match multiple svc VIPs in waypoint~~ ambient: match multiple svc VIPs in waypoint Jul 8, 2024

Refactor creating IP ranges

1774396

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow marked this pull request as ready for review July 8, 2024 15:16

jewertow requested review from a team as code owners July 8, 2024 15:16

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 8, 2024

jewertow requested a review from howardjohn July 8, 2024 15:16

howardjohn reviewed Jul 8, 2024

View reviewed changes

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jul 8, 2024

howardjohn approved these changes Jul 8, 2024

View reviewed changes

Sort service addresses

02b4792

Signed-off-by: Jacek Ewertowski <[email protected]>

jewertow removed the do-not-merge/hold Block automatic merging of a PR. label Jul 8, 2024

istio-testing merged commit 12c8284 into istio:master Jul 8, 2024
28 checks passed

howardjohn mentioned this pull request Jul 8, 2024

ambient: make waypoints handle dual stack services #51913

Closed

hzxuzhonghu reviewed Jul 9, 2024

View reviewed changes

This was referenced Jul 16, 2024

networking: match multiple VIPs in sidecar outbound listener #51967

Merged

networking: fix DNS auto-allocation #52147

Merged

tests: use single ServiceEntry with multiple addresses in ambient tests #52188

Merged

jewertow added a commit to jewertow/upstream-istio that referenced this pull request Jul 23, 2024

Apply missing changes from istio#51939

7e5daef

Signed-off-by: Jacek Ewertowski <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ambient: match multiple svc VIPs in waypoint #51939

ambient: match multiple svc VIPs in waypoint #51939

jewertow commented Jul 8, 2024 •

edited

Loading

istio-testing commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

howardjohn left a comment

howardjohn Jul 8, 2024

jewertow Jul 8, 2024

howardjohn Jul 8, 2024

jewertow Jul 8, 2024

jewertow Jul 19, 2024

howardjohn Jul 8, 2024

jewertow Jul 8, 2024

howardjohn left a comment

hzxuzhonghu Jul 9, 2024

jewertow Jul 9, 2024

hzxuzhonghu Jul 9, 2024

jewertow Jul 10, 2024

howardjohn commented Jul 9, 2024 via email

ambient: match multiple svc VIPs in waypoint #51939

ambient: match multiple svc VIPs in waypoint #51939

Conversation

jewertow commented Jul 8, 2024 • edited Loading

istio-testing commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

jewertow commented Jul 8, 2024

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

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

howardjohn commented Jul 9, 2024 via email

jewertow commented Jul 8, 2024 •

edited

Loading