feat(translator): xds route and vhost metadata #3602

guydc · 2024-06-12T23:44:40Z

What this PR does / why we need it:

Design doc for metadata propagation
Implement propagation of HTTPRoute/GRPCRoute metadata to envoy route resources
Implement propagation of Gateway and Listener to Virtual Host metadata
e2e test based on access logs - verifying that metadata is printed to the log. If injection of METADATA-based header is supported (Support METADATA operator in header mutation envoy#34671), this test can be simplified to make an assertion on generated headers.

Which issue(s) this PR fixes:
Relates to #3318

codecov · 2024-06-12T23:51:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.97%. Comparing base (ec9945a) to head (e54b00f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3602      +/-   ##
==========================================
+ Coverage   68.81%   68.97%   +0.16%     
==========================================
  Files         175      176       +1     
  Lines       21524    21610      +86     
==========================================
+ Hits        14811    14906      +95     
+ Misses       5636     5628       -8     
+ Partials     1077     1076       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-06-17T12:33:53Z

/retest

zhaohuabing · 2024-06-17T18:38:58Z

internal/cmd/egctl/testdata/translate/out/default-resources.all.yaml

@@ -1082,6 +1082,13 @@ xds:
            routes:
            - match:
                prefix: /
+              metadata:
+                filterMetadata:
+                  io.envoyproxy.gateway.metadata:


Would we need to add other metadata in the future? If so, we might need another level within the metadata structure to accommodate them.

This may look like:

io.envoyproxy.gateway.metadata: HTTPRoute: name: backend namespace: default Gateway: name: foo namespace: bar xxxx: .....

I changed the field to be a list of resources for future proofing.

Actually, I'm having second thoughts about a list here. It's not so easy to address metadata in a list. So, I think that I'll go with your approach, but maybe slightly generalize to Gateway, Route and Backend top-level resources, which can then be GW/GWC, HTTP/GRPCRoute, Service/ServiceImport/Backend.

I just thought the metadata structure should be able to accommodate multiple resources, but I don't know how exactly the structure should look like either. Let's brainstorm in the next meeting?

zhaohuabing · 2024-06-17T18:49:15Z

internal/cmd/egctl/testdata/translate/out/default-resources.all.yaml

@@ -1082,6 +1082,13 @@ xds:
            routes:
            - match:
                prefix: /
+              metadata:
+                filterMetadata:
+                  io.envoyproxy.gateway.metadata:


metadata.filterMetadata.io.envoyproxy.gateway.metadata seems a bit redundant, maybe just useio.envoyproxy.gateway as the key?

Signed-off-by: Guy Daich <[email protected]>

shawnh2 · 2024-06-19T02:43:01Z

site/content/en/contributions/design/metadata.md

+- [TCP/UDPRoute][tcpr] and [TLSRoute][tlsr] resource attributes are not propagated. These resources are translated to envoy filter chains, which do not currently support static metadata. 
+- [Service][svc], [ServiceImport][svci] and [Backend][] metadata and port name are propagated to envoy [cluster] metadata.
+
+## Usage


about usgae, can we also highlight this in the https://gateway.envoyproxy.io/latest/tasks/observability/proxy-observability/#logs ?

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-06-20T21:49:19Z

/retest

guydc · 2024-06-21T10:40:02Z

/retest

arkodg · 2024-06-25T18:50:04Z

internal/ir/xds.go

+
+// ResourceMetadata is metadata from the provider resource that is translated to an envoy resource
+// +k8s:deepcopy-gen=true
+type ResourceMetadata struct {


thoughts on leaving out SectionName and GroupVersion in the first iteration ?

will remove GroupVersion. I'll add VHost to this PR in a bit, and it'll include a useful SectionName, so prefer to keep it.

arkodg · 2024-06-25T19:27:27Z

internal/xds/translator/route.go

+	retryDefaultRetryOn                 = "connect-failure,refused-stream,unavailable,cancelled,retriable-status-codes"
+	retryDefaultRetriableStatusCode     = 503
+	retryDefaultNumRetries              = 2
+	envoyGatewayMetadataNamespace       = "io.envoyproxy.gateway"


should this be envoy-gateway instead ?

Several parts of Envoy configuration (e.g. listeners, routes, clusters) contain a metadata where arbitrary key-value pairs can be encoded. The typical pattern is to use the filter names in reverse DNS format as the key and encode filter specific configuration metadata in the value.

https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/data_sharing_between_filters#metadata

However, I agree that:

There is no filter in this case, so, we're not bound by these conventions. Istio uses a namespace called istio (and not io.istio) for similar purposes.

the long name is not convenient for access log formatters accessing the relevant keys.

arkodg · 2024-06-25T19:43:55Z

internal/xds/translator/route.go

+	envoyGatewayMetadataKeyNamespace    = "namespace"
+	envoyGatewayMetadataKeyAnnotations  = "annotations"
+	envoyGatewayMetadataKeySectionName  = "sectionName"
+	envoyGatewayMetadataKeyRoute        = "route"


to which resource are we building this key off ? input gateway api, IR or xds filter ?

will change to envoyGatewayXds* to make it clear that this is a const used in generated XDS.

arkodg · 2024-06-25T19:44:45Z

@zirain @guydc thoughts on adding this metadata by default to our default access log definition ?

guydc · 2024-06-25T21:46:10Z

/retest

guydc · 2024-06-25T22:43:23Z

/retest

guydc · 2024-06-26T11:29:37Z

/retest

zirain · 2024-06-26T11:50:57Z

@zirain @guydc thoughts on adding this metadata by default to our default access log definition ?

I don't think we should add them by default, users who want can add the command.

guydc · 2024-06-26T12:30:06Z

Also fine with allowing users to add this if needed.

guydc · 2024-06-27T16:25:29Z

/retest

zirain · 2024-06-28T00:35:40Z

/retest

zhaohuabing · 2024-06-28T01:56:57Z

@zirain @guydc thoughts on adding this metadata by default to our default access log definition ?

IMO, adding them to the default access log format would be a pleasant surprise to EG users. I would recommend adding it unless we have strong reasons against it.

zhaohuabing

LGTM. This can be extremely helpful for EG users when debugging. I strongly suggest adding the metadata to the default log format - hiding this information by default is a huge waste.

arkodg · 2024-07-01T17:08:01Z

internal/gatewayapi/route.go

@@ -31,6 +31,8 @@ const (
 	// Request timeout, which is defined as Duration, specifies the upstream timeout for the route
 	// If not specified, the default is 15s
 	HTTPRequestTimeout = "15s"
+	// egPrefix is a prefix of annotation keys that are processed by Envoy Gateway


used in filterEGPrefix to filter the annotations that should be picked up for metadata.

how do we document this opt in behavior ? user guide ?

arkodg · 2024-07-01T17:28:11Z

internal/xds/translator/metadata.go

+	envoyGatewayXdsMetadataKeyNamespace   = "namespace"
+	envoyGatewayXdsMetadataKeyAnnotations = "annotations"
+	envoyGatewayXdsMetadataKeySectionName = "sectionName"
+	envoyGatewayXdsMetadataKeyRoute       = "route"


why are these keys needed ?
as a consumer I may want to access the envoy-gateway metadata, but I may not know if the xds resource maps to a gateway or route or both

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-07-01T22:17:46Z

Sorry for force-push, I messed up the recent commit.

We will go with the list approach, since it's most future proof, considering situations where multiple resources of the same kind (e.g. policy and overriding policy) can contribute metadata.

Since there's currently no convenient way to reference a specific member of a list in envoy formatter at the moment, I prefer to leave metadata out of the default access log format, and not log all metadata by default.

arkodg · 2024-07-02T00:30:33Z

internal/gatewayapi/route.go

+	out := map[string]string{}
+	for k, v := range in {
+		if strings.HasPrefix(k, egPrefix) {
+			out[strings.TrimPrefix(k, egPrefix)] = v


should we trim the prefix or let it be ?

It feels a bit redundant to me, since only annotations with this prefix will be included.

cool, lets leave as is

Signed-off-by: Guy Daich <[email protected]>

arkodg

LGTM thanks !
super excited to use this feature

zhaohuabing

🚀

guydc requested a review from a team as a code owner June 12, 2024 23:44

guydc marked this pull request as draft June 12, 2024 23:44

guydc force-pushed the xds-metadata branch from c3d15ee to 5a4dd0f Compare June 12, 2024 23:49

guydc force-pushed the xds-metadata branch from 5a4dd0f to 5cb74a5 Compare June 13, 2024 22:39

xds route metadata

4201299

Signed-off-by: Guy Daich <[email protected]>

guydc force-pushed the xds-metadata branch from 5cb74a5 to 4201299 Compare June 13, 2024 22:41

guydc added 2 commits June 17, 2024 05:51

Merge branch 'main' into xds-metadata

cefa13f

fix gen

e6a7769

Signed-off-by: Guy Daich <[email protected]>

guydc marked this pull request as ready for review June 17, 2024 12:34

zhaohuabing reviewed Jun 17, 2024

View reviewed changes

guydc added 4 commits June 18, 2024 16:25

Merge branch 'main' into xds-metadata

759ca96

use list of resources

cafd0f0

Signed-off-by: Guy Daich <[email protected]>

fix gen

47fc780

Signed-off-by: Guy Daich <[email protected]>

scope metadata to top-level route object

dcf75e7

Signed-off-by: Guy Daich <[email protected]>

shawnh2 reviewed Jun 19, 2024

View reviewed changes

guydc added 3 commits June 20, 2024 16:24

add info to observability guide

482f4b6

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into xds-metadata

ee0eda4

fix gen

fd3dd71

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into xds-metadata

5daf1c0

guydc requested review from zhaohuabing and shawnh2 June 24, 2024 11:39

arkodg reviewed Jun 25, 2024

View reviewed changes

guydc changed the title ~~feat(translator): xds route metadata~~ feat(translator): xds route and vhost metadata Jun 25, 2024

guydc requested a review from arkodg June 26, 2024 19:12

zhaohuabing previously approved these changes Jun 29, 2024

View reviewed changes

arkodg reviewed Jul 1, 2024

View reviewed changes

guydc dismissed zhaohuabing’s stale review via b44da84 July 1, 2024 21:50

guydc added 2 commits July 1, 2024 17:10

use list 2

af54b09

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into xds-metadata

249581b

Signed-off-by: Guy Daich <[email protected]>

guydc force-pushed the xds-metadata branch from b44da84 to 249581b Compare July 1, 2024 22:13

fix test

3ef2950

Signed-off-by: Guy Daich <[email protected]>

arkodg reviewed Jul 2, 2024

View reviewed changes

guydc added 3 commits July 2, 2024 07:16

fix test 2

62d2804

Signed-off-by: Guy Daich <[email protected]>

Merge branch 'main' into xds-metadata

e6c802a

fix

e54b00f

Signed-off-by: Guy Daich <[email protected]>

guydc requested review from zhaohuabing and arkodg July 2, 2024 12:39

arkodg approved these changes Jul 3, 2024

View reviewed changes

zirain approved these changes Jul 3, 2024

View reviewed changes

zirain merged commit acce649 into envoyproxy:main Jul 3, 2024
27 checks passed

zhaohuabing approved these changes Jul 3, 2024

View reviewed changes

guydc mentioned this pull request Jul 11, 2024

Support Route and VHost Metadata #3318

Closed

feat(translator): xds route and vhost metadata #3602

feat(translator): xds route and vhost metadata #3602

Conversation

guydc commented Jun 12, 2024 • edited Loading

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

guydc commented Jun 17, 2024

zhaohuabing Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guydc Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guydc commented Jun 20, 2024

guydc commented Jun 21, 2024

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

arkodg commented Jun 25, 2024

guydc commented Jun 25, 2024

guydc commented Jun 25, 2024

guydc commented Jun 26, 2024

zirain commented Jun 26, 2024

guydc commented Jun 26, 2024

guydc commented Jun 27, 2024

zirain commented Jun 28, 2024

zhaohuabing commented Jun 28, 2024 • edited Loading

zhaohuabing left a comment • 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

guydc commented Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

zhaohuabing left a comment

Choose a reason for hiding this comment

guydc commented Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

zhaohuabing Jun 17, 2024 •

edited

Loading

guydc Jun 19, 2024 •

edited

Loading

zhaohuabing Jun 21, 2024 •

edited

Loading

zhaohuabing commented Jun 28, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading

guydc commented Jul 1, 2024 •

edited

Loading