[BUG] Vault Secret Manager Plugin mishandles response for group_version="db"

[EternalDeiwos]

Describe the bug

Vault kv2 works well enough and the service allows deep referencing of specific values in the group parameter past the .../data/... part of the path. This is not the case for group_version="db" where the resulting credentials cannot be deeply referenced. See documentation.

The difficulty is when I specify a secret for a SQLAlchemyTask...

secrets = {
    "user": Secret(
        group="mydb/creds/test_role", key="username", group_version="db"
    ),
    "password": Secret(
        group="mydb/creds/test_role", key="password", group_version="db"
    ),
}

sql_task = SQLAlchemyTask(
    "sql_query",
    query_template="""
        select version()
    """,
    output_schema_type=DataSchema,
    task_config=SQLAlchemyConfig(
        uri="postgresql://host:1234/test",
        secret_connect_args=secrets,
    ),
    secret_requests=[*secrets.values()],
)

... the value returned by current_context().secret.get() in this context is a dict which looks like:

{ username: "...", password: "..." }

This results in errors being produced because the task is expecting a string value for username and password respectively and not a dict. I suspect this is because the result of the API call to vault is not flat and rather is nested under the data property name.

Expected behavior

Dynamically generated credentials from Vault Database Engine should be passed to SQLAlchemy in the format it is expecting (i.e. strings not a dict).

Additional context to reproduce

Running the workflow produced this error message (carefully redacted):

(psycopg2.OperationalError) connection to server at "host", port 1234 failed: FATAL:  password authentication failed for user "{"password":"...","username":"..."

Screenshots

No response

Are you sure this issue hasn't been raised already?

• Yes

Have you read the Code of Conduct?

• Yes

[hamersaw]

@bimtauer can you help clarify this? With your recent PR we enabled support for "db" in vault. When injecting the secret we're explicitly choosing not to template rather than using the secret key here, which could support separate username and password requests as suggested in this issue (making the "db" vault construct integrate more seamlessly into existing Flyte constructs). This is explicitly documented in the flytesnacks repo.

@EternalDeiwos very interested in your input as well. I don't have extensive experience integrating with Vault, want to make sure the is as seamless as possible, but also fully supportive of all use-cases.

TL;DR is there an advantage to mounting "db" secrets as a dict (ie. {username:foo, password:bar}) rather than mounting two separate secrets (ie. one for username and one for password) beyond performance of two calls?

[EternalDeiwos]

This is explicitly documented in the flytesnacks repo.

This makes sense to me from how Vault works and how one would write a request to fetch credentials — you can’t request twice because they are a single-use secret, vault doesn’t actually store the credentials it generates, just enough to be able to extend the lease or revoke them. That being said, I am pretty sure that SQLAlchemyTask is not the only type that is expecting strings and not a dict.

It’s not a problem if the secret provides a dict but then there needs to be a way to map the secret values to the input of the intended task or the feature is useless. This could be a fairly large feature that needs to be added to Flyte in order to fix a fairly small and apparently not often encountered bug.

You could also refactor the tasks themselves to allow dict inputs, however you would still need the ability to map keys on Flyte’s side because Vault will only provide keys called username and password. If a task is expecting secrets on a different key to those then you’d be out of luck. You would also likely end up playing whack-a-mole on task types that haven’t been updated yet, or are not consistent with the pattern. Probably not recommended.

[EternalDeiwos]

@hamersaw @bimtauer any further feedback on this?

[hamersaw]

@EternalDeiwos thanks for checking back in here. I appreciate the breakdown of the problem, it provides much clarity. Do you have a suggestion for a fix? IIUC we need a singular secret mount in the task decorator (encompassing both username and password) to support the db group_version creating the dynamic pair and then need to refactor plugins to support reading both from the same mount?

With some guidance, is this something you might you be willing to contribute?

[EternalDeiwos]

@hamersaw I don't think I'll be able to make this contribution; however as for a suggestion, I think the best UX is going to be some way to resolve or map values within a secret to the parameter names expected by the downstream interface (e.g. SQLAlchemyTask).

Inevitably some secrets are going to return a dict and there should be some way to resolve and/or template those such that the db group_version can be split into two separate parameters. Perhaps a wrapper class of some kind? I haven't really looked at the Secret source code too closely.

Edit: It looks like we will have to move away from using the Vault plugin regardless due to other limitations around supplying different secrets to the same workflow in different domains; i.e. staging secrets to staging workflow, production secrets to production workflow, etc.

[github-actions]

Hello 👋, this issue has been inactive for over 9 months. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will engage on it to decide if it is still applicable.
Thank you for your contribution and understanding! 🙏

[EternalDeiwos]

Would still like to see this fixed