Currently, all crates in the proxy have `#![deny(warnings)]` attributes.
These turn all rustc warnings into errors globally for these crates.
There are two primary issues with this approach:
1. `#![deny(warnings)]` will deny the "deprecated" warning, turning
upstream deprecations into broken builds. However, APIs may be
deprecated in semver-compatible releases, so this means a
non-breaking dependency release can break our builds. See [here][1]
for details.
2. Sometimes, while working on an in-development change, warnings may be
temporarily introduced. For example, if a developer is working on
some change and adds a function that will be used as part of that
change but isn't called yet because the change is incomplete, this
will emit a dead code warning and break the build. Because these
warnings are turned into errors, this can prevent a developer from
running tests while working on a change that's in an incomplete
state.
The typical solution to this is to temporarily remove the
`#![deny(warnings)]` attributes while working on a change that
temporarily introduces warnings. However, the downside of this is
this can accidentally be committed, and then warnings will no longer
be denied for a particular crate. This means we have another thing to
look out for when reviewing PRs.
As an alternative, this branch changes the proxy's CI configuration so
that warnings are denied on CI builds using `RUSTFLAGS="-D warnings"`,
rather than using `#![deny(...)] attributes in the code itself. This has
the advantage that when building locally, warnings don't fail the build.
In addition, I've added a separate CI job in the `deps` workflow that's
specifically for checking for deprecation warnings using `RUSTFLAGS="-D
deprecated". All the other builds now pass `RUSTFLAGS="-D warnings -A
deprecated" to allow deprecations, so that they can't break other
builds. The deprecations CI job should be allowed to fail, as it's
informational --- it shouldn't block PRs from merging, but it provides
information about things that should be addressed when possible.
In the future, it would be nice to change this job so that it creates
new issues for deprecation warnings rather than failing, similarly to
what we currently do for security advisories. That way, new deprecations
no longer block PRs from merging, but instead create issues that can be
fixed separately.
[1]: https://rust-unofficial.github.io/patterns/anti_patterns/deny-warnings.html
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This branch updates the proxy's dependencies on Tokio and Tower to
1.17.0 and 0.4.12, respectively. These releases contain fixes for panics
when `std::time::Instant` fails to be properly monotonic.
Previously, we were using Git dependencies to ensure we got these fixes.
Now, we can remove those patches, as the released versions of these
crates have the necessary changes.
We use `parking_lot` locks throughout our code. This change disallows the
introduction of std::sync's locks.
This change also enforces the use of `tokio::time::Instant`, which
allows for a mockable time source in tests.
Signed-off-by: Oliver Gould <ver@buoyant.io>
Per rust-lang/rust#90308, this is potentially a data race. In practice,
I don't think it was actually problematic here, but it also wasn't doing
anything of value, so we should remove it.
This is currently the only `env::set_var` or `env::remove_var` call in
the proxy.
To prevent uses of these functions in the future, I also added a
`disallowed-methods` configuration in `.clippy.toml` to emit a
warning for any uses of `std::env::set_var` and
`std::env::remove_var`. Unfortunately, this required adding a
`deny` attribute basically everywhere, which is why this diff touches
so many files.
Closeslinkerd/linkerd2#7651
Currently, the proxy will retry requests with bodies if and only if they
have a `content-length` header with a value <= 64 KB. If the request has
a body but no `content-length` header, we currently assume that its body
will exceed the maximum buffered size, and skip trying to retry it.
However, this means gRPC requests will never be retried, because it
turns out gRPC requests don't include a `content-length` header (see
linkerd/linkerd2#7141). Whoops!
This PR fixes this by changing the retry logic to use `Body::size_hint` to
determine if buffering should be attempted. This value will be set from
`content-length` when it is set and may be set in additional situations
where the body length is known before the request is processed.
We are still protected against unbounded buffering because the buffering
body will stop buffering and discard any previously buffered data if the
buffered length ever exceeds the maximum.
Co-authored-by: Oliver Gould <ver@buoyant.io>
This change updates `tokio`, `hyper`, and `socket2` (from v0.3 to v0.4).
This has a few implications:
* `tokio::select!` can now be used in place of
`futures::select_biased!`;
* We now only depend on the core `futures` functionality with all
features disabled.
* `socket2` had several small API changes that have been addressed.
This branch updates the `futures` crate to v0.3.15. This includes a fix
for task starvation with `FuturesUnordered` (added in 0.3.13). This may
or may not be related to issues that have been reported in the proxy
involving the load balancer (linkerd/linkerd2#6086), but we should
update to the fixed version regardless. This may also improve
performance in some cases, since we may now have to do fewer poll-wakeup
cycles when a load balancer has a large number of pending endpoints.
This change adds the `forbid(unsafe_code)` directive to most modules,
ensuring that these modules are "safe". There are three modules this
cannot apply to:
- `duplex` -- due to the implementation of `BufMut` for `CopyBuf`; and
- `proxy::transport` -- due to socket option interactions.
- `system` -- for system-level counter access
Furthermore, this change adds `deny(warnings)` directives to a few
modules that were missing it.
This branch updates the Rust toolchain to 1.52.1. This includes a [major
bugfix][1] for an issue effecting incremental compilation. Additionally,
Rust 1.51 enabled the `resolver = "2"` feature in Cargo.toml, which can
be used to fix the feature flagging issues with tracing in a more
principled way than the current solution.
A *very* large amount of new lints were added since Rust 1.49.0; in
particular:
* clippy now warns when an `Into` impl could be a `From` impl, because
`From` impls provide `Into` impls for free, but the reverse is not
the case
* panic messages must now *always* be `format_args!`
* exact comparisons of floating-point numbers (rather than within an
error margin) now produce a warning
This branch also fixes all the new warnings.
[1]: https://blog.rust-lang.org/2021/05/10/Rust-1.52.1.html
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
The recently released v0.4.5 of Tower includes a change to
`tower::buffer` to use `tokio-util`'s `PollSemaphore`, which should
reduce allocations when polling `Buffer` services. Additionally, it
includes a fix for `tracing` spans not being propagated to the tasks
spawned by the `SpawnReady` middleware.
No functional changes.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This change renames most subcrates to be linkerd-* instead of
linkerd2-*, matching the repository layout.
The `linkerd2-proxy` and `linkerd2-proxy-api` crate names remain
unchanged, as they reflect their respective repository names.
The cargo-deny utility provides a convenient way to audit the proxy's
dependency tree to audit security vulnerabilities & licensing.
Furthermore, it helps detect whether we have multiple versions of a
given dependency.
This change adds a `deny.toml` that configures the project to detect all
such issues and it adds a CI task that can help prevent us from
regressing.
This change introduces a number of dependency updates to eliminate
issues flagged by cargo-deny:
* `futures` has been updated to v0.3.9 to fix safety issues;
* `pin-project` has been updated to v1;
* `regex` has been updated to v1;
* `quickcheck` has been updated to v1, which caught minor issues in
exp-backoff that have been fixed;
Furthermore, this change updates our github actions to use more more
recent versions of the base container image and updates actions/checkout
to v2.
This branch updates the proxy to Tokio 1.0 and its ecosystem, including:
- `tokio` 1.0 (obviously)
- the `tokio` 1.0 versions of `tokio-test` and `tokio-util`
- `bytes` 1.0
- `tower` 0.4
- `hyper` 0.14.2
- `h2` 0.3
- `tonic`, via git dependency
- `prost` 0.7
- `tokio-rustls` 0.22
- `rustls` 0.19
- `trust-dns-resolver` 0.20
- `linkerd2-proxy-api`, via git dependency
- `rand` 0.8
For the most part, the change here is pretty straightforward, just
updating the specified dependency versions. The API changes in `tokio`
were mostly made in 0.3, so we have already tracked most of them.
However, the following small changes were necessary:
- Hyper now has feature flags that must be enabled.
- `bytes` renamed `Buf::bytes` and `Buf::bytes_vectored` and
`BufMut::bytes_mut` to `chunk`, `chunk_vectored`, and `chunk_mut`,
respectively.
- `tokio::sync::Semaphore` now has a `close` method, and `acquire`
returns a `Result`. Updated `linkerd2-channel` to track this. This
also allowed removing some kind of janky jury-rigged code for closing
a semaphore.
- The `tokio::Runtime::builder::max_threads` method was renamed to
`max_blocking_threads`.
There were also a couple of larger changes. In particular:
- `tokio` no longer re-exports the `Stream` trait from `Futures`, for
stability reasons. I added a wrapper API for implementing `Stream` for
`mpsc` receivers and for `tokio::time::Interval`.
- `tokio::time::Sleep` is now `!Unpin`, since it no longer contains an
internal heap allocation. This reduces heap overhead when used with
async/await syntax. However, for manual `Future` and `tower::Service`
impls, the `Box` must be added _outside_ the `Sleep` to preserve
`Unpin`nedness. I've done this in all places where it was necessary. I
also changed some `Service` impls (`failfast` and `switch_ready`) which
repeatedly create new `Sleep` futures, to use `Sleep::reset` instead,
allowing them to avoid reallocations.
This also means that `tower`'s `Timeout` middleware's future is no
longer `Unpin`. Since connect futures are required to be `Unpin`, and
the connect stacks contain timeout layers, I added a new `BoxResponse`
middleware layer for boxing *just* response futures, while preserving
the type of the wrapped `Service` (so that connect stacks are still
`Clone`).
- Removed the `linkerd-app-profiling` crate, which has bit-rotted.
The latest `tower` release also adds upstream implementations of a
number of middlewares we've written our own versions of. As a follow-up,
we probably want to replace some of this stuff with the upstream
versions. However, I thought that we would probably want to do this in a
separate PR, to minimize the size of this change.
We haven't run clippy on this project in a looong time. This change
fixes or acknowledges all current clippy warnings. There should be no
functional changes.
Clippy linting has also been added to CI.
This change updates the proxy to use Tokio 0.3 and the Tokio 0.3
versions of various ecosystem crates. This includes `tower` 0.4 and
`bytes` 0.6, as well as the Tokio 0.3 versions of `tokio-util`, `hyper`,
`tonic`, etc. Due to API changes in Tokio and in other dependencies, it
was necessary to make some code changes as well as updating
dependencies, but there should be no functional change.
In particular:
* Tokio's support for vectored IO changed significantly in 0.3, so this
branch updates our use of `AsyncWrite` to participate in the new
vectored write APIs
* Hyper's HTTP/1.1 upgrade API changed in 0.14, so this branch changes the
proxy's code for handling CONNECT to use the new API
* Tokio removed support for some socket options, which now need to be
set using `socket2`
* Tokio removed the `poll_ready` method was removed from the bounded
MPSC channel, so the proxy's buffers (`linkerd2-buffer` and the
`buffer` module in `linkerd2-proxy-discover`) had to be switched to
our own implementation (this merged separately, in PR #759).
Several ecosystem crates have yet to be released, so we depend on them
via Git dependencies for now. The patches in Cargo.toml can be
removed as other dependencies publish their Tokio 0.3 versions.
This branch updates the proxy codebase from `futures` 0.1 and Tokio 0.1
to `std::future`, Tokio 0.2, and the associated ecosystem. Wherever I
could, I've tried to keep this translation as mechanical as possible. In
a few places, some significant structural changes and refactoring was
necessary, largely due to stack pinning requirements, but there should
be no behavioral changes. The new ecosystem does present opportunities
for more significant refactoring, but we should probably do that in
follow-up changes instead.
All of this code has already been reviewed in a series of incremental
PRs to the `master-tokio-0.2` branch. This branch's history can be read
for details on the update process. In addition, all the proxy's
integration tests have been updated and now pass against this branch,
and performance testing indicates that there have been no significant
changes.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
* internal: internal: Spilt app from linkerd2-proxy
The `linkerd2-proxy crate currently comprises the entirety of the
application logic for the proxy. This unfortunately leads to exceedingly
high compile times (35+ minutes to compile the application with tests).
Specifically:
* Any change to the inbound or outbound proxy configuration necessitated
recompiling the other; and this compilation could not be parallelized.
* Integration tests depended on the `linkerd2-proxy` executable, adding
about 10 minutes to every build.
* The tests/support module (which is also extremely costly to build) was
compiled _for each integration test_.
This change restructures the crates in this repository to allow `cargo`
to cache intermediate code that was otherwise being compiled
redundantly or serially:
* The `linkerd2-proxy` crate now contains _only_ the executable and need
not be built during tests.
* The `linkerd2-app` crate exposes the app's `Main`, but uses
`linkerd2-app-inbound` and `linkerd2-app-outbound` subcrates to
improve parellization/cacheability.
* The rest of the top-level application code
* The `linkerd2-app-integration` crate now contains all of the
integration test support code (as well as the tests themselves), so
that the tests only need to compile the support library once.
All in all, this reduces compile time to under 20 minutes.
Oliver Gould
Eliza Weisman
dependabot[bot]