Skip to content
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

inbound: determine default policies using the opaque ports env var #2395

Merged
merged 15 commits into from
Apr 25, 2023

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Apr 19, 2023

The proxy injector populates an environment variable,
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION, with a list
of all ports marked as opaque. Currently, however, _the proxy does not
actually use this environment variable
. Instead, opaque ports are
discovered from the policy controller. The opaque ports environment
variable was used only when running in the "fixed" inbound policy mode,
where all inbound policies are determined from environment variables,
and no policy controller address is provided. This mode is no longer
supported, and the policy controller address is now required, so the
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION environment
variable is not currently used to discover inbound opaque ports.

There are two issues with the current state of things. One is that
inbound policy discovery is non-blocking: when an inbound proxy
receives a connection on a port that it has not previously discovered a
policy for, it uses the default policy until it has successfully
discovered a policy for that port from the policy controller. This means
that the proxy may perform protocol detection on the first connection to
an opaque port. This isn't great, as it may result in a protocol
detection timeout error on a port that the user had previously marked as
opaque. It would be preferable for the proxy to read the environment
variable, and use it to determine whether the default policy for a port
is opaque, so that ports marked as opaque disable protocol detection
even before the "actual" policy is discovered.

The other issue with the
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION environment
variable is that it is currently a list of individual port numbers,
while the proxy injector can accept annotations that specify ranges of
opaque ports. This means that when a very large number of ports are
marked as opaque, the proxy manifest must contain a list of each
individual port number in those ranges, making it potentially quite
large. See linkerd/linkerd2#9803 for details on this issue.

This branch addresses both of these problems. The proxy is changed so
that it will once again read the
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION environment
variable, and use it to determine which ports should have opaque
policies by default. The parsing of the environment variable is changed
to support specifying ports as a list of ranges, rather than a list of
individual port numbers. Along with a proxy-injector change, this would
resolve the manifest size issue described in linkerd/linkerd2#9803.

This is implemented by changing the inbound::policy::Store type to
also include a set of port ranges that are marked as opaque. When the
Store handles a get_policy call for a port that is not already in
the cache, it starts a control plane watch for that port just as it did
previously. However, when determining the initial default value for
the policy, before the control plane discovery provides one, it checks
whether the port is in a range that is marked as opaque, and, if it is,
uses an opaque default policy instead.

This approach was chosen rather than pre-populating the Store with
policies for all opaque ports to better handle the case where very large
ranges are marked as opaque and are used infrequently. If the Store
was pre-populated with default policies for all such ports, it would
essentially behave as though all ports in
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION were also in
LINKERD2_PROXY_INBOUND_PORTS, and the proxy would immediately start a
policy controller discovery watch for all opaque ports, which would be
kept open for the proxy's entire lifetime. In cases where the opaque
ports ranges include ~10,000s of ports, this causes significant
unnecessary load on the policy controller. Storing opaque port ranges
separately and using them to determine the default policy as needed
allows opaque port policies to be treated the same as non-default ports,
which are discovered as needed and can be evicted from the cache if they
are unused. If a port is in both
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION and
LINKERD2_PROXY_INBOUND_PORTS, the proxy will start discovery eagerly
and retain the port in the cache forever, but the default policy will be
opaque.

I've also added a test for the behavior of opaque ports where the port's
policy has not been discovered from the policy controller. That test
fails on main, as the proxy attempts protocol detection, but passes on
this branch.

In addition, I changed the parsing of the LINKERD2_PROXY_INBOUND_PORTS
environment variable to also accept ranges, because it seemed like a
nice thing to do while I was here. :)

hawkw added 4 commits April 19, 2023 10:08
This adds a test for the handling of inbound opaque connections when the
port is *not* in the `LINKERD2_PROXY_INBOUND_PORTS` environment variable
and the opaque policy is not eagerly discovered from the control plane
on startup. Currently, this test fails because the proxy does not know
to handle connections on this port as opaque, and attempts to perform
protocol detection instead.
This changes the `policy::Store` type to accept a set of port ranges
which should be marked as opaque. This allows the default policy used to
initialize the policy watch to differ based on whether or not a port is
marked as opaque. This way, when starting discovery for an opaque port,
an opaque default policy will be used until the port's actual policy is
discovered from the control plane.
@hawkw hawkw requested a review from a team as a code owner April 19, 2023 17:51
@hawkw hawkw self-assigned this Apr 19, 2023
hawkw and others added 7 commits April 24, 2023 14:09
Looks like we accidentally merged PR #2375 without a CI build against
the latest state of `main`. In the meantime since #2375 was last built
on CI, PR #2374 added an additional metadata field to
`policy::HttpParams`, which the `HttpParams` constructed in the test
added from #2375 doesn't populate. Therefore, merging this PR broke the
build. Whoops!

This commit populates the `meta` field, fixing it.
; Conflicts:
;	linkerd/app/integration/src/tests/transparency.rs
@hawkw hawkw merged commit ad4d5b6 into main Apr 25, 2023
@hawkw hawkw deleted the eliza/opaque-port-ranges branch April 25, 2023 16:56
hawkw added a commit to linkerd/linkerd2 that referenced this pull request Apr 25, 2023
This proxy release fixes an issue where outbound proxies would attempt
protocol detection on ports that are marked as opaque. It also adds
support for ranges (such as `10-20`) in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` and
`LINKERD2_PROXY_INBOUND_PORTS` environment variables. Finally, it
changes the proxy to synthesize default client policies when the policy
controller returns an `Unimplemented` gRPC status code, allowing 2.13
proxies to coexist with 2.12 control planes in downgrade scenarios.

---

* outbound: determine protocol based on `OutboundPolicy` (linkerd/linkerd2-proxy#2397)
* set default `trust_dns` log level to `ERROR` (linkerd/linkerd2-proxy#2393)
* outbound: test load balancer behavior with failure accrual (linkerd/linkerd2-proxy#2375)
* outbound: add missing `meta` field in test policy (linkerd/linkerd2-proxy#2400)
* inbound: determine default policies using the opaque ports env var (linkerd/linkerd2-proxy#2395)
* outbound: synthesize client policies on `Unimplemented` (linkerd/linkerd2-proxy#2396)
* build(deps): bump io-lifetimes from 1.0.4 to 1.0.10 (linkerd/linkerd2-proxy#2379)
* chore: allow `syn` v1 and v2 to coexist peacefully (linkerd/linkerd2-proxy#2401)
* build(deps): bump futures from 0.3.26 to 0.3.28 (linkerd/linkerd2-proxy#2370)
* build(deps): bump async-trait from 0.1.66 to 0.1.68 (linkerd/linkerd2-proxy#2368)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to linkerd/linkerd2 that referenced this pull request Apr 26, 2023
This proxy release fixes an issue where outbound proxies would attempt
protocol detection on ports that are marked as opaque. It also adds
support for ranges (such as `10-20`) in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` and
`LINKERD2_PROXY_INBOUND_PORTS` environment variables. Finally, it
changes the proxy to synthesize default client policies when the policy
controller returns an `Unimplemented` gRPC status code, allowing 2.13
proxies to coexist with 2.12 control planes in downgrade scenarios.

---

* outbound: determine protocol based on `OutboundPolicy` (linkerd/linkerd2-proxy#2397)
* set default `trust_dns` log level to `ERROR` (linkerd/linkerd2-proxy#2393)
* outbound: test load balancer behavior with failure accrual (linkerd/linkerd2-proxy#2375)
* outbound: add missing `meta` field in test policy (linkerd/linkerd2-proxy#2400)
* inbound: determine default policies using the opaque ports env var (linkerd/linkerd2-proxy#2395)
* outbound: synthesize client policies on `Unimplemented` (linkerd/linkerd2-proxy#2396)
* build(deps): bump io-lifetimes from 1.0.4 to 1.0.10 (linkerd/linkerd2-proxy#2379)
* chore: allow `syn` v1 and v2 to coexist peacefully (linkerd/linkerd2-proxy#2401)
* build(deps): bump futures from 0.3.26 to 0.3.28 (linkerd/linkerd2-proxy#2370)
* build(deps): bump async-trait from 0.1.66 to 0.1.68 (linkerd/linkerd2-proxy#2368)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to linkerd/linkerd2 that referenced this pull request Apr 27, 2023
Currently, the proxy injector will expand lists of opaque port ranges
into lists of individual port numbers. This is because the proxy has
historically not accepted port ranges in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment
variable. However, when very large ranges are used, the size of the
injected manifest can be quite large, since each individual port number
in a range must be listed separately.

Proxy PR linkerd/linkerd2-proxy#2395 changed the proxy to accept ranges
as well as individual port numbers in the opaque ports environment
variable, and this change was included in the latest proxy release
(v2.200.0). This means that the proxy-injector no longer needs to expand
large port ranges into individual port numbers, and can now simply
forward the list of ranges to the proxy. This branch changes the proxy
injector to do this, resolving issues with manifest size due to large
port ranges.

Closes #9803
risingspiral pushed a commit to linkerd/linkerd2 that referenced this pull request May 4, 2023
This proxy release fixes an issue where outbound proxies would attempt
protocol detection on ports that are marked as opaque. It also adds
support for ranges (such as `10-20`) in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` and
`LINKERD2_PROXY_INBOUND_PORTS` environment variables. Finally, it
changes the proxy to synthesize default client policies when the policy
controller returns an `Unimplemented` gRPC status code, allowing 2.13
proxies to coexist with 2.12 control planes in downgrade scenarios.

---

* outbound: determine protocol based on `OutboundPolicy` (linkerd/linkerd2-proxy#2397)
* set default `trust_dns` log level to `ERROR` (linkerd/linkerd2-proxy#2393)
* outbound: test load balancer behavior with failure accrual (linkerd/linkerd2-proxy#2375)
* outbound: add missing `meta` field in test policy (linkerd/linkerd2-proxy#2400)
* inbound: determine default policies using the opaque ports env var (linkerd/linkerd2-proxy#2395)
* outbound: synthesize client policies on `Unimplemented` (linkerd/linkerd2-proxy#2396)
* build(deps): bump io-lifetimes from 1.0.4 to 1.0.10 (linkerd/linkerd2-proxy#2379)
* chore: allow `syn` v1 and v2 to coexist peacefully (linkerd/linkerd2-proxy#2401)
* build(deps): bump futures from 0.3.26 to 0.3.28 (linkerd/linkerd2-proxy#2370)
* build(deps): bump async-trait from 0.1.66 to 0.1.68 (linkerd/linkerd2-proxy#2368)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
risingspiral pushed a commit to linkerd/linkerd2 that referenced this pull request May 4, 2023
Currently, the proxy injector will expand lists of opaque port ranges
into lists of individual port numbers. This is because the proxy has
historically not accepted port ranges in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment
variable. However, when very large ranges are used, the size of the
injected manifest can be quite large, since each individual port number
in a range must be listed separately.

Proxy PR linkerd/linkerd2-proxy#2395 changed the proxy to accept ranges
as well as individual port numbers in the opaque ports environment
variable, and this change was included in the latest proxy release
(v2.200.0). This means that the proxy-injector no longer needs to expand
large port ranges into individual port numbers, and can now simply
forward the list of ranges to the proxy. This branch changes the proxy
injector to do this, resolving issues with manifest size due to large
port ranges.

Closes #9803
risingspiral pushed a commit to linkerd/linkerd2 that referenced this pull request May 5, 2023
This proxy release fixes an issue where outbound proxies would attempt
protocol detection on ports that are marked as opaque. It also adds
support for ranges (such as `10-20`) in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` and
`LINKERD2_PROXY_INBOUND_PORTS` environment variables. Finally, it
changes the proxy to synthesize default client policies when the policy
controller returns an `Unimplemented` gRPC status code, allowing 2.13
proxies to coexist with 2.12 control planes in downgrade scenarios.

---

* outbound: determine protocol based on `OutboundPolicy` (linkerd/linkerd2-proxy#2397)
* set default `trust_dns` log level to `ERROR` (linkerd/linkerd2-proxy#2393)
* outbound: test load balancer behavior with failure accrual (linkerd/linkerd2-proxy#2375)
* outbound: add missing `meta` field in test policy (linkerd/linkerd2-proxy#2400)
* inbound: determine default policies using the opaque ports env var (linkerd/linkerd2-proxy#2395)
* outbound: synthesize client policies on `Unimplemented` (linkerd/linkerd2-proxy#2396)
* build(deps): bump io-lifetimes from 1.0.4 to 1.0.10 (linkerd/linkerd2-proxy#2379)
* chore: allow `syn` v1 and v2 to coexist peacefully (linkerd/linkerd2-proxy#2401)
* build(deps): bump futures from 0.3.26 to 0.3.28 (linkerd/linkerd2-proxy#2370)
* build(deps): bump async-trait from 0.1.66 to 0.1.68 (linkerd/linkerd2-proxy#2368)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eric Anderson <eric@buoyant.io>
risingspiral pushed a commit to linkerd/linkerd2 that referenced this pull request May 5, 2023
Currently, the proxy injector will expand lists of opaque port ranges
into lists of individual port numbers. This is because the proxy has
historically not accepted port ranges in the
`LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment
variable. However, when very large ranges are used, the size of the
injected manifest can be quite large, since each individual port number
in a range must be listed separately.

Proxy PR linkerd/linkerd2-proxy#2395 changed the proxy to accept ranges
as well as individual port numbers in the opaque ports environment
variable, and this change was included in the latest proxy release
(v2.200.0). This means that the proxy-injector no longer needs to expand
large port ranges into individual port numbers, and can now simply
forward the list of ranges to the proxy. This branch changes the proxy
injector to do this, resolving issues with manifest size due to large
port ranges.

Closes #9803

Signed-off-by: Eric Anderson <eric@buoyant.io>
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.

2 participants