[bugfix] Pinot config env substitution

[dd-willgan]

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

[walterddr]

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

[klsince]

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: #10785, mainly to keep things backward compatible as much as possible. Let's see how that one goes.

[walterddr]

lgtm mostly. good catch

[dd-willgan]

Closing since resolved in #10785, thanks @klsince and @walterddr

[walterddr]

👍