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

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

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

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

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

Closed

adutra mentioned this pull request Jan 24, 2025

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

Merged

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 RussellSpitzer, ashvina, collado-mike, dennishuo, dimas-b, ebyhr, eric-maynard, flyrain, jackye1995, jbonofre, snazy, takidau and vvcephei 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

Merged

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 Outdated 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

MonkeyCanCode mentioned this pull request Jan 31, 2025

License Issue - Polaris has dependency on chardet which is LGPL which makes it difficult to use inside enterprise. #821

Closed

Make regression tests use default authentication #804

Make regression tests use default authentication #804

Uh oh!

Conversation

adutra commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adutra commented Jan 24, 2025

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonahX Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flyrain Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flyrain Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adutra commented Jan 25, 2025

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flyrain commented Jan 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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