You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "liuml07 (via GitHub)" <gi...@apache.org> on 2024/05/22 23:55:54 UTC
[PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
liuml07 opened a new pull request, #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104
https://issues.apache.org/jira/browse/FLINK-35424
> In [FLINK-34369](https://issues.apache.org/jira/browse/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 #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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#issuecomment-2128004309
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1614194871
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
@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](https://docs.google.com/document/d/1514Wa_aNB9bJUen4xm5uiuXOooOJTtXqS_Jqk9KJitU/edit#heading=h.jf7lfvm64da4)
- 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#issuecomment-2132254635
Thanks for the valuable improvement @liuml07
thanks for the review @reta
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1613758123
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1613490458
##########
flink-connector-elasticsearch8/src/main/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8AsyncSinkBuilder.java:
##########
@@ -100,16 +111,62 @@ public Elasticsearch8AsyncSinkBuilder<InputT> setHeaders(Header... headers) {
}
/**
- * setCertificateFingerprint set the certificate fingerprint to be used to verify the HTTPS
- * connection.
+ * Allows to bypass the certificates chain validation and connect to insecure network endpoints
+ * (for example, servers which use self-signed certificates).
+ *
+ * @return this builder
+ */
+ public Elasticsearch8AsyncSinkBuilder<InputT> allowInsecure() {
Review Comment:
:+1: Looks like a straightforward backport of ES6/7 impl, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#issuecomment-2128158649
> @reta and @snuyanzin Could you take a look?
Sure, I will do that within next few days, thank you @liuml07 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#issuecomment-2128158218
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1613694681
##########
flink-connector-elasticsearch8/src/main/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8AsyncSinkBuilder.java:
##########
@@ -100,16 +111,62 @@ public Elasticsearch8AsyncSinkBuilder<InputT> setHeaders(Header... headers) {
}
/**
- * setCertificateFingerprint set the certificate fingerprint to be used to verify the HTTPS
- * connection.
+ * Allows to bypass the certificates chain validation and connect to insecure network endpoints
+ * (for example, servers which use self-signed certificates).
+ *
+ * @return this builder
+ */
+ public Elasticsearch8AsyncSinkBuilder<InputT> allowInsecure() {
Review Comment:
Thanks, I refactored this ES 8 a bit in #100 to assist this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1614235030
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1614242366
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#issuecomment-2127907729
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1613488632
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
Why do we need this class vs using existing `ElasticsearchSinkBaseITCase` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424][connectors/elasticsearch] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1613711076
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35424] Elasticsearch connector 8 supports SSL context [flink-connector-elasticsearch]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #104:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/104#discussion_r1613717772
##########
flink-connector-elasticsearch8/src/test/java/org/apache/flink/connector/elasticsearch/sink/Elasticsearch8TestUtils.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.flink.connector.elasticsearch.sink;
+
+import org.apache.http.util.EntityUtils;
+import org.elasticsearch.client.Request;
+import org.elasticsearch.client.Response;
+import org.elasticsearch.client.RestClient;
+import org.testcontainers.utility.DockerImageName;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Utility class for Elasticsearch8 tests. */
+public class Elasticsearch8TestUtils {
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org