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

interop handshake test failed for server #948

Closed
Tracked by #944
nhorman opened this issue Dec 3, 2024 · 6 comments
Closed
Tracked by #944

interop handshake test failed for server #948

nhorman opened this issue Dec 3, 2024 · 6 comments
Assignees

Comments

@nhorman
Copy link
Contributor

nhorman commented Dec 3, 2024

The quic-interop-runner handshake test expects to see the client and server preform a single handshake without retries. Because our server unilaterally issues a retry to do server validation, we fail this test (though strictly speaking its not a violation of the RFC for us to do so). The fix options here I think are:
a) disable the handshake test, as its not an RFC violation to send one, this test just checks for it
or
b) augment our server retry code to allow a setting to be passed from the application to not preform server address validation.
or
c) manually modify the test harness to not check for retries

I opened an issue with quic-interop-runner (quic-interop/quic-interop-runner#414) and the maintainer insists that this is a valid test, so doing an upstream fix to not check for retries doesn't appear to be a path forward. We could still carry the change for (c) if we wanted to, but following option (b) might be a useful solution for downstream users

@kroeckx
Copy link
Member

kroeckx commented Dec 4, 2024

It seems to me that the test is testing something that we (currently) don't support. I suggest to just skip the test for now.

@nhorman
Copy link
Contributor Author

nhorman commented Dec 4, 2024

@kroeckx Thats kind of the question at hand. We don't currently support not doing server address validation, but we also literally just added in server address validation to the QUIC server code. So the question in my mind is, given that this test explicitly looks for the ability to not do address validation (i.e. not send retry frames), should we add in a flag to skip doing so? I'm sort of on the fence about it. It seems like we shouldn't have to support avoiding this, but there is a performance benefit from skipping it, and if there are interop tests that check for it, perhaps its worthwhile?

@nhorman nhorman self-assigned this Dec 4, 2024
@nhorman nhorman moved this from Pre-Refinement to Waiting Review in Development Board Dec 4, 2024
@kroeckx
Copy link
Member

kroeckx commented Dec 4, 2024

I might have misunderstood things, but was under the impression we could also do the address validation without a retry packet, but that we currently don't support that.

I guess we could also not do address validation, and keep limiting the amount of data we send to 3 times what we received.

@nhorman
Copy link
Contributor Author

nhorman commented Dec 4, 2024

Address validation can also be done for "subsequent" connections using a NEW_TOKEN frame, which we have an outstanding todo for in issue #929. That should not be impacted by this flag, as its not strictly address validation on the connection which we are accepting.

Regarding the amplification limit, I believe that is covered via the discussion in #895

@t8m
Copy link
Member

t8m commented Dec 5, 2024

I guess we could also not do address validation, and keep limiting the amount of data we send to 3 times what we received.

We could do that, however in case we exceed the size of the initial packet from the client by 3 times we would have to stop sending and wait for client to send us more data, (i.e. ACK/PING/PADDING etc. frames). That would introduce extra delays in the handshake. It is also less than trivial to implement this limiting -> something for post MVP.

@nhorman
Copy link
Contributor Author

nhorman commented Dec 9, 2024

@t8m I don't know that it would be particularly complex to do. I'm investigating a possibly solution in the tx packetiser, and will report results here

@vavroch2010 vavroch2010 added this to the 3.5.0 milestone Dec 10, 2024
@nhorman nhorman closed this as completed Dec 14, 2024
@github-project-automation github-project-automation bot moved this from Waiting Review to Done in Development Board Dec 14, 2024
@github-project-automation github-project-automation bot moved this from Refining to Done in Project Board 3.5.0 planning Dec 14, 2024
@github-project-automation github-project-automation bot moved this from New to Done in Project Board Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants