Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
65c3012
initial commit
eric-maynard Nov 21, 2024
d9a1698
autolint
eric-maynard Nov 21, 2024
7b1e258
rebase
eric-maynard Nov 25, 2024
312453b
autolint
eric-maynard Nov 25, 2024
6e14558
check in; not working
eric-maynard Nov 26, 2024
4a3b1fd
autolint
eric-maynard Nov 26, 2024
ffcdf49
rename
eric-maynard Nov 27, 2024
910bb2a
rework tests
eric-maynard Nov 27, 2024
208b8cd
autolint
eric-maynard Nov 27, 2024
3b32662
stable
eric-maynard Nov 27, 2024
a7a4e67
autolint
eric-maynard Nov 27, 2024
f922055
resolve conflicts
eric-maynard Dec 2, 2024
d0aeeee
annotation change
eric-maynard Dec 5, 2024
fd49022
another annotation fix
eric-maynard Dec 6, 2024
6ee09b4
merge conflict fix
eric-maynard Dec 6, 2024
fbd11a1
autolint
eric-maynard Dec 6, 2024
96885d0
doc note
eric-maynard Dec 9, 2024
df1d642
style
eric-maynard Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,4 +206,11 @@ public static <T> Builder<T> builder() {
"If set to true, allows tables to be dropped with the purge parameter set to true.")
.defaultValue(true)
.build();

public static final PolarisConfiguration<Boolean> BOOTSTRAP_PRINT_CREDENTIALS =
PolarisConfiguration.<Boolean>builder()
.key("BOOTSTRAP_PRINT_CREDENTIALS")
.description("If set to true, credentials are printed to stdout by the bootstrap command")
.defaultValue(true)
.build();
}
52 changes: 44 additions & 8 deletions ...rc/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Map;
import java.util.function.Supplier;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisConfiguration;
import org.apache.polaris.core.PolarisDefaultDiagServiceImpl;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult;
Expand Down Expand Up @@ -86,7 +87,8 @@ private void initializeForRealm(RealmContext realmContext) {
}

@Override
public synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(List<String> realms) {
public final synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(
List<String> realms) {
Map<String, PrincipalSecretsResult> results = new HashMap<>();

bootstrap = true;
Expand All @@ -95,10 +97,26 @@ public synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(List<Str
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realmContext.getRealmIdentifier())) {
initializeForRealm(realmContext);
// While bootstrapping we need to act as a fake privileged context since the real
// CallContext hasn't even been resolved yet.
PolarisCallContext polarisContext =
new PolarisCallContext(
sessionSupplierMap.get(realmContext.getRealmIdentifier()).get(), diagServices);
PrincipalSecretsResult secretsResult =
bootstrapServiceAndCreatePolarisPrincipalForRealm(
realmContext, metaStoreManagerMap.get(realmContext.getRealmIdentifier()));
realmContext,
metaStoreManagerMap.get(realmContext.getRealmIdentifier()),
polarisContext);
results.put(realmContext.getRealmIdentifier(), secretsResult);
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);
}
Comment on lines +113 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}
} finally {
Expand Down Expand Up @@ -173,12 +191,9 @@ public void setStorageIntegrationProvider(PolarisStorageIntegrationProvider stor
* credentials and print them to stdout
*/
private PrincipalSecretsResult bootstrapServiceAndCreatePolarisPrincipalForRealm(
RealmContext realmContext, PolarisMetaStoreManager metaStoreManager) {
// While bootstrapping we need to act as a fake privileged context since the real
// CallContext hasn't even been resolved yet.
PolarisCallContext polarisContext =
new PolarisCallContext(
sessionSupplierMap.get(realmContext.getRealmIdentifier()).get(), diagServices);
RealmContext realmContext,
PolarisMetaStoreManager metaStoreManager,
PolarisCallContext polarisContext) {
CallContext.setCurrentContext(CallContext.of(realmContext, polarisContext));

PolarisMetaStoreManager.EntityResult preliminaryRootPrincipalLookup =
Expand All @@ -196,6 +211,20 @@ private PrincipalSecretsResult bootstrapServiceAndCreatePolarisPrincipalForRealm
throw new IllegalArgumentException(overrideMessage);
}

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

throw new IllegalArgumentException(failureMessage);
eric-maynard marked this conversation as resolved.
Show resolved Hide resolved
}

metaStoreManager.bootstrapPolarisService(polarisContext);

PolarisMetaStoreManager.EntityResult rootPrincipalLookup =
Expand Down Expand Up @@ -253,4 +282,11 @@ private void checkPolarisServiceBootstrappedForRealm(
"Realm is not bootstrapped, please run server in bootstrap mode.");
}
}

/** Whether or not to print credentials after bootstrapping */
protected boolean printCredentials(PolarisCallContext polarisCallContext) {
return polarisCallContext
.getConfigurationStore()
.getConfiguration(polarisCallContext, PolarisConfiguration.BOOTSTRAP_PRINT_CREDENTIALS);
}
}
36 changes: 32 additions & 4 deletions ...ris-core/src/main/java/org/apache/polaris/core/persistence/PrincipalSecretsGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.polaris.core.persistence;

import java.util.Locale;
import java.util.Map;
import java.util.function.Function;
import org.apache.polaris.core.entity.PolarisPrincipalSecrets;
import org.jetbrains.annotations.NotNull;
Expand Down Expand Up @@ -63,14 +64,17 @@ public interface PrincipalSecretsGenerator {
PolarisPrincipalSecrets produceSecrets(@NotNull String principalName, long principalId);

static PrincipalSecretsGenerator bootstrap(String realmName) {
return bootstrap(realmName, System.getenv()::get);
return bootstrap(realmName, PrincipalSecretsGenerator::getEnvironmentVariable);
}

/**
* Return a {@link PrincipalSecretsGenerator} either randomly-generated or from environment
* variables
*/
static PrincipalSecretsGenerator bootstrap(String realmName, Function<String, String> config) {
return (principalName, principalId) -> {
String propId = String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_ID", realmName, principalName);
String propSecret =
String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_SECRET", realmName, principalName);
String propId = clientIdEnvironmentVariable(realmName, principalName);
String propSecret = clientSecretEnvironmentVariable(realmName, principalName);

String clientId = config.apply(propId.toUpperCase(Locale.ROOT));
String secret = config.apply(propSecret.toUpperCase(Locale.ROOT));
Expand All @@ -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) {
eric-maynard marked this conversation as resolved.
Show resolved Hide resolved
Map<String, String> environmentVariables = System.getenv();
String clientIdKey = clientIdEnvironmentVariable(realmName, principalName);
String clientSecretKey = clientSecretEnvironmentVariable(realmName, principalName);
return environmentVariables.containsKey(clientIdKey)
&& environmentVariables.containsKey(clientSecretKey);
}

/** Load a single environment variable */
private static String getEnvironmentVariable(String key) {
return System.getenv(key);
}

/** Build the key for the env variable used to store client ID */
private static String clientIdEnvironmentVariable(String realmName, String principalName) {
return String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_ID", realmName, principalName);
}

/** Build the key for the env variable used to store client secret */
private static String clientSecretEnvironmentVariable(String realmName, String principalName) {
return String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_SECRET", realmName, principalName);
}
}
27 changes: 8 additions & 19 deletions ...n/java/org/apache/polaris/service/persistence/InMemoryPolarisMetaStoreManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,12 @@
package org.apache.polaris.service.persistence;

import com.fasterxml.jackson.annotation.JsonTypeName;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
import java.util.List;
import java.util.Set;
import java.util.function.Supplier;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.persistence.LocalPolarisMetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
Expand Down Expand Up @@ -56,7 +55,7 @@ public synchronized PolarisMetaStoreManager getOrCreateMetaStoreManager(
RealmContext realmContext) {
String realmId = realmContext.getRealmIdentifier();
if (!bootstrappedRealms.contains(realmId)) {
bootstrapRealmAndPrintCredentials(realmId);
bootstrapRealms(List.of(realmId));
}
return super.getOrCreateMetaStoreManager(realmContext);
}
Expand All @@ -66,24 +65,14 @@ public synchronized Supplier<PolarisMetaStoreSession> getOrCreateSessionSupplier
RealmContext realmContext) {
String realmId = realmContext.getRealmIdentifier();
if (!bootstrappedRealms.contains(realmId)) {
bootstrapRealmAndPrintCredentials(realmId);
bootstrapRealms(List.of(realmId));
}
return super.getOrCreateSessionSupplier(realmContext);
}

private void bootstrapRealmAndPrintCredentials(String realmId) {
Map<String, PrincipalSecretsResult> results =
this.bootstrapRealms(Collections.singletonList(realmId));
bootstrappedRealms.add(realmId);

PrincipalSecretsResult principalSecrets = results.get(realmId);

String msg =
String.format(
"realm: %1s root principal credentials: %2s:%3s",
realmId,
principalSecrets.getPrincipalSecrets().getPrincipalClientId(),
principalSecrets.getPrincipalSecrets().getMainSecret());
System.out.println(msg);
/** {@inheritDoc} */
@Override
protected boolean printCredentials(PolarisCallContext polarisCallContext) {
return true;
}
}