remove fixed inbound policy mode #2079
Closed
Conversation
This file contains hidden or 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
Currently, the inbound proxy is capable of operating in two modes: a fixed mode, where no policy controller address is provided, and inbound port policies are configured by the environment variables `LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION`, `LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_IDENTITY`, and `LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS`; and a discovery-based mode, where port policies are discovered from the policy controller. In practice, the fixed mode is never actually used, so we probably ought to remove it entirely in 2.13. This branch removes the fixed mode for inbound port policy. Starting the proxy without the `LINKERD2_PROXY_POLICY_SVC_ADDR` and `LINKERD2_PROXY_POLICY_SVC_IDENTITY` environment variables set is now an error. If the `LINKERD2_INBOUND_PORTS_REQUIRE_IDENTITY` or `LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS` environment variables are present, the proxy will now log a warning and ignore their values. The `LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment variable is still supported, however, in order to allow the proxy to avoid performing policy controller lookups when configured with a large list of opaque ports. The `Store::spawn_fixed` method on `linkerd_app_inbound::policy::Store` is no longer used when running the proxy normally, but it is retained for testing purposes, and has been made test-only.
This is necessary now that a policy controller is always required. Otherwise, the tests all fail. :)
hawkw
force-pushed
the
eliza/rm-fixed-policy
branch
from
377db61 to
014c443
Compare
hawkw
added a commit
that referenced
this pull request
Dec 21, 2022
Depends on #2079 Currently, the `LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment variable accepts a comma-delimited list of individual port numbers. Each of these ports is then stored as a separate cache entry in the port policy cache to indicate that the port is opaque. Unfortunately, this does not work well when a very large number of ports are marked as opaque, because the proxy-injector will generate a massive value for that environment variable, listing every individual port. This can cause issues when this manifest becomes larger than the Kubernetes API can reasonably handle. In addition, huge numbers of ports will all be kept in memory as a separate cache entry by the proxy, increasing proxy memory usage. See linkerd/linkerd2#9803 for details. This branch changes the proxy so that the `LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment variable may contain a list of individual port numbers *and* port ranges, which are specified as `<low>-<high>`. Opaque ports are now stored using a `RangeInclusiveSet` from the `rangemap` crate, rather than by creating individual cache entries for each port. This means we are no longer storing a separate entry in the cache for every port in a range, reducing memory consumption when there are very large ranges of opaque ports. This is the proxy half of the work towards resolving linkerd/linkerd2#9803. Once this branch lands, we'll also have to change the proxy-injector so that it no longer handles opaque port ranges by generating a list of all the individual ports in the range, and simply forwards those ranges as they were specified when generating the environment variable.
Contributor
Author
|
fuzzer build failure looks like maybe a nightly regression? https://github.com/linkerd/linkerd2-proxy/actions/runs/3752888848/jobs/6375571794 |
hawkw
commented
Dec 22, 2022
olix0r
reviewed
Dec 23, 2022
| /// Most policies are watched dynamically from the control plane. A default | ||
| /// policy is used when the controller does not provide a policy for a given | ||
| /// port. In addition, a pre-configured list of opaque ports can be provided | ||
| /// to avoid unnecessary policy lookups for ports which will always be opaque. |
Member
There was a problem hiding this comment.
I don't think this is correct.
Suggested change
| /// to avoid unnecessary policy lookups for ports which will always be opaque. | |
| /// to control the proxy's default behavior before policy is synced | |
| /// from the control plane policy. |
We still lookup policies for these... What we configure here is the default behavior, i.e. on the first request before the resolution is complete.
We don't currently block inbound connections on a policy existing. That could change in the future, I guess...
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.
Currently, the inbound proxy is capable of operating in two modes: a
fixed mode, where no policy controller address is provided, and inbound
port policies are configured by the environment variables
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION,LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_IDENTITY, andLINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS; and a discovery-based mode,where port policies are discovered from the policy controller. In
practice, the fixed mode is never actually used, so we probably ought to
remove it entirely in 2.13.
This branch removes the fixed mode for inbound port policy. Starting the
proxy without the
LINKERD2_PROXY_POLICY_SVC_ADDRandLINKERD2_PROXY_POLICY_SVC_IDENTITYenvironment variables set is now anerror. If the
LINKERD2_INBOUND_PORTS_REQUIRE_IDENTITYorLINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLSenvironment variables arepresent, the proxy will now log a warning and ignore their values. The
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTIONenvironmentvariable is still supported, however, in order to allow the proxy to
avoid performing policy controller lookups when configured with a large
list of opaque ports.
The
Store::spawn_fixedmethod onlinkerd_app_inbound::policy::Storeis no longer used when running the proxy normally, but it is retained
for testing purposes, and has been made test-only.
Finally, it was necessary to add a mock implementation of the policy
controller for integration tests to pass, since a policy controller is
now required. This is based on the implementation of a mock policy
controller added in #1992, but without the outbound policy API. PR #1992
should be able to rebase cleanly once this branch merges.
Closes #2076