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

Allow users to pass credentials through environment variables #2178

Merged
merged 20 commits into from
Jan 13, 2023

Conversation

jmholzer
Copy link
Contributor

@jmholzer jmholzer commented Jan 6, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Resolves #1909.

Development notes

OmegaConf resolves DictConfig objects on read. In the following example (where credentials.yml contains environment variable grammar such as "key": "${oc.env:TEST_KEY}"), the oc.env resolver would have to be registered, or line 3 will raise an UnsupportedInterpolationType exception.

conf = OmegaConfLoader(...)
credentials = conf["credentials"]
credentials["test"]

However, the scope of the issue is to keep the oc.env resolver switched off. The way around this is to use OmegaConf.resolve, which resolves DictConfig objects in-place when these objects are created, using the oc.env resolver. This is done in the _resolve_environment_variables method.

Includes four automated tests:

  1. Check that environment variables are resolved when the credentials is used as a key.
  2. Check that environment variables are not resolved when credentials is not used as a key.
  3. Check that the oc.env resolver is not registered after environment variables are read if it was not registered beforehand.
  4. Check that the oc.env resolver remains registered after environment variables are read if it was registered beforehand.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@jmholzer jmholzer changed the title Feat/allow credentials via env variables Allow users to pass credentials through environment variables Jan 6, 2023
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
…er if it was not registered

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@jmholzer jmholzer force-pushed the feat/allow-credentials-via-env-variables branch 2 times, most recently from 8a1bd5f to fc7f81e Compare January 9, 2023 16:37
jmholzer and others added 10 commits January 9, 2023 16:37
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
…entials'

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
…:kedro-org/kedro into feat/allow-credentials-via-env-variables

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
…ontrol flow variable

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@jmholzer jmholzer assigned merelcht and jmholzer and unassigned merelcht Jan 10, 2023
@jmholzer jmholzer marked this pull request as ready for review January 10, 2023 15:53
@jmholzer jmholzer requested a review from idanov as a code owner January 10, 2023 15:53
@jmholzer jmholzer requested review from merelcht and noklam and removed request for idanov January 10, 2023 15:54
@antonymilne
Copy link
Contributor

Stupid question... why can't we just leave oc.env switched on? Something to do with not wanting to allow environment variables except in credentials?

Just a quick comment on naming also: I think @idanov previously suggested that we might drop the oc. namespace for this and just have env instead.

@jmholzer
Copy link
Contributor Author

jmholzer commented Jan 10, 2023

Stupid question... why can't we just leave oc.env switched on? Something to do with not wanting to allow environment variables except in credentials?

Exactly, ideally we'd only load credentials in this way.

Just a quick comment on naming also: I think @idanov previously suggested that we might drop the oc. namespace for this and just have env instead.

Thanks for passing it on, I'll have a chat with him as I would personally be for keeping it.

Signed-off-by: Jannic Holzer <jannic.holzer@quantumblack.com>
@datajoely
Copy link
Contributor

so happy this is coming!

config {Dict[str, Any]} -- The configuration dictionary to resolve.
"""
if not OmegaConf.has_resolver("oc.env"):
OmegaConf.register_new_resolver("oc.env", oc.env)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can drop the oc. or simply support both env and oc.env?

Copy link
Contributor Author

@jmholzer jmholzer Jan 12, 2023

Choose a reason for hiding this comment

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

Do you mean drop the "oc." in:

  1. name of the resolver (first argument to register_new_resolver)
    or in:
  2. the name assigned to omegaconf.resolvers.oc.env (second argument to register_new_resolver)?

I personally wouldn't drop the oc. from 1., since this is the name assigned to the resolver by OmegaConf when it is instantiated. For 2. I would be ok with losing the namespace from the import: from omegaconf.resolvers.oc import env.

if not OmegaConf.has_resolver("oc.env"):
OmegaConf.register_new_resolver("oc.env", oc.env)
OmegaConf.resolve(config)
OmegaConf.clear_resolver("oc.env")
Copy link
Member

Choose a reason for hiding this comment

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

It would have been nice if OmegaConf did not rely on global state...

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

This looks great! I'm so excited for this functionality to be added 🤩

@jmholzer jmholzer force-pushed the feat/allow-credentials-via-env-variables branch from ab9784a to 7e6727f Compare January 13, 2023 15:18
@jmholzer jmholzer merged commit e126891 into main Jan 13, 2023
@jmholzer jmholzer deleted the feat/allow-credentials-via-env-variables branch January 13, 2023 16:11
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.

Allow users to pass credentials through environment variables
5 participants