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

Run additional HAProxy tests #1097

Merged
merged 6 commits into from
Jul 24, 2023
Merged

Run additional HAProxy tests #1097

merged 6 commits into from
Jul 24, 2023

Conversation

andrewhop
Copy link
Contributor

@andrewhop andrewhop commented Jul 17, 2023

Description of changes:

This installs the remaining missing test dependencies so our CI agrees with HAProxy's CI and the only failing test is ssl_dh.vtc due to our libssl not supporting FFDH ciphersuites. My proposed change in andrewhop/haproxy#1 will turn off this test when built with AWS-LC for now.

Also removed the duplicate curl on line 28 and 50 of the Dockerfile.

Call-outs:

The filtering can be removed if HAProxy takes the change proposed in andrewhop/haproxy#1.

Testing:

Before:

0 tests failed, 23 tests skipped, 140 tests passed

After:

0 tests failed, 9 tests skipped, 165 tests passed

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

# These tests pass when run locally but are not supported in CodeBuild, see CryptoAlg-1965.
# ssl_dh.vtc expects to use libssl with a FFDH ciphersuite which is unsupported, it will be gracefully turned off in
# ssl_dh.vtc with the change in https://github.com/andrewhop/haproxy/pull/1
excluded_tests=("mcli_show_info.vtc" "mcli_start_progs.vtc" "tls_basic_sync.vtc" "tls_basic_sync_wo_stkt_backend.vtc" "acl_cli_spaces.vtc" "http_reuse_always.vtc" "ocsp_auto_update.vtc" "ssl_dh.vtc")
Copy link
Contributor

@chipitsine chipitsine Jul 17, 2023

Choose a reason for hiding this comment

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

something wrong happened here ((

https://github.com/haproxy/haproxy/blob/master/reg-tests/peers/tls_basic_sync.vtc#L5

"tls_basic_sync.vtc" is marked as "SLOW", such tests are often not stable and should not be run regularly.

actually, supposed way of invocation is https://github.com/haproxy/haproxy/blob/master/.github/workflows/vtest.yml#L142

make reg-tests VTEST_PROGRAM=../vtest/vtest REGTESTS_TYPES=default,bug,devel

I consider that as lack of documentation. Probably we should add that convention somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would work once andrewhop/haproxy#1 is merged in which skips ssl_dh.vtc when HAProxy is built with AWS-LC. For now I have to keep the manual filter to skip ssl_dh.vtc.

@@ -25,10 +25,13 @@ export LD_LIBRARY_PATH="${AWS_LC_INSTALL_FOLDER}/lib"

function build_and_test_haproxy() {
cd ${HAPROXY_SRC}
make CC="${CC}" -j ${NUM_CPU_THREADS} TARGET=generic USE_OPENSSL=1 SSL_INC="${AWS_LC_INSTALL_FOLDER}/include" SSL_LIB="${AWS_LC_INSTALL_FOLDER}/lib/"
make CC="${CC}" -j ${NUM_CPU_THREADS} TARGET=generic USE_OPENSSL=1 SSL_INC="${AWS_LC_INSTALL_FOLDER}/include" \
SSL_LIB="${AWS_LC_INSTALL_FOLDER}/lib/" USE_LUA=1 LUA_LIB_NAME=lua5.4
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to add USE_QUIC=1 (not enabled automatically)

Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need USE_LUA=1 LUA_LIB_NAME=lua5.4 ?
what if we leave only USE_LUA=1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems to work, I was running into issues locally with the lua name detection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AWS-LC currently doesn't support HAProxy's use of QUIC as the names of some functions are different. You have if/defs for BoringSSL to address that issue. We are thinking about adding that mapping for greater OpenSSL compatibility in CryptoAlg-1982. Otherwise HAProxy could expand this BoringSSL macro to include AWS-LC.

@andrewhop andrewhop changed the title Install HAProxy test dependencies Run additional HAProxy tests Jul 18, 2023
@nebeid nebeid requested a review from skmcgrail July 18, 2023 17:57
@andrewhop andrewhop requested a review from a team as a code owner July 24, 2023 16:45

# Test with static AWS-LC libraries
aws_lc_build ${SRC_ROOT} ${AWS_LC_BUILD_FOLDER} ${AWS_LC_INSTALL_FOLDER} -DBUILD_SHARED_LIBS=0
aws_lc_build ${SRC_ROOT} ${AWS_LC_BUILD_FOLDER} ${AWS_LC_INSTALL_FOLDER} -DBUILD_SHARED_LIBS=0 -DBUILD_TESTING=0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could improve code-reuse for integration tests I suppose(?).

At some point.

@andrewhop andrewhop enabled auto-merge (squash) July 24, 2023 19:46
@andrewhop andrewhop merged commit 7453ec7 into aws:main Jul 24, 2023
@skmcgrail skmcgrail mentioned this pull request Aug 1, 2023
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