Skip to content

Conversation

@XN137
Copy link
Contributor

@XN137 XN137 commented May 27, 2025

after #1376 a comment shows that non-root credentials could get printed twice:

2025-05-07 23:40:06,767 INFO  [org.apa.pol.ser.qua.con.QuarkusProducers] [,] [,,,] (Quarkus Main Thread) Bootstrapping realm(s) 'POLARIS', if necessary, from root credentials set provided via the environment variable POLARIS_BOOTSTRAP_CREDENTIALS or Java system property polaris.bootstrap.credentials ...
realm: POLARIS root principal credentials: c15135cbf0977e3d:a38976b705f0959e0017e0cef831122e
2025-05-07 23:40:06,819 INFO  [org.apa.pol.ser.qua.con.QuarkusProducers] [,] [,,,] (Quarkus Main Thread) Realm 'POLARIS' automatically bootstrapped, credentials were not present in root credentials set provided via the environment variable POLARIS_BOOTSTRAP_CREDENTIALS or Java system property polaris.bootstrap.credentials, see separate message printed to stdout.
realm: POLARIS root principal credentials: c15135cbf0977e3d:a38976b705f0959e0017e0cef831122e

so we remove all logging/printing from the bootstrapRealms implementations as the outer callers QuarkusProducers.maybeBootstrap and BootstrapCommand.call are already taking care of that.

for (String realm : realms) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realmContext.getRealmIdentifier())) {
if (!metaStoreManagerMap.containsKey(realm)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decided to refactor this for clarity as the realmContext is built from realm right above

@XN137
Copy link
Contributor Author

XN137 commented May 27, 2025

wondering about this code:

spec.commandLine().getOut().printf("Realm '%s' successfully bootstrapped.%n", realm);
if (inputOptions.stdinOptions != null && inputOptions.stdinOptions.printCredentials) {
String msg =
String.format(
"realm: %1s root principal credentials: %2s:%3s",
result.getKey(),
result.getValue().getPrincipalSecrets().getPrincipalClientId(),
result.getValue().getPrincipalSecrets().getMainSecret());
spec.commandLine().getOut().println(msg);
}

whether it should also only print credentials that were not taken from the root credentials as is done by QuarkusProducers.maybeBootstrap

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this, @XN137 !

Not printing credentials to STDOUT in metastore implementations makes sense to me (even for the in-memory case).

I also support the idea of handling all initial realm setup in runtime-specific code (QuarkusProducers in this case) and the Admin tool.

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 27, 2025
Copy link
Contributor

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @dimas-b ! LGTM!

@flyrain flyrain merged commit 3b9d2b0 into apache:main May 28, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 28, 2025
@flyrain
Copy link
Contributor

flyrain commented May 28, 2025

Thanks @XN137 for the fix. Thanks everyone for the review.

@XN137 XN137 deleted the credentials-logged-twice branch May 28, 2025 07:35
snazy added a commit to snazy/polaris that referenced this pull request Jun 13, 2025
* fix: Remove duplicated code in IcebergCatalog (apache#1681)

* Fix SparkClient listGenericTable to use ListGenericTablesRESTResponse

ListTableResponse is the class used by iceberg endpoint, which is the same as ListGenericTablesRESTResponse. However, we are suppose to use ListGenericTablesRESTResponse to be correct

* fix: Remove info log about deprecated internal method from PolarisConfiguration (apache#1672)


Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages.

Phasing out old property names requires coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code.

Fixes apache#1666

* Create a single binary distribution bundle (apache#1589)

* fix(quickstart): Correct Quickstart Instructions (apache#1673)

* Remove Java URI validations for Blob Storage providers (apache#1604)

This is a retry at apache#1586, which I'll close if this PR is on the right direction instead.

Java URI does not actually apply any normalization to URIs if we do not call URI.normalize() (which we currently do not). Additionally, blob storage providers like S3 and GCS can provide ".." and "." as valid fragments in URLs - which Java URI would attempt to normalize incorrectly. As a result, attempting to validate and/or normalize URIs for blob storage providers using the Java URI class is the incorrect behavior. While we may want to add location validation via regex later, removing it first should at least unblock the bug we see in apache#1545.

* Improve test coverage for invalid inputs in Policy APIs (apache#1665)

* Fix getting-started docker start by PR apache#1532 (apache#1687)

* Fix the manual test broken by PR apache#1532 (apache#1688)

* Fix credentials printing twice (apache#1682)

* main: Update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.0.4 (apache#1690)

* INFO: last merged commit 493de03

---------

Co-authored-by: Alexandre Dutra <[email protected]>
Co-authored-by: gh-yzou <[email protected]>
Co-authored-by: Dmitri Bourlatchkov <[email protected]>
Co-authored-by: Yufei Gu <[email protected]>
Co-authored-by: Adnan Hemani <[email protected]>
Co-authored-by: William Hyun <[email protected]>
Co-authored-by: Christopher Lambert <[email protected]>
Co-authored-by: Mend Renovate <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants