feat: EnvoyPatchPolicy API by arkodg · Pull Request #1410 · envoyproxy/gateway

arkodg · 2023-05-09T18:47:33Z

Relates to #24

codecov · 2023-05-09T18:54:55Z

Codecov Report

Merging #1410 (b2133c5) into main (22bd06b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1410   +/-   ##
=======================================
  Coverage   61.40%   61.40%           
=======================================
  Files          79       79           
  Lines       11459    11459           
=======================================
  Hits         7036     7036           
  Misses       3962     3962           
  Partials      461      461

arkodg · 2023-05-10T01:06:03Z

cc @howardjohn @costinm in case you see any reuse/overlap w Istio w Gateway API

howardjohn · 2023-05-10T01:20:15Z

docs/latest/design/envoy-patch-policy.md

This seems like it will tank performance. Each resource will need to be marshalled to json, patched, then unmarshalled (?).

When we used to do similar it was 90% CPU cost of Istio (which took a ton of cpu) on just json. And we were only doing marshalling, not the other 2 steps

it won't be each resource, name of the resource here is mandatory, so we would need to find (guessing its O(N) based on

gateway/internal/xds/types/resourceversiontable.go

Line 16 in 0f710c4

type XdsResources = map[resourcev3.Type][]types.Resource

) the specific resource and marshal + patch + unmarshal

picked json over proto because it has an RFC and more commonly used in the ecosystem (k8s, kustomize) and REST APIs

thanks for outlining the CPU cost, we should quantity this performance hit and highlight it in docs

howardjohn · 2023-05-10T01:21:01Z

docs/latest/design/envoy-patch-policy.md

one note - I think you will need to import every single proto in envoy into the control plane. Unless you have some tricks. Istio does it - not too bad, but bloats the binary

Also means you could never use non-upsteam extensions to envoy

yeah we autogenerate and import today in the client side (egctl)
with this PR we will need to do it in server side too
https://github.com/envoyproxy/gateway/blob/main/internal/xds/extensions/extensions.gen.go

👍 one minor suggestion: don't import the v2 stuff. One less headache to deal with down the line... and in only sort of works IME

thanks, will rm them

raised #1442 to rm v2 imports

@zhaohuabing I dont think we'll be able to handle your use case if we purely relied on protojson tooling

It would be a headache for any user who adds their own custom filters to envoy since EG can't recognize the APIs of these filters.

I understand your sentiment, and it is something we should support, which can be tracked with a GH issue, for now, it doesnt require any changes to the API, or am I missing something ?

Yes, it's an implementation concern rather than API.

docs/latest/design/envoy-patch-policy.md

howardjohn · 2023-05-10T15:28:58Z

cc @howardjohn @costinm in case you see any reuse/overlap w Istio w Gateway API

I don't personally. Patching APIs are impossible to support, I don't think anything like this should be standardized in GW API

arkodg · 2023-05-10T20:52:58Z

cc @howardjohn @costinm in case you see any reuse/overlap w Istio w Gateway API

I don't personally. Patching APIs are impossible to support, I don't think anything like this should be standardized in GW API

@howardjohn I was referring to a common Gateway Extension API across the projects.
Since Istio already has EnvoyFIlter, that should work along side Gateway APIs, patching in Istio is already covered :) but this API could move the spec into Gateway API nouns

qicz

looks good to me

kflynn · 2023-05-11T01:07:00Z

docs/latest/design/envoy-patch-policy.md

Given that we can already configure rate limiting using the native Envoy Gateway ratelimit API, I'd like to see some other examples:

You have three different upstream services, each of which needs to use a specific certificate to originate TLS.

The current Gateway API doesn't have a lot of support for upstream TLS origination. There's ongoing work to support it natively, but until that's accepted, this would be a reasonable use for an extension mechanism.

Obviously you don't know the routes involved, you just know that you happen to have three upstream services that need certificates.

You need to allow the HTTP listener, but not the HTTPS listener, to route traffic to the path prefix /.well-known/acme-challenge/ over to the my-acme-server Service. (Other traffic may arrive on the HTTP listener too, but that traffic should be unaffected.)

