Fix remaining interop tests #25528

nhorman · 2024-09-24T10:29:38Z

We have 4 remaining interop tests that are currently failing

chacha20 fails against some servers because those servers do not support the use of an h3 alpn, and the client side setup uses curl which only supports an h3 alpn. Convert the test to use quic-hq-interop with support for cipher selection to make this universally compatible

the multiplexing tests fails against the mvfst server for an unkown reason. It works in a local container environment, but aborts sending requests early in CI (possibly due to some environmental limitation). Disable the test for now, and re-enable once we fully understand the root cause

Checklist

tests are added or updated

To run the chacha20 test in interop we need to: 1) negotiate an hq-interop alpn 2) only use chacha 20 Item 1 requires the use of quic-hq-interop, the latter requires this change

Need to update the docker interop container to use the quic-hq-interop client so that the right alpn is negotiated for chacha20 testing

t8m · 2024-09-24T15:45:49Z

Could you please add a temporary commit that will run the interop CI on this PR to see if it passes?

t8m · 2024-09-26T18:40:56Z

ping @nhorman re my comment above

nhorman · 2024-09-26T18:50:46Z

@t8m working on it now. In testing I discovered some transient failures that I'm looking at

The multiplexing test works on local runs, but appears to be failing in CI, possibly due to some environmental limitation (the test generates a large list of requests in an environment variable), leading to not sending all the requests needed. Disable the test for now, and look to re-enable it after release when we can appropriately diagnose the problem

nhorman · 2024-09-26T20:05:02Z

@t8m here you go, had to do some github gymnastics to get the test to run on the updated workflow, but this should prove its working:
https://github.com/openssl/openssl/actions/runs/11059398615

Sashan

looks good to me.

I have a question: would it make sense to enable multiplexing test for other servers? I vaguely remember earlier version of the PR did that.

nhorman · 2024-09-27T13:15:25Z

@Sashan in answer to your question, yes it definitely would be good to enable the multiplexing test everywhere, but we seem to be encountering some transient failures that don't occur when running outside of CI (the summary is that randomly, creating new QUIC streams fail). I'm looking at it now and will submit a subsequent PR when I figure out whats going on

openssl-machine · 2024-09-28T14:00:25Z

This pull request is ready to merge

test/quic-openssl-docker/run_endpoint.sh

shahsb

LGTM.

Co-authored-by: Viktor Dukhovni <[email protected]>

vdukhovni · 2024-09-29T19:41:15Z

test/quic-openssl-docker/run_endpoint.sh

+       for req in $REQUESTS
+       do
+           OUTFILE=$(basename $req)
+           printf "%s " "$OUTFILE" >> ./reqfile.txt
+           HOSTNAME=$(printf "%s\n" "$req" | sed -ne 's,^https://\([^/:]*\).*,\1,p')
+           HOSTPORT=$(printf "%s\n" "$req" | sed -ne 's,^https://[^:/]*:\([^/]*\).*,\1,p')
+       done


I've approved the PR as-is, but an alternative implementation, that is arguably cleaner, could be:

Suggested change

for req in $REQUESTS

do

OUTFILE=$(basename $req)

printf "%s " "$OUTFILE" >> ./reqfile.txt

HOSTNAME=$(printf "%s\n" "$req" | sed -ne 's,^https://$[^/:]*$.*,\1,p')

HOSTPORT=$(printf "%s\n" "$req" | sed -ne 's,^https://[^:/]*:$[^/]*$.*,\1,p')

done

reqpaths=()

for req in $REQUESTS

do

reqpaths=("${reqpaths[@]}" "$(basename $req)")

HOSTNAME=$(printf "%s\n" "$req" | sed -ne 's,^https://$[^/:]*$.*,\1,p')

HOSTPORT=$(printf "%s\n" "$req" | sed -ne 's,^https://[^:/]*:$[^/]*$.*,\1,p')

done

printf "%s\n" "${reqpaths[*]}" > ./reqfile.txt

Though (in the unlikely case it matters), this version appends a newline rather than a space after the last request path list element.

Yet another version could simply redirect the entire "for" loop, rather than each printf by using done > reqfile.txt

openssl-machine · 2024-10-01T09:00:35Z

This pull request is ready to merge

To run the chacha20 test in interop we need to: 1) negotiate an hq-interop alpn 2) only use chacha 20 Item 1 requires the use of quic-hq-interop, the latter requires this change Reviewed-by: Viktor Dukhovni <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #25528)

