8000 outbound: Return a default endpoint on reject by olix0r · Pull Request #690 · linkerd/linkerd2-proxy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

outbound: Return a default endpoint on reject #690

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

Merged
merged 2 commits into from
Oct 2, 2020
Merged

Conversation

olix0r
Copy link
Member
@olix0r olix0r commented Oct 2, 2020

When the resolver rejects resolution, we currently propagate that error
so that it can be handled via fallback. And due to recent HTTP router
changes, these resolution errors can propagate up across splits, etc.

This change simplifies this behavior by instead synthesizing a
resolution with a default endpoint.

The not_http reason has been removed, as it's no longer useful.

When the resolver rejects resolution, we currently propagate that error
so that it can be handled via fallback. And due to recent HTTP router
changes, these resolution errors can propagate up across splits, etc.

This change simplifies this behavior by isntead synthesizing a
resolution with a default endpoint.

The `not_http` reason has been removed, as it's no longer useful.
@olix0r olix0r requested a review from a team October 2, 2020 00:51
Copy link
Contributor
@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

👍

@olix0r olix0r requested a review from hawkw October 2, 2020 17:08
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.

lgtm, commented on a few minor things that aren't blockers

Comment on lines +17 to +18
#[derive(Clone, Debug)]
pub struct RecoverDefaultResolve<S>(S);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it'd be nice to have a comment explaining what this does --- the name is about as descriptive as i can think of, but i can see the purpose of this thing not being immediately obvious to new contributors?

Comment on lines +26 to +27
#[derive(Clone, Debug)]
pub struct Rejected(());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe worth a comment on the semantics of what this error means?

Comment on lines +47 to +49
.or_else(move |error| {
if Rejected::matches(&*error) {
tracing::debug!(%error, %addr, "Synthesizing endpoint");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/TIOLI: if we added addr to this message by instrumenting the future instead of moving it in, we wouldn't need this to be a move closure and could therefore remove the box around the future (since it's just combinators). not a blocker though; and the concrete type is probably pretty gross...

Copy link
Member Author

Choose a reason for hiding this comment

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

The addr is used on the next line (as part of the returend stream), so the move is needed either way. And, really, the addr is already on the context, so we could omit it here, but 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops, i overlooked that --- carry on!

@olix0r
Copy link
Member Author
olix0r commented Oct 2, 2020

I'll add comments in #691

@olix0r olix0r merged commit 9870faa into main Oct 2, 2020
@olix0r olix0r deleted the ver/optional-resolve branch October 2, 2020 18:30
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Oct 2, 2020
This release includes changes to TCP metrics to ensure that peer
identities are encoded via the `client_id` and `server_id` labels.

---

* outbound: Explicitly ignore the source address for tap (linkerd/linkerd2-proxy#680)
* Update proxy-api and tonic (linkerd/linkerd2-proxy#682)
* http: Lazily build http/tcp stacks (linkerd/linkerd2-proxy#681)
* outbound: Remove required identity from HttpLogical (linkerd/linkerd2-proxy#683)
* profiles: Expose the fully_qualified_name (linkerd/linkerd2-proxy#684)
* request-filter: Support altering the request type (linkerd/linkerd2-proxy#685)
* tracing: Set contexts in new_service/make_service (linkerd/linkerd2-proxy#686)
* discover: Allow resolution streams to terminate (linkerd/linkerd2-proxy#689)
* metrics: add peer identities to all TLS metric labels (linkerd/linkerd2-proxy#687)
* outbound: Return a default endpoint on reject (linkerd/linkerd2-proxy#690)
* Skip endpoint resolution when profile lookup is rejected (linkerd/linkerd2-proxy#691)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Oct 2, 2020
This release includes changes to TCP metrics to ensure that peer
identities are encoded via the `client_id` and `server_id` labels.

---

* outbound: Explicitly ignore the source address for tap (linkerd/linkerd2-proxy#680)
* Update proxy-api and tonic (linkerd/linkerd2-proxy#682)
* http: Lazily build http/tcp stacks (linkerd/linkerd2-proxy#681)
* outbound: Remove required identity from HttpLogical (linkerd/linkerd2-proxy#683)
* profiles: Expose the fully_qualified_name (linkerd/linkerd2-proxy#684)
* request-filter: Support altering the request type (linkerd/linkerd2-proxy#685)
* tracing: Set contexts in new_service/make_service (linkerd/linkerd2-proxy#686)
* discover: Allow resolution streams to terminate (linkerd/linkerd2-proxy#689)
* metrics: add peer identities to all TLS metric labels (linkerd/linkerd2-proxy#687)
* outbound: Return a default endpoint on reject (linkerd/linkerd2-proxy#690)
* Skip endpoint resolution when profile lookup is rejected (linkerd/linkerd2-proxy#691)
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.

3 participants
0