feat: add support for ResponseHeaderModifier Filter by Xunzhuo · Pull Request #717 · envoyproxy/gateway

Xunzhuo · 2022-11-09T17:57:49Z

Resolves: #657

Signed-off-by: bitliu bitliu@tencent.com

arkodg · 2022-11-09T18:21:16Z

wow that was fast @Xunzhuo :)
hey @AliceProxy can you please help review this PR ( since you built out the requests headers feature :) ) ?

sunjayBhatia · 2022-11-09T18:25:48Z

worth noting theres an existing upstream conformance test for this filter that is currently broken so maybe hold off on enabling it, being fixed in kubernetes-sigs/gateway-api#1520

Xunzhuo · 2022-11-09T18:44:36Z

Thanks @arkodg and @sunjayBhatia, this PR is WIP, I will set @AliceProxy as a reviewer after it is ready:)

codecov-commenter · 2022-11-10T02:05:19Z

Codecov Report

Merging #717 (2e6be16) into main (13c1113) will increase coverage by 0.77%.
The diff coverage is 89.18%.

@@            Coverage Diff             @@
##             main     #717      +/-   ##
==========================================
+ Coverage   61.12%   61.89%   +0.77%     
==========================================
  Files          46       46              
  Lines        5674     5850     +176     
==========================================
+ Hits         3468     3621     +153     
- Misses       1978     1996      +18     
- Partials      228      233       +5

Impacted Files	Coverage Δ
internal/ir/zz_generated.deepcopy.go	`18.36% <0.00%> (-0.49%)`	⬇️
internal/gatewayapi/translator.go	`85.97% <92.48%> (+0.63%)`	⬆️
internal/ir/xds.go	`78.57% <100.00%> (+1.74%)`	⬆️
internal/xds/translator/route.go	`83.33% <100.00%> (+1.98%)`	⬆️
internal/provider/kubernetes/controller.go	`50.09% <0.00%> (-0.59%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tools/make/kube.mk

go.mod

Xunzhuo · 2022-11-10T16:14:01Z

Hey @AliceProxy, this one is mostly ready, just some blockers from GWAPI v0.6.0 release. PTAL when you have some time : )

I have applied latest crds of GWAPI to run the conformance test of ResponseHeaderModifier Filter locally, and it passed : )

I added a doc to explain how to use ResponseHeaderModifier: https://github.com/envoyproxy/gateway/blob/ace87cd7d307e6fc3371e291a94224fe8ffe9895/docs/latest/user/http-response-headers.md

And cc @skriss @youngnick @arkodg @danehans.

arkodg · 2022-11-10T17:03:00Z

Hey @AliceProxy, this one is mostly ready, just some blockers from GWAPI v0.6.0 release. PTAL when you have some time : )

I have applied latest crds of GWAPI to run the conformance test of ResponseHeaderModifier Filter locally, and it passed : )

I added a doc to explain how to use ResponseHeaderModifier: https://github.com/envoyproxy/gateway/blob/ace87cd7d307e6fc3371e291a94224fe8ffe9895/docs/latest/user/http-response-headers.md

And cc @skriss @youngnick @arkodg @danehans.

ha amazing ! recommended performing the GWAPI bump in a separate PR

arkodg · 2022-11-10T17:37:44Z

this PR is blocked until #716 is completed

Signed-off-by: bitliu <bitliu@tencent.com>

docs/latest/user/http-request-headers.md

internal/xds/translator/route.go

internal/xds/translator/translator_test.go

arkodg · 2022-12-08T16:43:26Z

internal/gatewayapi/translator.go

 							)
 						}
+					case v1beta1.HTTPRouteFilterResponseHeaderModifier:
+						// Make sure the header modifier config actually exists


can this code be moved into a function so it can be reused by request headers as well ?

I suggest keeping this for now, and raise another pr to refactor the processing http routes. Not only the response header modifier, but also other filters could be moved to a func.

sounds good, can you please raise a issue to track this

arkodg · 2022-12-08T16:45:30Z

hey @Xunzhuo the PR looks good, thanks for also enabling the conformance tests !
added some minor comments around code refactoring and reuse

Signed-off-by: bitliu <bitliu@tencent.com>

arkodg

LGTM, thanks !

Xunzhuo changed the title ~~feat: add Support for HTTPRoute ResponseHeaderModifier Filter~~ feat: add support for ResponseHeaderModifier Filter Nov 9, 2022

Xunzhuo force-pushed the feat-response-header branch from 3a1ed0c to 6b4d7a6 Compare November 9, 2022 18:52

danehans mentioned this pull request Nov 9, 2022

Bump Echoserver Image Tag #719

Closed

Xunzhuo force-pushed the feat-response-header branch 7 times, most recently from e537ca1 to baf4c49 Compare November 10, 2022 01:59

Xunzhuo force-pushed the feat-response-header branch 10 times, most recently from fe333a3 to 8b61fcb Compare November 10, 2022 15:39

Xunzhuo commented Nov 10, 2022

View reviewed changes

tools/make/kube.mk Outdated Show resolved Hide resolved

Xunzhuo marked this pull request as ready for review November 10, 2022 15:45

Xunzhuo requested a review from a team as a code owner November 10, 2022 15:45

Xunzhuo commented Nov 10, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Xunzhuo force-pushed the feat-response-header branch from 8b61fcb to ace87cd Compare November 10, 2022 16:13

arkodg mentioned this pull request Nov 10, 2022

Update to the v0.6.0 Gateway API release #716

Closed

Xunzhuo added this to the 0.3.0-rc.1 milestone Nov 11, 2022

Xunzhuo marked this pull request as draft November 15, 2022 03:24

Xunzhuo force-pushed the feat-response-header branch 3 times, most recently from 89e779d to 4610578 Compare December 8, 2022 06:15

Xunzhuo marked this pull request as ready for review December 8, 2022 06:41

Xunzhuo added area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. area/xds-server Issues related to the xDS Server used for managing Envoy configuration. area/conformance Gateway API Conformance Related Issues labels Dec 8, 2022

feat: add support for ResponseHeaderModifier Filter

2e6be16

Signed-off-by: bitliu <bitliu@tencent.com>

Xunzhuo force-pushed the feat-response-header branch from 72b4f27 to 2e6be16 Compare December 8, 2022 07:00

Xunzhuo commented Dec 8, 2022

View reviewed changes

docs/latest/user/http-request-headers.md Show resolved Hide resolved

arkodg reviewed Dec 8, 2022

View reviewed changes

internal/xds/translator/route.go Outdated Show resolved Hide resolved

arkodg reviewed Dec 8, 2022

View reviewed changes

internal/xds/translator/translator_test.go Show resolved Hide resolved

arkodg reviewed Dec 8, 2022

View reviewed changes

merge add header

cc73907

Signed-off-by: bitliu <bitliu@tencent.com>

arkodg approved these changes Dec 13, 2022

View reviewed changes

arkodg merged commit f80ad61 into envoyproxy:main Dec 13, 2022

Xunzhuo deleted the feat-response-header branch December 16, 2022 04:21

Xunzhuo mentioned this pull request Dec 22, 2022

feat: add support for URLRewrite filter and refactor GWAPI translator #819

Merged

Conversation

Xunzhuo commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented Nov 9, 2022

Uh oh!

sunjayBhatia commented Nov 9, 2022

Uh oh!

Xunzhuo commented Nov 9, 2022

Uh oh!

codecov-commenter commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Xunzhuo commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented Nov 10, 2022

Uh oh!

arkodg commented Nov 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arkodg Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

Xunzhuo Dec 13, 2022 • 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!

arkodg Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

arkodg commented Dec 8, 2022

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Xunzhuo commented Nov 9, 2022 •

edited

Loading

codecov-commenter commented Nov 10, 2022 •

edited

Loading

Xunzhuo commented Nov 10, 2022 •

edited

Loading

Xunzhuo Dec 13, 2022 •

edited

Loading