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

Enable lazy certificates for Elasticsearch #7991

Merged

Conversation

pioorg
Copy link
Contributor

@pioorg pioorg commented Dec 21, 2023

The latest versions of Elasticsearch (8.x) come with security enabled.
The somewhat older versions (which are still in use) can have security enabled.

It is handy for people to be able to enable security in their tests also for version 7.x as one of the required steps. This way they can start using the clients in the way that is going to work with 8.x as well.

The proposed changes basically postpone fetching the certificate until the client is created, instead of doing that as part of the container spin-up procedure.

Copy link
Contributor

@dadoonet dadoonet left a comment

Choose a reason for hiding this comment

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

Great! And I like the test a lot.

@pioorg pioorg force-pushed the elasticsearch-lazy-certificates branch from 54cca34 to f777482 Compare January 9, 2024 15:53
@pioorg pioorg marked this pull request as ready for review January 9, 2024 15:56
@pioorg pioorg requested a review from a team as a code owner January 9, 2024 15:56
@pioorg
Copy link
Contributor Author

pioorg commented Jan 9, 2024

Hi!

One thing I'm not sure of is what to do with:
protected void containerIsStarted(InspectContainerResponse containerInfo) {}

Shall the removal of it be allow-listed, or shall it be kept (e.g. with a comment), despite being empty?

Comment on lines +151 to +153
if (StringUtils.isBlank(certPath)) {
return Optional.empty();
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this will never be executed because certPath has a default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... What if one calls withCertPath("") when creating the container?

Copy link
Member

Choose a reason for hiding this comment

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

Do you see any real use case when using ElasticsearchContainer and use withCertPath("")? If so, better to add a test to cover the scenario and also serve as a documentation. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for this comment @eddumelendez, it made me think ;-)
And as the result of this process I suggest yet another small change, basically the constructor of the ElasticsearchContainer should set the certPath by default only for versions, which do that by default, that is 8.
Previously it set it also for 7, which (after consideration) isn't optimal IMHO. I think, if version 7 requires manual opt-in to enable certificates, then this should be reflected in calling withCertPath(thePath). No opt in -> no path for 7.
Similarly for version 8, which has the certs enabled by default. If the user decides to disable them, ideally they should also state this intent by calling withCertPath("").

@eddumelendez eddumelendez added this to the next milestone Mar 4, 2024
@eddumelendez eddumelendez merged commit 4b5b34a into testcontainers:main Mar 4, 2024
97 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @pioorg !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants