Enable Support to get DB Username/Password from enivonment variable for jberet.properties #367 #368
Conversation
|
@lokeshnaik808 This looks good to me @chengfang Do we need to ask the contributor to agree to the license? Here is an example: resteasy/resteasy-examples#53 (comment) |
| @@ -146,11 +146,23 @@ public JdbcRepository(final Properties configProperties) { | |||
| this.dbUrl = dbUrl; | |||
| final String dbUser = configProperties.getProperty(DB_USER_KEY); | |||
| if (dbUser != null) { | |||
| dbProperties.setProperty("user", dbUser.trim()); | |||
There was a problem hiding this comment.
jberet.properties configuration only applies to jberet-se, so it will be good to limit this change to jberet-se. For instance, we can contain this changes in BatchSEEnvironment right after loading the properties. This also has the benefit of getting the right values as early as possible.
db config values are also used by mongo job repository.
| Environment variable name given (eg. ENV_VAR_NAME from example) if it exists or | ||
| the default value (e.g default_value from example) | ||
| */ | ||
| private String parseValuefromEnvVariables(String rawValue){ |
| @@ -146,11 +146,23 @@ public JdbcRepository(final Properties configProperties) { | |||
| this.dbUrl = dbUrl; | |||
| final String dbUser = configProperties.getProperty(DB_USER_KEY); | |||
| if (dbUser != null) { | |||
| dbProperties.setProperty("user", dbUser.trim()); | |||
| if (dbUser.startsWith("${")){ | |||
| final String dbPasswordFromEnvVar = parseValuefromEnvVariables(dbUser); | |||
There was a problem hiding this comment.
this variable name seems wrong. dbUserFrom...?
| } | ||
| else { | ||
| dbProperties.setProperty("password", dbPassword.trim()); | ||
| } |
There was a problem hiding this comment.
I think the check should be a bit stricter whether it's a clear text value or expression. For example, ${xxx can be a perfectly valid clear-text password, not an expression.
We could check if it follows the pattern of ${.*}, meaning it should start with ${ and end with } with some chars in between.
There was a problem hiding this comment.
we should have some unit tests to verify parsing of various forms of values.
| String[] valueList = rawValue.split(":",2); | ||
| String valueFromEnvVariable = System.getenv(valueList[0].replace("${","").trim()); | ||
| if (valueFromEnvVariable == null){ | ||
| return valueList[1].replace("}","").trim(); |
There was a problem hiding this comment.
val[1] can be non-existent, as in ${abc}. so check for vals length is needed to avoid ArrayOutOfBoundException
There was a problem hiding this comment.
getenv() requires RuntimePermission and could throw SecurityException, so we should instead use org.wildfly.security.manager.WildFlySecurityManager#getEnvPropertyPrivileged
|
@lokeshnaik808 Are there and updates to this PR? |
Yes, I could not work on it because of my other commitments, but I am working on it, I shall check in the updates soon. |
|
@lokeshnaik808 Thanks for the update! |
|
The Work is still under Progress, there is still some refactoring around test to do |
|
@lokeshnaik808 thanks for working on that. Do we really need powermock in test? If possible, I'd like to avoid adding additional dependency. I think the basic tests we can have are some unit-level tests, verifying that parsing the value and expression works properly. And optionally, we can configure some env variables in maven surefire plugin configuration, and make sure they are picked up. The relevant methods in jberet-se impl code can be package private, which can then be accessed by the test that is in the same java package. |
|
Powermock was used to mock the behaviour of method getEnvPropertyPrivileged because I was not able to test this function (I am not able to se mock env variable for testing). I added some Surefire variable in pom.xml file but that does not seem to work. Can you suggest what I can do in this case? |
|
@lokeshnaik808 Thanks for the update! I'll find some time to check this by this weekend. |
|
this week I'm on holiday and will check this after coming back. |
|
@lokeshnaik808 Sorry for the deploy, I'm just back from PTO and looking into other issues, I'll come back to check this. |
liweinan
force-pushed
the
add_env_variable_parsing_capability_jdbc
branch
from
7a7675b to
22e82eb
Compare
|
Sorry I was busy on some other higher priority tasks, I'll come back to this issue. |
|
I need to do a release to include #381 and put this issue into next release cycle. |
|
Sorry for the delay on processing this PR. I have just finished working on other tasks and I'll start to work on this. |
liweinan
changed the title
|
@lokeshnaik808 I have added the feature with slightly different approach and it's approved by @chengfang. Please let me know if there are any usage problems. |
This code creates the capability for jberet to read environment variables for dbUser and db-password when given as ${ENV_VAR_NAME:default_value}. If the value is given as plain text, then this value will be used as was being used before