inbound: Consolidate port-based switching #1170

olix0r · 2021-07-28T07:09:36Z

The inbound::prevent_loop module implements predicates for switching
based on the target port. But there's no reason for this control flow to
be decoupled from our stack consruction.

In preparation for further changes to inbound-port-based policy, this
change eliminates the prevent_loop module. The tcp connection stack is
updated to handle loop detection (instead of the TCP forward stack) so
that we are totally unable to initiate looping connections (i.e. if some
higher part of the stack were to do something unexpected).

The `inbound::prevent_loop` module implements predicates for switching based on the target port. But there's no reason for this control flow to be decoupled from our stack consruction. In preparation for further changes to inbound-port-based policy, this change eliminates the `prevent_loop` module. The tcp connection stack is updated to handle loop detection (instead of the TCP forward stack) so that we are totally unable to initiate looping connections (i.e. if some higher part of the stack were to do something unexpected).

olix0r · 2021-07-28T07:11:35Z

linkerd/app/core/src/svc.rs

-    where
-        S: Service<T>,
-        S::Error: Into<Error>,
-    {


These constraints are unnecessary and caused us to have to document T. Easier to just remove the constraints.

olix0r · 2021-07-28T07:13:01Z

linkerd/app/inbound/src/lib.rs

                // Limits the time we wait for a connection to be established.
                .push_connect_timeout(*timeout)
-                .push(svc::stack::BoxFuture::layer())


This BoxFuture layer is redundant -- there's already a BoxFuture in the HTTP router where it's actually needed.

codecov-commenter · 2021-07-28T07:35:24Z

Codecov Report

Merging #1170 (241ab83) into main (6f7968f) will decrease coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
- Coverage   75.08%   75.06%   -0.02%     
==========================================
  Files         235      234       -1     
  Lines       15095    15085      -10     
==========================================
- Hits        11334    11324      -10     
  Misses       3761     3761

Impacted Files	Coverage Δ
linkerd/app/inbound/src/lib.rs	`86.77% <78.94%> (-2.21%)`	⬇️
linkerd/app/core/src/svc.rs	`75.58% <100.00%> (ø)`
linkerd/app/inbound/src/direct.rs	`37.07% <0.00%> (-1.13%)`	⬇️
linkerd/proxy/tap/src/grpc/match_.rs	`62.31% <0.00%> (+1.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f7968f...241ab83. Read the comment docs.

kleimkuhler

👍

mateiidavid

Looks great!

mateiidavid · 2021-07-29T11:27:14Z

linkerd/app/inbound/src/lib.rs

                    .push_switch(
-                        PreventLoop::from(server_port).to_switch(),
+                        // If the connection targets the inbound proxy port, the connection is most


Thanks for the comment here! Before reviewing, I went through the inbound stack to see if I can figure out what's going on. The comment was really useful to test my understanding.

mateiidavid · 2021-07-29T11:36:05Z

linkerd/app/inbound/src/lib.rs

                        },
                        opaque
                            .instrument(|_: &TcpAccept| debug_span!("forward"))
                            .into_inner(),
                    )
-                    .check_new_service::<T, I>()


I'm curious why these have been removed. If I understand correctly, check_new_service::<T, I> would check if our target T implements the same traits as a type I so we can check whether our target provides Peek, AsyncRead (and write).

First, I'm not sure if my understanding here is fully correct, I see that in the function implementation we say "stack serves T typed targets", which is another way of saying the target we have and the request type are fit for this stack?

Second, are these checks unnecessary because we have replaced the outer stack with a switch? (I guess checks is the wrong way to put it since the first two layers from the bottom are just allocating it all on the heap?)

Sorry for the long comment!

These checks are really just compile-time assertions. They can be helpful for debugging when a stack doesn't compile, but there's no inherent benefit to having them aside from clarity. We could probably restore this without any issue, though....

mateiidavid · 2021-07-29T11:39:32Z

linkerd/app/inbound/src/lib.rs

@@ -285,31 +290,37 @@ where
                        identity: rt.identity.clone(),
                    }))
            })
-            .map_stack(|cfg, _, detect| {
+            .map_stack(|cfg, rt, detect| {


One more question for this stack: do we need to consider this closure arg? I don't see it being used, why not leave it as ignored? I think it makes more sense to not ignore it, as a newbie it helps me visualise it better so I'm just curious if it's for visibility.

It's used below the diff (I combined two map_stacks into one). Our compilation settings (#[deny(warning)]) won't actually let us compile with unused variables :)

linkerd/app/inbound/src/lib.rs

This release includes only dependency updates an internal changes to support upcoming policy features. No user-facing changes are expected. --- * build(deps): bump tokio from 1.8.2 to 1.9.0 (linkerd/linkerd2-proxy#1162) * build(deps): bump hyper from 0.14.10 to 0.14.11 (linkerd/linkerd2-proxy#1163) * build(deps): bump codecov/codecov-action from 2.0.1 to 2.0.2 (linkerd/linkerd2-proxy#1165) * build(deps): bump futures from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#1166) * build(deps): bump softprops/action-gh-release (linkerd/linkerd2-proxy#1167) * build(deps): bump softprops/action-gh-release from 0.1.6 to 1 (linkerd/linkerd2-proxy#1168) * stack: Introduce ExtractParam and InsertParam (linkerd/linkerd2-proxy#1169) * inbound: Consolidate port-based switching (linkerd/linkerd2-proxy#1170) * build(deps): bump socket2 from 0.4.0 to 0.4.1 (linkerd/linkerd2-proxy#1171) * build(deps): bump serde_json from 1.0.64 to 1.0.65 (linkerd/linkerd2-proxy#1172) * build(deps): bump async-trait from 0.1.50 to 0.1.51 (linkerd/linkerd2-proxy#1173) * Update to Rust v1.54.0 (linkerd/linkerd2-proxy#1175) * build(deps): bump serde_json from 1.0.65 to 1.0.66 (linkerd/linkerd2-proxy#1176) * Add BoxTcp and BoxNewTcp service type aliases (linkerd/linkerd2-proxy#1177) * http: Use ExtractParam to support dynamic detect configuration (linkerd/linkerd2-proxy#1174) * Split the inbound server into multiple modules (linkerd/linkerd2-proxy#1179)

This release includes only dependency updates an internal changes to support upcoming policy features. No user-facing changes are expected. --- * build(deps): bump tokio from 1.8.2 to 1.9.0 (linkerd/linkerd2-proxy#1162) * build(deps): bump hyper from 0.14.10 to 0.14.11 (linkerd/linkerd2-proxy#1163) * build(deps): bump codecov/codecov-action from 2.0.1 to 2.0.2 (linkerd/linkerd2-proxy#1165) * build(deps): bump futures from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#1166) * build(deps): bump softprops/action-gh-release (linkerd/linkerd2-proxy#1167) * build(deps): bump softprops/action-gh-release from 0.1.6 to 1 (linkerd/linkerd2-proxy#1168) * stack: Introduce ExtractParam and InsertParam (linkerd/linkerd2-proxy#1169) * inbound: Consolidate port-based switching (linkerd/linkerd2-proxy#1170) * build(deps): bump socket2 from 0.4.0 to 0.4.1 (linkerd/linkerd2-proxy#1171) * build(deps): bump serde_json from 1.0.64 to 1.0.65 (linkerd/linkerd2-proxy#1172) * build(deps): bump async-trait from 0.1.50 to 0.1.51 (linkerd/linkerd2-proxy#1173) * Update to Rust v1.54.0 (linkerd/linkerd2-proxy#1175) * build(deps): bump serde_json from 1.0.65 to 1.0.66 (linkerd/linkerd2-proxy#1176) * Add BoxTcp and BoxNewTcp service type aliases (linkerd/linkerd2-proxy#1177) * http: Use ExtractParam to support dynamic detect configuration (linkerd/linkerd2-proxy#1174) * Split the inbound server into multiple modules (linkerd/linkerd2-proxy#1179) Signed-off-by: Sanni Michael <sannimichaelse@gmail.com>

olix0r requested a review from a team July 28, 2021 07:09

olix0r commented Jul 28, 2021

View reviewed changes

Merge branch 'main' into ver/prevent-loop

9ce7f10

kleimkuhler approved these changes Jul 28, 2021

View reviewed changes

mateiidavid approved these changes Jul 29, 2021

View reviewed changes

olix0r commented Jul 29, 2021

View reviewed changes

linkerd/app/inbound/src/lib.rs Outdated Show resolved Hide resolved

Update linkerd/app/inbound/src/lib.rs

241ab83

olix0r merged commit 8d8d261 into main Jul 30, 2021

olix0r deleted the ver/prevent-loop branch July 30, 2021 15:57

olix0r mentioned this pull request Aug 5, 2021

proxy: v2.151.0 linkerd/linkerd2#6607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inbound: Consolidate port-based switching #1170

inbound: Consolidate port-based switching #1170

olix0r commented Jul 28, 2021

olix0r Jul 28, 2021

olix0r Jul 28, 2021

codecov-commenter commented Jul 28, 2021 •

edited

Loading

kleimkuhler left a comment

mateiidavid left a comment

mateiidavid Jul 29, 2021

mateiidavid Jul 29, 2021

olix0r Jul 29, 2021

mateiidavid Jul 29, 2021

olix0r Jul 29, 2021

inbound: Consolidate port-based switching #1170

inbound: Consolidate port-based switching #1170

Conversation

olix0r commented Jul 28, 2021

olix0r Jul 28, 2021

Choose a reason for hiding this comment

olix0r Jul 28, 2021

Choose a reason for hiding this comment

codecov-commenter commented Jul 28, 2021 • edited Loading

Codecov Report

kleimkuhler left a comment

Choose a reason for hiding this comment

mateiidavid left a comment

Choose a reason for hiding this comment

mateiidavid Jul 29, 2021

Choose a reason for hiding this comment

mateiidavid Jul 29, 2021

Choose a reason for hiding this comment

olix0r Jul 29, 2021

Choose a reason for hiding this comment

mateiidavid Jul 29, 2021

Choose a reason for hiding this comment

olix0r Jul 29, 2021

Choose a reason for hiding this comment

codecov-commenter commented Jul 28, 2021 •

edited

Loading