Switching Header and Query Param matching to use lists

[robscott]

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

What this PR does / why we need it:
This matches the API conventions that prefer lists of named subobjects over maps.

Does this PR introduce a user-facing change?:

Header and Query Param matching are now represented with lists of named subobjects instead of maps.

[k8s-ci-robot]

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

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

Question for clarification:

headers:
- name: foo
  value: bar
- name: Foo
   value: bar1

What happens if a request with header foo: bar1 comes in? Is it matched or not?
I'm not sure if I clearly understand the meaning of "precedence" here.

[robscott]

Yeah, this could be clarified better. I was trying to say foo: bar should be given precedence here since it appears first in the list. That means that foo: bar1 would not match (unless type was "Prefix").

[jpeach]

This is in conflict with the general rule, which is "multiple match values are ANDed". So I'm not sure that we need to add the complexity of a special case for duplicates (as a quality of implementation issue, maybe validation could reject this case or a controller could emit a warning).

[robscott]

so in the above example no requests should match because the result of an AND here would always be false?

[jpeach]

so in the above example no requests should match because the result of an AND here would always be false?

Yup, exactly.

[robscott]

There's a lot of nuance here and it's difficult to get the wording right. For example, the following would be valid:

- name: foo
  type: RegularExpression
  value: [a-z]
- name: Foo
  type: RegularExpression
  value: [a-z0-9]

It may not actually make any sense to configure a Route that way, but a value like z would be valid. And similar ambiguity would likely also exist for ImplementationSpecific match types. So the only thing we can say for sure is the following:

Specifying different values with an exact match type for equivalent header names will not match any requests.

And once I took the time to write that out, it seemed obvious, so I just omitted it. Maybe providing the specific example you suggested would be a better way to explain this than the sentence I have above?

[robscott]

Added an example here that I think helps explain how this should be interpreted.

[bowei]

Actually given the regex example, we should just say if you repeat items with the same name, it will be "implementation specific" how this is implemented. Some infra may not support AND'ing regex together -- and ANDing regex is not an operator that is easy to do with a regex.

[robscott]

Good point that ANDing a regex is simply not a practical expectation here. Instead of falling back to implementation specific handling of this, I think it may make the most sense simply to revert back to something resembling the original approach. Specifying that implementations must ignore subsequent matches for equivalent names and only implement the first one should be relatively easy to implement for everyone and results in a consistent experience.

[bowei]

+1 makes sense, this is both easy to implement and consistent for users.

[stevesloka]

lgtm with @hbagdi's comments. =)

[jpeach]

On May 13, 2021 at 3:37 PM, Harry ***@***.***> wrote: @hbagdi commented on this pull request. In apis/v1alpha2/httproute_types.go:

@@ -406,15 +409,24 @@ type HTTPRouteMatch struct {

// +kubebuilder:default={type: "Prefix", value: "/"} Path *HTTPPathMatch `json:"path,omitempty"` - // Headers specifies a HTTP request header matcher. + // Headers specifies HTTP request header matchers. Multiple match values are + // ANDed together, meaning, a request must match all the specified headers + // to select the route. + // + // If multiple entries specify equivalent header names, the first entry with + // an equivalent name MUST be given precedence. Due to the + // case-insensitivity of header names, "foo" and "Foo" are considered + // equivalent. That is hard to understand based on the current language. I've a suggestion to improve the language a little bit here. I think having validation to prevent the user from providing two different values for the same header with type Exact is a good idea. We should open an issue to track it if you all agree. We can just be more explicit about  cane-insensitivity of HTTP headers (i.e. call it out in a separate sentence up front rather than an aside at the end). I agree that adding duplicate detection in the validation is good, but you will want to be careful with character sets (ie. USASCII, not UTF8).

[hbagdi]

I found a bug in our verify script and opened up #670 to fix it.
/lgtm

[hbagdi]

@robscott It seems the examples didn't pass validation, I'm not sure why they failed. Could you rebase on the latest HEAD on our dev branch?

[robscott]

@hbagdi it was actually a great catch - I had Name as the json field name instead of name. Should be fixed now.

[hbagdi]

/lgtm