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.

Sign up for GitHub

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

Jump to bottom

Add ability to read fence config(s) from env vars #1088

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add ability to read fence config(s) from env vars #1088

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ea18ec
Add ability to read fence config(s) from env vars
jawadqur Apr 17, 2023
8512475
Add ability to read fence config(s) from env vars
jawadqur Apr 19, 2023
a6bee03
Merge branch 'master' into feat/config_env
jawadqur Apr 19, 2023
bccd595
Merge branch 'master' into feat/config_env
jawadqur Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions fence/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,14 @@ def post_process(self):
for default in defaults:
self.force_default_if_none(default, default_cfg=default_config)

# allow setting DB connection string via env var
if os.environ.get("DB"):
logger.info(
"Found environment variable 'DB': overriding 'DB' field from config file"
)
self["DB"] = os.environ["DB"]
else:
logger.info(
"Environment variable 'DB' empty or not set: using 'DB' field from config file"
)
# Read in all environment variables starting with FENCE_
for env_var, env_val in os.environ.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we can revert this then and just use this logic for the DB url?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

@jawadqur jawadqur Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these getting to env vars for Helm Charts?

https://github.com/uc-cdis/gen3-helm/blob/master/helm/fence/values.yaml#L200-L255

We put a lot of work into the YAML-based configuration support for default values and allowing overrides.

We need a way to be able to mount k8s secrets into specific fields. Env var seems to be the best option, I am all ears for other ideas.

It's not clear to me how this fully replaces that. How does this handle nested configurations like OIDC providers?

It does not replace anything, it's just a supplement, and it does not support nested configs as far as I can tell.

This is to be used for DB password, and the INDEXD_PASSWORD as those are k8s secrets that are potentially created by other charts, and consumed by fence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also removed the DB specific ones, and I am adding an extra env var in fence helm chart to support the new and old format for backwards compatibility.

if env_var.startswith("FENCE_"):
config_key = env_var[6:] # remove "FENCE_" prefix
self[config_key] = env_val
logger.info(
f"Found environment variable '{env_var}': overriding '{config_key}' field from config file"
)

if "ROOT_URL" not in self._configs and "BASE_URL" in self._configs:
url = urllib.parse.urlparse(self._configs["BASE_URL"])
Expand Down