Add product tests for file password authenticator plugin #1913

electrum · 2019-10-31T01:59:25Z

This is all of #1912 plus product tests.
Only the product tests commit should be reviewed in this PR.

kokosing

Reviewed two last commits

kokosing · 2019-11-04T09:34:29Z

presto-product-tests/bin/run_on_docker.sh

+  fi
+  run_in_application_runner_container \
+    bash -c "export PRESTO_PASSWORD=$password;
+    /docker/volumes/conf/docker/files/presto-cli.sh --execute 'SHOW CATALOGS' | grep -iq hive"


I think PRESTO_PASSWORD should be handled inside /docker/volumes/conf/docker/files/presto-cli.sh otherwise things like ./bin/run_presto_cli_on_docker.sh are not going to work for this environment.

I would add PRESTO_PASSWORD env variable to presto-product-tests/conf/docker/fileauth/docker-compose.yml

there is no need to change presto-cli.sh

I would add PRESTO_PASSWORD env variable to presto-product-tests/conf/docker/fileauth/docker-compose.yml

this should suffice

kokosing · 2019-11-04T09:35:32Z

presto-product-tests/bin/run_on_docker.sh

@@ -89,6 +94,10 @@ if [[ ! -f "$DOCKER_CONF_LOCATION/$ENVIRONMENT/compose.sh" ]]; then
  usage
 fi

+if [[ "$ENVIRONMENT" == "fileauth" ]]; then
+    CLI_ARGUMENTS="--server https://presto-master:8443 --user userCrypt --password --truststore-path /etc/ldap/cacerts.jks --truststore-password testldap"


define this in presto-product-tests/conf/docker/fileauth/docker-compose.yml

it's already there e61633d#diff-26231eb27080877fabe76bdf699c4e39R14

so should be enough to remove it from here

kokosing · 2019-11-04T09:36:40Z

presto-product-tests/conf/docker/files/presto-launcher-wrapper.sh

+    if [ -f "$fileAuthCopy" ]; then
+      rm -f $fileAuthCopy
+    fi
+    cp /docker/volumes/conf/presto/etc/file_auth $fileAuthCopy


I think it would be easier to use volume in presto-product-tests/conf/docker/fileauth/docker-compose.yml

can we have volume with copy-on-write semantics?
(the file is being modified in the PrestoCliFileAuthTests.testAuthTokenCacheUpdate test)

btw, do we need PrestoCliFileAuthTests.testAuthTokenCacheUpdate() as a product test? i think unit test would be sufficient for this

@electrum Shall we get rid of this test? This will also resolve the "file_auth_copy" issues.

kokosing · 2019-11-04T09:37:05Z

presto-product-tests/conf/presto/etc/file_auth_copy

@@ -0,0 +1,2 @@
+userPBKDF2:1000:5b4240333032306164:f38d165fce8ce42f59d366139ef5d9e1ca1247f0e06e503ee1a611dd9ec40876bb5edb8409f5abe5504aab6628e70cfb3d3a18e99d70357d295002c3d0a308a0


why do we need to copy this?

ie remove the _copy from the repo

I created a copy and use it in the tests to avoid duplicate entries in case of product test failures.
For example, if the product test fails just after adding in the original file (the delete part is not executed), during the next run the add part will again add the same entries leading to duplicate entries.

kokosing · 2019-11-04T09:37:47Z

presto-product-tests/conf/presto/etc/file_auth

@@ -0,0 +1,2 @@
+userPBKDF2:1000:5b4240333032306164:f38d165fce8ce42f59d366139ef5d9e1ca1247f0e06e503ee1a611dd9ec40876bb5edb8409f5abe5504aab6628e70cfb3d3a18e99d70357d295002c3d0a308a0
+userBCrypt:$2y$10$BqTb8hScP5DfcpmHo5PeyugxHz5Ky/qf3wrpD7SNm8sWuA3VlGqsa


how about having this file under presto-product-tests/conf/docker/fileauth/presto-master/etc/presto/file_auth and the using docker volume in presto-product-tests/conf/docker/fileauth/docker-compose.yml?

kokosing · 2019-11-04T09:38:40Z

presto-product-tests/conf/presto/etc/fileauth.properties

@@ -0,0 +1,24 @@
+#


same comments like for presto-product-tests/conf/presto/etc/file_auth. See #1766

@kokosing If I follow #1766 here will it affect the existing tests?

What do you mean? In general #1766 should not affect tests, but it is more about tests infra (file locations etc)

kokosing · 2019-11-04T09:39:11Z

presto-product-tests/conf/presto/etc/password-authenticator.properties

@@ -0,0 +1,3 @@
+password-authenticator.name=file


same as above

kokosing · 2019-11-04T09:39:40Z

presto-product-tests/conf/presto/etc/fileauth.properties

+http-server.https.enabled=true
+http-server.https.keystore.path=/etc/openldap/certs/coordinator.jks
+http-server.https.keystore.key=testldap
+http-server.authentication.type=PASSWORD


missing new line before EOF (same for other files)

kokosing · 2019-11-04T09:40:53Z

presto-product-tests/src/main/java/io/prestosql/tests/cli/PrestoCliFileAuthTests.java

+    @Override
+    public Requirement getRequirements(Configuration configuration)
+    {
+        return compose(immutableTable(NATION));


compose looks unnecessary

Yes. Need to remove this.

findepi · 2019-11-04T11:14:27Z

presto-product-tests/bin/run_on_docker.sh

+  fi
+  run_in_application_runner_container \
+    bash -c "export PRESTO_PASSWORD=$password;
+    /docker/volumes/conf/docker/files/presto-cli.sh --execute 'SHOW CATALOGS' | grep -iq hive"


there is no need to change presto-cli.sh

I would add PRESTO_PASSWORD env variable to presto-product-tests/conf/docker/fileauth/docker-compose.yml

this should suffice

findepi · 2019-11-04T11:15:11Z

presto-product-tests/bin/run_on_docker.sh

@@ -89,6 +94,10 @@ if [[ ! -f "$DOCKER_CONF_LOCATION/$ENVIRONMENT/compose.sh" ]]; then
  usage
 fi

+if [[ "$ENVIRONMENT" == "fileauth" ]]; then
+    CLI_ARGUMENTS="--server https://presto-master:8443 --user userCrypt --password --truststore-path /etc/ldap/cacerts.jks --truststore-password testldap"


it's already there e61633d#diff-26231eb27080877fabe76bdf699c4e39R14

so should be enough to remove it from here

findepi · 2019-11-04T11:17:47Z

presto-product-tests/conf/docker/files/presto-launcher-wrapper.sh

+    if [ -f "$fileAuthCopy" ]; then
+      rm -f $fileAuthCopy
+    fi
+    cp /docker/volumes/conf/presto/etc/file_auth $fileAuthCopy


can we have volume with copy-on-write semantics?
(the file is being modified in the PrestoCliFileAuthTests.testAuthTokenCacheUpdate test)

btw, do we need PrestoCliFileAuthTests.testAuthTokenCacheUpdate() as a product test? i think unit test would be sufficient for this

findepi · 2019-11-04T11:18:14Z

presto-product-tests/conf/presto/etc/file_auth_copy

@@ -0,0 +1,2 @@
+userPBKDF2:1000:5b4240333032306164:f38d165fce8ce42f59d366139ef5d9e1ca1247f0e06e503ee1a611dd9ec40876bb5edb8409f5abe5504aab6628e70cfb3d3a18e99d70357d295002c3d0a308a0


ie remove the _copy from the repo

findepi · 2019-11-04T11:19:36Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-fileauth.yaml

@@ -0,0 +1,9 @@
+databases:
+  hive:
+    host: hadoop-master


redundant, default

findepi · 2019-11-04T11:19:40Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-fileauth.yaml

+  hive:
+    host: hadoop-master
+  presto:
+    host: presto-master


redundant, default

however, don't you need to use presto-master.docker.cluster here, like other product tests with SSL do?

findepi · 2019-11-04T11:19:51Z

presto-product-tests/conf/tempto/tempto-configuration-for-docker-fileauth.yaml

+    host: hadoop-master
+  presto:
+    host: presto-master
+    configured_hdfs_user: hive


redundant, default

findepi · 2019-11-04T11:22:49Z

Which product test suite runs these tests?

rupamk · 2019-11-09T04:35:31Z

test suite runs these tests?

presto-product-tests/bin/run_on_docker.sh fileauth -t io.prestosql.tests.cli.PrestoCliFileAuthTests

2019-10-11 06:14:14 INFO: Completed 6 tests
2019-10-11 06:14:14 INFO: 6 SUCCEEDED / 0 FAILED / 0 SKIPPED
2019-10-11 06:14:14 INFO: Tests execution took 1 minutes and 3 seconds

findepi · 2019-11-09T20:58:34Z

test suite runs these tests?

presto-product-tests/bin/run_on_docker.sh fileauth -t io.prestosql.tests.cli.PrestoCliFileAuthTests

Is fileauth env registered in any suite files? I mean the *suite-* files here: https://github.com/prestosql/presto/tree/0a031808c33c5fa0a371890410c5bad47989bce3/presto-product-tests/bin

electrum and others added 3 commits October 30, 2019 18:56

Move LDAP authenticator to separate package

0739adf

Add file based password authenticator plugin

98d9f8b

Add product tests for file password authenticator plugin

e61633d

cla-bot bot added the cla-signed label Oct 31, 2019

kokosing reviewed Nov 4, 2019

View reviewed changes

findepi reviewed Nov 4, 2019

View reviewed changes

electrum closed this Sep 25, 2020

electrum deleted the password-tests branch September 25, 2020 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add product tests for file password authenticator plugin #1913

Add product tests for file password authenticator plugin #1913

electrum commented Oct 31, 2019 •

edited

Loading

kokosing left a comment

kokosing Nov 4, 2019

findepi Nov 4, 2019

kokosing Nov 4, 2019

findepi Nov 4, 2019

kokosing Nov 4, 2019

findepi Nov 4, 2019

rupamk Nov 18, 2019

kokosing Nov 4, 2019

findepi Nov 4, 2019

rupamk Nov 9, 2019

kokosing Nov 4, 2019

kokosing Nov 4, 2019

rupamk Nov 18, 2019

kokosing Nov 19, 2019

kokosing Nov 4, 2019

kokosing Nov 4, 2019

kokosing Nov 4, 2019

rupamk Nov 18, 2019

findepi Nov 4, 2019

findepi Nov 4, 2019

findepi Nov 4, 2019

findepi Nov 4, 2019

findepi Nov 4, 2019

findepi Nov 4, 2019

findepi Nov 4, 2019

findepi commented Nov 4, 2019

rupamk commented Nov 9, 2019

findepi commented Nov 9, 2019

		@@ -0,0 +1,2 @@
		userPBKDF2:1000:5b4240333032306164:f38d165fce8ce42f59d366139ef5d9e1ca1247f0e06e503ee1a611dd9ec40876bb5edb8409f5abe5504aab6628e70cfb3d3a18e99d70357d295002c3d0a308a0

		@@ -0,0 +1,2 @@
		userPBKDF2:1000:5b4240333032306164:f38d165fce8ce42f59d366139ef5d9e1ca1247f0e06e503ee1a611dd9ec40876bb5edb8409f5abe5504aab6628e70cfb3d3a18e99d70357d295002c3d0a308a0
		userBCrypt:$2y$10$BqTb8hScP5DfcpmHo5PeyugxHz5Ky/qf3wrpD7SNm8sWuA3VlGqsa

Add product tests for file password authenticator plugin #1913

Add product tests for file password authenticator plugin #1913

Conversation

electrum commented Oct 31, 2019 • edited Loading

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Nov 4, 2019

rupamk commented Nov 9, 2019

findepi commented Nov 9, 2019

electrum commented Oct 31, 2019 •

edited

Loading