Add implementer's guide #2454
Add implementer's guide #2454
Conversation
Signed-off-by: Nick Young <nick@isovalent.com>
k8s-ci-robot
added
release-note
k8s-ci-robot
added
size/L
| @@ -1,4 +1,4 @@ | |||
| # Implementation guidelines | |||
There was a problem hiding this comment.
I started this PR reviewing this page, but it turned out that these guidelines are actually for us, the project contributors and maintainers, not for implementers. So "Design guidelines" felt more appropriate.
Signed-off-by: Nick Young <nick@isovalent.com>
youngnick
force-pushed
the
implementers-guide
branch
from
634da66 to
09e2c65
Compare
|
Hey @youngnick, fantastic idea. One thing that might be interesting to see in this document is a recent Slack question about filters and policies. Seeing more context, including differences and the implementer PoV, would be nice. |
|
|
||
| Because of our backwards compatibility guarantees, it's also safe for a controller | ||
| to flip the install channel between "standard" and "experimental", although | ||
| implementations MUST NOT do this without consulting the implementation owner. |
There was a problem hiding this comment.
Who is the "implementation owner"? In my reading, if I substitute phrases, this says "although
Istio MUST NOT do this without consulting the Istio maintainers" which seems odd? Or is it the user?
Anyhow, one risk with flipping the channel is another controller flipping it back endlessly
There was a problem hiding this comment.
Thanks for pointing this out, I was intending to say "implementations shouldn't do this without asking a human first" - I've updated, PTAL.
|
|
||
| Implementations MAY require the installation to be done manually; if so, the | ||
| installation instructions SHOULD include commands to check if there are any other | ||
| CRDs installed already and verify that the installation will not be a downgrade. |
There was a problem hiding this comment.
As a followup maybe we should provide a common command to do this.
Istio, for instance, has an example of a suboptimal approach: https://istio.io/latest/docs/tasks/traffic-management/ingress/gateway-api/#setup
There was a problem hiding this comment.
Agreed, this is a great followup. I'll create an issue a bit later for it.
|
|
||
| [versioning]: /concepts/versioning | ||
|
|
||
| ### Changes to the Gateway API CRDs are backwards compatible |
There was a problem hiding this comment.
One of my concerns here is that this could get out of sync with our versioning guide or vice versa, since a lot of this section is overlapping. Wherever possible, I'd like to have a single source of truth in our docs for a topic so we don't end up with accidentally conflicting docs in the future.
There was a problem hiding this comment.
I understand, but I think that having a summary of the key parts is really important for implementers. I'm not sure how to trim this to tell people about the backward compatibility guarantees in any shorter way.
| - that the CRD definitions are only installed if they are a higher bundle version | ||
| than any existing Gateway API CRDs |
There was a problem hiding this comment.
Even this may not be safe in the case of experimental channel. I think if we say that this can be done, we also need to clearly state the risks of breaking other implementations.
| Because of our backwards compatibility guarantees, it's also safe for a controller | ||
| to flip the install channel between "standard" and "experimental", although |
There was a problem hiding this comment.
This is only safe in one direction: standard -> experimental. Once you've gone that direction, I think it's impossible to safely transition back to standard channel.
| In all of these cases, there are some relatively-common Condition types that have | ||
| similar meanings: |
There was a problem hiding this comment.
I'm worried that the definitions we have here will become out of sync with the definitions we have in the spec, do we need to define these in both places?
There was a problem hiding this comment.
I don't see these changing any time soon, so I'm not worried about this.
| The following section goes through each Gateway API object, indicates expected | ||
| behaviors, and which conformance profiles that object is included in. | ||
|
|
||
| #### GatewayClass |
There was a problem hiding this comment.
How do we decide what belongs in here vs https://gateway-api.sigs.k8s.io/api-types/gatewayclass/ vs API Spec? I'm worried that we're going to end up with subtle differences in each individual source if we're not very careful.
There was a problem hiding this comment.
I've tried to keep this very general, with only the main status being captured here. I don't see us changing the status handling or controllerName behavior - we can't once we go GA.
Maybe the guideline here is that we can only mention stable or core fields?
| @@ -0,0 +1,367 @@ | |||
| # Gateway API Implementer's Guide | |||
There was a problem hiding this comment.
Nit: This location doesn't feel quite right. So far all our guides are targeted at users of the API, and this one is decidedly different than those other ones. Maybe this belongs under the "Reference" tab instead?
That's a great idea, I'll make a note to come back and add that in a later PR. |
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
|
|
||
| #### HTTPRoute | ||
|
|
||
| HTTPRoutes route HTTP traffic that is _unencrypted_ and available for inspection. |
There was a problem hiding this comment.
I think this may be a bit confusing when combined with BackendTLSPolicy. My understanding is that HTTPRoute can/should be combined with BackendTLSPolicy, but in that (and other) cases, traffic to the backend would be encrypted. I think it might be more accurate to say that this applies to HTTP(S) traffic that is terminated at the Gateway, but not TLS Passthrough traffic. I'm actually not sure how to word this, just need to be careful that we're not being unnecessarily restrictive here.
There was a problem hiding this comment.
I updated this wording, PTAL.
Signed-off-by: Nick Young <nick@isovalent.com>
youngnick
force-pushed
the
implementers-guide
branch
from
94a2121 to
2026c7b
Compare
| Because of all these caveats, we DO NOT recommend doing automatic CRD management | ||
| at this time. |
There was a problem hiding this comment.
This feels like something that should be closer to the top of the section.
There was a problem hiding this comment.
I reworded this pretty substantially, PTAL.
Signed-off-by: Nick Young <nick@isovalent.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: howardjohn, robscott, shaneutt, youngnick 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 |
|
Thanks @youngnick! /lgtm |
k8s-ci-robot
added
the
lgtm
/kind documentation
What this PR does / why we need it:
This adds an implementer's guide for Gateway API. The new guide is also now part of the specification, so MUST, SHOULD, and MAY there are treated the same as in the godoc or rendered Specification page.
Does this PR introduce a user-facing change?: