Adding QueryParam matching to HTTPRoute #631
Conversation
k8s-ci-robot
requested review from
bowei,
danehans,
hbagdi,
jpeach and
youngnick
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
apis/v1alpha1/httproute_types.go
Outdated
| // QueryParamMatchType constants. | ||
| const ( | ||
| QueryParamMatchExact HeaderMatchType = "Exact" | ||
| QueryParamMatchPresence HeaderMatchType = "Presence" |
There was a problem hiding this comment.
I'm not very familiar with query parameter matching myself, is Presence a commonly supported or asked one?
There was a problem hiding this comment.
https://www.envoyproxy.io/docs/envoy/latest/operations/admin#post--drain_listeners is one example in the wild where a Presence could be useful?
It does seem inconsistent with Header at least.
There was a problem hiding this comment.
To be more clear, I mean there should probably be one common way to describe this in the API. So add Presence to Header or do something else, but they should be consistent.
I think you could express it with regex of "" possibly.
For a prior art, Istio has ~everything as the same StringMatch type: https://istio.io/latest/docs/reference/config/networking/virtual-service/#StringMatch
There was a problem hiding this comment.
So add Presence to Header or do something else, but they should be consistent.
But then Presence as PathType makes little sense.
I think it is okay to have different match types for different metadata pieces like query string, header, path, etc because they have different properties and uses.
There was a problem hiding this comment.
I agree they don't need to be the exact same type, but they should be consistent where it makes sense? Presence in path definitely would be odd. I see headers and query params as pretty similar so I would envision them having the same options
There was a problem hiding this comment.
they should be consistent where it makes sense?
We need to dig deeper into what consistency means here.
I agree that they should be consistent for the end-user - kubectl user, meaning having a RegularExpression match on path/header/query param should do the same thing and not surprise the user.
For the implementers, I think having separate Go types and different subsets is okay and good because that makes this API (our go package) easy to use.
There was a problem hiding this comment.
Yeah that makes sense, I'm fine with dropping Presence from this PR to keep it smaller in scope. We can add it later to both if there's interest.
There was a problem hiding this comment.
I was only talking about the yaml API surface. Whether we use a single or different go type I don't have a strong opinion, dedicated types seems reasonable.
There was a problem hiding this comment.
Yep agree that we should still use separate Go types, kept those in place while dropping "Presence"
apis/v1alpha1/httproute_types.go
Outdated
| // Please read the implementation's documentation to determine the supported | ||
| // dialect. | ||
| // | ||
| // HTTP query parameter matching MUST be case-sensitive. |
There was a problem hiding this comment.
Both key and value? If yes, let's be precise.
There was a problem hiding this comment.
Agreed, clarified that both key and value should be case-sensitive. This is based on the following guidance from the RFC (emphasis mine):
The scheme and host are case-insensitive and normally provided in lowercase; all other components are compared in a case-sensitive manner.
There was a problem hiding this comment.
Case-sensitive is a source of confusion and conflict usually, although I'm not sure how much of that is a problem with query param matching.
Can we include the link to the RFC and mention the section which states the case-sensitiveness?
|
Looks good to me. |
| Type *QueryParamMatchType `json:"type,omitempty"` | ||
|
|
||
| // Values is a map of HTTP query parameters to be matched. It MUST contain | ||
| // at least one entry. |
There was a problem hiding this comment.
Nit: If this must contain at least one entry, should we set the validation minimum?
There was a problem hiding this comment.
Unfortunately it does not look like it's possible to validate that with annotations. It does seem like something we should catch with the webhook though (for both this and header matching).
| // query parameter is the map value. | ||
| // | ||
| // Multiple match values are ANDed together, meaning, a request must match | ||
| // all the specified query parameters to select the route. |
There was a problem hiding this comment.
Nit: I feel like we should be clear that, even if you specify one of the ImplementationSpecific matches, we shouldn't allow regexes or other craziness in the query param name, just the value?
Something like:
| // all the specified query parameters to select the route. | |
| // all the specified query parameters to select the route. | |
| // | |
| // Note that the query param type matching MUST only be used for the query param value, not the query param name. |
There was a problem hiding this comment.
More direct would be (don't refer to param matching type to avoid indirection):
Query parameter key is must be an exact match (string comparison).
There was a problem hiding this comment.
Good call, added in #636
k8s-ci-robot
added
the
lgtm
This is a small follow up to kubernetes-sigs#631 that addresses some of the tiny nits that were leftover on that PR.
This is a small follow up to kubernetes-sigs#631 that addresses some of the tiny nits that were leftover on that PR.
|
|
||
| // QueryParamMatchType constants. | ||
| const ( | ||
| QueryParamMatchExact QueryParamMatchType = "Exact" |
There was a problem hiding this comment.
These need godoc comments for the generated API reference.
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This adds query param matching to HTTPRoute
Which issue(s) this PR fixes:
Fixes #259
Does this PR introduce a user-facing change?:
/cc @bowei @danehans @hbagdi @jpeach @youngnick