ambient: service entry initial impl #45621

GregHanson · 2023-06-23T14:49:36Z

Adds support for a subset of the service entry api to ambient mesh. The api supported is as follows:

addresses (the VIPS)
auto VIP address allocation (addresses is optional)
endpoints (static only)
location (prefer mesh external for now for DIRECT passthrough in ztunnel)
workloadSelector (selects pods and workload entries)

notable exclusions:

hosts (coming per Initial DNS proxy implementation ztunnel#536)
exportTo
resolution (everything is static)
subjectAltNames (pending ztunnel support)

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

Adds support for a subset of the service entry api to ambient mesh. The api supported is as follows: - addresses (the VIPS) - auto VIP address allocation (addresses is optional) - endpoints (static only) - location (prefer mesh external for now for DIRECT passthrough in ztunnel) - workloadSelector (selects pods and workload entries) notable exclusions: - hosts (coming per istio/ztunnel#536) - exportTo - resolution (everything is static) - subjectAltNames (pending ztunnel support) Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

wulianglongrd · 2023-06-25T12:56:53Z

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

+					c.ambientIndex.byPod[networkAddr] = newWl
+				}
+				c.ambientIndex.byUID[c.generatePodUID(pod)] = newWl
+				updates[model.ConfigKey{Kind: kind.Address, Name: newWl.ResourceName()}] = struct{}{}


Insert should be used here

and L480 L492

addressed in 946171f

kdorosh · 2023-06-28T17:26:32Z

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

+				if m.serviceEntryController != nil {
+					m.serviceEntryController.AppendServiceHandler(kubeRegistry.ServiceEntryHandler)
+				}


i think we need to append this service entry handler regardless of features.EnableK8SServiceSelectWorkloadEntries (see line 247)

addressed in 7055f90

kdorosh · 2023-06-28T17:27:39Z

tests/integration/ambient/baseline_test.go

+	framework.NewTest(t).
+		Features("traffic.ambient").
+		Run(func(t framework.TestContext) {
+			t.Skip("this will work if we set DNS_AUTO_ALLOCATE=true and once we have https://github.com/istio/ztunnel/pull/536")


now that this PR has merged we should be able to get the hostname resolved trivially (maybe a small CNI change to ensure DNS udp+tcp traffic is captured by ztunnel?)

tested this locally. it worked with some minor changes to ztunnel, is sort of blocked by istio/ztunnel#582 or other ztunnel work for now

kdorosh · 2023-06-28T17:28:32Z

tests/integration/ambient/baseline_test.go

+								// RequestHeader=Accept:*/*
+								// RequestHeader=User-Agent:curl/7.81.0
+								// Hostname=uncaptured-v1-868c9b59b5-rxvfq
+								Check: check.BodyContains(`Hostname=uncaptured-v`),


nit; stronger assertion to have both v1 +v2 checks serially to ensure we are load balancing

kdorosh · 2023-06-28T17:29:00Z

tests/integration/ambient/baseline_test.go

+						// RequestHeader=Accept:*/*
+						// RequestHeader=User-Agent:curl/7.81.0
+						// Hostname=uncaptured-v1-868c9b59b5-rxvfq
+						Check: check.BodyContains(`Hostname=uncaptured-v`),


nit; stronger assertion to have both v1 +v2 checks serially to ensure we are load balancing

kdorosh · 2023-06-28T17:36:03Z

pilot/pkg/model/service.go

+	// The service entry spec (if any) this service was derived from.
+	ServiceEntry *networking.ServiceEntry


this part in particular is the most awkward part of this PR; open to other ideas on how to integrate with existing VIP auto-allocation and also allow access to the service entry fields we need in the ambient index. @howardjohn for any ideas

another idea im having. create a new handler interface and pass the original SE along

istio/pilot/pkg/serviceregistry/serviceentry/controller.go

Lines 356 to 357 in 8ba3e35

currentServiceEntry := curr.Spec.(*networking.ServiceEntry)

cs := convertServices(curr)

with the converted service (unchanged in shape) cc @stevenctl

ah unfortunately this only works in one of the two places we need this.. the other one is during VIP re-auto allocation where we don't have the parent SEs. this is why I added this to the Service itself

I will hopefully come back to this later with a more productive comment but my initial reaction is pretty negative to this

After looking through things I don't have any obvious suggestions but I feel highly uncomfortable with this.

We were already maxing out our tech debt in the ambient controller v1. Then we added WE support and added a lot more (basically 2x). Now SE doubles that again. And all of that is mostly fine since its all isolated to the mess of ambientindex.go -- but I feel very uncomfortable having this leak out into our fundamental core types that risks permeating through the rest of Istio

yeah i also was not a fan, was hoping you'd have a concrete idea or suggestion. I suppose I could just switch this to use a k8s controller and mirror the way we do it for WEs; and we can punt for now on auto allocation in ambient. my biggest concern there being, if and when we do want to support it, we may need to refactor a fair bit. in fact, i already feel that way; it feels more natural to reuse model.Service in ambient index if we can rather than treat SE and k8s services differently

how do you feel about the mergeability of this PR without the vip auto allocation in mind? do you think we will definitely want to support that at some point in the future? (I know personally I don't use it often but wanted to plan to support for backwards compat)

I do think the whole thing needs a rewrite -- which I have something in mind for (#45426 for ambient only, https://docs.google.com/document/d/1-ywpCnOfubqg7WAXSPf4YgbaFDBEU9HIqMWcxZLhzwE/edit for all of Istio) but I don't want to block on that. I think its plausible we could get something reasonable in the next 3 months or so - I don't want to block anything on it but I don't mind a niche feature like auto allocation waiting

it would probably be good long term to support auto_allocation but it is not GA I think so its not necessarily strictly required. I personally think its a bad idea and we should kill it (#43952 (comment)).

I also wouldn't mind if it was just inefficient for now.

I guess in order of preference:

Remove the need for this and have no auto alloc support for now

Remove the need for this and have inefficient auto alloc support for now

Just do it anyways and feel bad about it

I can live with any of them

One thing I was thinking is to make the handler pass Service+ServiceEntry but its kind of hairy to pipe that through the whole serviceentry controller

decided to just remove auto vip allocation per discussion above in 37e605d

kdorosh · 2023-06-28T17:38:18Z

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

+		idx.mu.Lock()
+		defer idx.mu.Unlock()


nit; move this lock below the nil check on line 420

addressed in 2473dc2

kdorosh · 2023-06-28T17:44:53Z

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

+	for _, svc := range c.ambientIndex.servicesMap {
+
+		if svc.Attributes.ServiceEntry == nil {
+			// if we are here then this is dev error
+			log.Warn("dev error: service entry spec is nil; it should have been populated by the service entry handler")
+			continue
+		}
+
+		if svc.Attributes.ServiceEntry.WorkloadSelector == nil {
+			// nothing to do. we construct the ztunnel config if `endpoints` are provided in the service entry handler
+			continue
+		}
+
+		if svc.Attributes.ServiceEntry.Endpoints != nil {
+			// it is an error to provide both `endpoints` and `workloadSelector` in a service entry
+			continue
+		}
+
+		sel := svc.Attributes.ServiceEntry.WorkloadSelector.Labels
+		if !labels.Instance(sel).SubsetOf(pod.Labels) {
+			continue
+		}
+
+		vipsToPorts := getVIPsFromServiceEntry(svc, nil)
+		for vip, ports := range vipsToPorts {
+			vips[vip] = ports
+		}
+	}


this should be DRY'ed with the other implementation (which is more correct, since it has the necessary vip := c.network.String() + "/" + vip // services must be on our network, don't inherit from workload lines)

addressed in 5a0ee02

kdorosh · 2023-06-28T17:48:03Z

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

+		vipsToPorts := getVIPsFromServiceEntry(oldServiceEntry, nil)
+		for vip := range vipsToPorts {
+			// we also need to remove the VIP from any pod that has it (e.g. from service entry `workloadSelector`)
+			for nwAddr := range a.byPod {


we also need to add this logic for standalone WEs that are just selected by SEs. we should update the unit tests to cover this case

the unit tests actually already covered this scenario, and this code can be removed since pod cleanup is handled during re-construction of the pod workloads on SE update. removed dead code in 0eb683e

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

…rkloadEntries setting Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

stevenctl · 2023-07-05T22:01:52Z

pilot/pkg/serviceregistry/serviceentry/controller.go

@@ -617,7 +627,9 @@ func (s *Controller) Cluster() cluster.ID {
 }

 // AppendServiceHandler adds service resource event handler. Service Entries does not use these handlers.
-func (s *Controller) AppendServiceHandler(_ model.ServiceHandler) {}
+func (s *Controller) AppendServiceHandler(h model.ServiceHandler) {


istio/pilot/pkg/model/controller.go

Line 67 in c8fdbc7

type ControllerHandlers struct {

We have a util class to dedupe most of the handler stuff.

this was necessary so the service entry controller could fire events on new auto vip allocation

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

stevenctl · 2023-07-05T22:15:22Z

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

+			}
+		}
+
+		workloadEntries := c.getAllControllerWorkloadEntries()


I don't really understand this logic. Shouldn't it be more like getWorkloadEntriesInService, possibly making that func reusable with labels.Selector?

Same with pods.

addressed in eb05c10

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh · 2023-07-06T17:42:43Z

@howardjohn PTAL

howardjohn · 2023-07-06T20:59:15Z

pilot/pkg/model/service.go

+	// The service entry spec (if any) this service was derived from.
+	ServiceEntry *networking.ServiceEntry


I will hopefully come back to this later with a more productive comment but my initial reaction is pretty negative to this

howardjohn · 2023-07-06T21:32:38Z

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

@@ -340,7 +354,7 @@ func (a *AmbientIndexImpl) extractWorkload(p *v1.Pod, c *Controller) *model.Work

 	policies := c.selectorAuthorizationPolicies(p.Namespace, p.Labels)
 	policies = append(policies, c.convertedSelectorPeerAuthentications(p.Namespace, p.Labels)...)
-	wl := c.constructWorkload(p, waypoint, policies)
+	wl := c.constructWorkload(p, waypoint, policies, a.servicesMap)


nit: we could just move constructWorkload to ambientIndex and we don't need to pass a.servicesMap since it can access it directly?

addressed in 99b26b2

howardjohn · 2023-07-06T21:34:27Z

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

+	}
+}
+
+func getWorkloadServices(serviceEntries map[types.NamespacedName]*model.Service, workloadEntry *v1alpha3.WorkloadEntry,


nit: getWorkloadServiceEntries may be more clear?

addressed in 04b7435

howardjohn · 2023-07-06T21:38:59Z

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

+		a.servicesMap[serviceEntryNamespacedName] = svc
+	}
+
+	sel := klabels.Set(svc.Attributes.ServiceEntry.WorkloadSelector.GetLabels()).AsSelectorPreValidated()


nit: us ValidatedSetSelector instead, its (much) faster

addressed in b632775

howardjohn · 2023-07-06T21:46:52Z

pilot/pkg/model/service.go

+	// The service entry spec (if any) this service was derived from.
+	ServiceEntry *networking.ServiceEntry


After looking through things I don't have any obvious suggestions but I feel highly uncomfortable with this.

We were already maxing out our tech debt in the ambient controller v1. Then we added WE support and added a lot more (basically 2x). Now SE doubles that again. And all of that is mostly fine since its all isolated to the mess of ambientindex.go -- but I feel very uncomfortable having this leak out into our fundamental core types that risks permeating through the rest of Istio

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh · 2023-07-06T23:17:50Z

addressed all feedback again; most importantly, I removed the auto vip allocation support since it was the root cause of tech debt escaping into core istio types (model.Service)

PTAL @howardjohn

kdorosh · 2023-07-06T23:23:08Z

/test unit-tests

GregHanson requested review from a team as code owners June 23, 2023 14:49

istio-policy-bot added the area/ambient Issues related to ambient mesh label Jun 23, 2023

istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 23, 2023

GregHanson added 2 commits June 23, 2023 15:38

update more tests to use common functions

74cb6ed

add release note

11781d2

GregHanson mentioned this pull request Jun 23, 2023

ambient: service entry initial impl kdorosh/istio#1

Closed

12 tasks

wulianglongrd reviewed Jun 25, 2023

View reviewed changes

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

kdorosh reviewed Jun 28, 2023

View reviewed changes

This was referenced Jun 28, 2023

ambient: support multicluster VMs #45718

Closed

ambient: add support for workload entries #45471

Merged

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 30, 2023

kdorosh force-pushed the ambient-service-entries branch from a3d6589 to 7858ff5 Compare June 30, 2023 21:16

Kevin Dorosh added 2 commits June 30, 2023 17:43

Prefer updates.Insert()

946171f

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Move lock lower so there's less lock contention

2473dc2

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh force-pushed the ambient-service-entries branch from 7858ff5 to 2473dc2 Compare June 30, 2023 21:43

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 30, 2023

Kevin Dorosh added 2 commits June 30, 2023 17:47

Ambient SE handler should work regardless of EnableK8SServiceSelectWo…

7055f90

…rkloadEntries setting Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Merge branch 'master' into ambient-service-entries

e6672a0

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 3, 2023

stevenctl reviewed Jul 5, 2023

View reviewed changes

pilot/pkg/serviceregistry/kube/controller/multicluster.go Outdated Show resolved Hide resolved

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 5, 2023

stevenctl reviewed Jul 5, 2023

View reviewed changes

Merge branch 'master' into ambient-service-entries

ce14466

kdorosh force-pushed the ambient-service-entries branch from 3aa0e6e to ce14466 Compare July 6, 2023 14:12

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 6, 2023

Ensure service entries cannot select across namespaces

eb05c10

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh force-pushed the ambient-service-entries branch from 3f7840c to 001c2c6 Compare July 6, 2023 15:11

Remove customizations for skipped test

bef3d72

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh force-pushed the ambient-service-entries branch from 001c2c6 to bef3d72 Compare July 6, 2023 15:13

Kevin Dorosh added 4 commits July 6, 2023 11:45

Optimize pod xds events if not selected by service entry

eaac012

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Optimize workload entry xds events if not selected by service entry

02882ed

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Move logic for HandleServiceEntry into AmbientIndex interface

707f66f

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Don't pass around entire ambient index

13d1862

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh requested a review from howardjohn July 6, 2023 17:42

howardjohn reviewed Jul 6, 2023

View reviewed changes

Kevin Dorosh added 4 commits July 6, 2023 18:01

Prefer ValidatedSetSelector for performance

b632775

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Rename function to be clearer

04b7435

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Don't pass around ambient index child

99b26b2

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

Remove auto vip allocation

37e605d

Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>

kdorosh force-pushed the ambient-service-entries branch from 12744d3 to 37e605d Compare July 6, 2023 23:12

howardjohn approved these changes Jul 7, 2023

View reviewed changes

kdorosh mentioned this pull request Jul 7, 2023

Refactor ambient tests #45797

Merged

istio-testing merged commit 7d6139b into istio:master Jul 7, 2023
27 checks passed

kdorosh deleted the ambient-service-entries branch July 7, 2023 16:19

		// The service entry spec (if any) this service was derived from.
		ServiceEntry *networking.ServiceEntry

	currentServiceEntry := curr.Spec.(*networking.ServiceEntry)
	cs := convertServices(curr)

ambient: service entry initial impl #45621

ambient: service entry initial impl #45621

Conversation

GregHanson commented Jun 23, 2023 • edited by istio-policy-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdorosh Jun 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdorosh Jun 28, 2023 • edited

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

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

kdorosh Jun 28, 2023 • edited

Choose a reason for hiding this comment

kdorosh Jun 30, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdorosh commented Jul 6, 2023

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

kdorosh commented Jul 6, 2023

kdorosh commented Jul 6, 2023

GregHanson commented Jun 23, 2023 •

edited by istio-policy-bot

kdorosh Jun 30, 2023 •

edited

kdorosh Jun 28, 2023 •

edited

kdorosh Jun 28, 2023 •

edited

kdorosh Jun 30, 2023 •

edited