app: Split inbound/outbound constructors into components #533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change adds (flakey) tests for loop detection. The tests are flakey because they require static ports to work properly. (We cannot configure the original dst port to be the same as the interface port if the interface port is not known).
e77fe18 introduced loop detection to the outbound HTTP proxy. This change extends this behavior to the inbound HTTP proxy and the TCP proxy for both inbound and outbound. This helps ensure malicious requests can't consume proxy resources.
This will allow us to share the outbound http stack with the inbound proxy to support HTTP gateway.
Also, use the ConnectAddr trait to reduce boilerplate.
Extract the refine service into its own constructor, and modify the service to return the set of IPs with the name. This enables the service to be used by a gateway.
This reverts commit d59b350.
|
Note that the majority of this change can be understood by reviewing |
hawkw
approved these changes
May 27, 2020
There was a problem hiding this comment.
as far as i can tell, there doesn't seem to be any functional change, so this seems fine to me! i had some style complaints about things that i'm sure absolutely nobody cares about and you should feel free to disregard.
will you be able to port this to master-tokio-0.2 eventually? there's currently some churn in inbound and outbound's lib.rs on that branch, as parts of the stack are still being re-enabled, so it might be better to wait until that's done?
Comment on lines
+104
to
+105
| > + Clone | ||
| + Send { |
There was a problem hiding this comment.
nit: weird formatting here, is this rustfmt?
Comment on lines
+72
to
+87
| tcp_connect.clone(), | ||
| prevent_loop, | ||
| profiles_client, | ||
| tap_layer, | ||
| metrics.clone(), | ||
| span_sink.clone(), | ||
| ); | ||
| self.build_server( | ||
| listen, | ||
| prevent_loop, | ||
| tcp_connect, | ||
| http_router, | ||
| local_identity, | ||
| metrics, | ||
| span_sink, | ||
| drain, |
There was a problem hiding this comment.
oof, this is a lot to pass around...does it make sense to combine some of the stuff that's passed to both functions (metrics and span sink could maybe be a shared observability thing, at least). might not be worth the effort...
There was a problem hiding this comment.
it's also a little weird to me that arguments we pass to both functions are passed in different orders, but again, probably nobody cares about this except me.
There was a problem hiding this comment.
no, i think that's a valid point. I'll clean it up in the followup.
Comment on lines
+92
to
+93
| + Clone | ||
| + Send { |
There was a problem hiding this comment.
nit: if rustfmt is going to indent this the way seems to have, i'd kinda prefer to put the curly on the next line...i'd rather have the opening curly line up with the closing curly.
i know this is wildly unimportant and nobody cares, so feel free to ignore!
There was a problem hiding this comment.
i don't know if rustfmt will allow that but i will try
There was a problem hiding this comment.
it does not. i also agree all of this formatting is weird. but, take it up with the boss?
olix0r
added a commit
that referenced
this pull request
May 30, 2020
This change does not change any functionality. It only restructures the inbound and outbound proxy modules so that the clients and servers can be instantiated separately. This will support gatewaying requests between the inbound and outbound proxy.
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jun 1, 2020
The proxy can now operate as gateway, routing requests from its inbound proxy to the outbound proxy, without passing the requests to a local application. This supports Linkerd's multicluster feature by adding a `Forwarded` header to propagate the original client identity and assist in loop detection. --- * Add loop detection to inbound & TCP forwarding (linkerd/linkerd2-proxy#527) * Test loop detection (linkerd/linkerd2-proxy#532) * fallback: Unwrap errors recursively (linkerd/linkerd2-proxy#534) * app: Split inbound/outbound constructors into components (linkerd/linkerd2-proxy#533) * Introduce a gateway between inbound and outbound (linkerd/linkerd2-proxy#540) * gateway: Add a Forwarded header (linkerd/linkerd2-proxy#544) * gateway: Return errors instead of responses (linkerd/linkerd2-proxy#547) * Fail requests that loop through the gateway (linkerd/linkerd2-proxy#545)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Jun 3, 2020
This change modifies the linkerd-gateway component to use the inbound proxy, rather than nginx, for gateway. This allows us to detect loops and propagate identity through the gateway. This change also cleans up port naming to `mc-gateway` and `mc-probe` to resolve conflicts with Kubernetes validation. --- * proxy: v2.99.0 The proxy can now operate as gateway, routing requests from its inbound proxy to the outbound proxy, without passing the requests to a local application. This supports Linkerd's multicluster feature by adding a `Forwarded` header to propagate the original client identity and assist in loop detection. --- * Add loop detection to inbound & TCP forwarding (linkerd/linkerd2-proxy#527) * Test loop detection (linkerd/linkerd2-proxy#532) * fallback: Unwrap errors recursively (linkerd/linkerd2-proxy#534) * app: Split inbound/outbound constructors into components (linkerd/linkerd2-proxy#533) * Introduce a gateway between inbound and outbound (linkerd/linkerd2-proxy#540) * gateway: Add a Forwarded header (linkerd/linkerd2-proxy#544) * gateway: Return errors instead of responses (linkerd/linkerd2-proxy#547) * Fail requests that loop through the gateway (linkerd/linkerd2-proxy#545) * inject: Support config.linkerd.io/enable-gateway This change introduces a new annotation, config.linkerd.io/enable-gateway, that, when set, enables the proxy to act as a gateway, routing all traffic targetting the inbound listener through the outbound proxy. This also removes the nginx default listener and gateway port of 4180, instead using 4143 (the inbound port). * proxy: v2.100.0 This change modifies the inbound gateway caching so that requests may be routed to multiple leaves of a traffic split. --- * inbound: Do not cache gateway services (linkerd/linkerd2-proxy#549)
olix0r
added a commit
that referenced
this pull request
Jun 11, 2020
* outbound: Prevent loops (#525) This change modifies the outbound proxy to fail to build services targetting localhost:4140 (where 4140 is the outbound port). This prevents looping and will result in 502s. * Add loop detection to inbound & TCP forwarding (#527) e77fe18 introduced loop detection to the outbound HTTP proxy. This change extends this behavior to the inbound HTTP proxy and the TCP proxy for both inbound and outbound. This helps ensure malicious requests can't consume proxy resources. * Test loop detection (#532) This change adds (flakey) tests for loop detection. The tests are flakey because they require static ports to work properly. (We cannot configure the original dst port to be the same as the interface port if the interface port is not known). * fallback: Unwrap errors recursively (#534) This change modifies the fallback layer to inspect error sources recursively to determine if the given error type is satisfied. A stack-helper is also added for this case. * app: Split inbound/outbound constructors into components (#533) This change does not change any functionality. It only restructures the inbound and outbound proxy modules so that the clients and servers can be instantiated separately. This will support gatewaying requests between the inbound and outbound proxy. * Introduce a gateway between inbound and outbound (#540) When the proxy receives inbound requests without an original dst address (or with a original dst address matching the inbound listener), the proxy currently fails these requests. This change modifies the proxy to attempt to accept these requests and forward them back through the outbound router. The gateway requires that all requests are received over an mTLS-secured connection. It also refines the destination through DNS to determine the canonical-form name as well as an outbound original dst IP. All gatewayed destinations must have a suffix as set by the `LINKERD2_PROXY_INBOUND_GATEWAY_SUFFIXES` environment variable. All requests that do not meet these criteria are failed with a `403 Forbidden` status. * gateway: Add a Forwarded header When the gateway forwards requests, it now adds a `Forwarded` header including the source identity, the local identity, and the destination authority. * Fail requests that loop through the gateway This change uses the gateway's `Forwarded` header to detect if the request has already transited through this gateway. This is determination is made by comparing ID strings, so this will prevent gateway daisy-chaining when clusters do not use distinct identity domains. * gateway: Return errors instead of responses (#547) This ensures that error metrics are recorded and that logging is emitted uniformly. This also ensures that gRPC requests don't get HTTP error responses. * Fail requests that loop through the gateway (#545) This change uses the gateway's `Forwarded` header to detect if the request has already transited through this gateway. This is determination is made by comparing ID strings, so this will prevent gateway daisy-chaining when clusters do not use distinct identity domains. * inbound: Do not cache gateway services (#549) When the inbound caches gateway services, it eagerly obtains an outbound service to cache. If the outbound service employs a traffic split, this inbound service is pinned to a specific leaf, and requests will never be routed to the other leaf. This change moves the gateway fallback to be outside all of the inbound caches, so that outbound splits work as intended.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change does not change any functionality. It only restructures the inbound and outbound proxy modules so that the clients and servers can be instantiated separately. This will support gatewaying requests between the inbound and outbound proxy.