Make regression tests use default authentication #804

adutra · 2025-01-16T15:08:10Z

... and align realm names to use POLARIS consistently.

Using default authenticators with real JWT tokens, the regression tests are better aligned with a production-like setup.

adutra · 2025-01-24T10:15:44Z

@MonkeyCanCode @flyrain @HonahX with this PR we should get regression tests running with production-like profile. This should supersede #866 and fix #865. Let me know what you think.

HonahX

@adutra Thanks for updating these!

HonahX · 2025-01-24T19:01:44Z

regtests/run_spark_sql.sh

@@ -55,7 +55,7 @@ if [ -z "${SPARK_HOME}"]; then
  export SPARK_HOME=$(realpath ~/${SPARK_DISTRIBUTION})
 fi

-SPARK_BEARER_TOKEN="${REGTEST_ROOT_BEARER_TOKEN:-principal:root;realm:default-realm}"
+SPARK_BEARER_TOKEN="${REGTEST_ROOT_BEARER_TOKEN:-principal:root;realm:POLARIS}"


Since the authenticator is default now, do we still need this token default value, principal:root;realm:POLARIS. Seems we will need to run the added script in run.sh to get a token using root credential first before running run_spark_sql.sh.

Additionally, is run_spark_sql.sh expected to work without first running run.sh? If so, may be we should consider adding code to get a token within this script to make it work out-of-box.

do we still need this token default value

Indeed I thought about removing the default values everywhere, but I wasn't sure whether all these scripts are always executed from the run.sh entrypoint. Do you think we should remove the default values?

may be we should consider adding code to get a token within this script to make it work out-of-box.

Good catch! The script is definitely meant to be executed individually. I will add support for default auth in that script as well.

I will add support for default auth in that script as well.

Done, let me know what you think!

Done, let me know what you think!

Thanks for adding that! Look great!

but I wasn't sure whether all these scripts are always executed from the run.sh entrypoint. Do you think we should remove the default values?

My understanding is that the default values will always fail since default authenticator reject principal:root:realm:* token anyway. We will need to either execute run.sh to store a real token in env or adding the default auth like run_spark_sql.sh to make scripts able to be executed without run.sh.

Keeping default values there will have no affect on tests but I feel it will cause additional work if we want to change realm name for regression test in the future. WDYT?

I think I agree. Should I remove the default values in this PR or as a follow-up?

I am ok with either way. As you pointed out in #865 (comment), we will need some follow-up PRs to clean-up/fix things so I think that could also be a good place to remove these values

Okay, I took note to open a follow-up PR for that 👍

HonahX · 2025-01-24T19:08:56Z

regtests/docker-compose.yml

-      polaris.realm-context.realms: default-realm,realm1
-      quarkus.log.file.enable: false
+      POLARIS_BOOTSTRAP_CREDENTIALS: POLARIS,root,secret
+      quarkus.log.file.enable: "false"


This can also help simplify the docker-compose.yml in getting-started examples: e.g. #868

I am happy to update that PR if this one goes in first : )

HonahX

LGTM! Thanks for updating these

README.md

regtests/run_spark_sql.sh

flyrain · 2025-01-24T21:33:20Z

regtests/t_spark_sql/src/spark_sql_s3_cross_region.sh

@@ -24,7 +24,7 @@ if [ -z "$AWS_CROSS_REGION_TEST_ENABLED" ] || [ "$AWS_CROSS_REGION_TEST_ENABLED"
  exit 0
 fi

-SPARK_BEARER_TOKEN="${REGTEST_ROOT_BEARER_TOKEN:-principal:root;realm:realm1}"
+SPARK_BEARER_TOKEN="${REGTEST_ROOT_BEARER_TOKEN:-principal:root;realm:POLARIS}"


Do we need to keep -principal:root;realm:POLARIS as token will be fetched by run.sh? Given the direction that we want to to is to use the default authenticator always for reggresstion test, I think we should remove them to avoid confusion, as principal:root;realm:POLARIS doesn't work for default athenticator.

With @HonahX we decided to postpone that, but I think you are right: leaving this default value will create confusion. So I went ahead and removed them.

regtests/t_cli/src/test_cli.py

flyrain · 2025-01-24T22:31:31Z

README.md

@@ -62,11 +62,8 @@ Apache Polaris is built using Gradle with Java 21+ and Docker 27+.
 - `./gradlew assemble` - To skip tests.
 - `./gradlew test` - To run unit tests and integration tests.
 - `./gradlew polarisServerRun` - To run the Polaris server locally, with profile `prod`; the server 


I'm not familiar with Quarkus conventions, but "with profile prod" seems a bit odd to me, it is no way close to a prod deployment. Is "with the default profile" more accurate? Or we don't even have to mention it in that case.

You are right. Rephrased.

flyrain · 2025-01-24T22:54:27Z

I think we should also remove this line,

polaris/getting-started/trino/README.md

Line 56 in 7d854f2

    
           The Polaris catalog setup script uses the credential `principal:root;realm:default-realm`. This credential is used so users do not need to fetch credentials from Apache Polaris' console output.

, as we changed the way it works. cc @HonahX

Also align realm names and fix docs.

adutra · 2025-01-25T10:53:58Z

Rebased (to get the tox removal commit) and addressed feedback.

I am now able to run the regression tests both locally and with docker-compose. I am also able to run ./regtests/run_spark_sql.sh and play interactively with a running Polaris server.

flyrain

+1

flyrain · 2025-01-25T17:26:55Z

Thanks a lot for the fix @adutra ! Thanks for the review @HonahX!

This was referenced Jan 17, 2025

Update poetry version from 1.5.0 to 1.8.5 #726

Merged

Failed to run run_spark_sql.sh #865

Open

adutra mentioned this pull request Jan 24, 2025

Avoid Spark driver IP binding failure due to VPN setting #866

Open

adutra force-pushed the regtests-default-auth branch from 0ae2b98 to 266e9b3 Compare January 24, 2025 10:06

adutra changed the title ~~[WIP] Make regression tests use default authentication~~ Make regression tests use default authentication Jan 24, 2025

adutra marked this pull request as ready for review January 24, 2025 10:08

adutra requested review from ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, flyrain and ebyhr as code owners January 24, 2025 10:08

adutra force-pushed the regtests-default-auth branch from 266e9b3 to f83ec9a Compare January 24, 2025 11:08

HonahX reviewed Jan 24, 2025

View reviewed changes

flyrain mentioned this pull request Jan 24, 2025

Fix Trino Getting-started Example #868

Merged

HonahX mentioned this pull request Jan 24, 2025

Update Trino Getting-started example to use default authenticator #877

Open

HonahX approved these changes Jan 24, 2025

View reviewed changes

flyrain reviewed Jan 24, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

flyrain reviewed Jan 24, 2025

View reviewed changes

regtests/run_spark_sql.sh Show resolved Hide resolved

flyrain reviewed Jan 24, 2025

View reviewed changes

regtests/t_cli/src/test_cli.py Outdated Show resolved Hide resolved

flyrain reviewed Jan 24, 2025

View reviewed changes

flyrain mentioned this pull request Jan 24, 2025

Remove tox #867

Merged

adutra added 4 commits January 25, 2025 11:43

Make regression tests use default authentication

30971ac

Also align realm names and fix docs.

Adapt run_spark_sql.sh

d972107

review

a45be42

review

f71b14f

adutra force-pushed the regtests-default-auth branch from a8a1c40 to f71b14f Compare January 25, 2025 10:52

flyrain approved these changes Jan 25, 2025

View reviewed changes

flyrain merged commit db3b3b8 into apache:main Jan 25, 2025
5 checks passed

HonahX added a commit to HonahX/polaris that referenced this pull request Jan 26, 2025

update after apache#804 is merged

5d7e237

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make regression tests use default authentication #804

Make regression tests use default authentication #804

adutra commented Jan 16, 2025 •

edited

Loading

adutra commented Jan 24, 2025

HonahX left a comment

HonahX Jan 24, 2025

adutra Jan 24, 2025

adutra Jan 24, 2025

HonahX Jan 24, 2025 •

edited

Loading

adutra Jan 24, 2025

HonahX Jan 24, 2025

adutra Jan 24, 2025

HonahX Jan 24, 2025

HonahX left a comment

flyrain Jan 24, 2025 •

edited

Loading

adutra Jan 25, 2025

flyrain Jan 24, 2025 •

edited

Loading

adutra Jan 25, 2025

flyrain commented Jan 24, 2025 •

edited

Loading

adutra commented Jan 25, 2025

flyrain left a comment

flyrain commented Jan 25, 2025

Make regression tests use default authentication #804

Make regression tests use default authentication #804

Conversation

adutra commented Jan 16, 2025 • edited Loading

adutra commented Jan 24, 2025

HonahX 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

HonahX Jan 24, 2025 • edited Loading

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

HonahX left a comment

Choose a reason for hiding this comment

flyrain Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain commented Jan 24, 2025 • edited Loading

adutra commented Jan 25, 2025

flyrain left a comment

Choose a reason for hiding this comment

flyrain commented Jan 25, 2025

adutra commented Jan 16, 2025 •

edited

Loading

HonahX Jan 24, 2025 •

edited

Loading

flyrain Jan 24, 2025 •

edited

Loading

flyrain Jan 24, 2025 •

edited

Loading

flyrain commented Jan 24, 2025 •

edited

Loading