envoy: use upstream http filter for metadata #51206

kyessenov · 2024-05-22T20:34:53Z

Change-Id: Idcc1f98fa1242d93fdb19c8c9838a7748856ebed

Please provide a description of this PR:

Change-Id: Idcc1f98fa1242d93fdb19c8c9838a7748856ebed Signed-off-by: Kuat Yessenov <[email protected]>

howardjohn

Overall looks like a good direction, just not sure how/if it works as-is

cc @keithmattix

howardjohn · 2024-05-22T20:37:46Z

pkg/istio-agent/agent.go

+ }
+ metadata := base64.RawStdEncoding.EncodeToString(bytes)
+ // Set metadata exchange environment variables
+ _ = os.Setenv("ISTIO_PEER_METADATA_ID", node.ID)


should we set this on the envoy process call instead of in the ambient environment? This feels a bit sketchy

Sure, I'm checking whether this is feasible - will clean up once it works.

howardjohn · 2024-05-22T20:38:00Z

pkg/istio-agent/agent.go

@@ -321,6 +322,15 @@ func (a *Agent) initializeEnvoyAgent(_ context.Context) error {
 }
 a.envoyOpts.ConfigPath = out
 a.envoyOpts.ConfigCleanup = true
+
+ bytes, err := proto.Marshal(node.Metadata.ToStruct())


I thought we used flatbuffers? does it encode to the same or am I just wrong about that?

Flatbuffers is an internal representation, that we can remove without user facing changes.

What about cross-version calls?

We never send flatbuffer on the wire if that's the question.

Why still use flat buffers then? Would be nice if we could standardize on proto if perf impact is minimal (not for this PR obviously)

It was used because Wasm can't handle protos efficiently. Now that we don't recommend Wasm for telemetry, it could be a cleanup, although some users are probably still using internal flatbuffers and would be impacted - including our own SD extension.

If we are going to be breaking the internal protocol between proxies this needs broad discussion around the cost vs benefits.

@howardjohn There's no difference on the network between this and before. Flatbuffers were always internal to Istio and never sent on the wire. I don't think I understand the concern here.

Oh so the wire format is the same?

We can do

meta := ... roundtrip := flatbuffer_unmarshal(proto_marshal(meta)) meta == roundtrip

?

If so, no concern

Yeah, flatbuffer is just a subset of struct metadata with 4-5 fields and converted internally in Envoy from proto to flatbuffers because Wasm. We can construct the subset here as well, and not even use flatbuffers to roundtrip.

howardjohn · 2024-05-22T20:39:07Z

pilot/pkg/networking/core/listener_builder.go

@@ -443,9 +443,5 @@ func appendMxFilter(httpOpts *httpListenerOpts, filters []*hcm.HttpFilter) []*hc
 if httpOpts.class == istionetworking.ListenerClassSidecarInbound {
 return append(filters, xdsfilters.SidecarInboundMetadataFilter)
 }
-
- if httpOpts.skipIstioMXHeaders {


we seem to lose this? skipIstioMXHeaders

Yes, will add it later.

howardjohn · 2024-05-22T20:39:33Z

pilot/pkg/xds/filters/filters.go

- "istio_headers": map[string]any{},
- },
- map[string]any{
- "workload_discovery": map[string]any{},


How does workload_discovery work now?

It can be moved to network metadata exchange.

I imagine that needs to be done in this PR, yes?

No, this PR is about sending headers, not reading them.

IIUC this removed code is what enabled workload discovery for waypoints. If so, how are we going to get that information now

howardjohn · 2024-05-22T20:39:58Z

pilot/pkg/xds/filters/filters.go

- "upstream_propagation": []any{
- map[string]any{
- "istio_headers": map[string]any{
- "skip_external_clusters": true,


Did we lose this?

This gets a lot easier actually because skipping is done per cluster.

howardjohn · 2024-05-22T20:40:17Z

pilot/pkg/xds/filters/filters.go

- SidecarOutboundMetadataFilterSkipHeaders = &hcm.HttpFilter{
- Name: MxFilterName,
+
+ InjectIstioHeaders = &hcm.HttpFilter{


Any performance impact? I'd imagine not but would be good to check

Don't think so, but we can check and probably need both ways temporarily.

Change-Id: If50ce31054b5cba895c95e7767cef3deec6a96c6 Signed-off-by: Kuat Yessenov <[email protected]>

Change-Id: I4875e1bcc373570ddf8ac5a45d292660b6bbb7cc Signed-off-by: Kuat Yessenov <[email protected]>

Change-Id: I5fd8be85a6650c233edb1ec28cd4e9e32a2d17e9 Signed-off-by: Kuat Yessenov <[email protected]>

Change-Id: I69f03d414658079bed72e1db7eee731352bccbe7 Signed-off-by: Kuat Yessenov <[email protected]>

Change-Id: Ie041c77f420927d9071755d4b413946fb98c9b2d Signed-off-by: Kuat Yessenov <[email protected]>

sschepens · 2024-05-22T23:03:41Z

Is this changing HTTP MX to use upstream filters?

I was wondering if using upstream filters could allow us to only send headers on the first request within a connection as an optimization to prevent sending MX headers on every request.

kyessenov · 2024-05-22T23:05:44Z

@sschepens Yeah, but I think it would be better to use METADATA frames.

sschepens · 2024-05-22T23:32:12Z

@sschepens Yeah, but I think it would be better to use METADATA frames.

Cool, using METADATA frames would be nice, but only be supported for h2, I guess that's fine though.

Were you planning on taking this changes as well?

kyessenov · 2024-05-22T23:54:14Z

@sschepens First thing first, let's start using upstream HTTP filters for metadata, because there seems to be some bugs based on the results of the PR here.

Change-Id: Iab97c7e2ce047c4543501c322fa0c58cb3e2ab3a Signed-off-by: Kuat Yessenov <[email protected]>

Change-Id: Ib28f1400c41f46d98bf93a4c0261f20667ca7224 Signed-off-by: Kuat Yessenov <[email protected]>

istio-testing · 2024-05-23T20:30:18Z

@kyessenov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_istio	`19cbc35`	link	true	`/test release-notes`
lint_istio	`19cbc35`	link	true	`/test lint`
unit-tests-arm64_istio	`19cbc35`	link	true	`/test unit-tests-arm64`
unit-tests_istio	`19cbc35`	link	true	`/test unit-tests`
integ-ipv6_istio	`19cbc35`	link	true	`/test integ-ipv6`
integ-basic-arm64_istio	`19cbc35`	link	true	`/test integ-basic-arm64`
integ-distroless_istio	`19cbc35`	link	true	`/test integ-distroless`
integ-ds_istio	`19cbc35`	link	true	`/test integ-ds`
integ-telemetry-istiodremote_istio	`19cbc35`	link	true	`/test integ-telemetry-istiodremote`
integ-telemetry_istio	`19cbc35`	link	true	`/test integ-telemetry`
integ-pilot-istiodremote_istio	`19cbc35`	link	true	`/test integ-pilot-istiodremote`
integ-telemetry-mc_istio	`19cbc35`	link	true	`/test integ-telemetry-mc`
integ-cni_istio	`19cbc35`	link	true	`/test integ-cni`
integ-pilot_istio	`19cbc35`	link	true	`/test integ-pilot`
integ-pilot-istiodremote-mc_istio	`19cbc35`	link	true	`/test integ-pilot-istiodremote-mc`
integ-pilot-multicluster_istio	`19cbc35`	link	true	`/test integ-pilot-multicluster`

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-sigs/prow repository. I understand the commands that are listed here.

istio-testing · 2024-06-20T22:03:21Z

PR needs rebase.

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-sigs/prow repository.

keithmattix · 2024-06-22T13:19:46Z

Not stale

keithmattix · 2024-07-23T12:04:45Z

@kyessenov do you need some help picking this back up?

kyessenov · 2024-07-23T21:47:02Z

@keithmattix Maybe, or we can skip this iteration and try to use "header operations" directly in the upstream HTTP filter.

envoy: use upstream http filter for metadata

7305d79

Change-Id: Idcc1f98fa1242d93fdb19c8c9838a7748856ebed Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov requested review from a team as code owners May 22, 2024 20:34

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 22, 2024

howardjohn reviewed May 22, 2024

View reviewed changes

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

kyessenov added 3 commits May 22, 2024 20:53

add uc

66faf0f

Change-Id: If50ce31054b5cba895c95e7767cef3deec6a96c6 Signed-off-by: Kuat Yessenov <[email protected]>

bug

ffc2f19

Change-Id: I4875e1bcc373570ddf8ac5a45d292660b6bbb7cc Signed-off-by: Kuat Yessenov <[email protected]>

add passthrough

59c77c1

Change-Id: I5fd8be85a6650c233edb1ec28cd4e9e32a2d17e9 Signed-off-by: Kuat Yessenov <[email protected]>

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 22, 2024

kyessenov added 2 commits May 22, 2024 22:02

still need discovery

501e272

Change-Id: I69f03d414658079bed72e1db7eee731352bccbe7 Signed-off-by: Kuat Yessenov <[email protected]>

check

4b1c71d

Change-Id: Ie041c77f420927d9071755d4b413946fb98c9b2d Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov added 2 commits May 23, 2024 19:23

fix?

39be053

Change-Id: Iab97c7e2ce047c4543501c322fa0c58cb3e2ab3a Signed-off-by: Kuat Yessenov <[email protected]>

fix

19cbc35

Change-Id: Ib28f1400c41f46d98bf93a4c0261f20667ca7224 Signed-off-by: Kuat Yessenov <[email protected]>

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

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jun 22, 2024

istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jun 22, 2024

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 23, 2024

istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

envoy: use upstream http filter for metadata #51206

envoy: use upstream http filter for metadata #51206

kyessenov commented May 22, 2024 •

edited by istio-policy-bot

Loading

howardjohn left a comment

howardjohn May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024 •

edited

Loading

keithmattix May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

keithmattix May 22, 2024

kyessenov May 22, 2024

keithmattix May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

howardjohn May 22, 2024

kyessenov May 22, 2024

sschepens commented May 22, 2024

kyessenov commented May 22, 2024

sschepens commented May 22, 2024

kyessenov commented May 22, 2024

istio-testing commented May 23, 2024

istio-testing commented Jun 20, 2024

keithmattix commented Jun 22, 2024

keithmattix commented Jul 23, 2024

kyessenov commented Jul 23, 2024

envoy: use upstream http filter for metadata #51206

Are you sure you want to change the base?

envoy: use upstream http filter for metadata #51206

Conversation

kyessenov commented May 22, 2024 • edited by istio-policy-bot Loading

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

kyessenov May 22, 2024 • edited Loading

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

Choose a reason for hiding this comment

sschepens commented May 22, 2024

kyessenov commented May 22, 2024

sschepens commented May 22, 2024

kyessenov commented May 22, 2024

istio-testing commented May 23, 2024

istio-testing commented Jun 20, 2024

keithmattix commented Jun 22, 2024

keithmattix commented Jul 23, 2024

kyessenov commented Jul 23, 2024

kyessenov commented May 22, 2024 •

edited by istio-policy-bot

Loading

kyessenov May 22, 2024 •

edited

Loading