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

Switch to testcontainers #107

Merged

Conversation

stormshield-gt
Copy link
Collaborator

Fixes #80

This PR migrates the whole test suite to testcontainers.

This notably removes dependencies against the unmaintained dockertest and dockertest-server. This will unlock #68 and other developments that were blocked by upstream issues. Notably we can now bump the tested Vault version.

To be able to run tests in parallel and factorize the test setup logic, I've introduced a new crate vaultrs-tests and update the CI accordingly.
The CI workflow now go from 9m 18s to 4m 23s. The local test duration is also greatly reduced.

Note: I've also updated the rust edition that uses the new cargo resolver, implying that we now use the cargo resolver v2 with feature unification. Because of this, reqwest will choose native-tls over rustls if both features are activated. I use use_rustls_tls() in that case to override this behavior to force rustls. This matches the old behavior and I think it makes more sense.

Note: I tried to strictly port the test doing the less refactoring possible to ease the review. I plan to do a followup PR, to further cleanup the test suite.

Copy link

gitguardian bot commented Oct 31, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
- - Generic Password 2777cb9 vaultrs-tests/tests/api_tests/userpass.rs View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@stormshield-gt
Copy link
Collaborator Author

The git guardian warning is because I introduced hard-coded private keys to ease the test setup.
I do not consider this as bad practice because the secrets are not sensitive. This is widely used by well-known crates as rustls.
However, if we can't add an exception to the git guardian check, I will consider generating the certificate on the flight with rcgen.

@stormshield-gt
Copy link
Collaborator Author

If fixed the GitGuardian issue, @Haennetz do you know when you will have some bandwidth to review this?

@Haennetz
Copy link
Collaborator

If fixed the GitGuardian issue, @Haennetz do you know when you will have some bandwidth to review this?

I hope that i can review it on Wednesday

@Haennetz
Copy link
Collaborator

Wednesday

I need some additional time to review it I will finish the review on Friday. Looks really good so fare. Nice work 👍

@stormshield-gt
Copy link
Collaborator Author

Thanks for taking a look!

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

@stormshield-gt Really nice work, I only have 3 questions, that are only out of curiosity. I will approve it already. You should be able to merge it by your self.

Cargo.toml Show resolved Hide resolved
vaultrs-tests/src/lib.rs Outdated Show resolved Hide resolved
vaultrs-tests/tests/api_tests/nginx.rs Outdated Show resolved Hide resolved
@stormshield-gt stormshield-gt force-pushed the switch_to_testcontainers branch from df32c5a to 65ef402 Compare November 19, 2024 18:05
@stormshield-gt
Copy link
Collaborator Author

Thanks again for your review @Haennetz, I will merge this now

@stormshield-gt stormshield-gt merged commit 27b18db into jmgilman:master Nov 19, 2024
6 of 7 checks passed
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.

Considering migrating to testcontainers
2 participants