Tightening validation + adding basic validation tests #772
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,6 +262,7 @@ type HTTPPathMatch struct { | |
| // | ||
| // +optional | ||
| // +kubebuilder:default="/" | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| Value *string `json:"value,omitempty"` | ||
| } | ||
|
|
||
|
|
@@ -282,6 +283,24 @@ const ( | |
| HeaderMatchImplementationSpecific HeaderMatchType = "ImplementationSpecific" | ||
| ) | ||
|
|
||
| // HTTPHeaderName is the name of an HTTP header. | ||
|
There was a problem hiding this comment. Do we need to say it's case-insensitive? There was a problem hiding this comment. We say in the place where this type is used. I'm not sure what is the right place for it. The place of use seems more correct because that gets added to the CRD. I can argue either way though. There was a problem hiding this comment. I think it's more appropriate to specify that it's case insensitive on the match field itself (current state). There's nothing on this type definition that can require that matching is case insensitive. There was a problem hiding this comment. I'm ok with anything as long as the generated documentation is clear. |
||
| // | ||
| // Valid values include: | ||
| // | ||
| // * "Authorization" | ||
| // * "Set-Cookie" | ||
| // | ||
| // Invalid values include: | ||
| // | ||
| // * ":method" - ":" is an invalid character. This means that pseudo headers are | ||
| // not currently supported by this type. | ||
| // * "/invalid" - "/" is an invalid character | ||
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=256 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$` | ||
| type HTTPHeaderName string | ||
|
|
||
| // HTTPHeaderMatch describes how to select a HTTP route by matching HTTP request | ||
| // headers. | ||
| type HTTPHeaderMatch struct { | ||
|
|
@@ -314,10 +333,7 @@ type HTTPHeaderMatch struct { | |
| // Generally, proxies should follow the guidance from the RFC: | ||
| // https://www.rfc-editor.org/rfc/rfc7230.html#section-3.2.2 regarding | ||
| // processing a repeated header, with special handling for "Set-Cookie". | ||
| Name HTTPHeaderName `json:"name"` | ||
|
|
||
| // Value is the value of HTTP Header to be matched. | ||
| // | ||
|
|
@@ -423,14 +439,20 @@ type HTTPRouteMatch struct { | |
| // ANDed together, meaning, a request must match all the specified headers | ||
| // to select the route. | ||
| // | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Headers []HTTPHeaderMatch `json:"headers,omitempty"` | ||
robscott marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // QueryParams specifies HTTP query parameter matchers. Multiple match | ||
| // values are ANDed together, meaning, a request must match all the | ||
| // specified query parameters to select the route. | ||
| // | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=16 | ||
|
There was a problem hiding this comment. Is there any harm to increase this to 30-50? I can see hitting 16 limit, probably not 30 There was a problem hiding this comment. I hadn't really thought of a use case with that many matches, but I can increase to 32. There was a problem hiding this comment. Actually we're using 16 as a limit in a lot of places, including header matching. Unless there's a clear use case I'd be tempted to leave that as is to minimize the changes here. |
||
| QueryParams []HTTPQueryParamMatch `json:"queryParams,omitempty"` | ||
|
|
||
| // Method specifies HTTP method matcher. | ||
|
|
@@ -574,10 +596,7 @@ type HTTPHeader struct { | |
| // entries with an equivalent header name MUST be ignored. Due to the | ||
| // case-insensitivity of header names, "foo" and "Foo" are considered | ||
| // equivalent. | ||
| Name HTTPHeaderName `json:"name"` | ||
|
|
||
| // Value is the value of HTTP Header to be matched. | ||
| // | ||
|
|
@@ -604,6 +623,9 @@ type HTTPRequestHeaderFilter struct { | |
| // my-header: bar | ||
| // | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Set []HTTPHeader `json:"set,omitempty"` | ||
|
|
||
| // Add adds the given header(s) (name, value) to the request | ||
|
|
@@ -623,6 +645,9 @@ type HTTPRequestHeaderFilter struct { | |
| // my-header: bar | ||
| // | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Add []HTTPHeader `json:"add,omitempty"` | ||
|
|
||
| // Remove the given header(s) from the HTTP request before the | ||
|
|
@@ -659,29 +684,32 @@ type HTTPRequestRedirect struct { | |
| // +optional | ||
| // +kubebuilder:validation:Enum=HTTP;HTTPS | ||
| Protocol *string `json:"protocol,omitempty"` | ||
|
|
||
| // Hostname is the hostname to be used in the value of the `Location` | ||
| // header in the response. | ||
| // When empty, the hostname of the request is used. | ||
| // | ||
| // Support: Core | ||
| // | ||
| // +optional | ||
| Hostname *Hostname `json:"hostname,omitempty"` | ||
|
|
||
| // Port is the port to be used in the value of the `Location` | ||
| // header in the response. | ||
| // When empty, port (if specified) of the request is used. | ||
| // | ||
| // Support: Extended | ||
| // | ||
| // +optional | ||
| Port *PortNumber `json:"port,omitempty"` | ||
|
|
||
| // StatusCode is the HTTP status code to be used in response. | ||
| // | ||
| // Support: Core | ||
| // | ||
| // +optional | ||
| // +kubebuilder:default=302 | ||
| // +kubebuilder:validation:Enum=301;302 | ||
|
There was a problem hiding this comment. I feel like we should just include all the valid redirects from here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#redirection_messages There was a problem hiding this comment. Agree that we should add this, but didn't want to expand the scope of this PR so added #793 to track this. There was a problem hiding this comment. There was a conformance concern for not adding them in the first place. |
||
| StatusCode *int `json:"statusCode,omitempty"` | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apparently 2000 is the accepted folk limit around the web
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opinion:
Can you imagine anyone using a 2000 character path to do matching though?
If anything, I think we are already generous here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's likely safer to start with the lower limit and expand if necessary. At least that's been our practice so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? There is data that 2000 is effectively the max which is well known. For the others, yes, we have less info so just have to pick a number. I'm not super tied to increase this -- just that we know that more about what the max is.