Support ServiceInternalTrafficPolicy #42551

dddddai · 2022-12-22T11:49:32Z

Please provide a description of this PR:
Fixes #42377

NOTE: This PR does not take ProxyTerminatingEndpoints into account

howardjohn · 2022-12-22T16:41:22Z

NOTE: This PR does not take ProxyTerminatingEndpoints into account

FWIW after looking at it more I think that feature is orthogonal

pilot/pkg/model/service.go

howardjohn · 2022-12-22T19:56:37Z

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

-		// As this may contain node topology labels, which could not be got from aggregator controller
-		out[model.LocalityLabel] = locality
-		return out
+		// set k8s node name label, for ServiceInternalTrafficPolicy


I think this should be in AugmentLabels

For endpoint labels we can use AugmentLabels (updated), but for proxy labels I'm not sure how, is there any other place to get the k8s node?

This is mutating the underlying pod labels

can set it in setTopologyLabels

This is mutating the underlying pod labels

Thanks for reminding, fixed

can set it in setTopologyLabels

I think we have to set the host label here, as we can't get it elsewhere

Why not AugmentLabels then fetch it like setTopologyLabels ( proxy.ServiceInstances[0].Endpoint.Locality.Node)

Actually we can add NODE_NAME to the injection template (downward API) and get it from there 99% of the time. And fallback to ^ when its not there.

The experimental-ambient branch already does this if you would like a reference

pilot/pkg/xds/endpoint_builder.go

dddddai · 2022-12-23T01:03:37Z

FWIW after looking at it more I think that feature is orthogonal

Yeah I think it also applies to internalTrafficPolicy: Cluster. It's a bit confusing that the user-facing change says:

When enabled, kube-proxy will attempt to route traffic to terminating pods when the traffic policy is Local and there are only terminating pods remaining on a node.

hzxuzhonghu · 2023-01-03T08:31:18Z

pilot/pkg/model/service.go

@@ -129,6 +129,8 @@ const (
 	Passthrough
 	// DNSRoundRobinLB implies that the proxy will resolve a DNS address and forward to the resolved address
 	DNSRoundRobinLB
+	// NodeLocal implies that the proxy will only forward to node local endpoints
+	NodeLocal


Using resolution is a little hacky,

Any suggestions?

how about add a NodeLocal bool field in ServiceAttributes

hzxuzhonghu

LGTM

howardjohn

Main implementation lgtm, just think we can improve how we get the node by passing it over XDS.

howardjohn · 2023-01-04T21:49:51Z

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

-		// As this may contain node topology labels, which could not be got from aggregator controller
-		out[model.LocalityLabel] = locality
-		return out
+		// set k8s node name label, for ServiceInternalTrafficPolicy


Why not AugmentLabels then fetch it like setTopologyLabels ( proxy.ServiceInstances[0].Endpoint.Locality.Node)

howardjohn · 2023-01-04T21:50:45Z

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

-		// As this may contain node topology labels, which could not be got from aggregator controller
-		out[model.LocalityLabel] = locality
-		return out
+		// set k8s node name label, for ServiceInternalTrafficPolicy


Actually we can add NODE_NAME to the injection template (downward API) and get it from there 99% of the time. And fallback to ^ when its not there.

howardjohn · 2023-01-04T21:51:23Z

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

@@ -98,7 +99,7 @@ func NewEndpointBuilderFromMetadata(c controllerInterface, proxy *model.Proxy) *
 	if len(proxy.IPAddresses) > 0 {
 		networkID = out.endpointNetwork(proxy.IPAddresses[0])
 	}
-	out.labels = labelutil.AugmentLabels(proxy.Labels, c.Cluster(), locality, networkID)
+	out.labels = labelutil.AugmentLabels(proxy.Labels, c.Cluster(), locality, "", networkID)


once we add node name like I mentioned, make "" be proxy.Metadata.NodeName

howardjohn · 2023-01-04T21:53:07Z

pilot/pkg/xds/ads.go

@@ -630,7 +630,7 @@ func setTopologyLabels(proxy *model.Proxy) {

 	locality := util.LocalityToString(proxy.Locality)
 	// add topology labels to proxy labels
-	proxy.Labels = labelutil.AugmentLabels(proxy.Labels, proxy.Metadata.ClusterID, locality, proxy.Metadata.Network)
+	proxy.Labels = labelutil.AugmentLabels(proxy.Labels, proxy.Metadata.ClusterID, locality, "", proxy.Metadata.Network)


In addition to modifying Labels we should have a proxy.NodeName we set, just like L624

howardjohn · 2023-01-04T21:53:25Z

pilot/pkg/xds/endpoint_builder.go

@@ -132,6 +133,10 @@ func (b EndpointBuilder) Key() string {
 		hash.Write(b.failoverPriorityLabels)
 		hash.Write(Separator)
 	}
+	if b.service.Attributes.NodeLocal {
+		hash.Write([]byte(b.proxy.Labels[label.LabelHostname]))


Suggested change

hash.Write([]byte(b.proxy.Labels[label.LabelHostname]))

hash.Write([]byte(b.proxy.NodeName))

Once other suggestions are done

howardjohn · 2023-01-04T21:53:54Z

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

-		// As this may contain node topology labels, which could not be got from aggregator controller
-		out[model.LocalityLabel] = locality
-		return out
+		// set k8s node name label, for ServiceInternalTrafficPolicy


The experimental-ambient branch already does this if you would like a reference

howardjohn · 2023-01-05T15:15:58Z

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

 	}
 	var networkID network.ID
 	if len(proxy.IPAddresses) > 0 {
 		networkID = out.endpointNetwork(proxy.IPAddresses[0])
 	}
-	out.labels = labelutil.AugmentLabels(proxy.Labels, c.Cluster(), locality, networkID)
+	out.labels = labelutil.AugmentLabels(proxy.Labels, c.Cluster(), locality, proxy.Metadata.NodeName, networkID)


we use GetNodeName in other places, is this intended?

No, have changed to proxy.GetNodeName()

hzxuzhonghu · 2023-01-06T01:37:04Z

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

+		if len(proxy.GetNodeName()) == 0 {
+			// this can happen for an "old" proxy which has no `Metadata.NodeName` set
+			// in this case we set the node name in labels on the fly
+			nodeName = pod.Spec.NodeName


This can be removed in a future release, please add a TODO

dddddai

https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio/42551/gencheck_istio/1610947968523833344
I don't know why those generated yamls have no ISTIO_META_NODE_NAME, is that expected? Am I missing something?
I'm not very familiar with updating charts, could you please give some hints?

dddddai · 2023-01-06T13:29:18Z

manifests/charts/gateways/istio-ingress/templates/deployment.yaml

@@ -224,6 +224,10 @@ spec:
          {{- end }}
          - name: ISTIO_META_CLUSTER_ID
            value: "{{ $.Values.global.multiCluster.clusterName | default `Kubernetes` }}"
+          - name: ISTIO_META_NODE_NAME


Curious what L164 is used for

howardjohn · 2023-01-06T16:03:10Z

https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio/42551/gencheck_istio/1610947968523833344 I don't know why those generated yamls have no ISTIO_META_NODE_NAME, is that expected? Am I missing something? I'm not very familiar with updating charts, could you please give some hints?

you can just run git apply <(curl -sL "https://gcsweb.istio.io/gcs/istio-prow/pr-logs/pull/istio_istio/42551/gencheck_istio/1610947968523833344/artifacts/check-clean-repo-diff.patch")

But those files are not actually based on manifests/ but a snapshot of them to avoid excessive churn,

dddddai · 2023-01-07T02:54:38Z

Get it, thanks!

The release-notes test failed because gh pr view --json files cannot find the release note I added, see cli/cli#5368
Maybe add a release-notes-none label for this pr? Or fix it in test-infra

hzxuzhonghu · 2023-01-09T02:07:40Z

/retest

dddddai requested review from a team as code owners December 22, 2022 11:49

istio-policy-bot added the area/networking label Dec 22, 2022

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 22, 2022

howardjohn reviewed Dec 22, 2022

View reviewed changes

dddddai force-pushed the internal-policy branch from e77534c to 3240a6f Compare December 23, 2022 03:38

hzxuzhonghu reviewed Jan 3, 2023

View reviewed changes

hzxuzhonghu approved these changes Jan 4, 2023

View reviewed changes

howardjohn reviewed Jan 4, 2023

View reviewed changes

suuport ServiceInternalTrafficPolicy

5af030a

dddddai force-pushed the internal-policy branch from 31fe230 to 5af030a Compare January 5, 2023 10:34

dddddai requested a review from a team as a code owner January 5, 2023 10:34

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 5, 2023

howardjohn approved these changes Jan 5, 2023

View reviewed changes

hzxuzhonghu reviewed Jan 6, 2023

View reviewed changes

address comments

d984c33

dddddai commented Jan 6, 2023

View reviewed changes

dddddai added 2 commits January 7, 2023 09:39

fix test

09d9d77

release note

0b4e397

hzxuzhonghu added the release-notes-none Indicates a PR that does not require release notes. label Jan 9, 2023

istio-testing merged commit 4a6312d into istio:master Jan 9, 2023

dddddai deleted the internal-policy branch January 9, 2023 07:56

zirain mentioned this pull request Feb 24, 2023

Does envoyOtelAls service support localhost or $(HOST_IP), thanks. #43575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ServiceInternalTrafficPolicy #42551

Support ServiceInternalTrafficPolicy #42551

dddddai commented Dec 22, 2022 •

edited by istio-policy-bot

howardjohn commented Dec 22, 2022

howardjohn Dec 22, 2022

dddddai Dec 30, 2022

hzxuzhonghu Jan 3, 2023

hzxuzhonghu Jan 3, 2023

dddddai Jan 3, 2023

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

dddddai commented Dec 23, 2022

hzxuzhonghu Jan 3, 2023

dddddai Jan 3, 2023

hzxuzhonghu Jan 4, 2023

dddddai Jan 4, 2023

hzxuzhonghu left a comment

howardjohn left a comment

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

howardjohn Jan 4, 2023

howardjohn Jan 5, 2023

dddddai Jan 6, 2023

hzxuzhonghu Jan 6, 2023

dddddai Jan 6, 2023

dddddai left a comment •

edited

dddddai Jan 6, 2023

howardjohn commented Jan 6, 2023

dddddai commented Jan 7, 2023

hzxuzhonghu commented Jan 9, 2023

	hash.Write([]byte(b.proxy.Labels[label.LabelHostname]))
	hash.Write([]byte(b.proxy.NodeName))

Support ServiceInternalTrafficPolicy #42551

Support ServiceInternalTrafficPolicy #42551

Conversation

dddddai commented Dec 22, 2022 • edited by istio-policy-bot

howardjohn commented Dec 22, 2022

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

dddddai commented Dec 23, 2022

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 left a 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

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

dddddai left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Jan 6, 2023

dddddai commented Jan 7, 2023

hzxuzhonghu commented Jan 9, 2023

dddddai commented Dec 22, 2022 •

edited by istio-policy-bot

dddddai left a comment •

edited