You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/10 22:54:06 UTC
[GitHub] [iceberg] rdblue opened a new pull request, #5018: Core: Resolve environment variables in REST catalog config
rdblue opened a new pull request, #5018:
URL: https://github.com/apache/iceberg/pull/5018
The REST catalog has support for access tokens that are used with Bearer authentication. To help protect these access tokens, some deployments may prefer to set them from environment variables. This adds support for resolving REST catalog configuration values using the JVM environment (`System.getenv()`). If a catalog config value is `env:SOME_VAR`, the value will be replaced with the value of the environment variable `SOME_VAR`. If the environment variable is missing, the value will be null.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5018: Core: Resolve environment variables in REST catalog config
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5018:
URL: https://github.com/apache/iceberg/pull/5018#discussion_r894942521
##########
core/src/main/java/org/apache/iceberg/util/EnvironmentUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed 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.
+ */
+
+/*
+ * Licensed 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.iceberg.util;
+
+import java.util.Collections;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+public class EnvironmentUtil {
+ private EnvironmentUtil() {
+ }
+
+ private static final String ENVIRONMENT_VARIABLE_PREFIX = "env:";
+
+ public static Map<String, String> resolveAll(Map<String, String> properties) {
+ return resolveAll(System.getenv(), properties);
+ }
+
+ @VisibleForTesting
+ static Map<String, String> resolveAll(Map<String, String> env, Map<String, String> properties) {
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((name, value) -> {
+ if (value != null && value.startsWith(ENVIRONMENT_VARIABLE_PREFIX)) {
+ result.put(name, env.get(value.substring(ENVIRONMENT_VARIABLE_PREFIX.length())));
+ } else {
+ result.put(name, value);
+ }
+ });
+
+ return Collections.unmodifiableMap(result);
Review Comment:
We currently build the property maps as immutable maps in several places. And that brings up an issue where if this produces a null key, those areas will fail. Instead I'm going to change this so that missing environment variables result in not adding anything to the map, and will switch to an `ImmutableMap` builder.
##########
core/src/main/java/org/apache/iceberg/util/EnvironmentUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed 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.
+ */
+
+/*
Review Comment:
Fixed.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5018: Core: Resolve environment variables in REST catalog config
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5018:
URL: https://github.com/apache/iceberg/pull/5018
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #5018: Core: Resolve environment variables in REST catalog config
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #5018:
URL: https://github.com/apache/iceberg/pull/5018#discussion_r894935276
##########
core/src/main/java/org/apache/iceberg/util/EnvironmentUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed 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.
+ */
+
+/*
+ * Licensed 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.iceberg.util;
+
+import java.util.Collections;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+public class EnvironmentUtil {
+ private EnvironmentUtil() {
+ }
+
+ private static final String ENVIRONMENT_VARIABLE_PREFIX = "env:";
+
+ public static Map<String, String> resolveAll(Map<String, String> properties) {
+ return resolveAll(System.getenv(), properties);
+ }
+
+ @VisibleForTesting
+ static Map<String, String> resolveAll(Map<String, String> env, Map<String, String> properties) {
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((name, value) -> {
+ if (value != null && value.startsWith(ENVIRONMENT_VARIABLE_PREFIX)) {
+ result.put(name, env.get(value.substring(ENVIRONMENT_VARIABLE_PREFIX.length())));
+ } else {
+ result.put(name, value);
+ }
+ });
+
+ return Collections.unmodifiableMap(result);
Review Comment:
I don't know that we modify the property map anywhere in the RESTCatalog, but this wasn't a requirement perviously.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #5018: Core: Resolve environment variables in REST catalog config
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #5018:
URL: https://github.com/apache/iceberg/pull/5018#discussion_r894932618
##########
core/src/main/java/org/apache/iceberg/util/EnvironmentUtil.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed 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.
+ */
+
+/*
Review Comment:
Double license headers
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #5018: Core: Resolve environment variables in REST catalog config
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5018:
URL: https://github.com/apache/iceberg/pull/5018#discussion_r895095138
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -109,8 +110,11 @@ public RESTSessionCatalog() {
}
@Override
- public void initialize(String name, Map<String, String> props) {
- Preconditions.checkArgument(props != null, "Invalid configuration: null");
+ public void initialize(String name, Map<String, String> unresolved) {
Review Comment:
Nit / non-blocking: What about `unresolvedProps` or maybe the standard `props` and then using `fullProps` or some combination?
I don't love my ideas, but `unresolved` by itself is somewhat confusing for me. It initially reads as somehow more special than just the `properties` map (be that with or without env variable resolution applied).
By the interface, it is `props` so I'm ok with it, but the name itself is a little misleading when reading it imo.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #5018: Core: Resolve environment variables in REST catalog config
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5018:
URL: https://github.com/apache/iceberg/pull/5018#discussion_r895095138
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -109,8 +110,11 @@ public RESTSessionCatalog() {
}
@Override
- public void initialize(String name, Map<String, String> props) {
- Preconditions.checkArgument(props != null, "Invalid configuration: null");
+ public void initialize(String name, Map<String, String> unresolved) {
Review Comment:
Nit / non-blocking: What about `unresolvedProps` or maybe the standard `props` and then using `fullProps` or some combination?
I don't love my ideas, but `unresolved` by itself is somewhat confusing for me. It initially reads as somehow more special than just the `properties` map (with or without env variable resolution applied).
By the interface, it is `props` so I'm ok with it, but the name itself is a little misleading when reading it imo.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org