Need to update the docker interop container to use the quic-hq-interop client so that the right alpn is negotiated for chacha20 testing Reviewed-by: Viktor Dukhovni <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #25528)

The multiplexing test works on local runs, but appears to be failing in CI, possibly due to some environmental limitation (the test generates a large list of requests in an environment variable), leading to not sending all the requests needed. Disable the test for now, and look to re-enable it after release when we can appropriately diagnose the problem Reviewed-by: Viktor Dukhovni <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #25528)

Co-authored-by: Viktor Dukhovni <[email protected]> Reviewed-by: Viktor Dukhovni <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #25528)

t8m · 2024-10-04T15:45:13Z

Merged to the master branch. Thank you.

nhorman added 2 commits September 24, 2024 04:28

Add SSL_CIPHER_SUITES env variable to quic-hq-interop

eacf014

To run the chacha20 test in interop we need to: 1) negotiate an hq-interop alpn 2) only use chacha 20 Item 1 requires the use of quic-hq-interop, the latter requires this change

Update quic interop docker container for chacha20

2febb3c

Need to update the docker interop container to use the quic-hq-interop client so that the right alpn is negotiated for chacha20 testing

nhorman self-assigned this Sep 24, 2024

nhorman added this to the 3.4.0 milestone Sep 24, 2024

nhorman linked an issue Sep 24, 2024 that may be closed by this pull request

Fix chacha20 and multiplexing quic interop tests #25524

Closed

t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present branch: 3.4 Merge to openssl-3.4 approval: review pending This pull request needs review by a committer labels Sep 24, 2024

nhorman requested review from vdukhovni, Sashan, t8m and mattcaswell September 26, 2024 15:34

nhorman force-pushed the fix-interop-chacha20 branch 2 times, most recently from d8c8485 to dc3569e Compare September 26, 2024 20:01

t8m previously approved these changes Sep 27, 2024

View reviewed changes

Sashan previously approved these changes Sep 27, 2024

View reviewed changes

nhorman added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 27, 2024

openssl-machine removed the approval: done This pull request has the required number of approvals label Sep 28, 2024

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Sep 28, 2024

vdukhovni requested changes Sep 28, 2024

View reviewed changes

test/quic-openssl-docker/run_endpoint.sh Outdated Show resolved Hide resolved

test/quic-openssl-docker/run_endpoint.sh Show resolved Hide resolved

shahsb approved these changes Sep 29, 2024

View reviewed changes

Update test/quic-openssl-docker/run_endpoint.sh

8eaa170

Co-authored-by: Viktor Dukhovni <[email protected]>

nhorman dismissed stale reviews from Sashan and t8m via 8eaa170 September 29, 2024 18:03

nhorman requested a review from vdukhovni September 29, 2024 18:03

nhorman added approval: review pending This pull request needs review by a committer and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Sep 29, 2024

vdukhovni approved these changes Sep 29, 2024

View reviewed changes

vdukhovni reviewed Sep 29, 2024

View reviewed changes

vdukhovni approved these changes Sep 29, 2024

View reviewed changes

t8m approved these changes Sep 30, 2024

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 30, 2024

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 1, 2024

t8m removed the branch: 3.4 Merge to openssl-3.4 label Oct 4, 2024

t8m closed this Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix remaining interop tests #25528

Fix remaining interop tests #25528

nhorman commented Sep 24, 2024

t8m commented Sep 24, 2024

t8m commented Sep 26, 2024

nhorman commented Sep 26, 2024

nhorman commented Sep 26, 2024

Sashan left a comment

nhorman commented Sep 27, 2024

openssl-machine commented Sep 28, 2024

shahsb left a comment

vdukhovni Sep 29, 2024 •

edited

Loading

openssl-machine commented Oct 1, 2024

t8m commented Oct 4, 2024

Fix remaining interop tests #25528

Fix remaining interop tests #25528

Conversation

nhorman commented Sep 24, 2024

Checklist

t8m commented Sep 24, 2024

t8m commented Sep 26, 2024

nhorman commented Sep 26, 2024

nhorman commented Sep 26, 2024

Sashan left a comment

Choose a reason for hiding this comment

nhorman commented Sep 27, 2024

openssl-machine commented Sep 28, 2024

shahsb left a comment

Choose a reason for hiding this comment

vdukhovni Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

openssl-machine commented Oct 1, 2024

t8m commented Oct 4, 2024

vdukhovni Sep 29, 2024 •

edited

Loading