inbound: connections wait for ServerPolicy discovery #2186

hawkw · 2023-01-25T21:55:12Z

Currently, when the inbound proxy gets a connection to a port it has not
previously configured, it starts doing ServerPolicy discovery for that
port but then uses the default policy until the configuration is synced
from the control plane. This behavior is fairly complex to reason about
and surprising for users. Therefore, we would like to remove this
defaulting behavior from the inbound proxy.

This branch changes how the inbound proxy performs policy discovery.
Rather than spawning the lookup in a background task and using a default
policy until a policy is discovered, the inbound proxy will now wait
until the policy for a port is discovered before continuing to process a
connection on that port. In terms of the inbound stack, this means that
policy discovery is now a MakeService rather than a NewService. The
cache is still used when a policy discovery watch has already been
started.

This branch does not completely remove all default policy
configuration from the proxy. The default policy environment variables
are still used when the proxy is configured without a control plane
policy controller address. However, this mode will be removed in a
subsequent PR. I thought it was preferable to limit the scope of this
change to just making inbound connection handling wait for policy
discovery, and remove the defaulting behavior completely as a separate
change, so that the individual changes are easier to review. However, if
we'd prefer to do this atomically, I can make that change as part of
this branch as well.

linkerd/app/inbound/src/direct.rs

linkerd/app/inbound/src/policy.rs

linkerd/app/inbound/src/policy/store.rs

linkerd/app/inbound/src/accept.rs

linkerd/app/inbound/src/direct.rs

Co-authored-by: Oliver Gould <ver@buoyant.io>

hawkw · 2023-01-31T19:15:01Z

Looking into potentially changing the ServerPolicy code to use the NewCachedDiscover type added in #2195, rather than implementing its own caching, although this would require figuring out a nice way to proactively start watches for pre-configured inbound ports. Will figure that out after removing the defaulting behavior.

linkerd/app/inbound/src/server.rs

linkerd/app/admin/src/stack.rs

now that it doesn't need additional bounds

linkerd/app/inbound/src/policy.rs

linkerd/app/inbound/src/direct.rs

This reverts commit c910ca9.

linkerd/app/inbound/src/policy/store.rs

linkerd/app/inbound/src/policy/discover.rs

linkerd/app/inbound/src/policy.rs

`(A, B)` is already `From<(A, B)>` via identity

thanks to the magic of ExtractParam!

Depends on #2186. Currently, the `linkerd-app-inbound` crate's `GetPolicy` trait performs lookups for `OrigDstAddr`s. This is not strictly correct: the `OrigDstAddr` newtype should specifically represent an address that was returned by a `getsockopt(..., SO_ORIGINAL_DST, ...)` call, and in the case of the inbound direct stack, the "original dst address" passed to `GetPolicy` may actually be synthesized from a port override in the `TransportHeader`. Therefore, it would be nicer if we had a `LookupAddr` type specifically representing an address used as an argument for policy lookups, analogous to the `profiles::LookupAddr` newtype used on the outbound side. See [this comment][1] for details. Therefore, this branch makes the following changes: - Policy types (e.g. `ServerPolicy`, `AllowPermit`, etc) which store addresses represent those addresses as `ServerAddr` rather than `OrigDstAddr`, and perform address matching on `ServerAddr`s. - The `inbound::policy` module now defines a `LookupAddr` newtype, which is passed as an argument to `GetPolicy`. - The inbound accept and direct stacks provide `ExtractParam` impls to the `policy::Discover` layer indicating how `LookupAddr`s are produced from targets in that stack. This makes the different behaviors between the normal accept stack and the direct stack more explicit at the callsite. [1]: #2186 (comment)

linkerd/idle-cache/src/lib.rs

linkerd/app/inbound/src/policy/store.rs

