Skip to content
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.

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

feat: Support for environment variables in config.yaml #422

Draft
wants to merge 3 commits into
base: 3.12
Choose a base branch
from

Conversation

JeremyEastham
Copy link
Contributor

@JeremyEastham JeremyEastham commented Apr 11, 2022

Summary of Change

This PR adds support for environment variables in config.yaml. This implementation allows environment variables to be specified with ${ENV_VAR}, where ENV_VAR is the name of the variable to use.

Implementation Details

  • The environment variable substitution is implemented by loading the config as a string, preprocessing that string, and then passing that preprocessed string into the Jackson ObjectMapper
  • The environment variable name can include any character except } or linebreak, since the allowed characters for environment variables likely vary by OS
  • The environment variable name can optionally have whitespace on either side, since the value is trimmed before use
  • The environment variable value has no restrictions and can contain any character including colons and linebreaks, so it can even contain multiple config keys & values
  • Each line of the config is only processed once, so environment variables cannot reference other environment variables
    • For example, if the config contains${ENV_VAR_A} and the value of ENV_VAR_A is ${ENV_VAR_B}, this will not output the value of ENV_VAR_B, but will instead output the string ${ENV_VAR_B}
  • A QuitProgramException is thrown if an environment variable in the config is not defined
    • It might be desired to instead just ignore the line that contains the nonexistent environment variable, falling back to the default, but this might be confusing in production environments
  • If an environment variable replacement is not desired, the sequence can be preceded by a \
    • \${ENV_VAR} would output the string ${ENV_VAR}, not the value of the environment variable ENV_VAR

Additional Proposed Features

  • I have considered adding support for a default value if an environment variable is not defined
    • ${ENV_VAR, 0} would try to get ENV_VAR and would output 0 if not defined
    • I could use a non-comma separator if desired (|, :, something else?)
    • This might be considered a more extensible replacement for the current Dockerfile environment variable config values (see Docker MySQL implementation and README)
    • For example, the config value host could default to ${SUPERTOKENS_HOST, 0.0.0.0}
  • I have also considered support for loading from a file, but this is probably beyond the scope of this feature
    • This could be implemented with syntax similar to this: ${file: ../db_config.yaml}

Examples

# Simple example
core_config_version: 0
host: ${SUPERTOKENS_HOST}
port: ${SUPERTOKENS_PORT}
base_path: ${ SUPERTOKENS_BASE_PATH } # With optional whitespace

${SUPERTOKENS_DB_CONFIG} # This could expand to a multiline string that contains multiple config values
# Proposed feature examples
core_config_version: 0
host: ${SUPERTOKENS_HOST, 0.0.0.0} # With default
port: ${SUPERTOKENS_PORT, "3567"} # With default in quotes

${file: ../db_config.yaml} # Load from file with relative path
${file: /etc/supertokens_config.yaml} # Load from file with absolute path
${file: C:\Program Files\supertokens\config.yaml} # Load from Windows-style path

Related Issues

(no related issues)

Test Plan

I have looked into ways of mocking environment variables. Java does not allow directly mocking calls to System.getenv(), so I could either extract that call into a mockable class, or we could consider using processes instead of threads for testing. Environment variables are immutable for the current process (including threads), but they could be modified when launching a subprocess (see ProcessBuilder.environment()).

Documentation changes

If implemented, the configuration file should probably get its own page in the documentation to explain the various usages of this syntax. Let me know where this page should go, or if it should be added to an already existing page. I can submit a PR once the implementation of this feature is finalized.

Checklist for important updates

  • Changelog has been updated
    • If there are any db schema changes, mention those changes clearly
  • coreDriverInterfaceSupported.json file has been updated (if needed)
  • pluginInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In build.gradle
  • Had installed and ran the pre-commit hook
  • If there are new dependencies that have been added in build.gradle, please make sure to add them in implementationDependencies.json.
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.

Remaining TODOs for this PR

  • Alter implementation to ignore ${} in comments (comments could easily be stripped before preprocessing)
  • Make sure no other code directly reads the config file other than CoreConfig.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant