Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection

[gpgn]

TL;DR

Introduces new fields to the Secret object:

• env_var
• file

Allowing users to directly specify a name or mountPath for the Secret, without having to specify a full PodTemplate(name). The old mount_requirement can still be used. Example:

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()

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

fixes flyteorg/flyte#3053

Follow-up issue

NA

Linked PRs

• Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #1726
• Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #minor flytepropeller#589
• Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection flyteidl#423
• Adds documentation on usage for new Secret fields env_var and file flytesnacks#1017

[welcome]

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

• Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
• Sign off your commits (Reference: DCO Guide).

[gpgn]

Guessing the builds/readthedocs check fails because this change has a dependency on flyteorg/flyteidl#423

[wild-endeavor]

actually i would love to move away from these model classes. The Secret object will be a mixed bag, but I think that's okay. Could we just use the raw pb generated classes for the new fields?

(background: we wrote the model files before there were .pyi files, so nothing had type hints, field hints and it made things easier to use. but now we do have them with pyi files)

[wild-endeavor]

Suggested change

	def check_env_name_key(env_name: Optional[str] = None):
	def assert_env_name_key(env_name: Optional[str] = None):

[wild-endeavor]

Suggested change

	self.check_env_name_key(env_name)
	self.assert_env_name_key(env_name)

[wild-endeavor]

is this redundant?

[wild-endeavor]

should we delete this function completely? it's no longer needed right?

[hamersaw]

Is there a way here to make API transitions more seamless? For example, rather than introducing new fields (ie. env_var and file) can we reuse the mount_requirement field? Then the API owuld be the exact same as it currenlty is, but you could add arguments to specify environment variable name or mount path - if the are no included (as current API) then they would default. So new API would be like:

@task(
    secret_requests=[
        Secret(
            group="user-info",
            key="user_secret",
            mount_requirement=Secret.MountType.FILE(name="foo"),
        ),
        Secret(
            group="user-info",
            key="user_secret",
            mount_requirement=Secret.MountType.ENV_VAR(path="/foo"),
        ),
    ]
)

We could use new variable types for Secret.MountType.FILE if necessary - and will certainly need new idl representation between old and new API. But I'm not sure we need to introduce new fields right?

[wild-endeavor]

@gpgn I'm okay with @hamersaw's suggestion on the flytekit side. I think keeping them separate things on the IDL side is good though. But the flytekit object is just a shim layer on top of the IDL. we should do whatever you think is the better ux.

[wild-endeavor]

@gpgn mind giving me write access to your fork so I can continue this pr?

[pingsutw]

@gpgn Do you have any chance to update this PR? I'd like to move this forward.

[gpgn]

Yeah sure thing, I think I gave @wild-endeavor write access to the fork, but I can pick it up. It's been a while since I looked at this but would be a nice exercise to get familiar with the new monorepo setup 👍

[gpgn]

I'm not sure what changed with the monorepo setup but I'm hitting many errors in setting up a development environment from the last time. I'll continue debugging but if others would like to take this in the mean time, please go ahead.

[kumare3]

@gpgn what type of errors?