You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "dd-willgan (via GitHub)" <gi...@apache.org> on 2023/05/05 14:34:41 UTC
[GitHub] [pinot] dd-willgan opened a new pull request, #10726: [bugfix] Pinot config env substitution
dd-willgan opened a new pull request, #10726:
URL: https://github.com/apache/pinot/pull/10726
Fix environment variable substitution for specific configs in #10719
- Fix `access.control.init.password` in controller config
- Fix `controller.segment.fetcher.auth.token` in controller config
- Fix `pinot.server.segment.fetcher.auth.token`, `pinot.server.segment.uploader.auth.token`, and `pinot.server.instance.auth.token` in server config
Changes
- Change `String PinotConfiguration.getProperty(String name, String defaultValue)` to call `String CompositeConfiguration.getProperty(String name, String defaultValue)` (which performs interpolation)
- This will fix `access.control.init.password` since it uses this function
- https://github.com/dd-willgan/pinot/blob/ea75326da28ef8c4176b463296a5067b6cac2261/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java#L827-L829
- Change `Object CommonConfigurationUtils mapValue(String key, Configuration configuration)` to utilize the output of `CompositeConfiguration.getStringArray` for greater than 0 outputs instead of greater than 1
- This will fix all the `auth.token` configs, which are parsed by the AuthProviderUtils class https://github.com/dd-willgan/pinot/blob/ea75326da28ef8c4176b463296a5067b6cac2261/pinot-common/src/main/java/org/apache/pinot/common/auth/AuthProviderUtils.java#L54
- `toMap` eventually calls the `mapValue` function in question. As long as there 1 element in the output of `getStringArray` we can take the result and join with commas.
Tested
- Added a test case checking the behavior with system properties (also interpolated but easier to mock than environment variables)
- Built Pinot Docker image / deployed with Helm
- Verified that environment variable substitution was working for `access.control.init.password` as default user when using ZK-based controller auth was working, previously it wasn't
- Verified that the environment variable substitution was working for the auth tokens by creating a table, uploading a test segment to controller, and verifying that server was able to download the segment. Previously it wasn't since it didn't pass the right credentials when trying to download the segment from the controller
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #10726: [bugfix] Pinot config env substitution
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10726:
URL: https://github.com/apache/pinot/pull/10726#issuecomment-1562085205
:+1:
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] dd-willgan commented on pull request #10726: [bugfix] Pinot config env substitution
Posted by "dd-willgan (via GitHub)" <gi...@apache.org>.
dd-willgan commented on PR #10726:
URL: https://github.com/apache/pinot/pull/10726#issuecomment-1562079582
Closing since resolved in #10785, thanks @klsince and @walterddr
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10726: [bugfix] Pinot config env substitution
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10726:
URL: https://github.com/apache/pinot/pull/10726#discussion_r1199273342
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -356,15 +354,7 @@ public long getProperty(String name, long defaultValue) {
* @return the property String value. Fallback to default value if missing.
*/
public String getProperty(String name, String defaultValue) {
- Object rawProperty = getRawProperty(name, defaultValue);
- if (rawProperty instanceof List) {
- return StringUtils.join(((ArrayList) rawProperty).toArray(), ',');
- } else {
- if (rawProperty == null) {
- return null;
- }
- return rawProperty.toString();
- }
+ return _configuration.getString(relaxPropertyName(name), defaultValue);
Review Comment:
If I understood that mapValue() method correctly, using `> 1` was on purpose. Basically, if a config has multi string values, those values are combined to a comma separated string; otherwise, return the raw property.
After discussion with @dd-willgan, I'm testing some ideas here: https://github.com/apache/pinot/pull/10785, mainly to keep things backward compatible as much as possible. Let's see how that one goes.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr closed pull request #10726: [bugfix] Pinot config env substitution
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr closed pull request #10726: [bugfix] Pinot config env substitution
URL: https://github.com/apache/pinot/pull/10726
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10726: [bugfix] Pinot config env substitution
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10726:
URL: https://github.com/apache/pinot/pull/10726#discussion_r1199242904
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -356,15 +354,7 @@ public long getProperty(String name, long defaultValue) {
* @return the property String value. Fallback to default value if missing.
*/
public String getProperty(String name, String defaultValue) {
- Object rawProperty = getRawProperty(name, defaultValue);
- if (rawProperty instanceof List) {
- return StringUtils.join(((ArrayList) rawProperty).toArray(), ',');
- } else {
- if (rawProperty == null) {
- return null;
- }
- return rawProperty.toString();
- }
+ return _configuration.getString(relaxPropertyName(name), defaultValue);
Review Comment:
we should probably keep the original logic for this one? (maybe check null first)
i don't think this change is needed as long as the previous bug fix is in. please double check
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org