This is the path used by the HTTP-01 ACME challenge, which always arrives over HTTP and must not be redirected to HTTPS. If a vendor wants to do custom ACME work, this might be a thing they'd need to do.

can you share some raw xds Config for the examples ?

kflynn · 2023-05-11T01:08:56Z

docs/latest/design/envoy-patch-policy.md

This was pretty fascinating to read. I agree with the sentiment and I feel compelled to note that if even the author acknowledges that it must forever remain experimental because it's too fragile to promote to stable, we should all but certainly not implement this in the first place unless there's a truly compelling reason to do so. I haven't seen that yet.

docs/latest/design/envoy-patch-policy.md

kflynn · 2023-05-11T01:12:47Z

@howardjohn writes:

I don't personally [see any overlap with utility for Istio]. Patching APIs are impossible to support, I don't think anything like this should be standardized in GW API

It's kind of rare for me to unequivocally agree with John (😉), but I do here. I'm extremely concerned with how support for this API would play out in the long term. Emissary used to offer a much more constrained capability to do something similar, and it caused us more than a few problems.

kflynn

GitHub says I'm reviewing rather than just commenting, so, uh, OK. 🙂 I definitely want to see some more example uses, and I think we should probably further discuss in the Envoy Gateway meetings before proceeding.

api/v1alpha1/envoypatchpolicy_types.go

zhaohuabing · 2023-05-22T04:05:36Z

api/v1alpha1/envoypatchpolicy_types.go

Does this imply that in order to apply a patch, a user need to know the absolute path of a listener/route/cluster from the configuration root?

yes, they could easily add to the beginning using 0 or end using -.If they want to insert somewhere in the middle , they would need to rely on egctl to make sure their config works correctly. This experience is identical to kubectl patch e.g. https://gateway.envoyproxy.io/v0.4.0/user/secure-gateways.html#tls-certificates. This experience might not be extremely use friendly, but its based on a known RFC/spec and is not meant to be a common case

zhaohuabing · 2023-05-22T04:07:40Z

api/v1alpha1/envoypatchpolicy_types.go

"EnvoyPatch" seems clear enough to me. Do we need to add "Policy" here?

previous reviewers for the RateLimit feature had requested RateLimit to suffix with Filter so we could differentiate Filter and Policy

api/v1alpha1/envoypatchpolicy_types.go

zhaohuabing · 2023-05-22T04:13:23Z

api/v1alpha1/envoypatchpolicy_types.go

Is there a "all" target or similar mechanism by which we can apply an EnvoyFilter to multiple targets? There might be scenarios multiple gateway resources need to be apply the same EnvoyFilters.

prefer if we started off with 1:1 mappings, since policy attachment UX is already perceived as complex, the use case can be satisfied today by creating another identical PolicyAttachment and applying it to another TargetRef. In the future we could make TargetRef plural, note this definition is taken from upstream

Relates to envoyproxy#24 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg · 2023-05-31T19:02:25Z

/retest

qicz

LGTM

Xunzhuo · 2023-06-01T03:00:49Z

docs/latest/design/envoy-patch-policy.md

+* This API will always be an experimental API and cannot be graduated into a stable API because Envoy Gateway cannot garuntee
+  * that the naming scheme for the generated resources names will not change across releases
+  * that the underlying Envoy Proxy API will not change across releases
+* This API needs to be explicitly enabled using the [EnvoyGateway][] API


I did not see the EnvoyGateway API changes ? Do I miss anything?

ive raised a separate issue to track this #1468

Xunzhuo · 2023-06-01T03:02:07Z

docs/latest/design/envoy-patch-policy.md

+semantics.
+
+## Design Decisions
+* This API will only support a single `targetRef` and can bind to only a `Gateway` resource. This simplifies reasoning of how


A EnvoyPatchPolicy can be only binded to one Gateway, right ? But can a Gateway be binded with multiple different EnvoyPatchPolicy APIs ?

yes, ive added a priority field with PolicyAttachment so user can optionally order the Policiies that are being bind to a Gateway

Xunzhuo

lgtm!

zhaohuabing

LGTM

arkodg requested a review from a team as a code owner May 9, 2023 18:47

arkodg force-pushed the envoypatchpolicy branch from f289864 to f084543 Compare May 9, 2023 18:50

arkodg marked this pull request as draft May 9, 2023 18:52

arkodg force-pushed the envoypatchpolicy branch from f084543 to 012f104 Compare May 9, 2023 18:58

arkodg marked this pull request as ready for review May 10, 2023 00:46

arkodg requested review from a team, LanceEa, Xunzhuo and qicz and removed request for a team May 10, 2023 00:46

howardjohn reviewed May 10, 2023

View reviewed changes

Xunzhuo reviewed May 10, 2023

View reviewed changes

docs/latest/design/envoy-patch-policy.md Outdated Show resolved Hide resolved

qicz previously approved these changes May 11, 2023

View reviewed changes

kflynn reviewed May 11, 2023

View reviewed changes

docs/latest/design/envoy-patch-policy.md Outdated Show resolved Hide resolved

kflynn suggested changes May 11, 2023

View reviewed changes

zirain reviewed May 22, 2023

View reviewed changes

api/v1alpha1/envoypatchpolicy_types.go Outdated Show resolved Hide resolved

zhaohuabing reviewed May 22, 2023

View reviewed changes

api/v1alpha1/envoypatchpolicy_types.go Outdated Show resolved Hide resolved

zhaohuabing reviewed May 22, 2023

View reviewed changes

qicz added kind/enhancement New feature or request area/api API-related issues labels May 26, 2023

qicz added the area/config Issues related to config management, e.g. Config Manager, Config Sources, etc. label May 26, 2023

qicz added this to the 0.5.0-rc1 milestone May 26, 2023

arkodg mentioned this pull request May 29, 2023

Support IP whitelist #1447

Closed

arkodg dismissed qicz’s stale review via 9d6ca36 May 31, 2023 01:41

arkodg force-pushed the envoypatchpolicy branch from eda9e03 to 9d6ca36 Compare May 31, 2023 01:41

Arko Dasgupta added 8 commits May 31, 2023 11:21

feat: EnvoyPatchPolicy API

a375ffb

Relates to envoyproxy#24 Signed-off-by: Arko Dasgupta <arko@tetrate.io>

wip design doc

82f9c6a

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

wrap up design

58be2bc

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

lint

e4544c4

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

update implementation

4decf03

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

address comments

2d2e1c4

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

lint

dc8b0e0

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

charts

28bb08e

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

arkodg force-pushed the envoypatchpolicy branch from 483fab7 to 28bb08e Compare May 31, 2023 18:23

arkodg requested review from Xunzhuo, kflynn, qicz, zhaohuabing and zirain May 31, 2023 19:02

This comment was marked as duplicate.

Sign in to view

qicz approved these changes May 31, 2023

View reviewed changes

Merge branch 'main' into envoypatchpolicy

b2133c5

Xunzhuo reviewed Jun 1, 2023

View reviewed changes

Xunzhuo approved these changes Jun 6, 2023

View reviewed changes

zhaohuabing approved these changes Jun 6, 2023

View reviewed changes

zirain approved these changes Jun 6, 2023

View reviewed changes

zirain merged commit 9fedf7d into envoyproxy:main Jun 6, 2023

arkodg mentioned this pull request Jun 6, 2023

Allow users to configure xDS resources #24

Closed

6 tasks

Conversation

arkodg commented May 9, 2023

Uh oh!

codecov bot commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arkodg commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

howardjohn commented May 10, 2023

Uh oh!

arkodg commented May 10, 2023

Uh oh!

qicz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kflynn commented May 11, 2023

Uh oh!

kflynn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhaohuabing May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaohuabing May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhaohuabing May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented May 9, 2023 •

edited

Loading

arkodg commented May 10, 2023 •

edited

Loading

zhaohuabing Jun 4, 2023 •

edited

Loading

zhaohuabing May 22, 2023 •

edited

Loading

zhaohuabing May 22, 2023 •

edited

Loading

zhaohuabing May 22, 2023 •

edited

Loading