ambient upstream: upstream XDS codegen and some misc #43422

howardjohn · 2023-02-17T01:12:30Z

Closes #40879

This is the final ambient upstream PR. With this PR, ambient mesh will be fully runn-able from the master branch, a part of our regular CI/CD, release, etc. experimental-ambient branch will be deprecated and not used any longer.

This PR imports the final xDS changes, as well as some minor ports that slipped through the cracks in previous PRs.

Most of this PR is new XDS for waypoints. This code, of course, only applies to waypoints so it is off-by-default. There is some changes to sidecar HBONE as well, which is off by default already. There should be no changes for sidecar users; everything is off by default

Quoting from the issue:

Reviewer guide

It is suggested that reviewers treat these PRs differently than normal, as the code has already been reviewed. PRs should be thoroughly reviewed for risk to non-ambient users, ensuring any new/risky/expensive code is disabled by default. However, code quality comments, nit picks, or even bugs in ambient code should not block a merge; modifying these PRs will be high cost and lead to a 3-way drift from master, experimental-ambient, and the PR. Instead, these comments should be turned into issues to complete following the merge.

Release notes will not be added for any PR. Instead, they will be written holistically for the ambient feature in 1.18 at a later point.

howardjohn · 2023-02-17T01:12:36Z

/test ?

istio-testing · 2023-02-17T01:12:39Z

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

/test analyze-tests
/test gencheck
/test integ-ambient
/test integ-basic-arm64
/test integ-cni
/test integ-distroless
/test integ-ds
/test integ-helm
/test integ-ipv6
/test integ-operator-controller
/test integ-pilot
/test integ-pilot-istiodremote
/test integ-pilot-istiodremote-mc
/test integ-pilot-multicluster
/test integ-security
/test integ-security-istiodremote
/test integ-security-multicluster
/test integ-telemetry
/test integ-telemetry-istiodremote
/test integ-telemetry-mc
/test lint
/test release-notes
/test release-test
/test unit-tests
/test unit-tests-arm64

The following commands are available to trigger optional jobs:

/test benchmark
/test integ-assertion

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

analyze-tests_istio
gencheck_istio
integ-basic-arm64_istio
integ-cni_istio
integ-distroless_istio
integ-ds_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-arm64_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.

howardjohn · 2023-02-17T01:12:51Z

/test integ-ambient

howardjohn · 2023-02-17T01:13:56Z

pilot/pkg/networking/core/v1alpha3/cluster.go

 		// Setup inbound clusters
 		inboundPatcher := clusterPatcher{efw: envoyFilterPatches, pctx: networking.EnvoyFilter_SIDECAR_INBOUND}
 		clusters = append(clusters, configgen.buildInboundClusters(cb, proxy, instances, inboundPatcher)...)
+		clusters = append(clusters, configgen.buildInboundHBONEClusters(cb, proxy, instances)...)


note: this looks suspicious but we check EnableHBONE in buildInboundHBONEClusters so it does NOT impact non-HBONE users

howardjohn · 2023-02-17T01:14:37Z

/retest

howardjohn · 2023-02-17T01:20:04Z

/test integ-ambient

hzxuzhonghu · 2023-02-17T01:27:37Z

cc @ramaraochavali @costinm @stevenctl

hzxuzhonghu

Generally LG, not find a risky point.

hzxuzhonghu · 2023-02-17T07:20:05Z

pilot/pkg/bootstrap/servicecontroller.go

@@ -82,6 +82,7 @@ func (s *Server) initKubeRegistry(args *PilotArgs) (err error) {
 		args.RegistryOptions.ClusterRegistriesNamespace,
 		args.RegistryOptions.KubeOptions,
 		s.serviceEntryController,
+		s.configController,


mark to check its usage

hzxuzhonghu · 2023-02-17T07:23:40Z

pilot/pkg/config/kube/gateway/deploymentcontroller_test.go

@@ -40,6 +40,8 @@ import (

 func TestConfigureIstioGateway(t *testing.T) {
 	test.SetForTest(t, &features.EnableAmbientControllers, true)
+	// Recompute with ambient enabled
+	classInfos = getClassInfos()


This is needed per the comment to rebuild. Since SetForTest is run after it is computed

pilot/pkg/networking/core/v1alpha3/cluster.go

hzxuzhonghu · 2023-02-17T07:49:44Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

+				ts := c.TransportSocket
+				c.TransportSocket = nil
+
+				c.TransportSocketMatches = []*cluster.Cluster_TransportSocketMatch{


abstract like HboneOrPlaintextSocket

hzxuzhonghu · 2023-02-17T07:58:07Z

pilot/pkg/networking/core/v1alpha3/listener.go

+	l := builder.getListeners()
+	if builder.node.EnableHBONE() {
+		if builder.node.IsAmbient() {
+			l = append(l, outboundTunnelListener(builder.push, builder.node))


move this after L107?

I think we need to finish patch listener before we do this. May be we should add buildWaypointListeners that calls buildSidecarListeners and does this stuff

hzxuzhonghu · 2023-02-17T08:31:57Z

pilot/pkg/networking/core/v1alpha3/listener_builder.go

+	if lb.node.Type == model.Router {
+		return lb.gatewayListeners
+	}
+	nInbound, nOutbound := len(lb.inboundListeners), len(lb.outboundListeners)
+	nHTTPProxy, nVirtual := 0, 0
+	if lb.httpProxyListener != nil {
+		nHTTPProxy = 1
+	}
+	if lb.virtualOutboundListener != nil {
+		nVirtual = 1
+	}

-		listeners := make([]*listener.Listener, 0, nListener)
-		listeners = append(listeners, lb.outboundListeners...)
-		if lb.httpProxyListener != nil {
-			listeners = append(listeners, lb.httpProxyListener)
-		}
-		if lb.virtualOutboundListener != nil {
-			listeners = append(listeners, lb.virtualOutboundListener)
-		}
-		listeners = append(listeners, lb.inboundListeners...)
+	nListener := nInbound + nOutbound + nHTTPProxy + nVirtual

-		log.Debugf("Build %d listeners for node %s including %d outbound, %d http proxy, "+
-			"%d virtual outbound",
-			nListener,
-			lb.node.ID,
-			nOutbound,
-			nHTTPProxy,
-			nVirtual,
-		)
-		return listeners
+	listeners := make([]*listener.Listener, 0, nListener)
+	listeners = append(listeners, lb.outboundListeners...)
+	if lb.httpProxyListener != nil {
+		listeners = append(listeners, lb.httpProxyListener)
 	}
-
-	return lb.gatewayListeners
+	if lb.virtualOutboundListener != nil {
+		listeners = append(listeners, lb.virtualOutboundListener)
+	}
+	listeners = append(listeners, lb.inboundListeners...)
+
+	log.Debugf("Build %d listeners for node %s including %d outbound, %d http proxy, "+
+		"%d virtual outbound",
+		nListener,
+		lb.node.ID,
+		nOutbound,
+		nHTTPProxy,
+		nVirtual,
+	)


where is the func updated? Just a refactoring router first?

hzxuzhonghu · 2023-02-17T09:05:34Z

pilot/pkg/xds/endpoint_builder.go

+			workloads := findWaypoints(b.push, e)
+			if len(workloads) > 0 {
+				// TODO: load balance
+				tunnelAddress = workloads[0].String()


Will this translate multi eps with same waytpoint address?

Same waypoint yes, but the endpoints are not identical, so we cannot merge them and add weight. Each one has two addresses associated: the waypoint, and the destination (set in :authority) we want.

hzxuzhonghu · 2023-02-17T09:07:29Z

pilot/pkg/xds/endpoint_builder.go

@@ -425,10 +460,54 @@ func buildEnvoyLbEndpoint(proxyless bool, e *model.IstioEndpoint) *endpoint.LbEn
 			},
 		}
 	}
+	if dir == model.TrafficDirectionInboundVIP {


Is it possible to move these to a waypoint endpoint builder like listener and cluster?

It may be a bit hard since this buildEnvoyLbEndpoint is a common function used by all types, we just have a small bit of custom logic here

hzxuzhonghu · 2023-02-17T09:09:02Z

cc @kyessenov to take a look at the waypoint builder

ramaraochavali

Generally Looks great. Nicely done. Did not fully review the waypoint* files. We can do once this is merged. Left some comments. None of them are blockers.

pilot/pkg/networking/core/v1alpha3/cluster.go

ramaraochavali · 2023-02-17T09:51:38Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

@@ -445,6 +449,7 @@ func (cb *ClusterBuilder) buildInboundClusterForPortOrUDS(clusterPort int, bind
 	}
 	localCluster := cb.buildDefaultCluster(clusterName, clusterType, localityLbEndpoints,
 		model.TrafficDirectionInbound, instance.ServicePort, instance.Service, allInstance)
+


nit: remove

pilot/pkg/networking/util/util.go

ramaraochavali · 2023-02-17T10:29:38Z

pilot/pkg/networking/core/v1alpha3/cluster_waypoint.go

+		model.TrafficDirectionInbound, &port, nil, nil)
+
+	// Ensure VIP cluster has services metadata for stats filter usage
+	im := getOrCreateIstioMetadata(localCluster.cluster)


Should this be enabled only when telemetry is on?

TBH, not sure its going to matter. The waypoint XDS size is going to be orders of magnitude smaller than sidecars, so a bit of metadata here and there may be fine. We can followup as needed though, telemetry for waypoint is still under construction

ramaraochavali · 2023-02-17T10:42:14Z

pilot/pkg/networking/core/v1alpha3/listener.go

+	l := builder.getListeners()
+	if builder.node.EnableHBONE() {
+		if builder.node.IsAmbient() {
+			l = append(l, outboundTunnelListener(builder.push, builder.node))


I think we need to finish patch listener before we do this. May be we should add buildWaypointListeners that calls buildSidecarListeners and does this stuff

ramaraochavali · 2023-02-17T10:44:41Z

pilot/pkg/networking/core/v1alpha3/listener_builder.go

-	lb.inboundListeners = lb.buildInboundListeners()
-	if lb.node.EnableHBONE() {
-		lb.inboundListeners = append(lb.inboundListeners, lb.buildInboundHBONEListeners()...)
+	if lb.node.IsWaypointProxy() {


Don't we need HBone stuff for waypoint?

We do but its in buildWaypointInbound. We have recently improved the HBONE impl for waypoint, so its a bit different than the sidecar one. There is work in #42789 to bring these in alignment which may clean this up

ramaraochavali · 2023-02-17T13:28:39Z

pilot/pkg/xds/endpoint_builder.go

 		supportsTunnel = false
 	}

 	// For outbound case, we selectively add tunnel info if the other side supports the tunnel
-	if supportsTunnel {
+	if dir != model.TrafficDirectionInboundVIP && supportsTunnel {


parse dir only if supportsTunnel is true and move this inside if supportsTunnel?

We actually need part of it even with !supportsTunnel. Let me add dir to endpointBuilder so we don't re-parse it and clean up this logic.

ramaraochavali · 2023-02-17T13:29:45Z

pilot/pkg/xds/endpoint_builder.go

-	if supportsTunnel {
+	if dir != model.TrafficDirectionInboundVIP && supportsTunnel {
+		// Support connecting to server side waypoint proxy, if the destination has one. This is for sidecars and ingress.
+		if dir == model.TrafficDirectionOutbound && !b.proxy.IsWaypointProxy() && !b.proxy.IsAmbient() {


Can this logic be enabled only when ambient mesh is enabled? Do we have such gloabl flag? findingWaypoints etc are not needed if ambient mesh is not enabled?

This is already the case :

if !b.proxy.EnableHBONE() { supportsTunnel = false }

which is controlled by bot a global flag AND a per-proxy flag enabled

But can't HBONE be enabled independently?

costinm · 2023-02-17T15:35:53Z

pilot/pkg/config/kube/gateway/testdata/deployment/waypoint.yaml

+metadata:
+  labels:
+    gateway.istio.io/managed: istio.io-mesh-controller
+  name: namespace-istio-waypoint


Can we add a TODO list of bug to track reviewing/changing all user-visible names ?

I don't want to delay this PR - it is a merge - but the review criteria in an experimental branch is slightly different from the review on main - that's why it's experimental. I expect code has been tested and reviewed for functionality, and it is off by default - but we got into a lot of problems by
reviewing APIs and names as 'experimental/alpha' and then having to move them to beta/stable
due to backwards compat.

Yes definitely agree. Opened #43437 to track

Also to confirm - now or in future we will allow the user to create a ServiceAccount with the same name and additional annotations ? And we won't add or merge this on top ?

Can you at least add a TODO: that this needs to be documented and discussed. It has to be documented since users may need to add annotations ( for example to allow waypoint to get 3p service account tokens) and used in RBAC rules.

The plan is if the resource already exists (without gateway.istio.io/managed) we will not touch it, so they can own it themselves. Note this isn't implemented but tracked in #43439

Note since we use Apply they can add annotations and we won't revert them btw. But it is useful to have them be able to fully own it I think.

kyessenov · 2023-02-17T16:48:08Z

@hzxuzhonghu I recently changed waypoint xDS and it's fine for now. We can iterate on the VirtualService translation for it later.

stevenctl

Looks safe to me.

howardjohn · 2023-02-17T19:17:44Z

/retest

howardjohn · 2023-02-17T19:18:48Z

/test integ-ambient

costinm · 2023-02-18T15:20:21Z

We will need to enable independently - for VMs and other environments. My understanding is that can be done by setting the ambient label on a workload - i.e. a proxyless gRPC or VM or pod with 'native' or sidecar-based ambient will have the label and will get requests as hbone. I haven't tested this - and we can add it later, but it is very important for 'other environments' and migrations.

…

On Fri, Feb 17, 2023 at 8:47 PM Rama Chavali ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/xds/endpoint_builder.go <#43422 (comment)>: > supportsTunnel = false } // For outbound case, we selectively add tunnel info if the other side supports the tunnel - if supportsTunnel { + if dir != model.TrafficDirectionInboundVIP && supportsTunnel { + // Support connecting to server side waypoint proxy, if the destination has one. This is for sidecars and ingress. + if dir == model.TrafficDirectionOutbound && !b.proxy.IsWaypointProxy() && !b.proxy.IsAmbient() { But can't HBONE be enabled independently? — Reply to this email directly, view it on GitHub <#43422 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2TIKGVO6ZYZB4LIWZDWYBH5PANCNFSM6AAAAAAU63KNIE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ericvn · 2023-02-20T15:11:05Z

It does seem this PR has a big negative effect on the post-submit tests, noting that it did re-enable the ambient tests.

shashankram · 2023-02-20T17:17:56Z

pkg/config/constants/constants.go

+	ManagedGatewayLabel               = "gateway.istio.io/managed"
+	ManagedGatewayController          = "istio.io-gateway-controller"
+	ManagedGatewayMeshControllerLabel = "istio.io-mesh-controller"
+	ManagedGatewayMeshController      = "istio.io/mesh-controller"


In general, these constants should have comments, otherwise they are ambiguous to discern.

make sense, i think these may even change

howardjohn · 2023-02-20T18:02:05Z

It does seem this PR has a big negative effect on the post-submit tests, noting that it did re-enable the ambient tests.

it's a tiny change we just need to build ztunnel in more cases. I'll fix when I'm back on Tuesday

ambient upstream: upstream XDS codegen and some misc

3c161a3

howardjohn requested review from a team as code owners February 17, 2023 01:12

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

howardjohn commented Feb 17, 2023

View reviewed changes

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Feb 17, 2023

howardjohn mentioned this pull request Feb 17, 2023

Upstream experimental-ambient branches to master #40879

Closed

9 tasks

fix config

a4962cd

hanxiaop mentioned this pull request Feb 17, 2023

Fix gatewayclass not being created correctly in ambient #43430

Closed

12 tasks

hzxuzhonghu reviewed Feb 17, 2023

View reviewed changes

ramaraochavali reviewed Feb 17, 2023

View reviewed changes

costinm reviewed Feb 17, 2023

View reviewed changes

howardjohn mentioned this pull request Feb 17, 2023

ambient: review all new names/APIs #43437

Open

howardjohn added 2 commits February 17, 2023 07:44

Address comments

5894f12

lint

266a1ee

stevenctl approved these changes Feb 17, 2023

View reviewed changes

costinm approved these changes Feb 17, 2023

View reviewed changes

howardjohn mentioned this pull request Feb 17, 2023

Make ambient job required istio/test-infra#4581

Merged

istio-testing merged commit 73e9b41 into istio:master Feb 17, 2023

shashankram reviewed Feb 20, 2023

View reviewed changes

ambient upstream: upstream XDS codegen and some misc #43422

ambient upstream: upstream XDS codegen and some misc #43422

Conversation

howardjohn commented Feb 17, 2023

howardjohn commented Feb 17, 2023

istio-testing commented Feb 17, 2023

howardjohn commented Feb 17, 2023

Choose a reason for hiding this comment

howardjohn commented Feb 17, 2023

howardjohn commented Feb 17, 2023

hzxuzhonghu commented Feb 17, 2023

hzxuzhonghu 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

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

hzxuzhonghu commented Feb 17, 2023

ramaraochavali 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

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

kyessenov commented Feb 17, 2023

stevenctl left a comment

Choose a reason for hiding this comment

howardjohn commented Feb 17, 2023

howardjohn commented Feb 17, 2023

costinm commented Feb 18, 2023 via email

ericvn commented Feb 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Feb 20, 2023

ericvn commented Feb 20, 2023 •

edited