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

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

Conversation

jawadqur
Copy link
Contributor

New Features

Breaking Changes

Bug Fixes

Improvements

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

Dependency updates

Deployment changes

@jawadqur jawadqur marked this pull request as draft April 17, 2023 18:28
@coveralls
Copy link

coveralls commented Apr 17, 2023

Pull Request Test Coverage Report for Build 13452

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 74.52%

Totals Coverage Status
Change from base Build 13442: 0.02%
Covered Lines: 7142
Relevant Lines: 9584

💛 - Coveralls

Copy link
Contributor

@paulineribeyre paulineribeyre left a comment

Choose a reason for hiding this comment

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

The logic lgtm, but tagging @Avantol13 in case he has any concerns about this (this is for Helm charts support)

@@ -49,6 +49,15 @@ def post_process(self):
for default in defaults:
self.force_default_if_none(default, default_cfg=default_config)

# 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

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.

@Avantol13
Copy link
Contributor

How are these getting to env vars for Helm Charts?

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

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

Copy link
Contributor

@Avantol13 Avantol13 left a comment

Choose a reason for hiding this comment

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

see comment on overall pr

Copy link
Contributor

@paulineribeyre paulineribeyre left a comment

Choose a reason for hiding this comment

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

lgtm

@jawadqur jawadqur marked this pull request as ready for review April 26, 2023 19:33
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.

4 participants