Details

    Description

      None of the existing e2e tests run with an SSL configuration but there should be such a test as well.

      Attachments

        Issue Links

          Activity

            sewen Stephan Ewen added a comment -

            To save some testing time, can we just enable SSL for some of the existing jobs, rather than having a new job?

            sewen Stephan Ewen added a comment - To save some testing time, can we just enable SSL for some of the existing jobs, rather than having a new job?
            nkruber Nico Kruber added a comment -

            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.

            nkruber Nico Kruber added a comment - 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 .
            githubbot ASF GitHub Bot added a comment -

            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

              1. 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.

              1. 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`
              1. Verifying this change

            This change is a test but currently does not work because of https://issues.apache.org/jira/browse/FLINK-9842

              1. 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*
              1. 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


            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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)

            githubbot ASF GitHub Bot added a comment - 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)
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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

            1. wait at most 10 seconds until the dispatcher is up
              + local QUERY_URL
              + if [ "x$USE_SSL" = "xON" ]; then
                • End diff –

            why the `x`?

            githubbot ASF GitHub Bot added a comment - 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`?
            githubbot ASF GitHub Bot added a comment -

            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()

            { EOL }

            +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.

            githubbot ASF GitHub Bot added a comment - 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() { EOL } +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.
            githubbot ASF GitHub Bot added a comment -

            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 [...]
            ```

            githubbot ASF GitHub Bot added a comment - 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 [...] ```
            githubbot ASF GitHub Bot added a comment -

            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

            1. 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`

            githubbot ASF GitHub Bot added a comment - 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`
            githubbot ASF GitHub Bot added a comment -

            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()

            { EOL }

            +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.

            githubbot ASF GitHub Bot added a comment - 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() { EOL } +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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            Github user zentol commented on the issue:

            https://github.com/apache/flink/pull/6327

            merging.

            githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/6327 merging.
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

            https://github.com/apache/flink/pull/6327

            githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/6327

            master: e2e090b1a105f9bd20b6e6d0d354fefd5ab0fce9
            1.5: 1f36a66ec5be338645e2f37e6a4bf7afd415702a

            chesnay Chesnay Schepler added a comment - master: e2e090b1a105f9bd20b6e6d0d354fefd5ab0fce9 1.5: 1f36a66ec5be338645e2f37e6a4bf7afd415702a

            People

              nkruber Nico Kruber
              nkruber Nico Kruber
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: