spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.54k stars 40.54k forks source link

Improve Elasticsearch ServiceConnection #35926

Open eddumelendez opened 1 year ago

eddumelendez commented 1 year ago

Currently, ElasticsearchContainerConnectionDetailsFactory only maps nodes and that works with Elasticsearch 7. However, Elasticsearch 8 has security enabled by default and additional configuration is required.

This is an example using Elasticsearch 7 and this one using Elasticsearch 8. As you can see, with Elasticsearch 8 the SSLContext is provided via a customizer so the connection through https is successful.

You can find a draft proposal here. I tried to do it via ssl bundles by copying the http_ca.crt from the container to the test resources directory but I couldn't set it up correctly.

Divyansh1711 commented 1 year ago

I m beginner and i have no idea ...how to start contributing for project.. Anyone any guides for me??

wilkinsona commented 1 year ago

Thanks for your interest in contributing. I'm not sure if we have any at the moment, but please keep an eye out for unassigned issues labelled with ideal for contribution or, if you haven't contributed before, first-timers only.

kevinwheeler commented 1 year ago

I'll see if this ends up being over my head or not, but I'm starting to investigate this / work on it. Feedback is always welcome. I am only slightly familiar with Spring Boot, having used JHipster to make a simple app recently, and I haven't used Elasticsearch yet.

kevinwheeler commented 1 year ago

Something came up that I'm going to have to work on for at least a few days before I can get back to this unfortunately.

kevinwheeler commented 1 year ago

It's going to be a while before I can get back to this actually. Perhaps a week.

Edit: I'm looking into this issue again currently.

kevinwheeler commented 1 year ago

Hi all, can you all confirm to me what the goal here is exactly? Are we wanting to make it such that users can use Elasticsearch 8 Testcontainers without having to configure the SSL manually using a RestClientBuilderCustomizer? Probably while keeping SSL enabled?

@eddumelendez the bulk of your edits in your draft proposal were in the DataElasticsearchTestIntegrationTests.java file, so I wanted to clarify and ask. That file is for tests that are for internal use by the Spring team, right? Were you just tinkering around in there for dev purposes? I assume we would be wanting to modify other files instead to provide the functionality we are going for, right? By the way passing in "docker.elastic.co/elasticsearch/elasticsearch:8.7.1" (or better yet, modifying DockerImageNames.elasticsearch8()) to the ElasticsearchContainer constructor in that test file in your draft proposal makes the tests pass, but you're probably already way ahead of me. Currently it's using Elasticsearch 7.

hantsy commented 11 months ago

I have updated another example project which based on Spring Boot 3.1.4 and ElasticSearch v7.17, check the tests, it is similar to the ElasticSearch 7 example provided by @eddumelendez, the difference I used Spring Reactive stack, the tests failed, consumeNextWith encountered an onComplete event instead, which means the data I tried to save in the @BeforeEach is not found.

hantsy commented 10 months ago

Is any update for this?

I almost copy the example tests provided by @eddumelendez, but they don't work with Spring Boot 3.2.0-RC2.

My example tests for Elasticsearch 7 and Elasticsearch 8.

eddumelendez commented 10 months ago

@hantsy looks like you are having another issue. My examples are using 3.2.0-RC2 without issues but there are some differences between yours and mine, the id in my case is autogenerated and in yours is null. I also left a comment in a previous issue opened by you. So, as already suggested, if the issue is reproduced without spring boot then opening a issue in spring-data-elasticsearch would be the right place.

hantsy commented 10 months ago

@eddumelendez The difference is I am using a Record as document definition.

scottfrederick commented 10 months ago

I've got another prototype that uses the SSL bundle abstractions to let the container create the SSLContext instead of copying certificates from the container. You can see that here: https://github.com/scottfrederick/spring-boot/tree/gh-35926-elastic8. We'll discuss this and see how something like this that requires additional beans to be created (like an SslBundleRegistrar) fits into the ServiceConnection design.

eddumelendez commented 10 months ago

Thanks for sharing, @scottfrederick! My only comment would be related to isVersion8OrGreater in the spring-boot-testcontainers module. Because for custom images it will not work due to there is not compatibility image version in Testcontainers at the moment. I think the getContainer().caCertAsBytes().isPresent() will play nice for current use cases and when Testcontainers supports compatible image version.

pioorg commented 8 months ago

Good time of the day to you!

I'd say the goal is to have support in SpringBoot Testcontainers for Elasticsearch for all Elasticsearch supported versions, which currently are 7.x and 8.x. At the moment of writing Spring Boot supports only 7.x in its default settings, as the security and HTTPS are ignored by default. For 8.x to work, one has to disable them, which might make things different than recommended production environment, hence the whole purpose of testing might get flawed here.

The thing is: both 7.x and 8.x can be secured (e.g. require password) and run with HTTPS (these can be turned on and off independently). In case of 8.x the default behaviour is to have both enabled and the user has to opt-out to disable them. For 7.x security and HTTPS can be enabled by opt-in.

The caCertAsBytes() currently provides only CA certificate (if not disabled and location not changed by e.g. manual configuration) for 8.x. (There's a PR for TC to enable that for 7.x too.)

I've tried to make both determined by examining the logs. Unfortunately, I haven't found a reliable, race-free way working for both versions so far (which would be also backward compatible). In theory the configuration of Elastcsearch could be checked, compared with env, and then we could compute if security and HTTPS are enabled. However, this would mean a) redoing some product logic, and b) maintaining it. Therefore I chose a different approach, actually testing if security and HTTPS are in action or not. I'd be more than happy to replace it with a cleaner approach, so any suggestions welcome.

The idea is to make two HTTP(S) requests, to determine if SSL and security are on or off.

AFAICT (but I'd love to be corrected) there's no other way of saying 100% correctly that Elasticsearch in the container is running with HTTPS or HTTP. The only 100% reliable method to determine what protocol shall be used (HTTP or HTTPS) I found so far (for both versions) is actually trying to establish secured connection and checking this test results.

Apart from security and HTTPS, there's a third issue, adding the self-signed certificate to trusted ones.

IMHO the ideal approach would be to make the Spring Boot Testcontainers module do that automatically, e.g. by creating SSL bundle and changing the properties accordingly. However, I don't know what would be the idiomatic way of doing this, so for now I'm relying on @DynamicPropertySource, as can be seen here.

I hope there's a Spring guru around, who could help to achieve the same effect without the need of adding an extra method every time the developer decides to use Elasticsearch with HTTPS enabled ;-) Perhaps we don't have to address all possible scenarios right now, OTOH I'd be not nice if the solution didn't require users configure their certificates, I guess.

scottfrederick commented 8 months ago

Thanks for taking a look at this and providing feedback @pioorg.

I wasn't very happy with either of the isVersion8OrGreater or getContainer().caCertAsBytes().isPresent() options for determining if SSL and security are enabled or disabled either. Your suggestions in #39092 for detecting those features look reasonable to me at a glance. We'll take that into consideration when refining this.

IMHO the ideal approach would be to make the Spring Boot Testcontainers module do that automatically, e.g. by creating SSL bundle and changing the properties accordingly. However, I don't know what would be the idiomatic way of doing this, so for now I'm relying on @DynamicPropertySource, as can be seen here.

I hope there's a Spring guru around, who could help to achieve the same effect without the need of adding an extra method every time the developer decides to use Elasticsearch with HTTPS enabled ;-) Perhaps we don't have to address all possible scenarios right now, OTOH I'd be not nice if the solution didn't require users configure their certificates, I guess.

This is the design challenge we're facing with this issue. There are a few other container types that support SSL, and we'd like to have a consistent way to set up the Spring Boot connection including SSL details that supports both @DynamicPropertySource and @ServiceConnection.

pioorg commented 8 months ago

Hello @scottfrederick Two more cents: I've chosen curl based approach, because a) it's present in ES containers, b) handling results seemed less verbose, than handling exceptions. However, should any other containers need similar testing too, perhaps it'd make sense to go for Java implementation, sitting somewhere in the module? Thank you for your comment about "injecting SSL", makes me feel better ;-)