-
Notifications
You must be signed in to change notification settings - Fork 212
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
add top-level HTTPRoute features doc #1657
Conversation
authentication policies][auth-policy]. | ||
|
||
{{< warning >}} | ||
Outbound HTTPRoutes are **incompatible with ServiceProfiles**. If a |
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.
If this is a temporary situation, I would call it out as such and say that this will be fixed in the future. If this is a permanent situation, I would say that ServiceProfiles override HTTPRoute behavior rather than saying they're incompatible, which to me suggests that something will break.
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'm happy to reword this. i should mention, though, that the text of this warning is copied from other docs pages --- do you think it makes sense to also apply the same change to other instances of this warning box in the docs?
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.
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'm good with that wording, there and elsewhere. Not really sure whether this should be a warning vs an info block, but I'll leave that to you. Thank you
|
||
The general pattern for Linkerd's dynamic, fine-grained policy is to define the | ||
traffic target that must be protected (via a combination of `Server` and | ||
`HTTPRoute` CRs); define the types of authentication that are required before | ||
[`HTTPRoute`] CRDs); define the types of authentication that are required before |
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.
was this change from CR to CRDs intentional?
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 thought this was a typo, but I think it's not actually...i'll undo this change!
Co-authored-by: William Morgan <[email protected]>
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 really like this! I've left some comments for minor things that I think need fixing, but really, they're minor, nice job. 🙂
One final note: in some places you talk about e.g. "the HTTPRoute
blah blah" and in other places it's "the HTTPRoute blah blah". We should probably pick one way and go with it. (IIRC the K8s standard is not to set type names in monospace, so that would be my vote, but it's not a terribly strong opinion...)
I tried to keep the use of monospace or not consistent with other existing uses within the same file. I didn't use monospace in the new docs I added, but I also didn't change any existing documentation. It might be worth doing a pass to make this more consistent across all the documentation, but I would prefer to do that in a separate branch. |
Yeah, I'm good with limiting scope on this one. Thanks! 🙂 |
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.
Hit it! 🙂
This commit updates the HTTPRoute/ServiceProfile incompatibility warning in the documentation to use the new wording suggested by @wmorgan in #1657 (comment).
This commit updates the HTTPRoute/ServiceProfile incompatibility warning in the documentation to use the new wording suggested by @wmorgan in #1657 (comment).
Currently, the website doesn't really have a top-level summary of what
the HTTPRoute resource is and what it can do, similar to the
ServiceProfiles feature page. I thought this would be useful to add, as
it can be used for other documentation to link to, and will also be
useful as a place to collect all the documentation on things users can
do using HTTPRoutes. In addition, we can also add stuff about GAMMA
support and about the difference between
policy.linkerd.io
andgateway.networking.kubernetes.io
HTTPRoutes here, as well.I've also updated some of the other documentation that mentions the
HTTPRoute resource to link to this page.