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

vdk-core: support overriding configs with secrets #3125

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

DeltaMichael
Copy link
Contributor

@DeltaMichael DeltaMichael commented Feb 16, 2024

Why?

VDK doesn't provide a way to set sensitive configuration like passwords, such as trino_password. The only way to currently do this is by adding config keys and fetching the values from secrets.

What?

Add a plugin that reconfigures the Configuration object in CoreContext based on secrets. Do this in the initialize_job hook. In this setup, secrets override options set by regular configs. For example if you set trino_password to "password" in config.ini, but also have a secret called trino_passowrd="another password", the value of trino_password will be "another_passowrd".

Note: The Configuration class and the CoreContext class are annotated with @dataclass(frozen=True). This enforces encapsulation, so in order to mutate the Configuration object after it's created, we have to add more public methods to the Configuration class.

https://docs.python.org/3/library/dataclasses.html#frozen-instances

How was this tested?

Functional test
CI/CD

What kind of change is this?

Feature/non-breaking

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/override-configs-with-secrets branch from 4713e03 to 9ac75cb Compare February 16, 2024 14:48
@DeltaMichael DeltaMichael linked an issue Feb 20, 2024 that may be closed by this pull request
@antoniivanov
Copy link
Collaborator

antoniivanov commented Feb 26, 2024

I was wondering if we need to finalize the configuration at vdk_configure.
Now below proposal is bigger and it may require a bit broader discussion (and is more costly so might not be worth at this time). And re-reading it I am not sure it's that beneficial.

So I am ok if we ship the current implementation as it is.

But this is not the first time we've had to create workarounds due to the limitation that configuration options can only be set during that phase. In the JobConfigIniPlugin, we check if the command is run and search for the config.ini file.

Perhaps we could allow for more dynamic configuration?

As it is currently, vdk_configure is used to define the available configuration options, but the actual values can be provided or overridden later by dynamically added providers.

@hookimpl
def vdk_configure(context): 
    context.add("team", description, default, ...)   

Configuration providers can be dynamically added at any point in the application's lifecycle.

@hookimpl
def run_job(context): 
    context.configuration.add_configuration_provider(
        name="secrets", 
        get_function=lambda context, key: context.secrets.get_secret(key), 
        priority_after="config_ini"  # The default priority is LIFO.
    )

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/override-configs-with-secrets branch from b3d3612 to a95415f Compare March 6, 2024 14:38
@DeltaMichael
Copy link
Contributor Author

Follow-up #3156

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/override-configs-with-secrets branch 5 times, most recently from 0a48b0e to 7a6ad23 Compare March 11, 2024 13:51
@DeltaMichael
Copy link
Contributor Author

Follow-ups

#3156
#3210

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/override-configs-with-secrets branch 2 times, most recently from 50e91e8 to 3bb12a2 Compare March 14, 2024 08:50
Why?

VDK doesn't provide a way to set sensitive configuration like passwords, such as trino_password.
The only way to currently do this is by adding config keys and fetching the values from secrets.

What?

Add a plugin that reconfigures the Configuration object in CoreContext based on secrets. Do this
in the initialize_job hook. In this setup, secrets override options set by regular configs. For example
if you set trino_password to "password" in config.ini, but also have a secret called trino_passowrd="another password",
the value of trino_password will be "another_passowrd".

How was this tested?

Functional test
CI/CD

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/override-configs-with-secrets branch from 3bb12a2 to 888b38d Compare March 14, 2024 08:50
@DeltaMichael DeltaMichael enabled auto-merge (squash) March 14, 2024 08:52
@DeltaMichael DeltaMichael merged commit e6df56c into main Mar 14, 2024
7 of 8 checks passed
@DeltaMichael DeltaMichael deleted the person/mdilyan/override-configs-with-secrets branch March 14, 2024 09:17
DeltaMichael pushed a commit that referenced this pull request Mar 19, 2024
Uncomment test after #3125

Signed-off-by: Dilyan Marinov <[email protected]>
duyguHsnHsn added a commit that referenced this pull request Mar 20, 2024
Uncomment test after
#3125

Signed-off-by: Dilyan Marinov <[email protected]>
Co-authored-by: Dilyan Marinov <[email protected]>
Co-authored-by: Duygu Hasan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set sensitive VDK configuration like passwords
4 participants