Enable lazy certificates for Elasticsearch #7991
Conversation
Added the test to ensure the security can be enabled for Elasticsearch 7.x too
pioorg
force-pushed
the
elasticsearch-lazy-certificates
branch
from
54cca34
to
f777482
Compare
|
Hi! One thing I'm not sure of is what to do with: Shall the removal of it be allow-listed, or shall it be kept (e.g. with a comment), despite being empty? |
| if (StringUtils.isBlank(certPath)) { | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
I think this will never be executed because certPath has a default value.
There was a problem hiding this comment.
Hmmm... What if one calls withCertPath("") when creating the container?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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("").
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
…hich have certificate generated by default, i.e. 8 or above
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
|
Thanks for your contribution, @pioorg ! |
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.