Details
-
Sub-task
-
Status: Closed
-
Blocker
-
Resolution: Fixed
-
1.6.0
Description
None of the existing e2e tests run with an SSL configuration but there should be such a test as well.
Attachments
Issue Links
- is blocked by
-
FLINK-9842 Job submission fails via CLI with SSL enabled
- Closed
- links to
Activity
That's what I originally planned but all streaming e2e tests test rather specific things - nothing generic like "streaming job xxx works" like the test_batch_allround.sh test, so I wrote something like this. Anyway, I'm currently blocked for testing this script because of FLINK-9842.
GitHub user NicoK opened a pull request:
https://github.com/apache/flink/pull/6327
FLINK-9839[e2e] add a streaming allround test with SSL enabled
-
- What is the purpose of the change
Currently, there's no simple streaming job test like the `test_batch_allround.sh` and none of the other streaming tests uses SSL.
-
- Brief change log
- add tools to enable SSL in the config (including key generation)
- create `test_streaming_allround.sh` (with SSL enabled) verifying that this job runs
- add `test_streaming_allround.sh` to `run-pre-commit-tests.sh`
-
- Verifying this change
This change is a test but currently does not work because of https://issues.apache.org/jira/browse/FLINK-9842
-
- Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): *no*
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: *no*
- The serializers: *no*
- The runtime per-record code paths (performance sensitive): *no*
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: *no*
- The S3 file system connector: *no*
-
- Documentation
- Does this pull request introduce a new feature? *no*
- If yes, how is the feature documented? *not applicable*
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/NicoK/flink flink-9839
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6327.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #6327
commit 05b88d9afcda6d0a4385dccf77b50dd023c1ef54
Author: Nico Kruber <nico@...>
Date: 2018-07-13T08:39:55Z
FLINK-9839[e2e] add a streaming allround test with SSL enabled
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6327
Could we save testing time by just activating SSL for existing test jobs?
Please also check the update of the SSL config keys that may come through #6326
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/6327
Thanks, I'll have a second look at this PR once I can actually test it
(maybe enabling SSL for the `test_streaming_kafka010.sh` test or so)
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/6327
@NicoK #6326 has been merged into master. Please rebase this PR.
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/6327
Rebased and enabled SSL only for already existing e2e tests - since the old configuration values for enabling SSL communication for all components `security.ssl.enabled` was retained though, #6326 did not really influence this test.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6327#discussion_r202646757
— Diff: flink-end-to-end-tests/test-scripts/common.sh —
@@ -183,9 +221,15 @@ function start_cluster {
"$FLINK_DIR"/bin/start-cluster.sh
- wait at most 10 seconds until the dispatcher is up
+ local QUERY_URL
+ if [ "x$USE_SSL" = "xON" ]; then-
- End diff –
-
why the `x`?
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6327#discussion_r202648993
— Diff: flink-end-to-end-tests/test-scripts/common.sh —
@@ -148,6 +151,41 @@ function create_ha_config()
+function set_conf_ssl {
+
+ # clean up the dir that will be used for SSL certificates and trust stores
+ if [ -e "${TEST_DATA_DIR}/ssl" ]; then
+ echo "File ${TEST_DATA_DIR}/ssl exists. Deleting it..."
+ rm -rf "${TEST_DATA_DIR}/ssl"
+ fi
+ mkdir -p "${TEST_DATA_DIR}/ssl"
+ NODENAME=`hostname -f`
+ SANSTRING="dns:${NODENAME}"
+ for NODEIP in `hostname -I | cut -d' ' -f1` ; do
+ SANSTRING="${SANSTRING},ip:${NODEIP}"
+ done
+
+ # create certificates
+ keytool -genkeypair -alias ca -keystore "${TEST_DATA_DIR}/ssl/ca.keystore" -dname "CN=Sample CA" -storepass password -keypass password -keyalg RSA -ext bc=ca:true
+ keytool -keystore "${TEST_DATA_DIR}/ssl/ca.keystore" -storepass password -alias ca -exportcert > "${TEST_DATA_DIR}/ssl/ca.cer"
+ keytool -importcert -keystore "${TEST_DATA_DIR}/ssl/ca.truststore" -alias ca -storepass password -noprompt -file "${TEST_DATA_DIR}/ssl/ca.cer"
+
+ keytool -genkeypair -alias node -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -dname "CN=${NODENAME}" -ext SAN=${SANSTRING} -storepass password -keypass password -keyalg RSA
+ keytool -certreq -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -storepass password -alias node -file "${TEST_DATA_DIR}/ssl/node.csr"
+ keytool -gencert -keystore "${TEST_DATA_DIR}/ssl/ca.keystore" -storepass password -alias ca -ext SAN=${SANSTRING} -infile "${TEST_DATA_DIR}/ssl/node.csr" -outfile "${TEST_DATA_DIR}/ssl/node.cer"
+ keytool -importcert -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -storepass password -file "${TEST_DATA_DIR}/ssl/ca.cer" -alias ca -noprompt
+ keytool -importcert -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -storepass password -file "${TEST_DATA_DIR}/ssl/node.cer" -alias node -noprompt
+
+ # adapt config
+ set_conf security.ssl.enabled true
— End diff –
maybe add a comment that this relies on component-specific ssl switches being enabled by default.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/6327
probably out-of-scope of this PR (since the ssl setup follows the documentation), but when running the test this warning was printed:
```
Warning:
The JKS keystore uses a proprietary format. It is recommended to migrate to PKCS12 which is an industry standard format using [...]
```
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/6327#discussion_r202650635
— Diff: flink-end-to-end-tests/test-scripts/common.sh —
@@ -183,9 +221,15 @@ function start_cluster {
"$FLINK_DIR"/bin/start-cluster.sh
- wait at most 10 seconds until the dispatcher is up
+ local QUERY_URL
+ if [ "x$USE_SSL" = "xON" ]; then-
- End diff –
-
A guard against empty `USE_SSL` - just tested without and it seems working (probably because of the quotes) but I would not bet that this is the behaviour in every shell...
If it was empty, and a shell executes this as `if [ $USE_SSL = ON ] ; then echo "ON"; fi` then you'll get something like this: `bash: [: =: unary operator expected`
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/6327#discussion_r202650892
— Diff: flink-end-to-end-tests/test-scripts/common.sh —
@@ -148,6 +151,41 @@ function create_ha_config()
+function set_conf_ssl {
+
+ # clean up the dir that will be used for SSL certificates and trust stores
+ if [ -e "${TEST_DATA_DIR}/ssl" ]; then
+ echo "File ${TEST_DATA_DIR}/ssl exists. Deleting it..."
+ rm -rf "${TEST_DATA_DIR}/ssl"
+ fi
+ mkdir -p "${TEST_DATA_DIR}/ssl"
+ NODENAME=`hostname -f`
+ SANSTRING="dns:${NODENAME}"
+ for NODEIP in `hostname -I | cut -d' ' -f1` ; do
+ SANSTRING="${SANSTRING},ip:${NODEIP}"
+ done
+
+ # create certificates
+ keytool -genkeypair -alias ca -keystore "${TEST_DATA_DIR}/ssl/ca.keystore" -dname "CN=Sample CA" -storepass password -keypass password -keyalg RSA -ext bc=ca:true
+ keytool -keystore "${TEST_DATA_DIR}/ssl/ca.keystore" -storepass password -alias ca -exportcert > "${TEST_DATA_DIR}/ssl/ca.cer"
+ keytool -importcert -keystore "${TEST_DATA_DIR}/ssl/ca.truststore" -alias ca -storepass password -noprompt -file "${TEST_DATA_DIR}/ssl/ca.cer"
+
+ keytool -genkeypair -alias node -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -dname "CN=${NODENAME}" -ext SAN=${SANSTRING} -storepass password -keypass password -keyalg RSA
+ keytool -certreq -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -storepass password -alias node -file "${TEST_DATA_DIR}/ssl/node.csr"
+ keytool -gencert -keystore "${TEST_DATA_DIR}/ssl/ca.keystore" -storepass password -alias ca -ext SAN=${SANSTRING} -infile "${TEST_DATA_DIR}/ssl/node.csr" -outfile "${TEST_DATA_DIR}/ssl/node.cer"
+ keytool -importcert -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -storepass password -file "${TEST_DATA_DIR}/ssl/ca.cer" -alias ca -noprompt
+ keytool -importcert -keystore "${TEST_DATA_DIR}/ssl/node.keystore" -storepass password -file "${TEST_DATA_DIR}/ssl/node.cer" -alias node -noprompt
+
+ # adapt config
+ set_conf security.ssl.enabled true
— End diff –
That's how `security.ssl.enabled` was specified and should remain to do so because of backwards compatibility. However, I can add a comment as well.
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/6327
Thanks for the review @zentol.
Yes, this PR will fail until #6340 is merged.
Regarding the warning: I did not want to bother with different formats here, but you're right: a follow-up PR should address this here and also in the documentation.
master: e2e090b1a105f9bd20b6e6d0d354fefd5ab0fce9
1.5: 1f36a66ec5be338645e2f37e6a4bf7afd415702a
To save some testing time, can we just enable SSL for some of the existing jobs, rather than having a new job?