impl: custom error response #4415

zhaohuabing · 2024-10-09T09:55:40Z

Implement: #4259

codecov · 2024-10-09T10:03:50Z

Codecov Report

Attention: Patch coverage is 74.39516% with 127 lines in your changes missing coverage. Please review.

Project coverage is 65.97%. Comparing base (33ac6ca) to head (6f7d2c1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/custom_response.go	80.11%	46 Missing and 22 partials ⚠️
internal/provider/kubernetes/controller.go	10.34%	23 Missing and 3 partials ⚠️
internal/provider/kubernetes/indexers.go	10.00%	17 Missing and 1 partial ⚠️
internal/provider/kubernetes/predicates.go	28.57%	6 Missing and 4 partials ⚠️
internal/gatewayapi/backendtrafficpolicy.go	94.44%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4415      +/-   ##
==========================================
+ Coverage   65.85%   65.97%   +0.11%     
==========================================
  Files         202      203       +1     
  Lines       30679    31154     +475     
==========================================
+ Hits        20205    20555     +350     
- Misses       9320     9415      +95     
- Partials     1154     1184      +30

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

api/v1alpha1/backendtrafficpolicy_types.go

api/v1alpha1/shared_types.go

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

internal/provider/kubernetes/predicates.go

api/v1alpha1/shared_types.go

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

arkodg · 2024-10-18T21:40:49Z

internal/gatewayapi/backendtrafficpolicy.go

+		if ro.Response.Body.Type != nil && *ro.Response.Body.Type == egv1a1.ResponseValueTypeValueRef {
+			foundCM := false
+			for _, cm := range configMaps {
+				if cm.Namespace == policy.Namespace && cm.Name == string(ro.Response.Body.ValueRef.Name) {


is this a ptr, will it work for default ?

if *ro.Response.Body.Type == egv1a1.ResponseValueTypeValueRef, then ro.Response.Body.ValueRef is not nil, otherwise, it would be a bug in Gateway API translation.

...nal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-invalid-valueref.in.yaml

arkodg · 2024-10-18T21:53:11Z

internal/xds/translator/testdata/out/xds-ir/custom-response.listeners.yaml

+                            name: cel-matcher
+                            typedConfig:
+                              '@type': type.googleapis.com/xds.type.matcher.v3.CelMatcher
+                              exprMatch:


trusting the e2e, this API is complex, thanks for implementing this !

The Envoy CEL matcher API is terrible :-) It's the AST for CEL: response.code >= 501 && response.code <= 511 .

arkodg

LGTM ! implementing the feature using the custom response filter doesnt look easy, thanks for building this out

zirain · 2024-10-21T02:41:50Z

api/v1alpha1/envoyproxy_types.go

@@ -124,6 +124,8 @@ type EnvoyProxySpec struct {
 	//
 	// - envoy.filters.http.ratelimit
 	//
+	// - envoy.filters.http.custom_response


is it possible to override the 429 response if we place custom response filter behind ratelimit filterm should this filter be the first one of http filters?

Good point. Should we allow users to override the ratelimit response if they added a matching conditing for 429 in the ResponseOrverride? @envoyproxy/gateway-maintainers

it's not just ratelimit, does authz/authn has same problem?

Yeah, the custom response filter has been added to the last because users may want to override the direct responses generated by other filters, if they explicitly adding these status codes in the matching conditions. For example, to set the response body of a denide request to a message format algined with the style of the reponses of the applications.

If we put it at the first, the users won't be able to override them even if they intend to.

IMO, it's better to put it at the first but no strong option, @arkodg please chime in as API designer.

cc @envoyproxy/gateway-maintainers

as a user, I would want the custom response to kick in after all filters have finished processing, to unify the way I deal with status code and error pages, so +1 to adding it to the end

we can change the order in the future, maybe we can open a ticket to continue our discussion.

we can change the order in the future, maybe we can open a ticket to continue our discussion.

Let's keep the custom response filter at the last since there're use cases to override the responses of other filters. Users can change the default order using EnvoyProxy if they want to reorder the filters.

zirain · 2024-10-21T23:31:15Z

@zhaohuabing can you make CI happy?

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

arkodg · 2024-10-22T00:23:26Z

e2es are failing ...

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing requested a review from a team as a code owner October 9, 2024 09:55

zhaohuabing marked this pull request as draft October 9, 2024 09:55

zhaohuabing commented Oct 10, 2024

View reviewed changes

api/v1alpha1/backendtrafficpolicy_types.go Show resolved Hide resolved

zhaohuabing force-pushed the custom-error-response branch 5 times, most recently from fce9138 to c0cb97e Compare October 15, 2024 06:04

zhaohuabing commented Oct 15, 2024

View reviewed changes

api/v1alpha1/shared_types.go Show resolved Hide resolved

zhaohuabing marked this pull request as ready for review October 15, 2024 07:04

zhaohuabing added 12 commits October 15, 2024 07:07

gateway api translation

c1a7d65

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

add tests for gateway api translation

5e68042

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

xds translation

92b050c

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

test for xds translation

ee8fb4c

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix lint

96398c3

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix lint

0dea08e

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

e2e test

d7b0df1

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

add cel validation

d143bd5

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix test

bb3d59d

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix test

9700516

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix test

9ef703b

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix lint

0631fa4

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing force-pushed the custom-error-response branch from 51b0e4a to 0631fa4 Compare October 15, 2024 07:07

zhaohuabing commented Oct 15, 2024

View reviewed changes

internal/provider/kubernetes/predicates.go Show resolved Hide resolved

arkodg reviewed Oct 15, 2024

View reviewed changes

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

zhaohuabing added 2 commits October 16, 2024 02:10

address comment

1f309e0

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Merge remote-tracking branch 'origin/main' into custom-error-response

e820fb2

zhaohuabing requested a review from arkodg October 16, 2024 02:23

Merge branch 'main' into custom-error-response

e41419b

zhaohuabing added 6 commits October 17, 2024 01:58

address comment

e3d528b

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix lint

dbed3c7

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Merge branch 'main' into custom-error-response

489503b

Merge branch 'main' into custom-error-response

19e123f

Merge branch 'main' into custom-error-response

376b710

Merge branch 'main' into custom-error-response

72650ea

arkodg reviewed Oct 18, 2024

View reviewed changes

...nal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-invalid-valueref.in.yaml Show resolved Hide resolved

arkodg reviewed Oct 18, 2024

View reviewed changes

arkodg previously approved these changes Oct 18, 2024

View reviewed changes

arkodg requested review from a team October 18, 2024 22:00

zirain reviewed Oct 21, 2024

View reviewed changes

arkodg requested review from zirain and a team October 21, 2024 22:47

make Body required

9334f53

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing dismissed arkodg’s stale review via 9334f53 October 21, 2024 23:32

Merge branch 'main' into custom-error-response

26b2c76

zirain previously approved these changes Oct 21, 2024

View reviewed changes

zirain requested a review from arkodg October 21, 2024 23:48

arkodg previously approved these changes Oct 22, 2024

View reviewed changes

fix e2e

6f7d2c1

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing dismissed stale reviews from arkodg and zirain via 6f7d2c1 October 22, 2024 00:26

arkodg approved these changes Oct 22, 2024

View reviewed changes

zirain approved these changes Oct 22, 2024

View reviewed changes

zhaohuabing merged commit 04fc944 into envoyproxy:main Oct 22, 2024
23 of 26 checks passed

zhaohuabing deleted the custom-error-response branch October 22, 2024 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl: custom error response #4415

impl: custom error response #4415

zhaohuabing commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

arkodg Oct 18, 2024

zhaohuabing Oct 21, 2024

arkodg Oct 18, 2024

arkodg Oct 18, 2024

zhaohuabing Oct 21, 2024

arkodg left a comment

zirain Oct 21, 2024

zhaohuabing Oct 21, 2024

zirain Oct 21, 2024

zhaohuabing Oct 21, 2024 •

edited

Loading

zirain Oct 21, 2024

arkodg Oct 21, 2024

zirain Oct 21, 2024

zhaohuabing Oct 22, 2024 •

edited

Loading

zirain commented Oct 21, 2024

arkodg commented Oct 22, 2024

impl: custom error response #4415

impl: custom error response #4415

Conversation

zhaohuabing commented Oct 9, 2024 • edited Loading

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

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

zhaohuabing Oct 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

Choose a reason for hiding this comment

zhaohuabing Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

zirain commented Oct 21, 2024

arkodg commented Oct 22, 2024

zhaohuabing commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

zhaohuabing Oct 21, 2024 •

edited

Loading

zhaohuabing Oct 22, 2024 •

edited

Loading