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(implementations): update neqo image to new repository #344

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Jan 17, 2024

Neqo's interop runner Docker image is now hosted on GitHub's container registry and continuously updated via a daily GitHub action.

See mozilla/neqo#1552 for details.

//CC @martinthomson and @larseggert

Neqo's interop runner Docker image is now hosted on GitHub's container registry
and continuously updated via a daily GitHub action.

See mozilla/neqo#1552 for details.
@mxinden
Copy link
Contributor Author

mxinden commented Jan 17, 2024

🚀 thanks Marten!

Before merging here, any way to give this new image a test run on CI? As far as I can tell the current step is only a linter.

@marten-seemann
Copy link
Collaborator

We don't have any CI action set up for that, but you can test it locally.

@mxinden
Copy link
Contributor Author

mxinden commented Jan 17, 2024

👍 On it. Will report back. Sorry for not doing that before opening a pull request.

@mxinden
Copy link
Contributor Author

mxinden commented Jan 17, 2024

Running locally was easy. This is excellent!


image

Not 100% sure why neqo-neqo is failing on all tests. Assuming it is related to the log line below:

Wrong version. Expected 0x1, got ['0x6b3343cf']

That said, I don't know where neqo chooses V1 instead of V2. It is not configured in interop.sh. Both client and server use the default. The default is V1.

@larseggert
Copy link
Contributor

larseggert commented Jan 17, 2024

The neqo server does compatible version upgrade to v2 by default. You probably need to add -V 1 to the server startup in interop.sh.

mxinden added a commit to mxinden/neqo that referenced this pull request 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](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.
larseggert pushed a commit to mozilla/neqo that referenced this pull request Jan 19, 2024
* fix(qns): always use Version1 on server

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.

* Only set if unset
@marten-seemann
Copy link
Collaborator

@mxinden What's the status of this PR?

@mxinden
Copy link
Contributor Author

mxinden commented Jan 24, 2024

With mozilla/neqo#1578 and all the other pull requests merged, the new image is on par with the old image.

image

Note that the amplification testcase failed with the previous qns image as well. I would assume that this is not a priority for neqo?

Received a 1337 byte Handshake packet from the server. Total: 5348
Server violated the amplification limit, but stayed within 3-4x amplification. Letting it slide.
Received a 1337 byte Handshake packet from the server. Total: 6685
Server violated the amplification limit.

Either way, I suggest moving forward here. Agreed @marten-seemann?

@marten-seemann
Copy link
Collaborator

Yes, there's no requirement to pass all tests to get merged here. As long as the image actually works and is doing QUIC transfers, it's totally fine to have some red in the table.

We should probably relax the amplification limit test case a bit anyway, previous discussion in the working group converged on up to 5x being ok.

@marten-seemann marten-seemann merged commit 3886d33 into quic-interop:master Jan 24, 2024
1 check passed
@mxinden
Copy link
Contributor Author

mxinden commented Jan 24, 2024

Thank you @marten-seemann!

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.

3 participants