-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Elasticsearch: Ensure Elasticsearch 8 works OOTB secure as default #5099
Elasticsearch: Ensure Elasticsearch 8 works OOTB secure as default #5099
Conversation
Since Elasticsearch 8.0 the default is to enable security, meaning TLS and authentication. This adds a check for Elasticsearch 8.0 to change the default behaviour to properly support this change, but you can still run Elasticsearch with security features disabled, if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing this PR 🙂
I was also thinking about the topics you mentioned in your description and commented accordingly in the review.
@dadoonet do you have an opinion on which wait strategy to use for each Elasticsearch version? Would we be fine to depend on LogMessageWaitStrategy
across all supported versions? This would simplify a bit of the logic and we would not need to differentiate between versions here.
The question of whether we need createSslContextFromCa
as part of the API or just in the docs is also a very valid one. It seems substantially easier for users, as we see from your tests, so I am inclined to add it to the API. WDYT @rnorth @bsideup?
...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
Show resolved
Hide resolved
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java
Outdated
Show resolved
Hide resolved
I would prefer that we have a new I agree that we could depend on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's great that we switched to LogMessageWaitStrategy
now and createSslContextFromCa
is also very convenient for users, thanks!
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
…search/ElasticsearchContainer.java Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
The Netlify failures can be ignored, they originate from: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
…he-box-with-secure-defaults
Since Elasticsearch 8.0 the default is to enable security, meaning TLS
and authentication.
This adds a check for Elasticsearch 8.0 to change the default behaviour
to properly support this change, but you can still run Elasticsearch
with security features disabled, if you want.
Closes #5048
Several things to discuss here that I am happy to change:
First I tried not to change behaviour for releases before 8.0 - neither when checking if Elasticsearch is ready, nor when changing options. This is merely to not break any existing applications and tests when upgrading.
Also, generating the
SSLContext
could something to be done outside of the application but I consider this a super common thing to do once Elasticsearch starts up as you need to have a custom SSL context due to the self signed certificate.Happy to get any feedback - also if you don't think this is a viable thing to do 😀