Add Set field to HTTPRequestHeaderFilter #475

nak3 · 2020-11-24T13:17:36Z

This patch adds Set field to HTTPRequestHeaderFilter.

Currently Add(append) and Remove are available in
HTTPRequestHeaderFilter. But it is not possible to overwrite a
header.

k8s-ci-robot · 2020-11-24T13:17:44Z

Hi @nak3. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

howardjohn

LGTM

hbagdi · 2020-11-24T16:25:02Z

apis/v1alpha1/httproute_types.go

+	//
+	// Support: Extended
+	// +optional
+	Set map[string]string `json:"set,omitempty"`


+1 to this change.
One concern. Can most implementations differentiate between addition and override?

From what I can tell, adding headers is pretty widely supported among other implementations, but setting/overwriting headers is certainly trickier. HAProxy has a very clear differentiation that would match this config closely (add-header and set-header). The distinction is less clear with nginx (add_header and proxy_set_header but defined in different modules), but fairly straightforward with envoy. ~~and I think you might need a custom filter to overwrite a header with envoy~~.

With all that said, I think this is a good addition for the implementations that can support it, and the extended support level feels appropriate.

hbagdi · 2020-11-24T16:32:45Z

/ok-to-test

robscott · 2020-11-24T18:02:13Z

Thanks for this addition @nak3! We're still trying to figure out what we'd like to include in updates to the v1alpha1 API vs new work on the v1alpha2 API. We should be able to discuss this more at our community meeting next week. Until then, I'd like to hold this change to ensure we're not adding more than we want to to v1alpha1.

/hold

howardjohn · 2020-11-24T18:08:53Z

Envoy can do this easily, see `append` flag: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/base.proto.html?highlight=headervalueoption#config-core-v3-headervalueoption FWIW its part of Istio API as well.

…

On Tue, Nov 24, 2020 at 10:02 AM Rob Scott ***@***.***> wrote: Thanks for this addition @nak3 <https://github.com/nak3>! We're still trying to figure out what we'd like to include in updates to the v1alpha1 API vs new work on the v1alpha2 API. We should be able to discuss this more at our community meeting next week. Until then, I'd like to hold this change to ensure we're not adding more than we want to to v1alpha1. /hold — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#475 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXJFOCFHFLPOT7WSGFTSRPYLNANCNFSM4UA4AZTQ> .

robscott · 2020-12-10T20:33:28Z

We discussed this a bit further yesterday and this seemed like a reasonable change. We also got #492 merged in yesterday which clarifies that we can make additive changes to an existing API. With that I think this is mostly good to go.

/hold cancel
/retest

robscott · 2020-12-10T20:34:44Z

I'm going to approve this and wait for a final LGTM from @hbagdi or @jpeach.

/approve

k8s-ci-robot · 2020-12-10T20:34:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hbagdi

This is very close. I've a couple of small documentation improvements.

hbagdi · 2020-12-11T17:10:41Z

apis/v1alpha1/httproute_types.go

+	//
+	// Input:
+	//   GET /foo HTTP/1.1
+	//   My-Header: ABC


Can we lower case this?
Header field names are case-insensitive but keeping this example consistent throughout the documentation would be ideal.

hbagdi · 2020-12-11T17:12:06Z

apis/v1alpha1/httproute_types.go

+	// Support: Extended
+	// +optional
+	Set map[string]string `json:"set,omitempty"`
+
 	// Add adds the given header (name, value) to the request


Since now we have Add and Set, we should clarify here what happens when a header already exists in the Add case.

nak3 · 2020-12-14T08:35:03Z

apis/v1alpha1/httproute_types.go

@@ -456,16 +476,16 @@ type HTTPRequestHeaderFilter struct {
 	//
 	// Input:
 	//   GET /foo HTTP/1.1
-	//   My-Header1: ABC
-	//   My-Header2: DEF
-	//   My-Header2: GHI


I believe this is a typo. Fixed by s/My-Header2/My-Heade3 in this PR.

hbagdi · 2020-12-14T16:19:20Z

apis/v1alpha1/httproute_types.go

+	//
+	// Output:
+	//   GET /foo HTTP/1.1
+	//   my-header: foo bar


This seems incorrect. Header values are allowed to contain whitespaces.
The usual textual representation for multiple headers is:

// GET /foo HTTP/1.1 // my-header: foo // my-header: bar

Thank you! Updated.

apis/v1alpha1/httproute_types.go

hbagdi · 2020-12-15T16:16:54Z

/lgtm

@jpeach How does #475 (comment) sound to you? We had discussed in the office hours to track the ordering bug separately.
/hold
Please cancel the hold if that is resolved.

There is another minor thing where if the PR was based on the latest master then the PR description could add release notes. This is minor enough that we can ignore.

nak3 · 2020-12-18T01:15:12Z

@jpeach Friendly ping. Could you please check above comment?

jpeach · 2020-12-20T01:08:47Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 24, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 24, 2020

k8s-ci-robot requested review from hbagdi and jpeach November 24, 2020 13:17

howardjohn reviewed Nov 24, 2020

View reviewed changes

hbagdi reviewed Nov 24, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 24, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2020

hbagdi reviewed Dec 11, 2020

View reviewed changes

nak3 added 3 commits December 14, 2020 10:54

Add Set to HTTPRequestHeaderFilter

be97300

Auto gen code

61ca462

Clarify Add function if a header already exists.

5c752d6

nak3 force-pushed the add-set branch from 1eb897c to 5c752d6 Compare December 14, 2020 08:32

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 14, 2020

nak3 commented Dec 14, 2020

View reviewed changes

hbagdi reviewed Dec 14, 2020

View reviewed changes

jpeach reviewed Dec 14, 2020

View reviewed changes

apis/v1alpha1/httproute_types.go Show resolved Hide resolved

Fix add header accurately

f58f741

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2020

k8s-ci-robot assigned hbagdi Dec 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2020

k8s-ci-robot merged commit 8a8b4e7 into kubernetes-sigs:master Dec 20, 2020

nak3 deleted the add-set branch January 23, 2021 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Set field to HTTPRequestHeaderFilter #475

Add Set field to HTTPRequestHeaderFilter #475

nak3 commented Nov 24, 2020

k8s-ci-robot commented Nov 24, 2020

howardjohn left a comment

hbagdi Nov 24, 2020

robscott Nov 24, 2020 •

edited

hbagdi commented Nov 24, 2020

robscott commented Nov 24, 2020 •

edited

howardjohn commented Nov 24, 2020 via email

robscott commented Dec 10, 2020

robscott commented Dec 10, 2020

k8s-ci-robot commented Dec 10, 2020

hbagdi left a comment

hbagdi Dec 11, 2020

hbagdi Dec 11, 2020

nak3 Dec 14, 2020

hbagdi Dec 14, 2020

nak3 Dec 15, 2020

hbagdi commented Dec 15, 2020

nak3 commented Dec 18, 2020

jpeach commented Dec 20, 2020

Add Set field to HTTPRequestHeaderFilter #475

Add Set field to HTTPRequestHeaderFilter #475

Conversation

nak3 commented Nov 24, 2020

k8s-ci-robot commented Nov 24, 2020

howardjohn left a comment

Choose a reason for hiding this comment

hbagdi Nov 24, 2020

Choose a reason for hiding this comment

robscott Nov 24, 2020 • edited

Choose a reason for hiding this comment

hbagdi commented Nov 24, 2020

robscott commented Nov 24, 2020 • edited

howardjohn commented Nov 24, 2020 via email

robscott commented Dec 10, 2020

robscott commented Dec 10, 2020

k8s-ci-robot commented Dec 10, 2020

hbagdi left a comment

Choose a reason for hiding this comment

hbagdi Dec 11, 2020

Choose a reason for hiding this comment

hbagdi Dec 11, 2020

Choose a reason for hiding this comment

nak3 Dec 14, 2020

Choose a reason for hiding this comment

hbagdi Dec 14, 2020

Choose a reason for hiding this comment

nak3 Dec 15, 2020

Choose a reason for hiding this comment

hbagdi commented Dec 15, 2020

nak3 commented Dec 18, 2020

jpeach commented Dec 20, 2020

robscott Nov 24, 2020 •

edited

robscott commented Nov 24, 2020 •

edited