Pam Config

[blitzkrieg393]

Hi Guys! I am thinking about changing PamConfig type for more definite, with concrete structure https://github.com/valkyrie-fnd/valkyrie/blob/main/configs/valkyrie_config.go#L92. Because of current PamConfig has type any for config values, environment variables cannot be resolved inside of config subsections, so it leads to some difficulties on parsing config via reflection. I think it would be better to use concrete config structure instead of using type any for config values. What do think about such suggestion?

[gnirb]

Hello @blitzkrieg393!

The PamConfig was intentionally left generic to support custom configuration of external PamClient implementations that use the module system vplugin (https://valkyrie.bet/docs/wallet/vplugin/vplugin-introduction).

Environment variables should however still be resolved in this config section, it is verified in the following test https://github.com/valkyrie-fnd/valkyrie/blob/main/configs/valkyrie_config_test.go#L152.

So it is the PamClient own responsibility to know what values to expect in PamConfig and parse them. In our example generic pam client we use the mapstruct library to parse this generic config in a convenient way, perhaps that can be used in your case too? https://github.com/valkyrie-fnd/valkyrie/blob/main/pam/genericpam/generic_client.go#L43

Hope this helps and feel free to reach out again if more questions arise.

[blitzkrieg393]

Thank you for the answer! There might be a misunderstanding due to my English)
Environment variables at the first level resolve normally ($FIRST_ENV in the example), but for nested configuration sections (inside 'projects' section in the example), I use the MapStruct library and send the resulting structure to the expandEnvVariables function again. This is why I had to make this function exportable and use it in different parts of the project, including this place: https://github.com/valkyrie-fnd/valkyrie/blob/main/pam/genericpam/generic_client.go#L43. But this, in my opinion, is a bad solution and it would be better to do it in the expandEnvVariables function: https://github.com/valkyrie-fnd/valkyrie/blob/main/configs/valkyrie_config.go#L166, or use a strict description of the configuration structure from the beginning, as i suggested before. I understand why you did it that way, but it's very inconvenient. If you agree with my opinion, then I can try to solve this and make a MR..
Config exmaple:

pam:
  name: some_pam
  env: $FIRST_ENV
  projects:
    - name: p1
      env: $SECOND_ENV
...

[gnirb]

Aha, I understand your problem now. Yes, expandEnvVariables should definitely support resolving nested environment variables too. A PR solving your issue would be great! Please include a unit test with your sample config too while you're at it. 👍