Add a flag to control whether credentials are printed during bootstrapping #461

eric-maynard · 2024-11-21T03:28:11Z

Description

This adds a new flag, BOOTSTRAP_PRINT_CREDENTIALS, that controls whether the bootstrap command prints root credentials to stdout.

If it's disabled, and environment variables were not provided to set the root credentials, bootstrapping will fail.

Fixes #450

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Credentials are now printed during bootstrap when it's enabled:

realm: default-realm root principal credentials: 2b98107557bcce20:f74281319ac8519ef30cbced6563223b

dimas-b · 2024-11-21T18:38:23Z

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

@@ -181,6 +196,19 @@ private PrincipalSecretsResult bootstrapServiceAndCreatePolarisPrincipalForRealm
      throw new IllegalArgumentException(overrideMessage);
    }

+    // TODO rebase onto #422, call a method like PrincipalSecretsGenerator.hasEnvironmentVariables


idea: maybe pass a flag down to PrincipalSecretsGenerator to not use random secrets if printCredentials is false? Then the PrincipalSecretsGenerator can simply throw if the specific realm/user combination is missing env. vars. WDYT?

I like that idea. If there is a good pathway from the bootstrap command down to the PrincipalSecretsGenerator then I think that works as well. It should hopefully be more clear when #422 merges.

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

eric-maynard · 2024-11-25T19:30:46Z

Hey @dimas-b, do you mind taking a look now that #422 has merged?

I think the integration is easy enough with some slight refactoring to PrincipalSecretsGenerator.

I left the current behavior wrt. using env variables even when printing is enabled, since if that's what the user decides to explicitly configure we can respect it. In the worst case we are just echoing env variables.

dimas-b · 2024-11-25T22:06:56Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/PrincipalSecretsGenerator.java

@@ -82,4 +86,28 @@ static PrincipalSecretsGenerator bootstrap(String realmName, Function<String, St
      }
    };
  }
+
+  /** Return true if environment variables for client ID & secret are set */
+  static boolean hasCredentialVariables(String realmName, String principalName) {


Exposing this static method does not look nice to be from the encapsulation point of view. I'd prefer to keep credential generation logic inside PrincipalSecretsGenerator instances.

We already have the bootstrap flag in LocalPolarisMetaStoreManagerFactory that controls secret generation under a synchronization lock. WDYT about converting it into something like PrincipalSecretsGenerator generatorFallback and making PrincipalSecretsGenerator.bootstrap() take an explicit fallback parameter (which is currently always RANDOM_SECRETS)? LocalPolarisMetaStoreManagerFactory would use RANDOM_SECRETS as the fallback by default, but during bootstrapping, we could have a custom impl. that produces random secrets if printing is enabled, but throws an exception if fallback if requested, but printing is disabled.

The issue is that the check against POLARIS_BOOTSTRAP_%s_%s_CLIENT_SECRET was buried inside bootstrap. One potential fix could be to sort of refactor PrincipalSecretsGenerator so that we have multiple implementations / providers and we could then use the type of the PrincipalSecretsGenerator to determine whether the credentials are user-provided or generated.

collado-mike · 2024-11-26T17:01:18Z

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

+          if (this.printCredentials(polarisContext)) {
+            String msg =
+                String.format(
+                    "realm: %1s root principal credentials: %2s:%3s",
+                    realmContext.getRealmIdentifier(),
+                    secretsResult.getPrincipalSecrets().getPrincipalClientId(),
+                    secretsResult.getPrincipalSecrets().getMainSecret());
+            System.out.println(msg);
+          }


I think this logic belongs to the secrets generator. The MetaStoreManager doesn't need to know anything about whether the secrets generated are provided by the user or if they've been generated randomly. So why would it be concerned with printing the credentials? The secrets generator knows if the secrets were provided explicitly or if they were randomly generated.

I think the bootstrap command should take a print-credentials config flag and the constructed secrets generator can react accordingly.

It seems like the PrincipalSecretsGenerator is doing exactly what the name suggests: generating secrets.

Whatever is done with those secrets -- persisting them, using them, printing them -- is outside the purview of the generator itself.

You are right that the MetaStoreManager doesn't need to know anything about printing either (it doesn't in this PR) and clearly this should be outside the purview of the metastore itself.

And so we landed on the factory. I would be happy to take this bootstrapping logic and excise it to somewhere more idiomatic if that is a concern. But right now the bootstrapping logic lives here (e.g. the purge check) and this seems like the most appropriate place that doesn't change the responsibility of either the metastore or generator classes.

Looking again, is your objection specifically to the protected method printCredentials?

That only exists to support the legacy behavior of the in-memory metastore always printing credentials, and if possible I would very much be in favor of removing that.

However it feels like pushing that logic down into an existing method (whether secretsGenerator, createMetaStoreSession, or elsewhere) could be a bit hacky if it winds up somewhere it doesn't belong.

collado-mike · 2024-11-26T17:03:49Z

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

+    boolean environmentVariableCredentials =
+        PrincipalSecretsGenerator.hasCredentialVariables(
+            realmContext.getRealmIdentifier(), PolarisEntityConstants.getRootPrincipalName());
+    if (!this.printCredentials(polarisContext) && !environmentVariableCredentials) {
+      String failureMessage =
+          String.format(
+              "It appears that environment variables were not provided for root credentials, and that printing "
+                  + "the root credentials is disabled via %s. If bootstrapping were to proceed, there would be no way "
+                  + "to recover the root credentials",
+              PolarisConfiguration.BOOTSTRAP_PRINT_CREDENTIALS.key);
+      LOGGER.error("\n\n {} \n\n", failureMessage);


Similar here - why is the metastore aware of whether the secrets were provided by environment variables? What if there are other impls of secrets generators that don't rely on env variables? E.g., we could have one that calls AWS SecretsManager to dynamically generate and store the secrets without any env variables. Should this code throw an exception?

dimas-b · 2024-11-26T22:07:22Z

...ore/src/main/java/org/apache/polaris/core/persistence/secrets/PrincipalSecretsGenerator.java


-      String clientId = config.apply(propId.toUpperCase(Locale.ROOT));


Did we lose uppercasing in the new code?

Yeah, I actually think this is wrong isn't it? It seems like you can have both a dimas and a DIMAS user, so how would you differentiate them in the env variables?

This is assuming we now allow the use of env variables for non-root users.

dimas-b · 2024-11-26T22:09:54Z

...ore/src/main/java/org/apache/polaris/core/persistence/secrets/PrincipalSecretsGenerator.java

+   * @return A {@link PrincipalSecretsGenerator} that can generate secrets through `produceSecrets`
+   */
+  public static PrincipalSecretsGenerator bootstrap(String realmName) {
+    return new DefaultPrincipalSecretsGenerator(realmName);


nit: maybe rename DefaultPrincipalSecretsGenerator -> BootstrapPrincipalSecretsGenerator?.. it is not actually default in LocalPolarisMetaStoreManagerFactory

Good point. It's just a wrapper, so I was not sure what to call it. Since it's only used during bootstrap, let me change to BootstrapPrincipalSecretsGenerator for now

eric-maynard added 2 commits November 20, 2024 19:23

initial commit

65c3012

autolint

d9a1698

eric-maynard mentioned this pull request Nov 21, 2024

Support providing root client ID via env. variables when bootstrapping #422

Merged

dimas-b reviewed Nov 21, 2024

View reviewed changes

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java Show resolved Hide resolved

eric-maynard added 2 commits November 25, 2024 11:29

rebase

7b1e258

autolint

312453b

eric-maynard requested a review from dimas-b November 25, 2024 19:31

eric-maynard marked this pull request as ready for review November 25, 2024 19:31

eric-maynard requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau, jackye1995, flyrain, collado-mike, ebyhr and adutra as code owners November 25, 2024 19:31

dimas-b reviewed Nov 25, 2024

View reviewed changes

collado-mike reviewed Nov 26, 2024

View reviewed changes

eric-maynard added 2 commits November 26, 2024 13:58

check in; not working

6e14558

autolint

4a3b1fd

dimas-b reviewed Nov 26, 2024

View reviewed changes

rename

ffcdf49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a flag to control whether credentials are printed during bootstrapping #461

Add a flag to control whether credentials are printed during bootstrapping #461

eric-maynard commented Nov 21, 2024

dimas-b Nov 21, 2024

eric-maynard Nov 21, 2024

eric-maynard commented Nov 25, 2024 •

edited

Loading

dimas-b Nov 25, 2024

eric-maynard Nov 26, 2024

collado-mike Nov 26, 2024

eric-maynard Nov 27, 2024

eric-maynard Nov 27, 2024

collado-mike Nov 26, 2024

dimas-b Nov 26, 2024

eric-maynard Nov 27, 2024

dimas-b Nov 26, 2024

eric-maynard Nov 27, 2024


		String clientId = config.apply(propId.toUpperCase(Locale.ROOT));

Add a flag to control whether credentials are printed during bootstrapping #461

Are you sure you want to change the base?

Add a flag to control whether credentials are printed during bootstrapping #461

Conversation

eric-maynard commented Nov 21, 2024

Description

Type of change

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard commented Nov 25, 2024 • 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

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

eric-maynard commented Nov 25, 2024 •

edited

Loading