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

Migrate to the latest version of Abscissa (0.7 or later) #5502

Closed
Tracked by #6390
arya2 opened this issue Oct 28, 2022 · 9 comments · Fixed by #6801
Closed
Tracked by #6390

Migrate to the latest version of Abscissa (0.7 or later) #5502

arya2 opened this issue Oct 28, 2022 · 9 comments · Fixed by #6801
Assignees
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-security Category: Security issues S-needs-triage Status: A bug report needs triage

Comments

@arya2
Copy link
Contributor

arya2 commented Oct 28, 2022

Motivation

We need to update our command-line argument parsing to bump abscissa_core from 0.5.2 to 0.7.0 or later.

This is also needed to make the output of the --help flag and help command consistent with that of invalid options or commands, by replacing gumdrop with clap.

It is also needed to avoid a compilation error that is likely to be added to the Rust 2024 edition:
rust-lang/rust#79813

This error is currently a warning on nightly.

Security

This will fix a low-severity security issue - we don't actually call the vulnerable abscissa 0.5.2 or tracing-subscriber 0.1.6 code, so it's only an issue at startup.

It will also make our dependency tree smaller, which improves compile speed, binary size, and security.

Vulnerability:
GHSA-wcg3-cvx6-7396
time-rs/time#293

Fix:
chronotope/chrono#602 (comment)

Steps

Migration guide

  1. Upgrade command-line argument parsing
  2. Upgrade abscissa
    a. Consider replacing Zebra's custom tokio and metrics components with the current Abscissa implementations
  3. Turn future incompatibility warnings back on, by reverting fix(clippy): Silence future-incompat warnings until we upgrade Abscissa #6024

Related Work

@arya2 arya2 added A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage P-Optional ✨ labels Oct 28, 2022
@teor2345
Copy link
Contributor

Thanks!

We're unlikely to do this any time soon, because we've replaced abscissa components with our own custom versions instead.

@teor2345 teor2345 changed the title Migrate to Abscissa 0.6.0 Migrate to Abscissa 0.6 Dec 6, 2022
@teor2345 teor2345 added P-Low ❄️ C-security Category: Security issues and removed P-Optional ✨ labels Dec 6, 2022
@teor2345 teor2345 changed the title Migrate to Abscissa 0.6 Migrate to the latest version of Abscissa (0.7 or later) Jan 23, 2023
@teor2345
Copy link
Contributor

Abscissa 0.5 is likely to cause Rust compiler errors in the Rust 2024 edition:
rust-lang/rust#79813

So we might want to migrate some time in the next year.

@teor2345
Copy link
Contributor

I bumped this up to medium because we can't disable the warning just for abscissa, we have to disable all warnings about any future incompatibility:
#6024

@teor2345
Copy link
Contributor

@mpguerra we might want to schedule this work after the audit, because we needed to turn off warnings about all upcoming Rust changes until we fix this issue. (We couldn't just turn off this specific warning.)

@teor2345
Copy link
Contributor

@mpguerra we might want to schedule this work before the first stable release, because it will change how command-line arguments work.

We might be able to emulate the old arguments, or we might not. Even if we could emulate the same argument processing, we might want to do a breaking change and make them more consistent.

@teor2345
Copy link
Contributor

It will also help us get rid of some insecure outdated dependencies. (Which we never call, but still appear in our dependency tree.)

Some of those dependencies are listed in #6391

@teor2345 teor2345 self-assigned this Mar 27, 2023
@mpguerra
Copy link
Contributor

@mpguerra we might want to schedule this work after the audit, because we needed to turn off warnings about all upcoming Rust changes until we fix this issue. (We couldn't just turn off this specific warning.)

This is already in the Zebra Stable Release Epic which is intended for anything that needs to be completed before we tag a stable release

@mpguerra
Copy link
Contributor

@mpguerra
Copy link
Contributor

mpguerra commented May 3, 2023

Since this is quite a large one, let's move it until after the audit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-security Category: Security issues S-needs-triage Status: A bug report needs triage
Projects
None yet
3 participants