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

Upgrade tonic and prost #593

Merged
merged 11 commits into from Jan 23, 2024
Merged

Conversation

diconico07
Copy link
Contributor

What this PR does / why we need it:
This PR initial goal is to upgrade tonic and prost so we can drop the build dependency to rustfmt, thus making it easier to re-build akri in an air-gaped system.

However, to attain this goal it was necessary to do two other unrelated changes (more details about each one after):

  • Upgrade h2
  • Upgrade to edition 2021 of rust

Upgrade h2

The only really needed part for tonic and prost is removing the pin to a patched h2 0.3.3, however for consistency I also upgraded actix (used in webhook part).
This solves several RUSTSEC issues (some of them already marked as resolved for akri, but a cargo audit still complained):

Upgrade to edition 2021 of rust

Prost now generates code using 2021 edition of rust, so we need to upgrade this in order to upgrade prost.

Special notes for your reviewer:
This might be a hard to read PR, I split it in several commits that should be a bit easier to parse (at least they only upgrade one thing)

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

diconico07 added a commit to diconico07/akri that referenced this pull request Apr 27, 2023
Needed by project-akri#593

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
diconico07 added a commit to diconico07/akri that referenced this pull request Apr 27, 2023
Needed by project-akri#593 for prost upgrade

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07
Copy link
Contributor Author

This PR depends on #594 as a change in intermediate containers is needed

diconico07 added a commit to diconico07/akri that referenced this pull request May 15, 2023
Needed by project-akri#593 for prost upgrade

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@@ -677,7 +677,7 @@ mod tests {
}

#[test]
fn test_deserialized_has_extra_array_element() {
async fn test_deserialized_has_extra_array_element() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all of these changed to async? They all look synchronous and are using the #[test] macro still.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actix_web now defines a test macro that got pulled here, went to fast during upgrade and thought it was already the case, fixed it now.

@kate-goldenring
Copy link
Contributor

@diconico07 not sure why the cross production code workflow is failing. Doesn't seem to be an issue with rust version compat https://crates.io/crates/cross

@diconico07
Copy link
Contributor Author

@kate-goldenring it is failing because it tries to use the intermediate container from #594

@diconico07
Copy link
Contributor Author

Okay, seems like we're going to need a way more recent protobuf-compiler package in the cross image than the one package in ubuntu 14.04, and upgrading to latest cross image will only takes us to 18.04 that don't have recent enough protobuf-compiler either, their next release should go to 20.04 that will have a recent enough version

@jbpaux
Copy link
Contributor

jbpaux commented Jun 12, 2023

I see the intermediate image is using a old cross image (not even from the officials) :

FROM rustembedded/cross:aarch64-unknown-linux-gnu-0.1.16

maybe you should rebuild them to target https://github.com/cross-rs/cross which seems to use ubuntu 20.04 and then in ubuntu 20, protobuf is from version 3.6

@diconico07
Copy link
Contributor Author

In fact the rustembedded one was the official one back then, but they moved to ghcr since.
The latest release from them (0.2.5) uses 18.04 with protobuf compiler in version 2.6.1 (not enough for us).
They should soon release a new one that is based on 20.04, we could use the "main" image for now to unblock this, but I think it's better to wait for the next cross release.

@jbpaux
Copy link
Contributor

jbpaux commented Jun 12, 2023

Indeed, I wasn't aware of this renaming 😅. I saw that too. Not sure when 0.3.0 will come out, soon I guess as they are already 2 months past due date, just 9 issues to close.

@diconico07
Copy link
Contributor Author

They (cross) moved their estimated due date, and still late on it, I'm more and more enticed to drop cross entirely, I'll raise the topic on the slack to gather some thoughts on this.

@diconico07
Copy link
Contributor Author

Rebased on #670 to make it build correctly, and upgraded all dependencies (with a focus on tonic and prost).

With that upgrade, the h2 patch is still needed (using the one from hyperium/h2#612, but as agreed with @kate-goldenring, we are going to maintain our own fork to use in Cargo.toml to ensure a better supply chain while we try to get this upstream).

Another patch is needed for OPCUA crate, this patch is only the main upstream branch, as we need an upstream not yet released fix to work with latest rust version (some checks are now stricter)

@diconico07 diconico07 force-pushed the upgrade-tonic branch 5 times, most recently from ef000ab to 1bfc410 Compare October 23, 2023 07:57
@kate-goldenring
Copy link
Contributor

Another patch is needed for OPCUA crate, this patch is only the main upstream branch, as we need an upstream not yet released fix to work with latest rust version (some checks are now stricter)

@diconico07 which change/ pr in the opcua crate are we waiting to be released? Looks like it still hasn't been released since the pr points to v12 when that hasn't been released yet https://github.com/locka99/opcua

@diconico07
Copy link
Contributor Author

Another patch is needed for OPCUA crate, this patch is only the main upstream branch, as we need an upstream not yet released fix to work with latest rust version (some checks are now stricter)

@diconico07 which change/ pr in the opcua crate are we waiting to be released? Looks like it still hasn't been released since the pr points to v12 when that hasn't been released yet https://github.com/locka99/opcua

This is the merged PR we are waiting: locka99/opcua#260

@diconico07 diconico07 marked this pull request as ready for review November 16, 2023 14:45
@kate-goldenring
Copy link
Contributor

@diconico07 maybe we take this as a sign to move the opcua DH out too?

Go back to upstream h2 version as the go-grpc bug is long solved

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Upgrade actix, actix-web and actix-rt to latest,
This solves the following audit issues:
 - RUSTSEC-2020-0016
 - RUSTSEC-2020-0056
 - RUSTSEC-2021-0124
 - RUSTSEC-2023-0034

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
This is needed to be able to upgrade prost dependency

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07
Copy link
Contributor Author

@kate-goldenring probably, but I'd wait for udev one to be moved before doing this. It hopefully means this override is going to be short-lived in this repository.

@diconico07
Copy link
Contributor Author

/version patch

@github-actions github-actions bot added the version/patch Patch version change is needed label Nov 20, 2023
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@kate-goldenring
Copy link
Contributor

Looks like opcua has a new release: https://github.com/locka99/opcua/releases/tag/0.12.0. I think we can update to use this release and be ready to merge

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07 diconico07 merged commit 58e2371 into project-akri:main Jan 23, 2024
34 checks passed
@diconico07 diconico07 deleted the upgrade-tonic branch January 23, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version/patch Patch version change is needed
Projects
None yet
3 participants