Skip to content

refactor(tls-harness): separate benchmark abstractions #5444

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

Merged
merged 11 commits into from
Jul 31, 2025

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jul 28, 2025

Description of changes:

Goal: This PR makes the benchmark crate more generic.

Why: This is being done to allow the harnesses to be reused for our integration testing.

How: This PR separates the bench crate into two separate crates
benchmarks/ contains all of the benchmarks as well as their setup functionality. Note that this includes benchmark-only enums like HandshakeType and CipherSuite.

tls-harness contains the actual generic TLS implementations communicating over shared memory. These are split into

  • harness: contains all of the traits and generic structs
  • cohort: contains all of the concrete implementations of TlsConnection

Previously, our "benchmark" unit tests were serving as our "harness" unit tests, but this refactor prevents that. So more generic unit tests were added. This specifically included adding a TlsConfigBuilder and TlsConfigBuilderPair. These are deliberately less powerful than the corresponding MakeBenchConfig trait, because it makes them more flexible.

This is preparing for the following dependency structure in the new integration tests:

flowchart TD
    benchmarks --> tls-harness
    integration --> tls-harness
Loading

The largest line diff comes from moving all of the benchmark config setups into their own module under bench_config. I also tried to make the naming a bit less confusing, so now instead of openssl::OpenSslConnection, there is cohort naming, e.g. cohort::OpenSslConnection. This will hopefully avoid confusion from the openssl crate vs the openssl module.

Testing:

Existing transfer and all_handshake unit tests were moved to benchmarks. New unit tests were added for tls-harness.

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

@github-actions github-actions bot added the s2n-core team label Jul 28, 2025
jmayclin added 3 commits July 29, 2025 00:59
* move benchmarks into a separate crate
* fix github ci path
* add copyright headers
@jmayclin jmayclin requested review from goatgoose and lrstewart July 29, 2025 18:12
@jmayclin jmayclin marked this pull request as ready for review July 29, 2025 18:12
/// Return the IANA Description of the negotiated cipher suite.
///
/// e.g. `TLS_AES_256_GCM_SHA384`
fn get_negotiated_cipher_suite(&self) -> String;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're returning a string now because the previous CipherSuite definition was only relevant to the benchmarks.

@@ -155,26 +155,6 @@ jobs:
echo "${output}"
echo "${output}" | grep -q "test result: ok. 1 passed; 0 failed; 0 ignored;"

# our benchmark testing includes interop tests between s2n-tls, rustls, and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed because we already have tests for the standard workspace.

jmayclin added 2 commits July 29, 2025 18:30
- cargo fmt *nightly*
- specify patch version of openssl
@jmayclin jmayclin requested a review from goatgoose July 30, 2025 02:51
Comment on lines 186 to 187

fn new_integration_config(mode: Mode) -> Self {
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 need to specify "integration" here? If we had interop tests using the same harness, would they have a different new_config method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I added the integration clarification is because the trait gets implemented on the actual struct, and I wanted to make sure that it was distinct from the vanilla "new" method.

// this uses the `new` method from the openssl crate
let builder = openssl::SslContextBuilder::new()

// this uses the method from our trait implementation
let builder = openssl::SslContextBuilder::new_integration_config()

// we could use a more generic name, but I worry that it would 
// get confused with the method from the actual `openssl` crate.
let builder = openssl::SslContextBuilder::new_config()

I would not expect interop tests to use a different new_*_config method.

I'm happy to change the name (although I don't have any ideas that I love), but in practice I don't expect this to be a huge issue because library consumers generally won't be using this method directly.

As an example, consider the following example of a record padding test.

    fn s2n_server_case(pad_to: usize) {
        let mut pair: TlsConnPair<OpenSslConnection, S2NConnection> = {
            let mut configs =
                TlsConfigBuilderPair::<SslContextBuilder, s2n_tls::config::Builder>::default();
            configs.set_cert(SigType::Ecdsa256);
            configs.client.set_block_padding(pad_to);
            configs.connection_pair()
        };

        pair.handshake().unwrap();
        for send in SEND_SIZES {
            assert!(pair.round_trip_assert(send).is_ok());
        }
        pair.shutdown().unwrap();
    }

The test directly configured the SslContext in the configs.client.set_block_padding` call, but it doesn't actually have to setup the raw config.

It always could setup the raw config, but then it would probably make more sense to use the completely idiomatic methods. SslContextBuilder::new(), etc.

@jmayclin jmayclin requested a review from lrstewart July 30, 2025 21:51
* rename the new_*_config method
@jmayclin jmayclin enabled auto-merge July 30, 2025 22:03
@jmayclin jmayclin added this pull request to the merge queue Jul 31, 2025
Merged via the queue into aws:main with commit 418313c Jul 31, 2025
49 checks passed
@jmayclin jmayclin deleted the 2025-07-28-bench-move branch July 31, 2025 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants