Skip to content

[WIP]: add async support to tls_fetcher/proxy and qos #517

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

Closed
wants to merge 4 commits into from

Conversation

Turnalek
Copy link
Contributor

Summary & Motivation (Problem vs. Solution)

This PR attempts to add async support to our Vsock/UnixSock and TcpStream abstractions to allow async calls in the IO bound tls_fetcher accross the board.

How I Tested These Changes

TODO

Pre merge check list

  • Update CHANGELOG.MD

allows for tokio-vsock usage
minimum required bumped due to tokio and tokio-vsock
this includes:
 - AsyncStream [qos_core]
 - AsyncListener [qos_core]
 - AsyncRequestProcessor [qos_core]
 - AsyncProxyConnection [qos_net]
 - AsyncProxy [qos_net]
 - AsyncProxyStream [qos_net]
@Turnalek Turnalek requested review from a team as code owners April 29, 2025 22:13
@Turnalek Turnalek marked this pull request as draft April 29, 2025 22:13
@cr-tk
Copy link
Collaborator

cr-tk commented Apr 30, 2025

@Turnalek two immediate observations:

  1. There are a lot of dependency changes. I can see how some of them are needed via tokio / libc, please ensure that the set of changes is as minimal as possible. In other words, no blanket cargo update or equivalent.
  2. You're switching from Rust 1.81 to 1.86. Is this necessary, and why? We'll have to update the stagex-based build system in QOS and other codebases to have support for this. That's probably best done as a separate PR, I expect.

@Turnalek
Copy link
Contributor Author

Turnalek commented Apr 30, 2025

@Turnalek two immediate observations:

  1. There are a lot of dependency changes. I can see how some of them are needed via tokio / libc, please ensure that the set of changes is as minimal as possible. In other words, no blanket cargo update or equivalent.
  2. You're switching from Rust 1.81 to 1.86. Is this necessary, and why? We'll have to update the stagex-based build system in QOS and other codebases to have support for this. That's probably best done as a separate PR, I expect.

Thank for taking a look! I didn't realize PRs in this repo go to ready state by default and only flagged to WIP afterwards.

I suspect I'll first get an overall functional PR and then split it into sections to get merged piece by piece.

For the Rust upgrade, it was a quick and dirty way to fix our currently broken main. Specifically, if in main you do cargo update && cargo build you'll get a dependency mismatch issue due to minimum rust version. This is IMO a bug and we should fix it one way or another.

There is however a second reason which is to say we need to upgrade to 1.83 as minimum due to tokio-vsock (which is the only new dependency actually, as we already use tokio) which also needs nix/libc wrapper to be bumped. I also noticed that the nix versioning is incosistent right now, with different versions accross crates and different equality requirements.

We should def. clean this up and if things need to be completely locked down it should be done using the = in Cargo.toml

So TLDR: this is a WIP POC PR to see if things will be functional the way we hope with this change. After all roadblocks are confirmed fix[able] I will split into smaller PRs.

@cr-tk
Copy link
Collaborator

cr-tk commented May 2, 2025

Specifically, if in main you do cargo update && cargo build you'll get a dependency mismatch issue due to minimum rust version.

This wouldn't be the typical workflow here, since we're very selective in QOS to avoid unnecessary dependency changes. We want to do some level of security review on the Rust code of dependencies, which quickly becomes very time-consuming when making broad changes. cargo update optionally takes specific crate names (and optionally, versions), which helps with a more fine-grained approach.

The fact that cargo selects dependency updates even if they violate the specified project Rust version (machine readable in src/rust-toolchain.toml) and older more compatible dependency versions are available is a known cargo bug. I believe this is fixable primarily by telling cargo to use the resolver = 3 strategy for resolving dependencies, but this was only very recently introduced in cargo 1.84 onwards and not yet available in 1.81, which means we'll have to deal with those situations semi-manually for now. For more documentation, see https://doc.rust-lang.org/edition-guide/rust-2024/cargo-resolver.html .

It's clear that if tokio-vsock needs >= 1.83, we'll have to adopt that if the feature is important enough. Let's discuss some of the related considerations, targeted version and adoption timeline internally, since it'll affect other code bases as well.

@Turnalek
Copy link
Contributor Author

superseded by #524

@Turnalek Turnalek closed this May 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants