-
Couldn't load subscription status.
- Fork 607
Expand and clarify Listeners definition #2288
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 2 commits
4f7706c
9e00746
3bf6c14
8a9acfb
c390381
3208a1f
dd27869
5af8b65
c6912a5
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 | ||
|---|---|---|---|---|
|
|
@@ -85,38 +85,52 @@ type GatewaySpec struct { | |||
| // | ||||
| // Port and protocol combinations not listed above are considered Extended. | ||||
| // | ||||
| // An implementation MAY group Listeners by Port and then collapse each | ||||
| // group of Listeners into a single Listener if the implementation | ||||
| // determines that the Listeners in the group are "compatible". An | ||||
| // implementation MAY also group together and collapse compatible | ||||
| // Listeners belonging to different Gateways. | ||||
| // | ||||
| // For example, an implementation might consider Listeners to be | ||||
| // compatible with each other if all of the following conditions are | ||||
| // met: | ||||
| // | ||||
| // 1. Either each Listener within the group specifies the "HTTP" | ||||
| // Protocol or each Listener within the group specifies either | ||||
| // the "HTTPS" or "TLS" Protocol. | ||||
| // | ||||
| // 2. Each Listener within the group specifies a Hostname that is unique | ||||
| // within the group. | ||||
| // | ||||
| // 3. As a special case, one Listener within a group may omit Hostname, | ||||
| // in which case this Listener matches when no other Listener | ||||
| // matches. | ||||
| // | ||||
| // If the implementation does collapse compatible Listeners, the | ||||
| // hostname provided in the incoming client request MUST be | ||||
| // matched to a Listener to find the correct set of Routes. | ||||
| // The incoming hostname MUST be matched using the Hostname | ||||
| // field for each Listener in order of most to least specific. | ||||
| // That is, exact matches must be processed before wildcard | ||||
| // A Gateway's Listeners are considered "compatible" if: | ||||
| // | ||||
| // 1. The implementation can serve them in compliance with the Addresses | ||||
| // requirement that all Listeners are available on all assigned | ||||
| // addresses. | ||||
| // 2. No Listeners sharing the same Port share the same Hostname value, | ||||
| // including the empty value, if this would prevent the implementation | ||||
| // from matching an inbound request to a specific Listener. | ||||
| // | ||||
| // Compatible combinations in Extended support are expected to vary across | ||||
| // implementations. A combination that is compatible for one implementation | ||||
| // may not be compatible for another. | ||||
| // | ||||
| // If this field specifies multiple Listeners that are not compatible, the | ||||
| // implementation MUST raise a true "Conflicted" condition in the Listener | ||||
| // Status. | ||||
rainest marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| // | ||||
| // Implementations MAY choose to still accept a Gateway with conflicted | ||||
| // Listeners if they accept a partial Listener set that contains no | ||||
| // incompatible Listeners. They MUST set a "ListenersNotValid" condition | ||||
|
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. Nit: ListenersNotValid is a reason that can be used with "Accepted", generally to set the condition to "False". I'm not sure what we'd recommend in the case where Listeners were not valid and the Gateway was accepted. 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. We say it can be used when Accepted is True. From the first point in my self-review, this is indeed a confusing case, but probably one that's intentionally ambiguous because we expect it to vary so much between implementations. If we want to leave that as-is, the change here is just to say (here) that implementations must set this, whereas previously it wasn't a strict requirement, and wasn't obvious unless you looked through the reason comments. Even if it changes, I think we should mention something here. Changing the current vague guidelines to something more formal would be a significant breaking change. Between that and the expected variance across vendors, my vote would be to defer it to post GA, when we have more practical experience regarding which approaches are in use and their pros and cons. |
||||
| // the Gateway Status when the Gateway contains incompatible Listeners | ||||
| // whether or not they accept the Gateway. | ||||
|
Comment on lines
+107
to
+111
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 the only reason we'd want to allow this is if a Gateway was already programmed with compatible listeners and then invalid/incompatible one(s) were added. It gets really tricky to represent this state. I may be remembering wrong here, but I think on GKE we will leave "Programmed" set to "True" with the last generation it was Programmed, but I don't think we have any clear guidance for these kinds of partially valid states in the spec. 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. |
||||
| // | ||||
| // For example, the following Listener scenarios may be compatible | ||||
| // depending on implementation capabilities: | ||||
| // | ||||
| // 1. Multiple Listeners with the same Port that all use the "HTTP" | ||||
|
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. this example is a core feature
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. Compatibility is weird because it ends up being about implementation capabilities rather than rules in the spec. If your implementation isn't compatible with something the core requires, it just can't implement the spec. We could bring this further out of core by making it something outside the language above and mentioning specific ports, e.g. "Multiple Listeners with Port "9999" that all use the "HTTP" protocol (and similar for the other examples, just to make them consistent). We could also separate examples that are within core or outside it, but it wouldn't be great for brevity. |
||||
| // Protocol that all have unique Hostname values. | ||||
|
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 know this is just an example, so it may not be critical here, but it might be helpful to define whether 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. 8a9acfb adds this to the following Hostname precedence section. Do you think that works? It's not explicitly saying they are considered distinct, but it is using them as distinct values in the context where it matters. |
||||
| // 2. Multiple Listeners with the same Port that use either the "HTTPS" or | ||||
| // "TLS" Protocol that all have unique Hostname values. | ||||
| // 3. A mixture of "TCP" and "UDP" Protocol Listeners, where no Listener | ||||
| // with the same Protocol has the same Port value. | ||||
| // | ||||
| // An implementation that cannot serve both TCP and UDP listens on the same | ||||
rainest marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| // address, or cannot mix HTTPS and generic TLS listens on the same port | ||||
| // would not consider those cases compatible. | ||||
| // | ||||
| // Implementations using the Hostname value to select between same-Port | ||||
| // Listeners MUST match inbound request hostnames from the most specific | ||||
| // to least specific Hostname values to find the correct set of Routes. | ||||
| // Exact matches must be processed before wildcard matches, and wildcard | ||||
| // matches must be processed before fallback (empty Hostname value) | ||||
| // matches. | ||||
| // | ||||
| // If this field specifies multiple Listeners that have the same | ||||
| // Port value but are not compatible, the implementation must raise | ||||
| // a "Conflicted" condition in the Listener status. | ||||
| // Implementations MAY collapse separate Gateways onto a single set of | ||||
| // Addresses if the Listeners across all Gateways are compatible. | ||||
rainest marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| // | ||||
| // Support: Core | ||||
| // | ||||
|
|
||||
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.
One advantage of the previous wording here was that it was an example of what an implementation might consider compatible. In this case, the wording might prevent support for multiple protocols on the same port, which at least some vendors can support. (I don't think this explicitly prevents it, but it's not obvious if it would be compatible).
Uh oh!
There was an error while loading. Please reload this page.
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.
Do we not think the examples below cover that? They do intentionally include that case and indicate these are compatible if you can support it.
From the second point in my earlier self-review, would we want to instead use broader, but more direct language? "The implementation can unambiguously match a request to a single Listener." instead, for example. That's ultimately what we're going for, but quite abstract, so maybe harder to understand.
There is Hostname-specific language later, so we can omit that here if we want:
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.
Reading over this again, it may be better to introduce the concept of
distinctfirst, and then use that as the basis forcompatible- I think that it makes what we're doing easier to understand.I think that this sort of language will make the later changes easier as well, in that it makes it more clear why combinations may be incompatible.
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.
Do we indeed want to codify specific compatibility cases or do we want to leave most of the extended space open to implementation capabilities? If we do want to codify the hostname/port/protocol relationships, we'd use something like @youngnick's suggestion, dispense with the separate examples, and just add "unless the implementation cannot serve otherwise compatible Listeners on the same addresses" as a final caveat.
I like the broad "what the implementation can route" version for its brevity, and think the select non-normative examples after ground that rule in more practical terms, without having to exhaustively list all cases.
Exhaustively listing compatible cases feels like it'd be more difficult to expand. We have to cover an N-dimensional matrix with complex rules (sometimes a dimension isn't relevant), so adding a new Listener field would create lots of cases.
We're hindered somewhat by needing to place the spec in struct comments--the more detailed rules would probably fit better in a breakout page, but AFAIK we don't use those in the reference.
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.
tbh I think this is a case where it's better to be more specific for the protocols defined in the spec - I tried to leave some space for the use of implementation-specific protocols, but I don't anticipate us adding many more
protocoloptions, or other options likehostname.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.
Trying to write an exhaustive set of rules from the angle of Listener properties is proving quite tricky. For example,
probably breaks when you have
HTTP:443:example.comandHTTPS:443:example.com. Those are unique (they differ by protocol) but I expect most implementations couldn't actually handle it, and it'd violate a rule (insofar as the example is halfway a rule) from the original:Trying to hew closer to the original and focus around sharing Port values gets confusing because transport (or the use of TLS) is implied but not part of the spec.
Ultimately, do we have goals for compatibility other than ensuring that an implementation can serve Listeners on the same address and match a request to exactly one Listener? If we do, and expect that compatibility will vary across implementations anyway, do we have a reason not to make that the actual rule?
Again, I understand the value of having practical examples--that's why this revision still has them, after this section. Do those not provide the clarity around common cases we expect to see?
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.
Yeah, the example you gave makes sense, but I think that in the suggestion I made, that comes under the "implementations couldn't actually serve this" rule.
You're right that, at its heart, the rule is "implementations can serve all Listeners on the same address, and match a request to exactly one Listener". We can make that the rule, but I think that without something like what I wrote, we will end up with big incompatibilities between implementations. Just having examples is, in my experience, not enough.
To put this another way, for the included Protocols, I think that we should be as specific as we can manage, with a rule like "if you can't serve a distinct set of listeners, then it's not compatible" to cover the other edge cases. The idea behind adding "distinct" as a thing is to try to make it clear what we mean by "match a request to only one Listener" - because it really needs to be "match a request to only one Listener, with no re-entry to processing if that Listener's Routes don't work out." (Since that's what started this conversation to begin with).
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.
Status quo is that we already only have examples (and effectively only one at that).
What incompatibilities do we expect in practice, and how bad will those be?
I grant that lack of upper bounds may allow some implementation to say "we do deep packet inspection, you can have HTTP, HTTPS, and TLS Listeners with the same Hostname, and a no-Hostname TCP Listener all on the same Port", and that this won't be portable. I think that's fine so long as there's a defined way for an implementation to report that they can't support that configuration, and expect that demand for it isn't so great that mixed support isn't going to break the ecosystem.
If there are areas where we're concerned about compatibility, we should be adding more SHOULD lower bounds after the existing MUSTs.
I could write something to the effect of:
but it's still likely to be incomplete and long despite, and I do think trying to limit the spec complexity is valuable.
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 agree, I thought that's what I was trying to do by saying "Listeners must be distinct", and defining rules for distinct.
I think that this is now at the point where we need to talk about it in the meeting though - since I don't understand how the suggestion I supplied doesn't make the spec clearer while managing the spec complexity.
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.
dd27869 rewords the second rule.
5af8b65 adds cascade matching language as a SHOULD elsewhere. IMO this would be a breaking change if it's a MUST. I'd need to review further to see if there was existing (if unclear) language that implied that, but I know there are at least no existing compatibility tests for it (we'd have failed it if so).
AFAIK we don't have much information from other implementations re their ability to cascade or not--I know it isn't Kong's routing behavior and isn't easily avoided.
The latter also can't be a compatibility rule since it doesn't block accepting both Listeners.