Criterion integrationv2 python changes #3534

dougch · 2022-10-04T16:19:04Z

Resolved issues:

One of three final PRs for #3130

Description of changes:

Changes to the integrationv2 test frame work to allow an optional criterion benchmark to be inserted into the test, instead of s2nc. The PR for changes to CodeBuild will be in a separate PR.

Delta's from previous PR:

All test specific changes moved up into the fixture, including selection the Criterion provider and adjusting the timeout.
Added an endpoint exception to allow skipping endpoints when using criterion (with one entry).

Call-outs:

An ad-hoc Integration job with all the criterion flags set will now be in a separate PR.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? CI

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

goatgoose

Just wondering - what caused this to break the sslyze tests? And how does this PR fix it?

tests/integrationv2/test_well_known_endpoints.py

tests/integrationv2/conftest.py

dougch · 2022-10-24T19:29:00Z

Added the diffs from the original PR in the description

tests/integrationv2/utils.py

tests/integrationv2/test_renegotiate.py

tests/integrationv2/providers.py

tests/integrationv2/utils.py

tests/integrationv2/configuration.py

tests/integrationv2/common.py

tests/integrationv2/utils.py

tests/integrationv2/test_well_known_endpoints.py

tests/integrationv2/fixtures.py

tests/integrationv2/test_well_known_endpoints.py

tests/integrationv2/providers.py

tests/integrationv2/conftest.py

tests/integrationv2/test_well_known_endpoints.py

tests/integrationv2/conftest.py

tests/integrationv2/utils.py

tests/integrationv2/providers.py

tests/integrationv2/README.md

tests/integrationv2/providers.py

lrstewart · 2022-12-16T19:45:45Z

tests/integrationv2/providers.py


+        # Copy the command arguments to an environment variable for the harness to read.


Why can't the harness accept the arguments as arguments? Is the environment variable necessary?

Was unable to find a way to access the arguments in the handler, the env. variable seemed the cleanest at the time.

Co-authored-by: Lindsay Stewart <[email protected]>

tests/integrationv2/test_well_known_endpoints.py

lrstewart · 2023-01-06T19:50:47Z

tests/integrationv2/test_well_known_endpoints.py

+    # TODO: Understand the failure with criterion and this endpoint.
+    if get_flag(S2N_USE_CRITERION) != "off" and 'www.netflix.com' in endpoint:


I'm a little paranoid that if we get the disabling / default value of this flag wrong, we would always skip netflix. Like if you change "off" to "disable" but forget to update this line. Hardcoded strings are dangerous, particularly if you're only checking "!=".

Can you put CriterionS2N's criterion_off somewhere accessible to this logic, as well as everywhere else you have "off" hardcoded? So they're all guaranteed to use the same value? Or maybe add a "is_criterion_enabled" type method.

Added a choices option to the pytest arguments to cause pytest to fail if the criterion flag is unexpected and moved the "is it off" logic into a function in global_flags.

tests/integrationv2/providers.py

lrstewart · 2023-01-06T19:59:31Z

tests/integrationv2/tox.ini

+        -o log_cli=true --log-cli-level=INFO \
        --provider-version={env:S2N_LIBCRYPTO} \


Why add log_cli?

it was helpful for early development, with only one xdist worker and cli-level increased

Co-authored-by: Lindsay Stewart <[email protected]>

tests/integrationv2/test_well_known_endpoints.py

@@ -105,6 +105,10 @@
    if get_flag(S2N_FIPS_MODE) is True:
        client_options.trust_store = TRUST_STORE_TRUSTED_BUNDLE

+    # TODO: Understand the failure with criterion and this endpoint.
+    if is_criterion_on() and 'www.netflix.com' in endpoint:


tests/integrationv2/fixtures.py

@@ -2,8 +2,10 @@
 import pytest
 import subprocess

+from global_flags import get_flag, is_criterion_on, S2N_USE_CRITERION


tests/integrationv2/test_well_known_endpoints.py

 from configuration import PROTOCOLS
 from common import ProviderOptions, Ciphers, pq_enabled
 from fixtures import managed_process  # lgtm [py/unused-import]
-from global_flags import get_flag, S2N_FIPS_MODE
+from global_flags import get_flag, is_criterion_on, S2N_FIPS_MODE, S2N_USE_CRITERION


github-actions bot added the s2n-core team label Oct 4, 2022

crypto-transport-libs-ci-bot force-pushed the criterion_tests branch from 5eedca3 to bcdbcf7 Compare October 6, 2022 23:37

dougch marked this pull request as ready for review October 10, 2022 22:40

dougch requested a review from a team as a code owner October 10, 2022 22:40

dougch requested a review from goatgoose October 10, 2022 22:41

goatgoose reviewed Oct 18, 2022

View reviewed changes

tests/integrationv2/test_well_known_endpoints.py Outdated Show resolved Hide resolved

tests/integrationv2/conftest.py Outdated Show resolved Hide resolved

dougch requested review from goatgoose and toidiu October 24, 2022 19:28

dougch force-pushed the criterion_tests branch from e751161 to 2aef968 Compare October 25, 2022 00:12

goatgoose reviewed Oct 25, 2022

View reviewed changes

tests/integrationv2/utils.py Outdated Show resolved Hide resolved

tests/integrationv2/test_renegotiate.py Outdated Show resolved Hide resolved

lrstewart reviewed Oct 25, 2022

View reviewed changes

dougch force-pushed the criterion_tests branch from 2aef968 to 470defd Compare October 25, 2022 18:01

goatgoose mentioned this pull request Oct 26, 2022

Add apache renegotiation test server to CI #3565

Merged

dougch force-pushed the criterion_tests branch from 470defd to 1a776b3 Compare October 27, 2022 00:04

goatgoose approved these changes Oct 28, 2022

View reviewed changes

lrstewart reviewed Oct 28, 2022

View reviewed changes

tests/integrationv2/utils.py Outdated Show resolved Hide resolved

tests/integrationv2/test_well_known_endpoints.py Outdated Show resolved Hide resolved

crypto-transport-libs-ci-bot force-pushed the criterion_tests branch 2 times, most recently from 1800996 to 305fd14 Compare November 8, 2022 19:53

dougch force-pushed the criterion_tests branch from 305fd14 to 3a0b337 Compare November 8, 2022 20:04

dougch enabled auto-merge (squash) November 8, 2022 23:21

dougch requested a review from lrstewart November 9, 2022 19:12

dougch commented Nov 9, 2022

View reviewed changes

tests/integrationv2/fixtures.py Outdated Show resolved Hide resolved

dougch commented Nov 9, 2022

View reviewed changes

tests/integrationv2/test_well_known_endpoints.py Outdated Show resolved Hide resolved

lrstewart reviewed Nov 10, 2022

View reviewed changes

dougch force-pushed the criterion_tests branch 4 times, most recently from 90d21c8 to ae6d0bf Compare November 17, 2022 00:11

dougch requested a review from lrstewart November 17, 2022 00:12

dougch added 2 commits December 12, 2022 11:18

Merge branch 'main' into criterion_tests

6564eb5

Merge branch 'main' into criterion_tests

cb2cedd

goatgoose approved these changes Dec 14, 2022

View reviewed changes

tests/integrationv2/conftest.py Outdated Show resolved Hide resolved

more pr changes

816daa0

lrstewart reviewed Dec 16, 2022

View reviewed changes

dougch and others added 7 commits December 16, 2022 17:35

Update tests/integrationv2/providers.py

a737dec

Co-authored-by: Lindsay Stewart <[email protected]>

Merge branch 'main' into criterion_tests

8fd86df

Python autoformat fixes and more specific import

bd2e5cb

Moving some README comments to code.

1fbeb4b

Pass test name to criterion via env var

5b10f6c

revert utils skip

d439ae4

Move endpoint skip to the test; autoformat

563756d

github-advanced-security bot found potential problems Dec 21, 2022

View reviewed changes

tests/integrationv2/test_well_known_endpoints.py Fixed Show fixed Hide fixed

Remove find cargo

283da43

dougch requested review from lrstewart and maruthiaws January 6, 2023 01:23

Merge branch 'main' into criterion_tests

dac25b7

lrstewart reviewed Jan 6, 2023

View reviewed changes

Update tests/integrationv2/providers.py

ef4fe22

Co-authored-by: Lindsay Stewart <[email protected]>

harrisonkaiser approved these changes Jan 9, 2023

View reviewed changes

Add safeguards around criterion flags

5ea4fd5

github-advanced-security bot found potential problems Jan 10, 2023

View reviewed changes

Comment about criterion/s2nc

4971c1d

dougch requested review from lrstewart and removed request for maruthiaws January 10, 2023 01:22

lrstewart approved these changes Jan 10, 2023

View reviewed changes

Merge branch 'main' into criterion_tests

3d9bfb5

dougch merged commit 4a73919 into aws:main Jan 10, 2023

dougch deleted the criterion_tests branch January 13, 2023 23:51

jmayclin pushed a commit to jmayclin/s2n-tls that referenced this pull request Jan 23, 2023

Criterion tests (aws#3534)

f9f6641

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Criterion integrationv2 python changes #3534

Criterion integrationv2 python changes #3534

dougch commented Oct 4, 2022 •

edited

Loading

goatgoose left a comment

dougch commented Oct 24, 2022

lrstewart Dec 16, 2022

dougch Dec 21, 2022

lrstewart Jan 6, 2023 •

edited

Loading

dougch Jan 10, 2023

lrstewart Jan 6, 2023

dougch Jan 9, 2023


		# Copy the command arguments to an environment variable for the harness to read.

		# TODO: Understand the failure with criterion and this endpoint.
		if get_flag(S2N_USE_CRITERION) != "off" and 'www.netflix.com' in endpoint:

		-o log_cli=true --log-cli-level=INFO \
		--provider-version={env:S2N_LIBCRYPTO} \

Criterion integrationv2 python changes #3534

Criterion integrationv2 python changes #3534

Conversation

dougch commented Oct 4, 2022 • edited Loading

Resolved issues:

Description of changes:

Call-outs:

Testing:

goatgoose left a comment

Choose a reason for hiding this comment

dougch commented Oct 24, 2022

lrstewart Dec 16, 2022

Choose a reason for hiding this comment

dougch Dec 21, 2022

Choose a reason for hiding this comment

lrstewart Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

dougch Jan 10, 2023

Choose a reason for hiding this comment

lrstewart Jan 6, 2023

Choose a reason for hiding this comment

dougch Jan 9, 2023

Choose a reason for hiding this comment

dougch commented Oct 4, 2022 •

edited

Loading

lrstewart Jan 6, 2023 •

edited

Loading