You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/02/04 12:37:54 UTC
[PR] Ability to read root credentials from file when bootstrapping [polaris]
adutra opened a new pull request, #940:
URL: https://github.com/apache/polaris/pull/940
This was initially done in support of #878, which has been closed.
But I think that the feature is still useful, so I went ahead and opened a PR.
`PolarisCredentialsBootstrap` has been refactored into two components in order to facilitate JSON and YAML parsing: `RootCredentialsSet` and `RootCredentials`.
The expected YAML format is as follows:
```yaml
realm1:
client-id: client1
client-secret: secret1
realm2:
client-id: client2
client-secret: secret2
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1941958197
##########
polaris-core/src/test/resources/org/apache/polaris/core/persistence/bootstrap/credentials.yaml:
##########
@@ -0,0 +1,25 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+realm1:
+ client-id: client1
+ client-secret: secret1
+realm2:
Review Comment:
nit: can we support multiple documents (`---`)
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
Do we still need this as a separate option? Why not extract after parting the `-c` value?
##########
quarkus/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTest.java:
##########
@@ -41,7 +61,76 @@ class BootstrapCommandTest {
"-c",
"realm2,root,s3cr3t"
})
- public void testBootstrap(LaunchResult result) {
- assertThat(result.getOutput()).contains("Bootstrap completed successfully.");
+ public void testBootstrapFromCommandLineArguments(LaunchResult result) {
+ assertThat(result.getOutput())
+ .contains("Realm 'realm1' successfully bootstrapped.")
+ .contains("Realm 'realm2' successfully bootstrapped.")
+ .contains("Bootstrap completed successfully.");
+ }
+
+ @Test
+ @Launch(
+ value = {
+ "bootstrap",
+ "-r",
+ "realm1",
+ "-c",
+ "invalid syntax",
+ },
+ exitCode = EXIT_CODE_BOOTSTRAP_ERROR)
+ public void testBootstrapInvalidCredentials(LaunchResult result) {
+ assertThat(result.getErrorOutput())
+ .contains("Invalid credentials format: invalid syntax")
+ .contains("Bootstrap encountered errors during operation.");
+ }
+
+ @Test
+ @Launch(
+ value = {"bootstrap", "-r", "realm1", "-f", "/irrelevant"},
+ exitCode = EXIT_CODE_USAGE)
+ public void testBootstrapInvalidArguments(LaunchResult result) {
+ assertThat(result.getErrorOutput())
+ .contains(
+ "Error: (-r=<realm> [-r=<realm>]... [-c=<realm,clientId,clientSecret>]...) "
+ + "and -f=<file> are mutually exclusive (specify only one)");
+ }
+
+ @Test
+ public void testBootstrapFromValidJsonFile(QuarkusMainLauncher launcher) {
+ LaunchResult result = launcher.launch("bootstrap", "-f", json.toString());
+ assertThat(result.exitCode()).isEqualTo(0);
+ assertThat(result.getOutput())
+ .contains("Realm 'realm1' successfully bootstrapped.")
+ .contains("Realm 'realm2' successfully bootstrapped.")
+ .contains("Bootstrap completed successfully.");
+ }
+
+ @Test
+ public void testBootstrapFromValidYamlFile(QuarkusMainLauncher launcher) {
+ LaunchResult result = launcher.launch("bootstrap", "-f", yaml.toString());
+ assertThat(result.exitCode()).isEqualTo(0);
+ assertThat(result.getOutput())
+ .contains("Realm 'realm1' successfully bootstrapped.")
+ .contains("Realm 'realm2' successfully bootstrapped.")
+ .contains("Bootstrap completed successfully.");
+ }
+
+ @Test
+ public void testBootstrapFromInvalidFile(QuarkusMainLauncher launcher) {
+ LaunchResult result = launcher.launch("bootstrap", "-f", "/non/existing/file");
+ assertThat(result.exitCode()).isEqualTo(EXIT_CODE_BOOTSTRAP_ERROR);
+ assertThat(result.getErrorOutput())
+ .contains("Failed to read credentials file: file:/non/existing/file")
+ .contains("Bootstrap encountered errors during operation.");
+ }
+
+ private static Path copyResource(String resource) throws IOException {
+ URL jsonSource = Objects.requireNonNull(BootstrapCommandTest.class.getResource(resource));
+ Path file = Files.createTempFile("credentials", "tmp");
Review Comment:
nit: use `@TempDir` provided by JUnit5?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
+
+ @CommandLine.Option(
+ names = {"-c", "--credential"},
+ paramLabel = "<realm,clientId,clientSecret>",
+ description =
+ "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
+ List<String> credentials;
+ }
+
+ static class FileInputOptions {
+ @CommandLine.Option(
+ names = {"-f", "--credentials-file"},
+ paramLabel = "<file>",
+ description = "A file containing root principal credentials to bootstrap.")
+ Path file;
+ }
+ }
@Override
public Integer call() {
- warnOnInMemory();
-
- PolarisCredentialsBootstrap credentialsBootstrap =
- credentials == null || credentials.isEmpty()
- ? PolarisCredentialsBootstrap.EMPTY
- : PolarisCredentialsBootstrap.fromList(credentials);
-
- // Execute the bootstrap
- Map<String, PrincipalSecretsResult> results =
- metaStoreManagerFactory.bootstrapRealms(realms, credentialsBootstrap);
-
- // Log any errors:
- boolean success = true;
- for (Map.Entry<String, PrincipalSecretsResult> result : results.entrySet()) {
- if (!result.getValue().isSuccess()) {
- String realm = result.getKey();
- spec.commandLine()
- .getErr()
- .printf(
- "Bootstrapping '%s' failed: %s%n",
- realm, result.getValue().getReturnStatus().toString());
- success = false;
+ try {
+ warnOnInMemory();
+
+ RootCredentialsSet rootCredentialsSet;
+ List<String> realms; // TODO Iterable
Review Comment:
Still `TODO`?
##########
polaris-core/src/test/java/org/apache/polaris/core/persistence/bootstrap/RootCredentialsSetTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.bootstrap;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.net.URL;
+import java.util.List;
+import org.junit.jupiter.api.Test;
+
+class RootCredentialsSetTest {
+
+ @Test
+ void nullString() {
+ RootCredentialsSet credentials = RootCredentialsSet.fromString(null);
+ assertThat(credentials.credentials()).isEmpty();
+ }
+
+ @Test
+ void emptyString() {
+ RootCredentialsSet credentials = RootCredentialsSet.fromString("");
+ assertThat(credentials.credentials()).isEmpty();
+ }
+
+ @Test
+ void blankString() {
+ RootCredentialsSet credentials = RootCredentialsSet.fromString(" ");
+ assertThat(credentials.credentials()).isEmpty();
+ }
+
+ @Test
+ void invalidString() {
+ assertThatThrownBy(() -> RootCredentialsSet.fromString("test"))
+ .hasMessage("Invalid credentials format: test");
+ }
+
+ @Test
+ void duplicateRealm() {
+ assertThatThrownBy(
+ () ->
+ RootCredentialsSet.fromString("realm1,client1a,secret1a;realm1,client1b,secret1b"))
+ .hasMessage("Duplicate realm: realm1");
+ }
+
+ @Test
+ void getSecretsValidString() {
+ RootCredentialsSet credentials =
+ RootCredentialsSet.fromString(
+ " ; realm1 , client1 , secret1 ; realm2 , client2 , secret2 ; ");
+ assertCredentials(credentials);
+ }
+
+ @Test
+ void getSecretsValidList() {
+ RootCredentialsSet credentials =
+ RootCredentialsSet.fromList(List.of("realm1,client1,secret1", "realm2,client2,secret2"));
+ assertCredentials(credentials);
+ }
+
+ @Test
+ void getSecretsValidSystemProperty() {
+ RootCredentialsSet credentials = RootCredentialsSet.fromEnvironment();
+ assertThat(credentials.credentials()).isEmpty();
+ try {
+ System.setProperty(
+ RootCredentialsSet.SYSTEM_PROPERTY, "realm1,client1,secret1;realm2,client2,secret2");
+ credentials = RootCredentialsSet.fromEnvironment();
+ assertCredentials(credentials);
+ } finally {
+ System.clearProperty(RootCredentialsSet.SYSTEM_PROPERTY);
+ }
+ }
+
+ @Test
+ void getSecretsValidJson() {
+ URL resource =
+ getClass().getResource("/org/apache/polaris/core/persistence/bootstrap/credentials.json");
Review Comment:
nit: I believe the path is not necessary here as the resource is in the same package as the class.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/RootCredentialsSet.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.bootstrap;
+
+import com.fasterxml.jackson.annotation.JsonAnyGetter;
+import com.fasterxml.jackson.annotation.JsonAnySetter;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nullable;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A utility to parse and provide credentials for Polaris realms and principals during a bootstrap
+ * phase.
+ */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableRootCredentialsSet.class)
+@JsonDeserialize(as = ImmutableRootCredentialsSet.class)
+@Value.Style(jdkOnly = true)
+public interface RootCredentialsSet {
+
+ RootCredentialsSet EMPTY = ImmutableRootCredentialsSet.builder().build();
+
+ String SYSTEM_PROPERTY = "polaris.bootstrap.credentials";
+ String ENVIRONMENT_VARIABLE = "POLARIS_BOOTSTRAP_CREDENTIALS";
+
+ /**
+ * Parse credentials from the system property {@value #SYSTEM_PROPERTY} or the environment
+ * variable {@value #ENVIRONMENT_VARIABLE}, whichever is set.
+ *
+ * <p>See {@link #fromString(String)} for the expected format.
+ */
+ static RootCredentialsSet fromEnvironment() {
+ return fromString(
+ System.getProperty(SYSTEM_PROPERTY, System.getenv().get(ENVIRONMENT_VARIABLE)));
+ }
+
+ /**
+ * Parse a string of credentials in the format:
+ *
+ * <pre>
+ * realm1,client1,secret1;realm2,client2,secret2;...
+ * </pre>
+ */
+ static RootCredentialsSet fromString(@Nullable String credentialsString) {
+ return credentialsString != null && !credentialsString.isBlank()
+ ? fromList(Splitter.on(';').trimResults().splitToList(credentialsString))
+ : EMPTY;
+ }
+
+ /**
+ * Parse a list of credentials; each element should be in the format: {@code
+ * realm,clientId,clientSecret}.
+ */
+ static RootCredentialsSet fromList(List<String> credentialsList) {
+ Map<String, RootCredentials> credentials = new HashMap<>();
+ for (String triplet : credentialsList) {
+ if (!triplet.isBlank()) {
+ List<String> parts = Splitter.on(',').trimResults().splitToList(triplet);
+ if (parts.size() != 3) {
+ throw new IllegalArgumentException("Invalid credentials format: " + triplet);
+ }
+ String realm = parts.get(0);
+ RootCredentials creds = ImmutableRootCredentials.of(parts.get(1), parts.get(2));
+ if (credentials.containsKey(realm)) {
+ throw new IllegalArgumentException("Duplicate realm: " + realm);
+ }
+ credentials.put(realm, creds);
+ }
+ }
+ return credentials.isEmpty() ? EMPTY : ImmutableRootCredentialsSet.of(credentials);
+ }
+
+ /**
+ * Parse credentials set from any URL containing a valid YAML or JSON credentials file.
+ *
+ * <p>The expected YAML format is:
+ *
+ * <pre>
+ * realm1:
+ * client-id: client1
+ * client-secret: secret1
+ * realm2:
+ * client-id: client2
+ * client-secret: secret2
+ * # etc.
+ * </pre>
+ *
+ * <p>The expected JSON format is:
+ *
+ * <pre>
+ * {
+ * "realm1": {
+ * "client-id": "client1",
+ * "client-secret": "secret1"
+ * },
+ * "realm2": {
+ * "client-id": "client2",
+ * "client-secret": "secret2"
+ * }
+ * }
+ * </pre>
+ */
+ static RootCredentialsSet fromUrl(URL url) {
+ try {
+ ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
+ return mapper.readValue(url, RootCredentialsSet.class);
Review Comment:
WDYT about making the mapper lenient so that it would be able to load files with extra attributes?
Cf. https://github.com/apache/polaris/pull/461#discussion_r1936527885
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1943566033
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
Ah, yes, I missed that use case :facepalm:
I was just trying to simplify the user experience for the manual case. WDYT about this:
```
--generate-secrets realm1 --generate-secrets realm2 --credentials realm2,id2,sec2 --credentials realm3,id3,sec3
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1943569687
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/RootCredentialsSet.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.bootstrap;
+
+import static com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES;
+
+import com.fasterxml.jackson.annotation.JsonAnyGetter;
+import com.fasterxml.jackson.annotation.JsonAnySetter;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
+import com.google.common.base.Splitter;
+import jakarta.annotation.Nullable;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/**
+ * A utility to parse and provide credentials for Polaris realms and principals during a bootstrap
+ * phase.
+ */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableRootCredentialsSet.class)
+@JsonDeserialize(as = ImmutableRootCredentialsSet.class)
+@Value.Style(jdkOnly = true)
+public interface RootCredentialsSet {
+
+ RootCredentialsSet EMPTY = ImmutableRootCredentialsSet.builder().build();
+
+ String SYSTEM_PROPERTY = "polaris.bootstrap.credentials";
+ String ENVIRONMENT_VARIABLE = "POLARIS_BOOTSTRAP_CREDENTIALS";
+
+ /**
+ * Parse credentials from the system property {@value #SYSTEM_PROPERTY} or the environment
+ * variable {@value #ENVIRONMENT_VARIABLE}, whichever is set.
+ *
+ * <p>See {@link #fromString(String)} for the expected format.
+ */
+ static RootCredentialsSet fromEnvironment() {
+ return fromString(
+ System.getProperty(SYSTEM_PROPERTY, System.getenv().get(ENVIRONMENT_VARIABLE)));
+ }
+
+ /**
+ * Parse a string of credentials in the format:
+ *
+ * <pre>
+ * realm1,client1,secret1;realm2,client2,secret2;...
+ * </pre>
+ */
+ static RootCredentialsSet fromString(@Nullable String credentialsString) {
+ return credentialsString != null && !credentialsString.isBlank()
+ ? fromList(Splitter.on(';').trimResults().splitToList(credentialsString))
+ : EMPTY;
+ }
+
+ /**
+ * Parse a list of credentials; each element should be in the format: {@code
+ * realm,clientId,clientSecret}.
+ */
+ static RootCredentialsSet fromList(List<String> credentialsList) {
+ Map<String, RootCredentials> credentials = new HashMap<>();
+ for (String triplet : credentialsList) {
+ if (!triplet.isBlank()) {
+ List<String> parts = Splitter.on(',').trimResults().splitToList(triplet);
+ if (parts.size() != 3) {
+ throw new IllegalArgumentException("Invalid credentials format: " + triplet);
+ }
+ String realm = parts.get(0);
+ RootCredentials creds = ImmutableRootCredentials.of(parts.get(1), parts.get(2));
+ if (credentials.containsKey(realm)) {
+ throw new IllegalArgumentException("Duplicate realm: " + realm);
+ }
+ credentials.put(realm, creds);
+ }
+ }
+ return credentials.isEmpty() ? EMPTY : ImmutableRootCredentialsSet.of(credentials);
+ }
+
+ /**
+ * Parse credentials set from any URL containing a valid YAML or JSON credentials file.
+ *
+ * <p>The expected YAML format is:
+ *
+ * <pre>
+ * realm1:
+ * client-id: client1
+ * client-secret: secret1
+ * realm2:
+ * client-id: client2
+ * client-secret: secret2
+ * # etc.
+ * </pre>
+ *
+ * Multiple YAMl documents are also supported; all documents will be merged into a single set of
+ * credentials.
+ *
+ * <p>The expected JSON format is:
+ *
+ * <pre>
+ * {
+ * "realm1": {
+ * "client-id": "client1",
+ * "client-secret": "secret1"
+ * },
+ * "realm2": {
+ * "client-id": "client2",
+ * "client-secret": "secret2"
+ * }
+ * }
+ * </pre>
+ */
+ static RootCredentialsSet fromUrl(URL url) {
+ YAMLFactory factory = new YAMLFactory();
+ ObjectMapper mapper = new ObjectMapper(factory).configure(FAIL_ON_UNKNOWN_PROPERTIES, false);
+ try (var parser = factory.createParser(url)) {
+ var values = mapper.readValues(parser, RootCredentialsSet.class);
+ var builder = ImmutableRootCredentialsSet.builder();
+ while (values.hasNext()) {
+ builder.putAllCredentials(values.next().credentials());
+ }
+ return builder.build();
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Failed to read credentials file: " + url, e);
+ }
+ }
+
+ @JsonAnyGetter
+ @JsonAnySetter
+ @Value.Parameter(order = 0)
+ Map<String, RootCredentials> credentials();
Review Comment:
nit: javadoc about map key being the realm ID
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1944878478
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
In the end, I think it's a minor point compared to the addition of reading secrets from files. We can always refactor that later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1942443578
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
+
+ @CommandLine.Option(
+ names = {"-c", "--credential"},
+ paramLabel = "<realm,clientId,clientSecret>",
+ description =
+ "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
+ List<String> credentials;
+ }
+
+ static class FileInputOptions {
+ @CommandLine.Option(
+ names = {"-f", "--credentials-file"},
+ paramLabel = "<file>",
+ description = "A file containing root principal credentials to bootstrap.")
+ Path file;
+ }
+ }
@Override
public Integer call() {
- warnOnInMemory();
-
- PolarisCredentialsBootstrap credentialsBootstrap =
- credentials == null || credentials.isEmpty()
- ? PolarisCredentialsBootstrap.EMPTY
- : PolarisCredentialsBootstrap.fromList(credentials);
-
- // Execute the bootstrap
- Map<String, PrincipalSecretsResult> results =
- metaStoreManagerFactory.bootstrapRealms(realms, credentialsBootstrap);
-
- // Log any errors:
- boolean success = true;
- for (Map.Entry<String, PrincipalSecretsResult> result : results.entrySet()) {
- if (!result.getValue().isSuccess()) {
- String realm = result.getKey();
- spec.commandLine()
- .getErr()
- .printf(
- "Bootstrapping '%s' failed: %s%n",
- realm, result.getValue().getReturnStatus().toString());
- success = false;
+ try {
+ warnOnInMemory();
+
+ RootCredentialsSet rootCredentialsSet;
+ List<String> realms; // TODO Iterable
Review Comment:
Yes, waiting for #889
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1942444984
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
The idea is that users may want to bootstrap a realm without providing credentials (they would be randomly-generated). Should I remove this option? It is true that the file-based approach does not permit randomly-generated credentials.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #940:
URL: https://github.com/apache/polaris/pull/940#issuecomment-2637881611
> [...] we need to standardize on what a "serialized" principal looks like across the APIs, the CLI, and this file format
I do not think establishing a standard for principal serialization should be a blocker for this PR. This change, IMHO, improves user experience already. If the future standard is different, we will fix the tool at the time the standard becomes effective.
Polaris is a young project without a release yet. Users should expect changes in file formats and CLI arguments are the project matures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #940:
URL: https://github.com/apache/polaris/pull/940
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #940:
URL: https://github.com/apache/polaris/pull/940#issuecomment-2634595668
cc @eric-maynard
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #940:
URL: https://github.com/apache/polaris/pull/940#issuecomment-2635708816
Conceptually I think this PR is great, but we need to standardize on what a "serialized" principal looks like across the APIs, the CLI, and this file format
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1943566033
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
Ah, yes, I missed that use case :facepalm:
I was just trying to simplify the user experience for the manual case. WDYT about `--generate-secrets realm1--generate-secrets realm2 --credentials realm2,id2,sec2 --credentials realm3,id3,sec3`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1943589579
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
I don't know, honestly... the more it goes, the more I'm in favor of requiring the user to provide all the credentials.
But your suggestion is at least coherent... I'm not opposed to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #940:
URL: https://github.com/apache/polaris/pull/940#issuecomment-2664393944
@dimas-b
> I do not think establishing a standard for principal serialization should be a blocker for this PR. This change, IMHO, improves user experience already. If the future standard is different, we will fix the tool at the time the standard becomes effective.
It's not necessarily about establishing a standard across the application, but at least within the CLI. #461 was already open proposing a JSON format for the CLI, so it seems odd that we should have merged a change to the CLI that accepts YAML without a consensus about the format to use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #940:
URL: https://github.com/apache/polaris/pull/940#discussion_r1941962994
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -30,51 +31,94 @@
description = "Bootstraps realms and root principal credentials.")
public class BootstrapCommand extends BaseCommand {
- @CommandLine.Option(
- names = {"-r", "--realm"},
- paramLabel = "<realm>",
- required = true,
- description = "The name of a realm to bootstrap.")
- List<String> realms;
+ @CommandLine.ArgGroup(multiplicity = "1")
+ InputOptions inputOptions;
- @CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
- description =
- "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
- List<String> credentials;
+ static class InputOptions {
+
+ @CommandLine.ArgGroup(multiplicity = "1", exclusive = false)
+ StandardInputOptions stdinOptions;
+
+ @CommandLine.ArgGroup(multiplicity = "1")
+ FileInputOptions fileOptions;
+
+ static class StandardInputOptions {
+
+ @CommandLine.Option(
+ names = {"-r", "--realm"},
+ paramLabel = "<realm>",
+ required = true,
+ description = "The name of a realm to bootstrap.")
+ List<String> realms;
Review Comment:
Do we still need this as a separate option? Why not extract after parsing the `-c` value?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ability to read root credentials from file when bootstrapping [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #940:
URL: https://github.com/apache/polaris/pull/940#issuecomment-2673524919
> [...] it seems odd that we should have merged a change to the CLI that accepts YAML without a consensus about the format to use.
I believe this PR did not have any impact on existing functionalities or use cases. So, I do not think this change is an "API" kind of change. Also, as far as I can tell the code in this PR is able to parse the JSON structures proposed in #461.
But point taken on reaching consensus. We can change the format of CLI inputs in #461 or another PR if you prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org