-
Notifications
You must be signed in to change notification settings - Fork 598
Singularizes and expands godocs of HTTPRoute hostname #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,11 +34,24 @@ type HTTPRouteSpec struct { | |
|
||
// HTTPRouteHost is the configuration for a given host. | ||
type HTTPRouteHost struct { | ||
// Hostnames are a list of hosts names that match this host | ||
// block. | ||
// Hostname is the fully qualified domain name of a network host, | ||
// as defined by RFC 3986. Note the following deviations from the | ||
// "host" part of the URI as defined in the RFC: | ||
// | ||
// TODO: RFC link | ||
Hostnames []string `json:"hostnames"` | ||
// 1. IPs are not allowed. | ||
// 2. The `:` delimiter is not respected because ports are not allowed. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// | ||
// Incoming requests are matched against Hostname before processing HTTPRoute | ||
// rules. For example, if the request header contains host: foo.example.com, | ||
// an HTTPRoute with hostname foo.example.com will match. However, an | ||
// HTTPRoute with hostname example.com or bar.example.com will not match. | ||
// If Hostname is unspecified, the Gateway routes all traffic based on | ||
// the specified rules. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does How does SNI fit in? I would guess that for a TLS connection, first some certificate must match, and then the ingress controller uses the first route that also matches? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Miciah this is a good point. @bowei do you agree that the value should be unique?. I don't see any mention of this in the ingress v1beta api docs.
I would expect similar behavior as ingress v1beta1. From the ingress docs:
Translating the above doc to ingress v2, I would expect the following flow for ingress https connections:
@bowei feel free to chime in if ^ is incorrect. It would also be helpful to understand if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to singularize the hostname? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be singular but no unique.
TLS and Routing details should not be clubbed together. Although, there is leakage between the layers of the OSI model, they are largely different layers. @danehans, in your description, I think Step 2 and 3 should be changed. Regarding, Sandor's point, I think this can be opened up to allow for prefix and suffix wildcards in future if there is a need for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case of HTTPRoutes, the gateway can terminate TLS and then use HTTP to select a HTTPRoute, but in the case of a TCPRoute, HTTP headers are unavailable. So faced with an incoming TLS request, the gateway could first use SNI to build a set of candidate routes, which must comprise either multiple HTTPRoutes or a single TCPRoute, and then the gateway could either (a) terminate TLS and use HTTP metadata to select a single HTTPRoute or (b) use the TLSRoute. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@haiyanmeng I submitted #68 to doc a typical ingress request flow, ptal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reference for uniqueness: #67 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm inclined to merge this and continue conversation about the wildcard on an issue specific to having wildcards. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// | ||
// Support: Core | ||
// | ||
// +optional | ||
Hostname string `json:"hostname,omitempty"` | ||
|
||
// Rules are a list of HTTP matchers, filters and actions. | ||
Rules []HTTPRouteRule `json:"rules"` | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Would it be simpler to say that the value must be a
reg-name
per RFC 3986?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 stating "IPs are not allowed" will make sense to most, if not all, users. I just spent a few minutes reviewing https://tools.ietf.org/html/rfc3986 and I still don't understand what a
reg-name
is. Maybe we can iterate on the godoc for this field to includereg-name
?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.
It is useful to state explicitly that IP addresses are prohibited, fair enough. I think referencing the specific syntax item is also useful. From the RFC: