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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gpgn
Copy link

@gpgn gpgn commented Jul 8, 2023

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

@welcome
Copy link

welcome bot commented Jul 8, 2023

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 gpgn force-pushed the feat-add-optional-environment-variable-name-to-secret branch from 2154a2d to 6a59568 Compare July 8, 2023 16:29
@gpgn
Copy link
Author

gpgn commented Jul 8, 2023

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

flytekit/core/context_manager.py Outdated Show resolved Hide resolved
flytekit/core/context_manager.py Outdated Show resolved Hide resolved
@@ -35,10 +36,42 @@ class MountType(Enum):
Caution: May not be supported in all environments
"""

@dataclass
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

if group is None or group == "":
raise ValueError("secrets group is a mandatory field.")

@staticmethod
def check_env_name_key(env_name: Optional[str] = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

self.check_group_key(group)
env_var = self.get_secrets_env_var(group, key, group_version)
fpath = self.get_secrets_file(group, key, group_version)
self.check_env_name_key(env_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.check_env_name_key(env_name)
self.assert_env_name_key(env_name)

"""
Returns a string that matches the ENV Variable to look for the secrets
"""
self.check_env_name_key(env_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this redundant?

@@ -384,10 +404,15 @@ def get_secrets_file(self, group: str, key: Optional[str] = None, group_version:
return os.path.join(self._base_dir, *l)

@staticmethod
def check_group_key(group: str):
def check_group_key(group: Optional[str] = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@hamersaw
Copy link
Contributor

hamersaw commented Jul 24, 2023

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
Copy link
Contributor

@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 wild-endeavor changed the title Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #minor Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection Aug 21, 2023
@wild-endeavor
Copy link
Contributor

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

@pingsutw
Copy link
Member

pingsutw commented May 7, 2024

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

@gpgn
Copy link
Author

gpgn commented May 8, 2024

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
Copy link
Author

gpgn commented May 10, 2024

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
Copy link
Contributor

kumare3 commented Jun 18, 2024

@gpgn what type of errors?

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.

[Core feature] control ENV var name for injected secrets
5 participants