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

fix(qns): always use Version1 on server #1563

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Jan 17, 2024

The Quic Interop Runner expects the server to use Version::Version1 in all testcases but versionnegotiation and v2.

See discussion in Quic Interop Runner issue and corresponding code in the Quic Interop Runner.

While Neqo's default Version is Version1, the default VersionConfig includes all versions (Version::all()) and thus the server will currently upgrade the connection to the compatible Version::Version2.

With this commit, the server will always choose Version1 when running a qns testcase. Given that Neqo's qns implementation does not support the versionnegotiation and v2 testcases, this fix is applicable for all supported testcases.

@larseggert I opted for a fix in neqo-server/src/main.rs instead of, as you suggested, in qns/interop.sh. I find it much easier to maintain this logic in Rust rather than a shell script. I don't feel strongly about it. Let me know whether you prefer the -V 1 fix in interop.sh.


While this does not fix all interop test failures, it does get us much closer.

Before:

image

After:

image

I will take a look at the remaining failures next.

The Quic Interop Runner expects the server to use `Version::Version1` in all
testcases but `versionnegotiation` and `v2`.

See discussion in [Quic Interop Runner
issue](quic-interop/quic-interop-runner#344 (comment))
and corresponding code in the [Quic Interop
Runner](https://github.com/quic-interop/quic-interop-runner/blob/ca27dcb5272a82d994337ae3d14533c318d81b76/testcases.py#L188-L195).

While Neqo's default `Version` is `Version1`, the default `VersionConfig`
includes all versions (`Version::all()`) and thus the server will currently
upgrade the connection to the compatible `Version::Version2`.

With this commit, the server will always choose `Version1` when running a qns
testcase. Given that Neqo's qns implementation does not support the
`versionnegotiation` and `v2` testcases, this fix is applicable for all
supported testcases.
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (64fb41f) 87.63% compared to head (7742df5) 87.60%.

Files Patch % Lines
neqo-server/src/main.rs 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1563      +/-   ##
==========================================
- Coverage   87.63%   87.60%   -0.03%     
==========================================
  Files         117      117              
  Lines       38323    38330       +7     
==========================================
- Hits        33584    33580       -4     
- Misses       4739     4750      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -836,6 +836,15 @@ fn main() -> Result<(), io::Error> {
init_db(args.db.clone());

if let Some(testcase) = args.qns_test.as_ref() {
assert!(
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather leave the footgun in place (that what these tools are for) and only set the version if it is unset.

@mxinden
Copy link
Collaborator Author

mxinden commented Jan 18, 2024

@martinthomson can you take another look? Let me know if you want me to remove the qwarn!.

Comment on lines +841 to +842
// Exceptions are testcases `versionnegotiation` and `v2`. Neither are
// supported by Neqo. Thus always set `Version1`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neqo does support v2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, it does not implement the v2 Quic Interop Runner testcase:

neqo/neqo-client/src/main.rs

Lines 989 to 1017 in 64fb41f

match testcase.as_str() {
"http3" => {}
"handshake" | "transfer" | "retry" => {
args.use_old_http = true;
}
"zerortt" | "resumption" => {
if args.urls.len() < 2 {
eprintln!("Warning: resumption tests won't work without >1 URL");
exit(127);
}
args.use_old_http = true;
args.resume = true;
}
"multiconnect" => {
args.use_old_http = true;
args.download_in_series = true;
}
"chacha20" => {
args.use_old_http = true;
args.ciphers.clear();
args.ciphers
.extend_from_slice(&[String::from("TLS_CHACHA20_POLY1305_SHA256")]);
}
"keyupdate" => {
args.use_old_http = true;
args.key_update = true;
}
_ => exit(127),
}

Am I missing something @larseggert?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to work on it once the current test failures (quic-interop/quic-interop-runner#344) are resolved.

@larseggert
Copy link
Collaborator

I'd prefer to lock the version in the shell script, but don't care strongly.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This is fine, though if Lars would prefer a simple version override in the interop runner script, that's also fine.

@mxinden
Copy link
Collaborator Author

mxinden commented Jan 19, 2024

@larseggert note that once we implement the versionnegotiation and v2 testcase, this logic will have to live in neqo-server/src/main.rs. We will then have to use Version::Version1 in all but those two cases. In other words, a simpel -V 1 override in interop.sh won't do.

Let me know if that changes your mind. If not, given that neither of us feels strongly about it, let's go with your approach, namely to override in interop.sh.

@larseggert
Copy link
Collaborator

That's a good point. Let's do it this way.

@larseggert larseggert merged commit 4de3279 into mozilla:main Jan 19, 2024
9 checks passed
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.

4 participants