feat(HTTPRouteMatcher): implement http method filter #733

secustor · 2021-07-23T14:24:25Z

What type of PR is this?
/kind feature
/kind api-change

What this PR does / why we need it:
This PR adds a HTTPMatcher to allow match one or more specific HTTP methods.
If multiple are defined it is defined as logical OR.

Which issue(s) this PR fixes:

Fixes #624

Does this PR introduce a user-facing change?:

HTTPRoute now supports matching based on HTTP methods.

k8s-ci-robot · 2021-07-23T14:24:32Z

Welcome @secustor!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-07-23T14:24:33Z

Hi @secustor. 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.

brunoarueira · 2021-07-23T14:44:55Z

apis/v1alpha2/httproute_types.go

@@ -408,6 +408,10 @@ type HTTPQueryParamMatch struct {
 	Value string `json:"value"`
 }

+// HTTPMethod describes how to select a HTTP route by matching the HTTP
+// method.
+type HTTPMethod string


Is it an appropriate layer to add some kind of enum to allow specific values? E.g. the http methods defined by the HTTP spec.

See https://datatracker.ietf.org/doc/html/rfc7231#section-4 and here https://datatracker.ietf.org/doc/html/rfc5789#section-2

I thinking more about a way to avoid mistakes when defining the paths and only when the config is applied to not route through to the correct service

Alternatively we could reuse the HttpMethod from the genproto package which is already a dependency. https://pkg.go.dev/google.golang.org/genproto/googleapis/cloud/scheduler/v1#HttpMethod

I like the idea of an enum here. I'd also recommend adding enum validation to the field as shown here: https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/httproute_types.go#L271.

I think I'd prefer defining our own custom types instead of reusing the ones defined in genproto. That will allow us more flexibility in terms of documentation and support levels.

Also appreciate the RFC references by @brunoarueira. Where possible, we like to reference specific RFCs when documenting a feature/field in the comments (example: https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/httproute_types.go#L342).

FWIW there is an IANA registry for HTTP methods

https://www.iana.org/assignments/http-methods/http-methods.xhtml

robscott

Thanks for the work on this!

/ok-to-test

apis/v1alpha2/httproute_types.go

robscott · 2021-07-23T18:40:39Z

apis/v1alpha2/httproute_types.go

@@ -408,6 +408,10 @@ type HTTPQueryParamMatch struct {
 	Value string `json:"value"`
 }

+// HTTPMethod describes how to select a HTTP route by matching the HTTP
+// method.
+type HTTPMethod string


I like the idea of an enum here. I'd also recommend adding enum validation to the field as shown here: https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/httproute_types.go#L271.

I think I'd prefer defining our own custom types instead of reusing the ones defined in genproto. That will allow us more flexibility in terms of documentation and support levels.

Also appreciate the RFC references by @brunoarueira. Where possible, we like to reference specific RFCs when documenting a feature/field in the comments (example: https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/httproute_types.go#L342).

apis/v1alpha2/httproute_types.go

…num instead of string

robscott

Mostly LGTM, just a few nits.

apis/v1alpha2/httproute_types.go

robscott · 2021-07-26T18:56:14Z

/cc @bowei @hbagdi @youngnick

hbagdi · 2021-07-26T19:47:27Z

apis/v1alpha2/httproute_types.go

+	HTTPMethodConnect   HTTPMethod = "CONNECT"
+	HTTPMethodOptions   HTTPMethod = "OPTIONS"
+	HTTPMethodTrace     HTTPMethod = "TRACE"
+	HTTPMethodPatch     HTTPMethod = "PATCH"


I'm not sure if this is a good idea or not.
Custom HTTP methods are a thing, although not common in practice.
For the 99% of the cases, these defined methods will suffice.
If there comes a time when we want to support custom HTTP methods, how can we do so without a breaking change?

If I'm not mistaken it is already possible to match the HTTP method via header value match.
In this case we could reference the user to that method.

The alternatives are dropping enum and revert to string or a more complex structure instead of the simple key-value pair.

If I'm not mistaken it is already possible to match the HTTP method via header value match.

How so?

cc @robscott @jpeach

With :method pseudo-header (https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.1)

If we go down this route we need to make it explicit in the spec that implementation must accept pseudo-headers though.

If we go down this route we need to make it explicit in the spec that implementation must accept pseudo-headers though.

We should clarify this regardless. Else implementations will drift in this aspect.

Yup, +1 on defining pseudo-header matching. Controllers can map the pseudo header name to the underlying proxy control.

I opened up #740 to track this conversation.

My only slight hesitation is that then there are two ways to do the same thing. In this case, that might be okay because custom HTTP methods are not common. Not really sure.

apis/v1alpha2/httproute_types.go

…pport form Core to Extended

apis/v1alpha2/httproute_types.go

Co-authored-by: Harry <[email protected]>

hbagdi

I was about to lgtm this but found a couple of small issues.

apis/v1alpha2/httproute_types.go

robscott · 2021-08-02T21:12:10Z

Thanks! Will leave LGTM for someone else.

/approve

k8s-ci-robot · 2021-08-02T21:12:18Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 · 2021-08-02T22:00:23Z

@secustor Thank you for your contribution!
/lgtm

feat(HTTPRouteMatcher): implement http method filter

b5dbf16

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jul 23, 2021

k8s-ci-robot requested review from danehans and jpeach July 23, 2021 14:24

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 23, 2021

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 23, 2021

brunoarueira reviewed Jul 23, 2021

View reviewed changes

robscott reviewed Jul 23, 2021

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 Jul 23, 2021

robscott reviewed Jul 23, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

feat(HTTPRouteMatcher): move method matcher to single value and use e…

2e34ed7

…num instead of string

secustor requested a review from robscott July 24, 2021 20:29

robscott reviewed Jul 26, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

k8s-ci-robot requested review from bowei, hbagdi and youngnick July 26, 2021 18:56

feat(HTTPRouteMatcher): use pointer and wrap at 80 chars

d654b18

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 Jul 26, 2021

hbagdi reviewed Jul 26, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

fix(HTTPRouteMatcher): rename HTTPMethodHeader to HTTPMethodHead

90939fd

robscott reviewed Jul 27, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

robscott added this to the v1alpha2 milestone Jul 28, 2021

hbagdi mentioned this pull request Jul 29, 2021

Clarify if psuedo-headers are supported within HTTP header matching #740

Closed

fix(HTTPRouteMatcher): remove NONE from HTTPMethod enum and change Su…

6e79b1c

…pport form Core to Extended

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

secustor requested review from hbagdi and robscott July 29, 2021 17:55

hbagdi reviewed Jul 29, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

secustor and others added 3 commits July 30, 2021 09:44

Update apis/v1alpha2/httproute_types.go

5c3e7c9

Co-authored-by: Harry <[email protected]>

Update apis/v1alpha2/httproute_types.go

59c049d

Co-authored-by: Harry <[email protected]>

chore(HTTPRouteMatcher): regenerate crd

f1db181

secustor requested a review from hbagdi July 31, 2021 11:39

hbagdi reviewed Aug 2, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

chore(HTTPRouteMatcher): move method and fix kubebuilder validation

c3811b9

robscott reviewed Aug 2, 2021

View reviewed changes

apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved

chore(HTTPRouteMatcher): wrap after 80chars

5eebc00

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 Aug 2, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2021

robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 2, 2021

k8s-ci-robot assigned hbagdi Aug 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2021

k8s-ci-robot merged commit c4a3a3e into kubernetes-sigs:master Aug 2, 2021

robscott mentioned this pull request Aug 3, 2021

Moving method match to v1alpha2 example #742

Merged

youngnick mentioned this pull request Aug 18, 2021

Migrate to Gateway API v1alpha2 projectcontour/contour#3954

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(HTTPRouteMatcher): implement http method filter #733

feat(HTTPRouteMatcher): implement http method filter #733

secustor commented Jul 23, 2021 •

edited by hbagdi

Loading

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

brunoarueira Jul 23, 2021

secustor Jul 23, 2021 •

edited

Loading

robscott Jul 23, 2021

jpeach Jul 27, 2021

robscott left a comment

robscott Jul 23, 2021

robscott left a comment

robscott commented Jul 26, 2021

hbagdi Jul 26, 2021

secustor Jul 26, 2021

hbagdi Jul 26, 2021

howardjohn Jul 26, 2021

hbagdi Jul 26, 2021

jpeach Jul 27, 2021

hbagdi Jul 29, 2021

hbagdi left a comment

robscott commented Aug 2, 2021

k8s-ci-robot commented Aug 2, 2021

hbagdi commented Aug 2, 2021

feat(HTTPRouteMatcher): implement http method filter #733

feat(HTTPRouteMatcher): implement http method filter #733

Conversation

secustor commented Jul 23, 2021 • edited by hbagdi Loading

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

Choose a reason for hiding this comment

secustor Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

robscott commented Jul 26, 2021

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

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

robscott commented Aug 2, 2021

k8s-ci-robot commented Aug 2, 2021

hbagdi commented Aug 2, 2021

secustor commented Jul 23, 2021 •

edited by hbagdi

Loading

secustor Jul 23, 2021 •

edited

Loading