New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add Environment Variable DynamicConfigProvider #11377

Merged

jihoonson merged 14 commits into apache:master from bananaaggle:add_environment_variable_DynamicConfigProvider

Aug 5, 2021

Contributor

bananaaggle commented Jun 22, 2021 •

edited

Loading

As #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",
  "variables":
         {
            "userNameVariable": "MY_USER_NAME_VAR",
            "passwordVariable": "MY_PASSWORD_VAR"
         }
}

This PR has:

been self-reviewed.
- using the concurrency checklist (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
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 is met.
added integration tests.
been tested in a test Druid cluster.


           add_environment_variable_DynamicConfigProvider

a95949b

bananaaggle mentioned this pull request

Adding CredentialsProvider, deprecating PasswordProvider #9351

Open

jihoonson reviewed

View reviewed changes

Contributor

jihoonson left a comment

@bananaaggle thanks for the PR. I left some comments.

core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java Outdated

+                    @JsonProperty("variables") Map<String, String> config
+                )
+                {
+                  this.variables = ImmutableMap.copyOf(config);

Contributor

jihoonson Jun 25, 2021

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.

Contributor Author

bananaaggle Jun 25, 2021

Fixed.

core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java Outdated

+              import java.util.Map;
+              import java.util.Objects;
+              public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider

Contributor

jihoonson Jun 25, 2021

Suggested change

      
            public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider
          
            public class EnvironmentVariableDynamicConfigProvider implements DynamicConfigProvider<String>

Contributor Author

bananaaggle Jun 25, 2021

Fixed.

core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java Outdated

+                }
+                @Override
+                public Map getConfig()

Contributor

jihoonson Jun 25, 2021

Suggested change

      
              public Map getConfig()
          
              public Map<String> getConfig()

Contributor Author

bananaaggle Jun 25, 2021

Fixed.

core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java Outdated

+                @Override
+                public int hashCode()
+                {
+                  return variables != null ? variables.hashCode() : 0;

Contributor

jihoonson Jun 25, 2021

variables doesn't seem to be able to null.

Contributor Author

bananaaggle Jun 25, 2021

Fixed.


          fix code

72a8a32

jihoonson reviewed

View reviewed changes

core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java Outdated

Comment on lines 43 to 49

+                  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;
+                  }

Contributor

jihoonson Jun 25, 2021

It would be nice to propagate the error to users so that they don't have to look up logs when this happens.

Suggested change

      
                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;
          
                }
          
                this.variables = ImmutableMap.copyOf(Preconditions.checkNotNull(config, "config"));

Contributor Author

bananaaggle Jun 26, 2021

fixed

clintropolis added the Area - Operations label

clintropolis reviewed

View reviewed changes

Member

clintropolis left a comment

👍

core/src/main/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProvider.java Outdated


		private static final Logger log = new Logger(EnvironmentVariableDynamicConfigProvider.class);

		private ImmutableMap<String, String> variables;

Member

clintropolis Jun 25, 2021

nit: this can be final

Contributor Author

bananaaggle Jun 26, 2021

fixed

yuanyi added 2 commits

June 26, 2021 12:35


          code fixed

a6f2042


          code fixed

02a4397

Contributor

jihoonson commented Jun 30, 2021

@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.


          add document

7b0f972

Contributor Author

bananaaggle commented Jun 30, 2021

@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.

yuanyi added 4 commits

June 30, 2021 12:50


          fix doc

5f297c7


          fix doc

18c22f1


          add more unit test

60fdd53


          fix style

cc774cc

Contributor Author

bananaaggle commented Jul 1, 2021

@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?

jihoonson reviewed

View reviewed changes

Contributor

jihoonson left a comment

@bananaaggle thank you for adding a documentation! I left some comments on it.

For unit tests, I was thinking of some tests that should run with a set of certain environment variables. So, for Travis, those variables should be set in .travis.yaml. To run those tests locally, those variables should be set and passed properly to the process that runs the tests. I thought this would be the easiest way to add tests, but if you think there is a better way, I'm OK with it as long as those tests are not flaky.

docs/operations/dynamic-config-provider.md Outdated

+              `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"}

Contributor

jihoonson Jul 2, 2021

We don't support dynamicConfig for metadata yet.

Suggested change

      
            druid.metadata.storage.connector.dynamicConfigProvider={"type": "environment","variables":{"user": "MY_USER_NAME_VAR","password": "MY_PASSWORD_VAR"}
          
            druid.some.config.dynamicConfigProvider={"type": "environment","variables":{"secret1": "SECRET1_VAR","secret2": "SECRET2_VAR"}

Contributor Author

bananaaggle Jul 2, 2021

Fixed. In #11389, I make a design to replace PasswordProvider. I think it can work with metadata and other classes which use PasswordProvider.

Contributor

jihoonson Jul 2, 2021

Cool, I will take a look at that PR too. Thanks!

docs/operations/dynamic-config-provider.md Outdated

               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:

Contributor

jihoonson Jul 2, 2021

Suggested change

      
            `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:
          
            `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:

Contributor Author

bananaaggle Jul 2, 2021

Fixed

yuanyi added 2 commits

July 2, 2021 12:20


          fix document

6d352bc


          bug fixed

13d62fe

jihoonson reviewed

View reviewed changes

Contributor

jihoonson left a comment

Hey @bananaaggle, thank you for adding unit tests. I have some concerns about the new code added. Please see my comments.

core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java Outdated

+                    theEnvironmentField.setAccessible(true);
+                    Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null);
+                    env.putAll(newenv);
+                    Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment");

Contributor

jihoonson Jul 11, 2021

What is this theCaseInsensitiveEnvironment variable? I don't see it in java.lang.ProcessEnvironment.

Contributor Author

bananaaggle Jul 12, 2021

Fixed.

core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java Outdated

Comment on lines 77 to 91

+                  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);
+                      }
+                    }
+                  }
+                }

Contributor

jihoonson Jul 11, 2021

Can you elaborate more on what this catch clause is doing?

Contributor Author

bananaaggle Jul 12, 2021

Fixed.

jihoonson reviewed

View reviewed changes

core/src/test/java/org/apache/druid/metadata/EnvironmentVariableDynamicConfigProviderTest.java Outdated

+                  Assert.assertEquals("123", ((EnvironmentVariableDynamicConfigProvider) provider).getConfig().get("password"));
+                }
+                protected static void setEnv(Map<String, String> newenv) throws Exception

Contributor

jihoonson Jul 11, 2021

This method seems to have a couple of issues.

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. You can get some hint from the stack overflow, but should not copy from it.
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.
It would be nice to add some comments to help others understand the code. I left some comments for this.

Contributor Author

bananaaggle Jul 12, 2021 •

edited

Loading

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().

yuanyi added 3 commits

July 12, 2021 19:44


          fix unit test

b32d3e0


          fix comment

47c7f44


          fix test

86be099

Contributor Author

bananaaggle commented Jul 21, 2021

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!

Contributor

jihoonson commented Aug 4, 2021

@bananaaggle sorry, I forgot about this PR. I restarted the timed-out tests. Will finish my review today.

jihoonson approved these changes

View reviewed changes

Contributor

jihoonson left a comment

LGTM. Thanks @bananaaggle!

jihoonson merged commit 23d7d71 into apache:master

Contributor Author

bananaaggle commented Aug 5, 2021

LGTM. Thanks @bananaaggle!

Thank you very much for review! In 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.

clintropolis added this to the 0.22.0 milestone

clintropolis mentioned this pull request

[Draft] 0.22.0 Release Notes #11657

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment