test: add rust well-known-endpoint tests #4884
Conversation
jmayclin
changed the title
* rustfmt
| #[tokio::test] | ||
| async fn http_get_test() -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
I'm not seeing any retry mechanism-- is that handled by hyper automatically, or do you plan to add that in another PR? How flaky are these tests? Same for the PQ tests, since s2n-tls-tokio definitely doesn't handle retries.
There was a problem hiding this comment.
I'll add more information about this in the callouts, but
- it is not handled by hyper automatically
- I do plan on adding it in another PR, but not until actually observing some failures.
- I don't have any concrete data, but I have not seen any failures locally.
We don't currently have a good understanding of whether the flakiness comes from. TCP? TLS? DNS? Being able to actually observe these failures will allow us to
- document where the flakiness actually comes from
- have tightly scoped retries to catch other (real) transient errors.
There was a problem hiding this comment.
Oh interesting. Do you suspect that the flakiness we attributed to the network was actually coming from the IO parsing?
There was a problem hiding this comment.
I don't expect that, but I certainly wouldn't be surprised by that outcome 🤷 .
I'm also wondering whether the amount of queries that we were doing (e.g. 30 failed TLS handshakes for each domain per test run) might have led some domains to start refusing connections.
Co-authored-by: Sam Clark <[email protected]>
* move akamai to more general tls client test * make logging handle concurrent tests * make kem_name return an Option<&str>
| Ok("NONE") => None, | ||
| Ok(name) => Some(name), | ||
| Err(_) => { | ||
| // Unreachable: This would indicate a non-utf-8 string literal in |
There was a problem hiding this comment.
I went back and forth on whether to make this an actual unreachable! statement. I ended up deciding no?
* cargo fmt
* pin test-log for msrv issues * pep8 python code
Co-authored-by: Lindsay Stewart <[email protected]>
* make tls client more general * add PQ feature
* add copyright header
* move tests to specific network module
| fn pq_sanity_check() -> Result<(), Box<dyn std::error::Error>> { | ||
| let config = testing::build_config(&Policy::from_version("KMS-PQ-TLS-1-0-2020-07")?)?; |
There was a problem hiding this comment.
"default_pq" might be a better / more readable option.
Might also be worth opening a little follow-up issue to add "default_pq" to the pq integration tests 🤔
There was a problem hiding this comment.
default_pq will require us to add the kem_group API.
I think it's gonna be neater to to that in a separate PR, so I opened an issue to track this: #4896 . The issue explicitly calls out
- updating the integration test
- updating the pq sanity check
as requirements for closing the issue.
Description of changes:
This test deletes the python well-known-endpoints test in favor of a rust well-known-endpoints test.
internet_http_clientusess2n-tls-hyperto make HTTP requests against a number of well known endpoints. The success of this test impliesinternet_kms_pq_clientusess2n-tls-tokioto make PQ requests against the kms service endpoints. The success of this test impliesCall-outs:
Opt In
The tests are not run by default, and must be explicitly opted into. This is because they are quite a bit slower than the other unit tests, taking about 10 seconds to run.
Retries
I do plan on adding retries to these tests to handle flaky network issues, but I don't want to do that until I have observed some of the specific errors. We don't currently have a good understanding of whether the flakiness comes from. TCP? TLS? DNS? Being able to actually observe these failures will allow us to
Testing:
A new github workflow step was added to run the internet tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.