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

[Core feature] control ENV var name for injected secrets #3053

Open
2 tasks done
flixr opened this issue Nov 5, 2022 · 9 comments · May be fixed by flyteorg/flytekit#1726 or flyteorg/flyteidl#423
Open
2 tasks done

[Core feature] control ENV var name for injected secrets #3053

flixr opened this issue Nov 5, 2022 · 9 comments · May be fixed by flyteorg/flytekit#1726 or flyteorg/flyteidl#423
Labels
backlogged For internal use. Reserved for contributor team workflow. enhancement New feature or request

Comments

@flixr
Copy link
Contributor

flixr commented Nov 5, 2022

Motivation: Why do you think this is important?

Split out from flyteorg/flytekit#1307:

I need to inject k8s secrets as env vars into my raw ContainerTasks (and actually for PythonTasks as well).
As an example I want to inject AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY.
Essentially I want to do what k8s already nicely provides:
https://kubernetes.io/docs/tasks/inject-data-application/distribute-credentials-secure/#define-a-container-environment-variable-with-data-from-a-single-secret

Using flyte secrets there is no way to control the ENV var name, something like e.g.
Secret(group="minio-write-creds", key="AWS_ACCESS_KEY_ID", mount_requirement=Secret.MountType.ENV_VAR)
will inject the secret as _FSEC_MINIO_WRITE_CREDS_AWS_ACCESS_KEY_ID and this is currently hardcoded in flytepropeller.
This seems sensible iff you need to get the secret in Python code via flytekit context/secret manager, but not for raw containers.
Even for a Python task it would often be easier to just control the env var name (as e.g. in the case of the S3 credentials that boto3 would automatically pick up then).

Goal: What should the final outcome look like, ideally?

Maybe something like

Secret(group="minio-write-creds", key="access_key", mount_requirement=Secret.MountType.ENV_VAR, name="AWS_ACCESS_KEY_ID")

To decouple group/key name from env var name, just like k8s itself.
If name is not given it could still work by naming as prefix + group + key.

Describe alternatives you've considered

Working around this by wrapping my container task in a shell script that pull out the secret from the env var (or file) and populate the correct env var for the actual process.
But this is super cumbersome and actually requires the wrapper script to know about the group name (so k8s secret name), which makes it super awkward to change credentials by just using a different secret.

Same goes for secrets injected as file...

Propose: Link/Inline OR Additional context

No response

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

  • Yes

Have you read the Code of Conduct?

  • Yes
@flixr flixr added enhancement New feature or request untriaged This issues has not yet been looked at by the Maintainers labels Nov 5, 2022
@welcome
Copy link

welcome bot commented Nov 5, 2022

Thank you for opening your first issue here! 🛠

@flixr
Copy link
Contributor Author

flixr commented Nov 5, 2022

An alternative could be using the Pod plugin, but any env specified there is overwritten and when trying to use env_from serialization fails.
Also ContainerTask currently doesn't allow specifying Pod via task_config...

@flixr
Copy link
Contributor Author

flixr commented Apr 6, 2023

Closing this as it can be now achieved with PodTemplates:
#3123
flyteorg/flytekit#1515

@flixr flixr closed this as completed Apr 6, 2023
@gpgn
Copy link

gpgn commented Jun 24, 2023

I'm currently facing this issue, and would really appreciate a name parameter to control a secret's name, for exactly the reasons listed above.

My use-case is that I have local packages that are (and should be) unaware of Flyte and Kubernetes; using environment variables that get read automatically with names like POSTGRES_PASSWORD. When injecting them in Flyte through Kubernetes Secrets, they become _FSEC_DB-CREDENTIALS_POSTGRES-PASSWORD, which I then have to manually change again, duplicating code (and environment variables) all over.

It seems very convoluted to do this via PodTemplate, as I don't care about configuring the rest of the Pod spec and it clutters the workflow (similar to the comment made in #3123 (comment)).

Please consider reopening @flixr .

NB: Similarly for Secrets mounted as files, would be great to be able to control the mountPath from the Task decorator.

@kumare3
Copy link
Contributor

kumare3 commented Jun 24, 2023

@gpgn thank you for your he comments. I actually not sure
If everything is controllable at the k8s layer, but this is a fair ask.
Cc @flixr / @EngHabu / @eapolinario

@gpgn
Copy link

gpgn commented Jul 1, 2023

Ive made a draft implementation here:

Quite new to Flyte (and not a Go dev), so probably I'm missing context and it needs more work in other places where Secrets are used, but my (small) local experiments work. Would be happy to improve it.

(also many thanks to @Yicheng-Lu-llll and @jeevb for help with setting up a local dev environment! 🙏 )

@gpgn
Copy link

gpgn commented Jul 8, 2023

Updated the PRs to allow for specifying mountPath as well. Both old and new way work:

from flytekit import task, workflow, Secret
import flytekit
import os


@task(
    secret_requests=[
        Secret(
            group="user-info",
            key="user_secret",
            mount_requirement=Secret.MountType.FILE,
        ),
        Secret(
            group="user-info",
            key="user_secret",
            mount_requirement=Secret.MountType.ENV_VAR,
        ),
    ]
)
def old() -> None:
    context = flytekit.current_context()
    print("old")

    # mount_requirement=ENV_VAR
    print(context.secrets.get(env_name="_FSEC_USER-INFO_USER_SECRET"))

    # mount_requirement=FILE
    with open('/etc/flyte/secrets/user-info/user_secret', 'r') as infile:
        print(infile.readlines())
    return True

@task(
    secret_requests=[
        Secret(
            group="user-info",
            key="user_secret",
            env_var=Secret.MountEnvVar(name="foo"),
        ),
        Secret(
            group="user-info",
            key="user_secret",
            file=Secret.MountFile(path="/foo"),
        ),
    ]
)

def new() -> None:
    context = flytekit.current_context()
    print("new")

    # env_var=
    print(context.secrets.get(env_name="foo"))

    # file=
    with open('/foo/user_secret', 'r') as infile:
        print(infile.readlines())


@workflow
def training_workflow(hyperparameters: dict) -> None:
    """Put all of the steps together into a single workflow."""
    old()
    new()
    

Copy link

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! 🙏

@github-actions github-actions bot added the stale label Jul 24, 2024
@EngHabu
Copy link
Contributor

EngHabu commented Jul 24, 2024

Reviving this. It's something that will likely be addressed by work @pingsutw and @pmahindrakar-oss are doing...

@eapolinario eapolinario removed the stale label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment