Our nightly build is failing due to some dead code warnings that aren't being
triggered on stable.
This change runs `cargo +nightly clippy --fix` and then manually fixes the
remaining issues.
This change also updates the nightly and beta workflows so that they may be
triggered manually.
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>
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 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>
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 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.
katelyn martin