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.

Sign up for GitHub

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

Jump to bottom

pd: 🔨 rework RootCommand::start auto-https logic #3652

Merged
merged 4 commits into from
Jan 26, 2024
Merged

pd: 🔨 rework RootCommand::start auto-https logic #3652

merged 4 commits into from
Jan 26, 2024

Commits on Jan 25, 2024

  1. pd: 🔨 rework RootCommand::start auto-https logic 

     ## 👀 overview

fixes #3627.

this reorganizes the logic in pd's startup code related to automatically
managed https functionality.

 ## 🎨 background & motivation

this PR, besides cleaning up the `rustls-acme`-related auto-https logic, is
also interested in *creating a state-of-affairs that will dovetail into
pr #3522*. in particular, this expression to start the GRPC serve given a
bound listener...

```rust
tokio::task::Builder::new()
    .name("grpc_server")
    .spawn(grpc_server.serve_with_incoming(tls_incoming))
    .expect("failed to spawn grpc server")
```

...should be adjusted so as to work with an `axum::Router`.

 ### ⚖️  `rustls-acme` and `tokio-rustls-acme`

quoth the #3627 description, citing an earlier comment:

> In the ~year since this code was written, there may be better options.
> `tokio-rustls-acme` seems promising
\- <#3522 (comment)>

for reference, the repositories for each live here, and here:
- <https://github.com/FlorianUekermann/rustls-acme>
- <https://github.com/n0-computer/tokio-rustls-acme>

after some comparison, i have come to the opinion that `rustls-acme` will still
be adequate for our purposes. the latter is a fork of the former, but active
development appears to have continued in the former, and i did not see any
particular "_must-have_" features for us in the latter.

 ## 🎴 changes

this commit moves some of the auto-https related code from the `main`
entrypoint, into standalone functions in `pd::main`.

some constants are defined, to keep control flow clear and to help
facilitate the addition of future options e.g. a flag to control the
LetsEncrypt environment to use.

 ## 🚰 dropping down to `axum`; a brief note on future upgrades

as stated above, we want to switch to an `axum::Router`. this means that
we won't be able to use the `AcmeConfig::incoming` function. the
`rustls-acme` library provides some "low-level" examples this work is
based upon

- <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_tokio.rs>
- <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_axum.rs>

we also use `tonic` 0.10.2 in pd, and elsewhere in the penumbra
monorepo. tonic isn't using hyper 1.x yet. this was being worked on in
hyperium/tonic#1583, continued on in hyperium/tonic#1595, and tracked in
hyperium/tonic#1579. that work also depends upon hyperium/hyper#3461.

we will need to be wait for tonic to finish its migration over to hyper
1.0, see:
hyperium/tonic#1579 (comment)

this is understandable, but i make note of this situation as a
signpost for our future selves when considering a migration to
recent versions of axum-server, axum, rustls-acme, or hyper.

for now, it's easiest to stay in lock-step with tonic, and we can revisit
the upgrade path(s) at a future date.

===

Refs: #3627
Refs: #3646
Refs: #3522
    @cratelyn
    cratelyn committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    52e89c6 View commit details
    Browse the repository at this point in the history

  2. pd: 📎 address obfuscated_if_else lint 

    i disagree, but i yield to clippy
    @cratelyn
    cratelyn committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    195184d View commit details
    Browse the repository at this point in the history

  3. pd: 📎 address redundant_static_lifetimes lint

    @cratelyn
    cratelyn committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    1716885 View commit details
    Browse the repository at this point in the history

  4. pd: 👋 remove unused axum dependency 

    we call into axum via axum-server for now, so we don't need this
dependency. (...yet)
    @cratelyn
    cratelyn committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    6fb7091 View commit details
    Browse the repository at this point in the history