Depends on #2186. Currently, the `linkerd-app-inbound` crate's `GetPolicy` trait performs lookups for `OrigDstAddr`s. This is not strictly correct: the `OrigDstAddr` newtype should specifically represent an address that was returned by a `getsockopt(..., SO_ORIGINAL_DST, ...)` call, and in the case of the inbound direct stack, the "original dst address" passed to `GetPolicy` may actually be synthesized from a port override in the `TransportHeader`. Therefore, it would be nicer if we had a `LookupAddr` type specifically representing an address used as an argument for policy lookups, analogous to the `profiles::LookupAddr` newtype used on the outbound side. See [this comment][1] for details. Therefore, this branch makes the following changes: - Policy types (e.g. `ServerPolicy`, `AllowPermit`, etc) which store addresses represent those addresses as `ServerAddr` rather than `OrigDstAddr`, and perform address matching on `ServerAddr`s. - The `inbound::policy` module now defines a `LookupAddr` newtype, which is passed as an argument to `GetPolicy`. - The inbound accept and direct stacks provide `ExtractParam` impls to the `policy::Discover` layer indicating how `LookupAddr`s are produced from targets in that stack. This makes the different behaviors between the normal accept stack and the direct stack more explicit at the callsite. [1]: #2186 (comment)

inbound: Remove default policies (#2204) When an inbound proxy accepts a connection, it needs a policy for the inbound port. If there is no known policy, then a default inbound policy is provided from the proxy's configuration. This presents a few problems: * The server-configured policy may be more restrictive than the default policy. This could all unauthorized connections to race into a pod (when ports are not documented in the the pod spec). * The server-configured protocol configuration may differ from the proxy's configuration. * The way the proxy reads environment configuration for default policies is inefficient for large port ranges. See linkerd/linkerd2#9803 ## Solution To simplify the proxy's configuration surface area, we remove synthetic default policy configurations so that all inbound policies are provided by the policy controller. This means that the proxy will require either a cached policy or a response from the policy controller before it can accept a connection. Proxies always discover and retain cached policies for all ports documented in `LINKERD2_PROXY_INBOUND_PORTS`. Other policies are discovered dynamically and cached for the discovery idle timeout. This inbound proxy now requires a policy controller client configuration. Tests are updated to use a mock policy controller service. The following environment variables are no longer used: * `LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_IDENTITY` * `LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS` * `LINKERD2_PROXY_INBOUND_DEFAULT_POLICY` * `LINKERD2_PROXY_POLICY_CLUSTER_NETWORKS See also #2186

This release updates the proxy to require control-plane-discovered policies for all inbound connections. Previously, the proxy would use a locally-configured default policy while it synced policies from the control plane. --- * trace: never log access log spans to stdout (linkerd/linkerd2-proxy#2244) * build(deps): bump thread_local from 1.1.4 to 1.1.7 (linkerd/linkerd2-proxy#2243) * build(deps): bump tj-actions/changed-files from 35.5.4 to 35.5.5 (linkerd/linkerd2-proxy#2242) * build(deps): bump serde_json from 1.0.91 to 1.0.93 (linkerd/linkerd2-proxy#2217) * dev: Remove docker config mount (linkerd/linkerd2-proxy#2246) * build(deps): bump anyhow from 1.0.68 to 1.0.69 (linkerd/linkerd2-proxy#2216) * build(deps): bump actions/checkout from 3.2.0 to 3.3.0 (linkerd/linkerd2-proxy#2130) * http: Use `ExtractParam` in `NewNormalizeUri` (linkerd/linkerd2-proxy#2245) * http: Replace `classify::CanClassify` with `Param` (linkerd/linkerd2-proxy#2247) * http: Improve timeout module (linkerd/linkerd2-proxy#2248) * outbound: Decouple outbound HTTP server from logical target (linkerd/linkerd2-proxy#2249) * build(deps): bump http from 0.2.8 to 0.2.9 (linkerd/linkerd2-proxy#2253) * build(deps): bump fastrand from 1.8.0 to 1.9.0 (linkerd/linkerd2-proxy#2252) * build(deps): bump signal-hook-registry from 1.4.0 to 1.4.1 (linkerd/linkerd2-proxy#2254) * Use `ExtractParam` in `http::NewHeaderFromTarget` (linkerd/linkerd2-proxy#2255) * outbound: Introduce a new outbound-route watch type (linkerd/linkerd2-proxy#2250) * build(deps): bump tj-actions/changed-files from 35.5.5 to 35.5.6 (linkerd/linkerd2-proxy#2256) * build(deps): bump hyper from 0.14.23 to 0.14.24 (linkerd/linkerd2-proxy#2257) * build(deps): bump once_cell from 1.17.0 to 1.17.1 (linkerd/linkerd2-proxy#2258) * build(deps): bump clang-sys from 1.4.0 to 1.6.0 (linkerd/linkerd2-proxy#2259) * outbound: Add a policy router (linkerd/linkerd2-proxy#2251) * inbound: connections wait for ServerPolicy discovery (linkerd/linkerd2-proxy#2186) * inbound: Remove default policies (linkerd/linkerd2-proxy#2204) * inbound: Introduce a `policy::LookupAddr` type (linkerd/linkerd2-proxy#2264) * build(deps): bump futures from 0.3.25 to 0.3.26 (linkerd/linkerd2-proxy#2263) * build(deps): bump proc-macro2 from 1.0.50 to 1.0.51 (linkerd/linkerd2-proxy#2261) * build(deps): bump tinyvec_macros from 0.1.0 to 0.1.1 (linkerd/linkerd2-proxy#2262) Signed-off-by: Oliver Gould <ver@buoyant.io>

The changes--specifically those in 93b06e6--prevent control plane boot-strapping: the identity controller is unable to serve requests because its proxy can't contact the destination pod for policy because the destination pod doesn't have identity yet. Ultimately, we probably want to change Linkerd's control plane deployment topology to avoid these bootstrapping issues. In the meantime, we'll need to revisit our approach to these changes. * Revert 89ee318 inbound: Introduce a `policy::LookupAddr` type (#2264) * Revert 93b06e6 inbound: Remove default policies (#2204) * Revert c186d88 inbound: connections wait for ServerPolicy discovery (#2186)

* proxy: v2.191.0 This release updates the proxy to require control-plane-discovered policies for all inbound connections. Previously, the proxy would use a locally-configured default policy while it synced policies from the control plane. --- * trace: never log access log spans to stdout (linkerd/linkerd2-proxy#2244) * build(deps): bump thread_local from 1.1.4 to 1.1.7 (linkerd/linkerd2-proxy#2243) * build(deps): bump tj-actions/changed-files from 35.5.4 to 35.5.5 (linkerd/linkerd2-proxy#2242) * build(deps): bump serde_json from 1.0.91 to 1.0.93 (linkerd/linkerd2-proxy#2217) * dev: Remove docker config mount (linkerd/linkerd2-proxy#2246) * build(deps): bump anyhow from 1.0.68 to 1.0.69 (linkerd/linkerd2-proxy#2216) * build(deps): bump actions/checkout from 3.2.0 to 3.3.0 (linkerd/linkerd2-proxy#2130) * http: Use `ExtractParam` in `NewNormalizeUri` (linkerd/linkerd2-proxy#2245) * http: Replace `classify::CanClassify` with `Param` (linkerd/linkerd2-proxy#2247) * http: Improve timeout module (linkerd/linkerd2-proxy#2248) * outbound: Decouple outbound HTTP server from logical target (linkerd/linkerd2-proxy#2249) * build(deps): bump http from 0.2.8 to 0.2.9 (linkerd/linkerd2-proxy#2253) * build(deps): bump fastrand from 1.8.0 to 1.9.0 (linkerd/linkerd2-proxy#2252) * build(deps): bump signal-hook-registry from 1.4.0 to 1.4.1 (linkerd/linkerd2-proxy#2254) * Use `ExtractParam` in `http::NewHeaderFromTarget` (linkerd/linkerd2-proxy#2255) * outbound: Introduce a new outbound-route watch type (linkerd/linkerd2-proxy#2250) * build(deps): bump tj-actions/changed-files from 35.5.5 to 35.5.6 (linkerd/linkerd2-proxy#2256) * build(deps): bump hyper from 0.14.23 to 0.14.24 (linkerd/linkerd2-proxy#2257) * build(deps): bump once_cell from 1.17.0 to 1.17.1 (linkerd/linkerd2-proxy#2258) * build(deps): bump clang-sys from 1.4.0 to 1.6.0 (linkerd/linkerd2-proxy#2259) * outbound: Add a policy router (linkerd/linkerd2-proxy#2251) * inbound: connections wait for ServerPolicy discovery (linkerd/linkerd2-proxy#2186) * inbound: Remove default policies (linkerd/linkerd2-proxy#2204) * inbound: Introduce a `policy::LookupAddr` type (linkerd/linkerd2-proxy#2264) * build(deps): bump futures from 0.3.25 to 0.3.26 (linkerd/linkerd2-proxy#2263) * build(deps): bump proc-macro2 from 1.0.50 to 1.0.51 (linkerd/linkerd2-proxy#2261) * build(deps): bump tinyvec_macros from 0.1.0 to 0.1.1 (linkerd/linkerd2-proxy#2262) Signed-off-by: Oliver Gould <ver@buoyant.io> * proxy: v2.191.2 Revert inbound policy discovery behavior changes (from v2.191.0) and update dependencies, especially h2. --- * Revert inbound policy discovery changes (linkerd/linkerd2-proxy#2267) * Bump v38 to v39 (linkerd/linkerd2-proxy#2269) * dev: override CXX for rust-analyzer (linkerd/linkerd2-proxy#2270) * build(deps): bump syn from 1.0.107 to 1.0.109 (linkerd/linkerd2-proxy#2274) * build(deps): bump tokio-util from 0.7.4 to 0.7.7 (linkerd/linkerd2-proxy#2272) * build(deps): bump tj-actions/changed-files from 35.5.6 to 35.6.0 (linkerd/linkerd2-proxy#2271) * build(deps): bump prost-build from 0.11.6 to 0.11.8 (linkerd/linkerd2-proxy#2273) * ci: Add a linkerd install workflow (linkerd/linkerd2-proxy#2268) * allow `opaque_hidden_inferred_bound` warning on nightly (linkerd/linkerd2-proxy#2275) * build(deps): bump h2 from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#2278) * build(deps): bump tempfile from 3.3.0 to 3.4.0 (linkerd/linkerd2-proxy#2277) * build(deps): bump tokio-stream from 0.1.11 to 0.1.12 (linkerd/linkerd2-proxy#2276) * stack: Make `failfast::Gate` general purpose (linkerd/linkerd2-proxy#2279) * build(deps): bump slab from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#2283) * build(deps): bump async-stream from 0.3.3 to 0.3.4 (linkerd/linkerd2-proxy#2282) * build(deps): bump jobserver from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#2281) * build(deps): bump tj-actions/changed-files from 35.6.0 to 35.6.1 (linkerd/linkerd2-proxy#2280) Signed-off-by: Oliver Gould <ver@buoyant.io> --------- Signed-off-by: Oliver Gould <ver@buoyant.io>

hawkw commented Jan 25, 2023

View reviewed changes

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

olix0r reviewed Jan 26, 2023

View reviewed changes

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

linkerd/app/inbound/src/policy/store.rs Outdated Show resolved Hide resolved

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

olix0r reviewed Jan 26, 2023

View reviewed changes

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

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

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

hawkw changed the title ~~[wip] perform ServerPolicy resolutions asynchronously~~ inbound: connections wait for ServerPolicy discovery Jan 26, 2023

hawkw marked this pull request as ready for review January 26, 2023 20:01

hawkw requested a review from a team as a code owner January 26, 2023 20:01

hawkw and others added 6 commits January 30, 2023 14:53

make server policy store async

8934ce0

worky (direct stack is gross)

91a8f40

Update linkerd/app/inbound/src/policy/store.rs

31d7622

Co-authored-by: Oliver Gould <ver@buoyant.io>

always use Error as the error type

c561eed

clean up direct stack a bit

f60833f

misc cleanup

228b1f7

hawkw force-pushed the eliza/no-default-policy branch from 7a085ea to 228b1f7 Compare January 30, 2023 23:33

Merge branch 'main' into eliza/no-default-policy

bf4b3de

Merge branch 'main' into eliza/no-default-policy

48375bf

hawkw mentioned this pull request Feb 1, 2023

inbound: remove default policies #2204

Merged

olix0r reviewed Feb 22, 2023

View reviewed changes

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

olix0r reviewed Feb 22, 2023

View reviewed changes

linkerd/app/admin/src/stack.rs Outdated Show resolved Hide resolved

hawkw added 4 commits February 22, 2023 11:03

put ready back

f5564e7

pull GetPolicy bounds to trait-level

5752a5b

Merge branch 'main' into eliza/no-default-policy

cdeeaba

GetPolicy can be passed as impl GetPolicy

2c34acf

now that it doesn't need additional bounds

hawkw requested a review from olix0r February 22, 2023 19:19

olix0r reviewed Feb 22, 2023

View reviewed changes

linkerd/app/inbound/src/policy.rs Show resolved Hide resolved

fixups

3a2e376

olix0r reviewed Feb 22, 2023

View reviewed changes

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

hawkw added 3 commits February 22, 2023 15:59

separately newtype policy::LookupAddrs

1252ca7

disembreak gateway tests

c910ca9

Revert "disembreak gateway tests"

635afad

This reverts commit c910ca9.

hawkw added 2 commits February 22, 2023 17:29

review feedback

9a54acc

blanket impl GetPolicy for Service

2476c77

hawkw requested a review from olix0r February 23, 2023 01:32

olix0r reviewed Feb 23, 2023

View reviewed changes

linkerd/app/inbound/src/policy/store.rs Outdated Show resolved Hide resolved

linkerd/app/inbound/src/policy/discover.rs Outdated Show resolved Hide resolved

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

olix0r and others added 3 commits February 22, 2023 19:12

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

f481cd5

Update linkerd/app/inbound/src/policy/store.rs

bc5d93e

cascade through lift_new_with_target

973b9e6

hawkw requested a review from olix0r February 23, 2023 17:56

hawkw added 4 commits February 23, 2023 09:59

diff ensmalleration

0006083

`(A, B)` is already `From<(A, B)>` via identity

-check

35b08e5

further diff ensmalleration

01f56be

ensmallerate direct stack diff a bit more

744feff

thanks to the magic of ExtractParam!

hawkw mentioned this pull request Feb 23, 2023

inbound: refactor newtypes for policy addresses #2264

Merged

olix0r reviewed Feb 23, 2023

View reviewed changes

linkerd/idle-cache/src/lib.rs Outdated Show resolved Hide resolved

olix0r added 2 commits February 23, 2023 21:01

implement Service for Store (instead of GetProfile)

604bbf7

simplify idlecache api change

c3deef4

hawkw commented Feb 23, 2023

View reviewed changes

linkerd/app/inbound/src/policy/store.rs Show resolved Hide resolved

olix0r approved these changes Feb 23, 2023

View reviewed changes

hawkw merged commit c186d88 into main Feb 23, 2023

hawkw deleted the eliza/no-default-policy branch February 23, 2023 21:32

olix0r mentioned this pull request Feb 24, 2023

proxy: v2.191.0 linkerd/linkerd2#10390

Closed

olix0r mentioned this pull request Feb 25, 2023

Revert inbound policy discovery changes #2267

Merged

olix0r mentioned this pull request Mar 1, 2023

proxy: v2.191.2 linkerd/linkerd2#10427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inbound: connections wait for ServerPolicy discovery #2186

inbound: connections wait for ServerPolicy discovery #2186

hawkw commented Jan 25, 2023 •

edited

Loading

hawkw commented Jan 31, 2023

inbound: connections wait for ServerPolicy discovery #2186

inbound: connections wait for ServerPolicy discovery #2186

Conversation

hawkw commented Jan 25, 2023 • edited Loading

hawkw commented Jan 31, 2023

hawkw commented Jan 25, 2023 •

edited

Loading