You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/06/22 10:39:19 UTC
[GitHub] [druid] bananaaggle opened a new pull request #11377: Add Environment Variable DynamicConfigProvider
bananaaggle opened a new pull request #11377:
URL: https://github.com/apache/druid/pull/11377
As [9351](https://github.com/apache/druid/issues/9351) described, DynamicConfigProvider should replace PasswordProvider. PasswordProvider has an implementation named EnvironmentVariablePasswordProvider, which allow users to read config from environment variable. This PR provides a same implementation for DynamicConfigProvider. There is an example for its usage:
```
"DynamicConfigProvider": {
"type": "environment",
{
"userNameVariable": "MY_USER_NAME_VAR",
"passwordVariable": "MY_PASSWORD_VAR"
}
}
```
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-893256818
> LGTM. Thanks @bananaaggle!
Thank you very much for review! In [11389](https://github.com/apache/druid/pull/11389), I make a design to replace PasswordProvider, but I'm not sure this design is proper or not. Can you help review it? If design is proper, I will involve more classes which use PasswordProvider in that PR. I also comment about it in [9351](https://github.com/apache/druid/issues/9351).
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-872131967
> @bananaaggle thanks for the quick response. The code change LGTM, but there are 2 things missing here, one is documentation and another is tests. For documentation, I think you can add some in `docs/operations/dynamic-config-provider.md`. For tests, you can add some unit tests in `EnvironmentVariableDynamicConfigProviderTest` that should run with a proper set of environment variables. To let Travis CI run those new tests successfully, you will need to add those environment variables in `.travis.yaml` too. I can help you with adding these, let me know if you have questions.
Hi, @jihoonson. I add unit test for getConfig(). To set environment variable, I find a method named `setEnv(Map<String, String> newenv)` in stackoverflow which can change jvm environment variable by reflection. Do you think it is a proper solution? Is necessary to add environment variables in `.travis.yaml`? If add environment variables in `.travis.yaml`, this unit test will work in travis, but users can not pass it without travis. What unit tests you think is necessary to be added in next step?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r659027125
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -22,21 +22,31 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableMap;
+import org.apache.druid.java.util.common.logger.Logger;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
-public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider<String>
{
- private final ImmutableMap<String, String> variables;
+
+ private static final Logger log = new Logger(EnvironmentVariableDynamicConfigProvider.class);
+
+ private ImmutableMap<String, String> variables;
@JsonCreator
public EnvironmentVariableDynamicConfigProvider(
@JsonProperty("variables") Map<String, String> config
)
{
- this.variables = ImmutableMap.copyOf(config);
+ try {
+ this.variables = ImmutableMap.copyOf(config);
+ }
+ catch (NullPointerException e) {
+ log.error(e, "Can not parse config by EnvironmentVariableDynamicConfigProvider! Please check your config file.");
+ throw e;
+ }
Review comment:
It would be nice to propagate the error to users so that they don't have to look up logs when this happens.
```suggestion
this.variables = ImmutableMap.copyOf(Preconditions.checkNotNull(config, "config"));
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #11377:
URL: https://github.com/apache/druid/pull/11377
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r667515646
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
Review comment:
This method seems to have a couple of issues.
1) This method seems to be copied from the code snippet in https://stackoverflow.com/a/7201825/4127682. If this is true, we cannot use it directly because it violates the [ASF license policy](https://www.apache.org/legal/resolved.html#stackoverflow). You can get some hint from the stack overflow, but should not copy from it.
2) As noted in the stack overflow link, the environment variables should be reset because they are shared by all tests run by the same JVM process.
3) It would be nice to add some comments to help others understand the code. I left some comments for this.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r662726773
##########
File path: docs/operations/dynamic-config-provider.md
##########
@@ -31,3 +31,16 @@ Users can create custom extension of the `DynamicConfigProvider` interface that
For more information, see [Adding a new DynamicConfigProvider implementation](../development/modules.md#adding-a-new-dynamicconfigprovider-implementation).
+## EnvironmentVariableDynamicConfigProvider
+
+`EnvironmentVariableDynamicConfigProvider` can be used to replace `EnvironmentVariablePasswordProvider`. This class allow users to avoid exposing passwords or other secret information in the runtime.properties file. You can set environment variables in the following example:
+```json
+druid.metadata.storage.connector.dynamicConfigProvider={"type": "environment","variables":{"user": "MY_USER_NAME_VAR","password": "MY_PASSWORD_VAR"}
Review comment:
Fixed. In [#11389](https://github.com/apache/druid/pull/11389), I make a design to replace `PasswordProvider`. I think it can work with metadata and other classes which use `PasswordProvider`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r658550753
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+{
+ private final ImmutableMap<String, String> variables;
+
+ @JsonCreator
+ public EnvironmentVariableDynamicConfigProvider(
+ @JsonProperty("variables") Map<String, String> config
+ )
+ {
+ this.variables = ImmutableMap.copyOf(config);
Review comment:
Fixed.
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
Review comment:
Fixed.
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+{
+ private final ImmutableMap<String, String> variables;
+
+ @JsonCreator
+ public EnvironmentVariableDynamicConfigProvider(
+ @JsonProperty("variables") Map<String, String> config
+ )
+ {
+ this.variables = ImmutableMap.copyOf(config);
+ }
+
+ @JsonProperty("variables")
+ public Map<String, String> getVariables()
+ {
+ return variables;
+ }
+
+ @Override
+ public Map getConfig()
Review comment:
Fixed.
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+{
+ private final ImmutableMap<String, String> variables;
+
+ @JsonCreator
+ public EnvironmentVariableDynamicConfigProvider(
+ @JsonProperty("variables") Map<String, String> config
+ )
+ {
+ this.variables = ImmutableMap.copyOf(config);
+ }
+
+ @JsonProperty("variables")
+ public Map<String, String> getVariables()
+ {
+ return variables;
+ }
+
+ @Override
+ public Map getConfig()
+ {
+ HashMap<String, String> map = new HashMap<>();
+ for (Map.Entry<String, String> entry : variables.entrySet()) {
+ map.put(entry.getKey(), System.getenv(entry.getValue()));
+ }
+ return map;
+ }
+
+ @Override
+ public String toString()
+ {
+ return "EnvironmentVariablePasswordProvider{" +
+ "variable='" + variables + '\'' +
+ '}';
+ }
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ EnvironmentVariableDynamicConfigProvider that = (EnvironmentVariableDynamicConfigProvider) o;
+
+ return Objects.equals(variables, that.variables);
+
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return variables != null ? variables.hashCode() : 0;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r662719166
##########
File path: docs/operations/dynamic-config-provider.md
##########
@@ -31,3 +31,16 @@ Users can create custom extension of the `DynamicConfigProvider` interface that
For more information, see [Adding a new DynamicConfigProvider implementation](../development/modules.md#adding-a-new-dynamicconfigprovider-implementation).
+## EnvironmentVariableDynamicConfigProvider
+
+`EnvironmentVariableDynamicConfigProvider` can be used to replace `EnvironmentVariablePasswordProvider`. This class allow users to avoid exposing passwords or other secret information in the runtime.properties file. You can set environment variables in the following example:
+```json
+druid.metadata.storage.connector.dynamicConfigProvider={"type": "environment","variables":{"user": "MY_USER_NAME_VAR","password": "MY_PASSWORD_VAR"}
Review comment:
We don't support dynamicConfig for metadata yet.
```suggestion
druid.some.config.dynamicConfigProvider={"type": "environment","variables":{"secret1": "SECRET1_VAR","secret2": "SECRET2_VAR"}
```
##########
File path: docs/operations/dynamic-config-provider.md
##########
@@ -31,3 +31,16 @@ Users can create custom extension of the `DynamicConfigProvider` interface that
For more information, see [Adding a new DynamicConfigProvider implementation](../development/modules.md#adding-a-new-dynamicconfigprovider-implementation).
+## EnvironmentVariableDynamicConfigProvider
+
+`EnvironmentVariableDynamicConfigProvider` can be used to replace `EnvironmentVariablePasswordProvider`. This class allow users to avoid exposing passwords or other secret information in the runtime.properties file. You can set environment variables in the following example:
Review comment:
```suggestion
`EnvironmentVariableDynamicConfigProvider` can be used to avoid exposing credentials or other secret information in the configuration files using environment variables. An example to use this configProvider is:
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r663287224
##########
File path: docs/operations/dynamic-config-provider.md
##########
@@ -31,3 +31,16 @@ Users can create custom extension of the `DynamicConfigProvider` interface that
For more information, see [Adding a new DynamicConfigProvider implementation](../development/modules.md#adding-a-new-dynamicconfigprovider-implementation).
+## EnvironmentVariableDynamicConfigProvider
+
+`EnvironmentVariableDynamicConfigProvider` can be used to replace `EnvironmentVariablePasswordProvider`. This class allow users to avoid exposing passwords or other secret information in the runtime.properties file. You can set environment variables in the following example:
+```json
+druid.metadata.storage.connector.dynamicConfigProvider={"type": "environment","variables":{"user": "MY_USER_NAME_VAR","password": "MY_PASSWORD_VAR"}
Review comment:
Cool, I will take a look at that PR too. Thanks!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r667514756
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
+ {
+ try {
+ Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment");
+ Field theEnvironmentField = processEnvironmentClass.getDeclaredField("theEnvironment");
+ theEnvironmentField.setAccessible(true);
+ Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null);
+ env.putAll(newenv);
+ Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment");
Review comment:
What is this `theCaseInsensitiveEnvironment` variable? I don't see it in `java.lang.ProcessEnvironment`.
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
+ {
+ try {
+ Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment");
+ Field theEnvironmentField = processEnvironmentClass.getDeclaredField("theEnvironment");
+ theEnvironmentField.setAccessible(true);
+ Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null);
+ env.putAll(newenv);
+ Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment");
+ theCaseInsensitiveEnvironmentField.setAccessible(true);
+ Map<String, String> cienv = (Map<String, String>) theCaseInsensitiveEnvironmentField.get(null);
+ cienv.putAll(newenv);
+ }
+ catch (NoSuchFieldException e) {
+ Class[] classes = Collections.class.getDeclaredClasses();
+ Map<String, String> env = System.getenv();
+ for (Class cl : classes) {
+ if ("java.util.Collections$UnmodifiableMap".equals(cl.getName())) {
+ Field field = cl.getDeclaredField("m");
+ field.setAccessible(true);
+ Object obj = field.get(env);
+ Map<String, String> map = (Map<String, String>) obj;
+ map.clear();
+ map.putAll(newenv);
+ }
+ }
+ }
+ }
Review comment:
Can you elaborate more on what this catch clause is doing?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-871093069
> @bananaaggle thanks for the quick response. The code change LGTM, but there are 2 things missing here, one is documentation and another is tests. For documentation, I think you can add some in `docs/operations/dynamic-config-provider.md`. For tests, you can add some unit tests in `EnvironmentVariableDynamicConfigProviderTest` that should run with a proper set of environment variables. To let Travis CI run those new tests successfully, you will need to add those environment variables in `.travis.yaml` too. I can help you with adding these, let me know if you have questions.
Document added. I'll add more tests in week.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-883904988
Hi @jihoonson! I've changed this unit test but CI tests failed. It seems caused by travis error, can you help me rerun it? And is there something in code needed to change? Thanks!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r659118211
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import org.apache.druid.java.util.common.logger.Logger;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider<String>
+{
+
+ private static final Logger log = new Logger(EnvironmentVariableDynamicConfigProvider.class);
+
+ private ImmutableMap<String, String> variables;
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-892705485
@bananaaggle sorry, I forgot about this PR. I restarted the timed-out tests. Will finish my review today.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r659087450
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+import org.apache.druid.java.util.common.logger.Logger;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider<String>
+{
+
+ private static final Logger log = new Logger(EnvironmentVariableDynamicConfigProvider.class);
+
+ private ImmutableMap<String, String> variables;
Review comment:
nit: this can be final
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r658507902
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+{
+ private final ImmutableMap<String, String> variables;
+
+ @JsonCreator
+ public EnvironmentVariableDynamicConfigProvider(
+ @JsonProperty("variables") Map<String, String> config
+ )
+ {
+ this.variables = ImmutableMap.copyOf(config);
Review comment:
`config` can be null if there is something wrong in the configuration. This will throw NPE in that case. It would be better to tell what is wrong than NPE with no message.
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+{
+ private final ImmutableMap<String, String> variables;
+
+ @JsonCreator
+ public EnvironmentVariableDynamicConfigProvider(
+ @JsonProperty("variables") Map<String, String> config
+ )
+ {
+ this.variables = ImmutableMap.copyOf(config);
+ }
+
+ @JsonProperty("variables")
+ public Map<String, String> getVariables()
+ {
+ return variables;
+ }
+
+ @Override
+ public Map getConfig()
+ {
+ HashMap<String, String> map = new HashMap<>();
+ for (Map.Entry<String, String> entry : variables.entrySet()) {
+ map.put(entry.getKey(), System.getenv(entry.getValue()));
+ }
+ return map;
+ }
+
+ @Override
+ public String toString()
+ {
+ return "EnvironmentVariablePasswordProvider{" +
+ "variable='" + variables + '\'' +
+ '}';
+ }
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ EnvironmentVariableDynamicConfigProvider that = (EnvironmentVariableDynamicConfigProvider) o;
+
+ return Objects.equals(variables, that.variables);
+
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return variables != null ? variables.hashCode() : 0;
Review comment:
`variables` doesn't seem to be able to null.
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
Review comment:
```suggestion
public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider<String>
```
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+{
+ private final ImmutableMap<String, String> variables;
+
+ @JsonCreator
+ public EnvironmentVariableDynamicConfigProvider(
+ @JsonProperty("variables") Map<String, String> config
+ )
+ {
+ this.variables = ImmutableMap.copyOf(config);
+ }
+
+ @JsonProperty("variables")
+ public Map<String, String> getVariables()
+ {
+ return variables;
+ }
+
+ @Override
+ public Map getConfig()
Review comment:
```suggestion
public Map<String> getConfig()
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r667859031
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
+ {
+ try {
+ Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment");
+ Field theEnvironmentField = processEnvironmentClass.getDeclaredField("theEnvironment");
+ theEnvironmentField.setAccessible(true);
+ Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null);
+ env.putAll(newenv);
+ Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment");
Review comment:
Fixed.
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
+ {
+ try {
+ Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment");
+ Field theEnvironmentField = processEnvironmentClass.getDeclaredField("theEnvironment");
+ theEnvironmentField.setAccessible(true);
+ Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null);
+ env.putAll(newenv);
+ Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment");
+ theCaseInsensitiveEnvironmentField.setAccessible(true);
+ Map<String, String> cienv = (Map<String, String>) theCaseInsensitiveEnvironmentField.get(null);
+ cienv.putAll(newenv);
+ }
+ catch (NoSuchFieldException e) {
+ Class[] classes = Collections.class.getDeclaredClasses();
+ Map<String, String> env = System.getenv();
+ for (Class cl : classes) {
+ if ("java.util.Collections$UnmodifiableMap".equals(cl.getName())) {
+ Field field = cl.getDeclaredField("m");
+ field.setAccessible(true);
+ Object obj = field.get(env);
+ Map<String, String> map = (Map<String, String>) obj;
+ map.clear();
+ map.putAll(newenv);
+ }
+ }
+ }
+ }
Review comment:
Fixed.
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
Review comment:
I've changed this unit test. Separated setting and getting environment variables. Add a method to recover system environment after test. Add comment for `getENVMap()`.
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
Review comment:
I've changed this unit test. Separated setting and getting environment variables. I use a map to record which environment variable is changed and add a method to recover system environment after test. Add comment for `getENVMap()`.
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
Review comment:
I've changed this unit test. Separated setting and getting environment variables. I use a map to record which environment variables are changed and add a method to recover those system environment after test. Add comment for `getENVMap()`.
##########
File path: core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.druid.metadata;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Map;
+
+public class EnvironmentVariableDynamicConfigProviderTest
+{
+ private static final ObjectMapper JSON_MAPPER = new ObjectMapper();
+
+ @Before
+ public void setupTest() throws Exception
+ {
+ setEnv(ImmutableMap.of("DRUID_USER", "druid", "DRUID_PASSWORD", "123"));
+ }
+
+ @Test
+ public void testSerde() throws IOException
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"testKey\":\"testValue\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("testValue", ((EnvironmentVariableDynamicConfigProvider) provider).getVariables().get("testKey"));
+ DynamicConfigProvider serde = JSON_MAPPER.readValue(JSON_MAPPER.writeValueAsString(provider), DynamicConfigProvider.class);
+ Assert.assertEquals(provider, serde);
+ }
+
+ @Test
+ public void testGetConfig() throws Exception
+ {
+ String providerString = "{\"type\": \"environment\", \"variables\" : {\"user\":\"DRUID_USER\",\"password\":\"DRUID_PASSWORD\"}}";
+ DynamicConfigProvider provider = JSON_MAPPER.readValue(providerString, DynamicConfigProvider.class);
+ Assert.assertTrue(provider instanceof EnvironmentVariableDynamicConfigProvider);
+ Assert.assertEquals("druid", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("user"));
+ Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+ }
+
+ protected static void setEnv(Map<String, String> newenv) throws Exception
Review comment:
I've changed this unit test. Separated setting and getting environment variables. I use a map to record which environment variables are changed and add a method to recover those system environment variables after test. Add comment for `getENVMap()`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-892705485
@bananaaggle sorry, I forgot about this PR. I restarted the timed-out tests. Will finish my review today.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-871056316
@bananaaggle thanks for the quick response. The code change LGTM, but there are 2 things missing here, one is documentation and another is tests. For documentation, I think you can add some in `docs/operations/dynamic-config-provider.md`. For tests, you can add some unit tests in `EnvironmentVariableDynamicConfigProviderTest` that should run with a proper set of environment variables. To let Travis CI run those new tests successfully, you will need to add those environment variables in `.travis.yaml` too. I can help you with adding these, let me know if you have questions.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11377:
URL: https://github.com/apache/druid/pull/11377#issuecomment-893256818
> LGTM. Thanks @bananaaggle!
Thank you very much for review! In [11389](https://github.com/apache/druid/pull/11389), I make a design to replace PasswordProvider, but I'm not sure this design is proper or not. Can you help review it? If design is proper, I will involve more classes which use PasswordProvider in that PR. I also comment about it in [9351](https://github.com/apache/druid/issues/9351).
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #11377:
URL: https://github.com/apache/druid/pull/11377
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r662726028
##########
File path: docs/operations/dynamic-config-provider.md
##########
@@ -31,3 +31,16 @@ Users can create custom extension of the `DynamicConfigProvider` interface that
For more information, see [Adding a new DynamicConfigProvider implementation](../development/modules.md#adding-a-new-dynamicconfigprovider-implementation).
+## EnvironmentVariableDynamicConfigProvider
+
+`EnvironmentVariableDynamicConfigProvider` can be used to replace `EnvironmentVariablePasswordProvider`. This class allow users to avoid exposing passwords or other secret information in the runtime.properties file. You can set environment variables in the following example:
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11377: Add Environment Variable DynamicConfigProvider
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11377:
URL: https://github.com/apache/druid/pull/11377#discussion_r659118254
##########
File path: core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java
##########
@@ -22,21 +22,31 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableMap;
+import org.apache.druid.java.util.common.logger.Logger;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
-public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
+public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider<String>
{
- private final ImmutableMap<String, String> variables;
+
+ private static final Logger log = new Logger(EnvironmentVariableDynamicConfigProvider.class);
+
+ private ImmutableMap<String, String> variables;
@JsonCreator
public EnvironmentVariableDynamicConfigProvider(
@JsonProperty("variables") Map<String, String> config
)
{
- this.variables = ImmutableMap.copyOf(config);
+ try {
+ this.variables = ImmutableMap.copyOf(config);
+ }
+ catch (NullPointerException e) {
+ log.error(e, "Can not parse config by EnvironmentVariableDynamicConfigProvider! Please check your config file.");
+ throw e;
+ }
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org