Project dependencies updated addressing security #232
Conversation
idna >=1.0.0 is required by https://rustsec.org/advisories/RUSTSEC-2024-0421 Rust cache step reverted to it original position as of incorrect usage in previous commit
And reorder CI steps to cache `cargo install` binaries
Refactor error handling and improve code readability in gRPC wrapper and example URL shortener. Updated conversion methods to use `try_from` for better error handling, simplified cloning of consumer stats, and enhanced logging messages with formatted strings.
- Changed Rust version from 1.88.0 to stable in linter workflow. - Added steps for installing nightly Rust and checking for unused dependencies in the linter workflow. - Introduced a new security workflow for YDB tests, including cargo audit and cargo pants checks. - Removed redundant cargo install and check steps from rust-tests workflow to streamline the process.
- Removed deprecated `partition_count_limit` in favor of `max_active_partitions` in topic-related structures. - Introduced `auto_partitioning_settings` to enhance partition management. - Updated `tonic` and `tower` versions in `Cargo.toml` and `Cargo.lock`. - Added handling for new session response types in `coordination_session.rs`. - Adjusted gRPC request and response structures to accommodate new partitioning features.
- Updated `rand` version in `Cargo.toml` and `Cargo.lock` to 0.9.1, enabling the use of new features. - Refactored random number generation to utilize the new `rng()` function instead of `thread_rng()` for improved performance and consistency. - Adjusted random string generation and endpoint selection methods to align with the updated `rand` API.
| RUST_VERSION_OLD: "1.68.0" | ||
| RUST_VERSION_NEW: "1.88.0" | ||
| RUST_VERSION_OLD: "1.81.0" # aka MSRV | ||
| RUST_VERSION_NEW: "stable" |
There was a problem hiding this comment.
"stable" for RUST_VERSION_NEW is bad for CI because it will be updated in background without commit. The new version is used not only for compilation (which should remain stable), but for linter too and linter from new version often causing it to break on older code.
It mean that if I want to fix a bug after new rust version released - I should to fix all linter errors as pre-requisite, or ignore it.
I prefer to use fixed versions for reproducible CI.
There was a problem hiding this comment.
Get it!
Will fix
What do you think about 3-version matrix in ci?
- minimal supported aka
RUST_VERSION_OLD - latest stable as version number aka
RUST_VERSION_NEW - "stable" – to catch future rust releases bugs as soon as possible. This check can be optional and not block PRs.
There was a problem hiding this comment.
third unblock stable - is ok, for early handle rusts bugs - nightly may be better.
.github/workflows/rust-tests.yml
Outdated
| @@ -11,8 +11,8 @@ on: | |||
| env: | |||
| CARGO_TERM_COLOR: always | |||
| CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse | |||
| RUST_VERSION_OLD: "1.68.0" | |||
| RUST_VERSION_NEW: "1.88.0" | |||
| RUST_VERSION_OLD: "1.81.0" # aka MSRV | |||
There was a problem hiding this comment.
It's about terminology.
MSRV (Minimal Supported Rust Version, value if the rust-version in the Cargo.toml) – well-known widely used term.
While "OLD" have to broad meaning for me.
So I kept the original variables in the code, but have added I bit more context by comment.
| submodules: true | ||
| key: ${{ env[matrix.rust_version] }} | ||
|
|
||
| - name: Show YDB server version |
There was a problem hiding this comment.
It's very convenient to see the server version at the beginning, and it's independent of the source code and Rust. Why did you move it?
There was a problem hiding this comment.
Because docker run more expensive operation than code checkout. About rust toolchain installation I am not sure by the way :)) But still: if you failed with very basic project setup it is a good idea not to execute anything more.
It's not important for me to stay at this point. If you say, that looking at YDB version as early as possible is required, I'll just rollback and make a comment in the code with such description.
There was a problem hiding this comment.
YDB run as service before. It starts before any step, the command show version only. It is fast. Faster, then install toolchain.
| uses: dtolnay/rust-toolchain@v1 | ||
| - name: Install nightly rust | ||
| if: matrix.rust_version == 'RUST_VERSION_NEW' | ||
| uses: dtolnay/rust-toolchain@nightly |
There was a problem hiding this comment.
Why do you need nightly toolchain?
There was a problem hiding this comment.
This is obsolete code. Will drop it.
It used to execute cargo-udeps but I moved it to a separate workflow file
| @@ -11,8 +11,8 @@ on: | |||
| env: | |||
| CARGO_TERM_COLOR: always | |||
| CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse | |||
| RUST_VERSION_OLD: "1.68.0" | |||
| RUST_VERSION_NEW: "1.88.0" | |||
| RUST_VERSION_OLD: "1.82" # aka MSRV | |||
There was a problem hiding this comment.
Why do you need rust newer, than 1.68?
It's ok to update an old version, when necessary, but the update should a reasonable justification.
There was a problem hiding this comment.
Because some of updated dependencies now require Rust 1.82
Probably I can downgrade some dependencies to keep MSRV 1.81 but not less.
Updates of dependencies which contains fixes for critical vulnerabilities reported by cargo audit requires at least 1.81.
| run: | | ||
| cargo audit | ||
|
|
||
| - name: Cargi pants |
There was a problem hiding this comment.
| - name: Cargi pants | |
| - name: Cargo pants |
| - name: Cargi pants | ||
| if: matrix.rust_version == 'RUST_VERSION_NEW' | ||
| run: | | ||
| cargo pants |
There was a problem hiding this comment.
All files must have new line at the end.
| if: matrix.rust_version == 'RUST_VERSION_NEW' | ||
| run: cargo install --locked cargo-audit cargo-pants | ||
|
|
||
| - name: Cargo audit |
| serde = { version = "1.0", features = ["derive"] } | ||
| tokio = { version = "1.18", features = ["macros", "rt-multi-thread", "net"] } | ||
| hashers = "1" | ||
| serde = { version = "1", features = ["derive"] } |
There was a problem hiding this comment.
The dependencies set to minimum supported version. If somebody want to update the version - it is free. Anybody can update version of the serde/tokio/... crate for own lib or end binary.
| @@ -160,14 +160,16 @@ impl From<AlterConsumer> for RawAlterConsumer { | |||
| #[derive(Debug, Clone, Default)] | |||
| pub struct PartitioningSettings { | |||
| pub min_active_partitions: i64, | |||
| pub partition_count_limit: i64, | |||
| pub max_active_partitions: i64, | |||
| pub auto_partitioning_settings: Option<ydb_grpc::ydb_proto::topic::AutoPartitioningSettings>, | |||
There was a problem hiding this comment.
Extract change api, internal logic and etc to other PR please. For separate reformats/style updates/dependency update from api/logic updates. It will be much easer for research changes reason in the future.
There was a problem hiding this comment.
many of style fixes already done within upgrade rust version for the CI #356
There was a problem hiding this comment.
What about split the PR to 3 parts?
- Add vulnerabilities linters and upgrade dependencies, MSRV
- Fix api fields
- Fix style (if need)
There was a problem hiding this comment.
Agreed! Will do it
Project dependencies updated addressing security
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Additionally, my inner perfectionist cleaned up the
Cargo.tomlusingcargo sort. I also removed the minor version from1.xdependencies and the patch version from0.xones, since the project uses a version-controlledCargo.lockfile that ensures version consistency. Now it's easier to update minor versions of dependencies with a simplecargo update, if needed.