Skip to content

Skip test_quic_peer_addr_v6 in quicapitest when IPv6 is disabled#30580

Closed
esyr wants to merge 4 commits into
openssl:masterfrom
esyr:esyr/quicapitest-no-ipv6
Closed

Skip test_quic_peer_addr_v6 in quicapitest when IPv6 is disabled#30580
esyr wants to merge 4 commits into
openssl:masterfrom
esyr:esyr/quicapitest-no-ipv6

Conversation

@esyr
Copy link
Copy Markdown
Member

@esyr esyr commented Mar 26, 2026

Addresses [1]. Also add the configuration with IPv6 disabled to the test matrix, patch up tests and TLSProxy to avoid using IPv6 loopback address when IPv6 is disabled, and clean up test/quicapitest.c a bit.

[1] #30574

@esyr esyr requested a review from quarckster as a code owner March 26, 2026 05:02
@esyr esyr added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 4.0 Applies to openssl-4.0 labels Mar 26, 2026
@esyr esyr changed the title Skip test_quic_peer_addr_v6 quicapitest when IPv6 is disabled Skip test_quic_peer_addr_v6 in quicapitest when IPv6 is disabled Mar 26, 2026
@esyr esyr force-pushed the esyr/quicapitest-no-ipv6 branch 2 times, most recently from c250c16 to 225c61f Compare March 26, 2026 05:12
return init($class, $filter, $execute, $cert, $debug, 0);
$debug,
$use_IPv6) = @_;
return init($class, $filter, $execute, $cert, $debug, 0, $use_IPv6);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure why you're passing in the result of have_IPv6() here. The TLSProxy code already decides weather to use ipv6 or not based on weather or not it is able to create an AF_INET6 socket. It seems like thats what we should be fixing here, though it doesn't appear as anything is wrong with it.

I'm also not sure why the TLSProxy code is being changed here, as the reported issue just references the quicapi tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm also not sure why the TLSProxy code is being changed here, as the reported issue just references the quicapi tests.

Because TLSProxy has no notion of OPENSSL_USE_IPV6=0, and make test after ./config -DOPENSSL_IPV6=0 blows up thusly:

Test Summary Report
-------------------
75-test_quicapi.t                      (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
70-test_sslrecords.t                   (Wstat: 1024 (exited 4) Tests: 34 Failed: 4)
  Failed tests:  19, 22-23, 34
  Non-zero exit status: 4

70-test_sslrecords.t fails only on DTLS tests, but that has revealed that TLSProxy doesn't respect library build configuration in this aspect.

It seems like thats what we should be fixing here, though it doesn't appear as anything is wrong with it.

For that, TLSProxy needs to obtain knowledge from configdata, and that requires importing of OpenSSL::Util package into TLSProxy, something that the latter doesn't do so far. I've opted out for following the debug option's example here, which could have also obtained from the environment, but is explicitly passed instead.

@esyr esyr requested a review from Sashan April 2, 2026 09:09
no-ui,
no-quic
no-quic,
-DOPENSSL_USE_IPV6=0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please move this to run-checker-merge or run-checker-daily or select another thing (I'd suggest no-ui or no-legacy.) to be moved if you think this is more important. We need to keep the number of CI jobs run on PR limited.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm in favor of dropping no-ui.

Comment thread test/quicapitest.c
TPARAM_OP_MUTATE
};

/* clang-format off */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Copy Markdown
Member Author

@esyr esyr Apr 2, 2026

Choose a reason for hiding this comment

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

It butchered alignment in TPARAM_CHECK_* array, which became difficult to read.

Comment thread test/quicapitest.c Outdated
static const unsigned char malformed_preferred_addr_2[42] = {
0x0d,
0x28, /* too short */
0x0d, 0x28, /* too short */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not need clang-format-off. Just drop the trailing comma.

Copy link
Copy Markdown
Member Author

@esyr esyr Apr 2, 2026

Choose a reason for hiding this comment

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

Not a great fan of commas polluting diffs on array changes. Also, from personal experience, absence of trailing commas blows up nicely when shuffling arrays of strings, not that it is necessarily applicable here, however.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Restricted the usage to TPARAM_CHECK_* parts only.

esyr added 4 commits April 2, 2026 12:34
Define and add the test only if OPENSSL_USE_IPV6 is set to 1.

Resolves: openssl#30574
Fixes: beec4e1 "Add SSL_get_peer_addr() function to query peer address for QUIC"
Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
Add a parameter to TLSProxy::Proxy->new()
and TLSProxy::Proxy->new_dtls() that indicates IPv6 usage preference
and pass have_IPv6() to it, so IPv6 usage is avoided when it is disabled.

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
Add -DOPENSSL_USE_IPV6=0 to run-checker-ci.yml and move no-ui
to run-checker-merge.

References: openssl#30574
Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
Shut off clang-format, as it is incapable of formatting arrays properly,
and just mangles everything instead.  Also, while at it, drop the trailing
commas from TPARAM_CHECK_* definitions, as they are pretty confusing.

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
@esyr esyr force-pushed the esyr/quicapitest-no-ipv6 branch from 225c61f to be23210 Compare April 2, 2026 10:35
@esyr esyr requested review from nhorman and t8m April 4, 2026 19:08
@openssl-machine openssl-machine 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 Apr 8, 2026
@t8m t8m added the tests: present The PR has suitable tests present label Apr 8, 2026
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Apr 9, 2026
@openssl-machine
Copy link
Copy Markdown
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Apr 9, 2026
@esyr esyr added the hold: merge after release This pull request must not be merged until after the release from the merge branch is done label Apr 13, 2026
@jogme jogme removed the hold: merge after release This pull request must not be merged until after the release from the merge branch is done label Apr 15, 2026
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Define and add the test only if OPENSSL_USE_IPV6 is set to 1.

Resolves: #30574
Fixes: beec4e1 "Add SSL_get_peer_addr() function to query peer address for QUIC"
Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:45:31 2026
(Merged from #30580)
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Add a parameter to TLSProxy::Proxy->new()
and TLSProxy::Proxy->new_dtls() that indicates IPv6 usage preference
and pass have_IPv6() to it, so IPv6 usage is avoided when it is disabled.

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:45:33 2026
(Merged from #30580)
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Add -DOPENSSL_USE_IPV6=0 to run-checker-ci.yml and move no-ui
to run-checker-merge.

References: #30574
Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:45:34 2026
(Merged from #30580)
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Shut off clang-format, as it is incapable of formatting arrays properly,
and just mangles everything instead.  Also, while at it, drop the trailing
commas from TPARAM_CHECK_* definitions, as they are pretty confusing.

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:45:37 2026
(Merged from #30580)
@jogme
Copy link
Copy Markdown
Contributor

jogme commented Apr 15, 2026

Merged to master and 4.0. Thank you!

@jogme jogme closed this Apr 15, 2026
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Define and add the test only if OPENSSL_USE_IPV6 is set to 1.

Resolves: #30574
Fixes: beec4e1 "Add SSL_get_peer_addr() function to query peer address for QUIC"
Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:46:04 2026
(Merged from #30580)
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Add a parameter to TLSProxy::Proxy->new()
and TLSProxy::Proxy->new_dtls() that indicates IPv6 usage preference
and pass have_IPv6() to it, so IPv6 usage is avoided when it is disabled.

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:46:06 2026
(Merged from #30580)
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Add -DOPENSSL_USE_IPV6=0 to run-checker-ci.yml and move no-ui
to run-checker-merge.

References: #30574
Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:46:07 2026
(Merged from #30580)
openssl-machine pushed a commit that referenced this pull request Apr 15, 2026
Shut off clang-format, as it is incapable of formatting arrays properly,
and just mangles everything instead.  Also, while at it, drop the trailing
commas from TPARAM_CHECK_* definitions, as they are pretty confusing.

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Wed Apr 15 12:46:09 2026
(Merged from #30580)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 4.0 Applies to openssl-4.0 tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants