[FLINK-35424] Elasticsearch connector 8 supports SSL context

[liuml07]

https://issues.apache.org/jira/browse/FLINK-35424

In  FLINK-34369, we added SSL support for the base Elasticsearch sink class that is used by both Elasticsearch 6 and 7. The Elasticsearch 8 connector is using the AsyncSink API and it does not use the aforementioned base sink class. It needs separate change to support this feature.

This is specially important to Elasticsearch 8 which enables secure by default. Meanwhile, it merits if we add integration tests for this SSL context support.

Main changes in this PR:

1. Add serializable SSL provider along with hostname verifier, follows the same way as [FLINK-34369][connectors/elasticsearch] Elasticsearch connector supports SSL context #91. Specially, the certificate fingerprint becomes just a special case of SSL context.
2. Refactor the tests to make it easier to add new ones
3. Add new integration tests using testcontainers for secure Elasticsearch 8

[liuml07]

@reta and @snuyanzin Could you take a look?

Also I'm not sure how to fix the pre-check failure:

Title Validator — Wrong Commit title: [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context

Thanks!

[reta]

Also I'm not sure how to fix the pre-check failure:

I think slight change to [FLINK-35424] Elasticsearch connector 8 supports SSL context would make it pass

[reta]

@reta and @snuyanzin Could you take a look?

Sure, I will do that within next few days, thank you @liuml07 !

[reta]

Why do we need this class vs using existing ElasticsearchSinkBaseITCase ?

[liuml07]

Good question, it's not required. ElasticsearchSinkBaseITCase creates a static ES container while the new test Elasticsearch8AsyncSinkSecureITCase needs to create the secure ES container. The new one does not really inherit this base class, but simply refer to those static fields. I think it's a bit clearer to extract those common fields and methods so secure vs. non-secure tests are completely independent.

I don't have strong preference on this and can move it back to the ElasticsearchSinkBaseITCase class if we feel it's better to keep them in the base class.

[reta]

I don't have strong preference on this and can move it back to the ElasticsearchSinkBaseITCase class if we feel it's better to keep them in the base class.

It looks more straightforward to me to be fair (with ElasticsearchSinkBaseITCase), the container creation could be parameterized if needed, thank you

[liuml07]

Indeed the parameterized tests will reuse the code at the max level and still keep separate cases/parameters independent. I thought of this idea previously but realized the ES_CONTAINER field is static (to amortize cost of container setup) and annotated @Container for managed lifecycle.

When parameterized, there will be multiple if-else checks depending on the parameter in the base class and child test classes, mainly for ES client and sink builder. This is not a problem per se, and just needs a bit more refactoring.

I'll move the fields / static methods back to the base class for now, and take another look at parameterized tests.

[liuml07]

@reta I'm working on the parameterized tests, which is purely a test code refactoring:

• Make base test class parameterized with secure parameter. As JUnit 5 has limited support for parameterized tests with inheritance, I used the ParameterizedTestExtension introduced in Flink, see this doc
• Manage the test container lifecycle instead of using the managed annotation @Testcontainers and @Container
• Create and use common methods in the base class that concrete test classes can be mostly parameter-agnostic

If you prefer we wait for that change, I can push to this branch after I have a working version. If you agree, I can also create a new PR of the testing code refactoring for future proof (new tests will be easily covered by secure clusters).

[reta]

If you prefer we wait for that change, I can push to this branch after I have a working version. If you agree, I can also create a new PR of the testing code refactoring for future proof (new tests will be easily covered by secure clusters).

I would agree with you that parameterized tests would very likely make things cleaner (we do have some level of duplication now). If you could pull it off, would be great, I sadly cannot merge, only review, so we still have time till committer comes in.

[liuml07]

Sounds good, @reta ! Thanks for the review and helpful comments.

Let's see if @snuyanzin has bandwidth to take a look while I'm trying to refactor the tests separately.

[reta]

👍 Looks like a straightforward backport of ES6/7 impl, thank you

[liuml07]

Thanks, I refactored this ES 8 a bit in #100 to assist this.

[snuyanzin]

Thanks for the valuable improvement @liuml07
thanks for the review @reta