Skip to content

Moved HTTPRoute reference doc into its own page#1590

Merged
hawkw merged 8 commits intomainfrom
alpeb/httprout-ref
Apr 10, 2023
Merged

Moved HTTPRoute reference doc into its own page#1590
hawkw merged 8 commits intomainfrom
alpeb/httprout-ref

Conversation

@alpeb
Copy link
Member

@alpeb alpeb commented Mar 21, 2023

Moved out of Authorization Policy given it's also used for Header-Based routing which is unrelated to Auth.

Also added missing fields in the ref.

Upcoming: Header-Based routing tutorial

@alpeb alpeb force-pushed the alpeb/httprout-ref branch 3 times, most recently from 8242b53 to 07f973d Compare March 23, 2023 14:53
@alpeb alpeb force-pushed the alpeb/httprout-ref branch from 07f973d to 78c06d4 Compare April 6, 2023 14:46
@alpeb alpeb enabled auto-merge (rebase) April 6, 2023 14:55
@alpeb alpeb disabled auto-merge April 6, 2023 14:56
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some potential suggestions on the docs. I know a lot of this wasn't added in this PR, but I hope the suggestions are still helpful!

|------|-------|
| `name`| Name of service for this backend.|
| `port`| Destination port number for this backend.|
| `weight`| Proportion of requests sent to this backend.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add that. But do we actually support cross-namespace traffic without ReferenceGrants?

@alpeb alpeb force-pushed the alpeb/httprout-ref branch from 8900445 to b84703e Compare April 8, 2023 11:23
@alpeb
Copy link
Member Author

alpeb commented Apr 8, 2023

Thanks for the comments @hawkw , this is ready for review again.

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple last suggestions


### parentReference

A reference to the parent resource this HTTPRoute is a part of.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be worth explaining what this does in terms of the proxy's behavior: it attaches an HTTPRoute's outbound policies to a Service, telling outbound proxies what to do when talking to that Service; or attaches an HTTPRoute's inbound policies to a Server, telling inbound proxies what to do when accepting traffic for that Server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had a section at the beginning of the doc explaining this difference, but your point is taken, so I'm moving that into this subsection instead 👍

Comment on lines +29 to +30
rerouted to different backend services. This can be used to perform dynamic
request routing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the phrase "dynamic request routing" should link to the dynamic request routing docs. we can add the link in that branch if this one merges first...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

policy](../authorization-policy/#authorizationpolicy) for specific routes served
on that Server.

HTTPRoutes can also be attached to a Service, in order to classify requests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if i love the use of the term "classify" here, since that feels a little overloaded with response classification. how about

Suggested change
HTTPRoutes can also be attached to a Service, in order to classify requests
HTTPRoutes can also be attached to a Service, in order to route requests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hawkw hawkw requested a review from kflynn April 10, 2023 19:56
@hawkw
Copy link
Contributor

hawkw commented Apr 10, 2023

Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like moving the HTTPRoute spec into its own doc! I made a couple of suggestions about referring to it, though...

authorizing traffic to that `HTTPRoute` only rather than to the entire [Server].
`HTTPRoutes` may also define filters which add processing steps that must be
completed during the request or response lifecycle.
When attached to a [Server], an `HTTPRoute` resource represents a subset of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just make this mention (or all mentions?) of HTTPRoute a link to the spec, rather than having the one-sentence paragraph at line 153?

type: "PathPrefix"
method: GET
```
Please refer to HTTPRoute's full [spec](../httproute/).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just drop this sentence in favor of linking out from mentions of HTTPRoute?

Copy link
Member Author

@alpeb alpeb Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph briefly describes HTTPRoute in the context of Authorization Policy and the title is an HTTPRoute anchor, targeted from other places in this doc. That's why other references in this doc point to this paragraph and then I have this sentence to clearly point out to the other doc with the full reference. Makes sense?

alpeb and others added 6 commits April 10, 2023 15:54
Moved out of Authorization Policy given it's also used for
Header-Based routing which is unrelated to Auth.

Also added missing fields in the ref.

Upcoming: Header-Based routing tutorial
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@alpeb alpeb force-pushed the alpeb/httprout-ref branch from ba4f5b8 to b998fd7 Compare April 10, 2023 21:08
@hawkw hawkw requested a review from kflynn April 10, 2023 22:46
@hawkw hawkw enabled auto-merge (squash) April 10, 2023 23:03
Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this landed. 🙂

@hawkw hawkw merged commit 9cf7ae1 into main Apr 10, 2023
@hawkw hawkw deleted the alpeb/httprout-ref branch April 10, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants