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 #minor #1

Closed

Conversation

gpgn
Copy link
Owner

@gpgn gpgn commented Jul 1, 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

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@gpgn gpgn marked this pull request as ready for review July 1, 2023 09:20
Copy link

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
I see the original issue is marked as closed as it suggests using PodTemplate. Is that not enough? or not ergonomic enough?

Comment on lines 46 to 50

// The name of the environment variable, if the Secret is injected as environment variable. If ommitted, the default
// FLYTE_SECRETS_ENV_PREFIX prefix will be used.
// +optional
string env_name = 5;
Copy link

Choose a reason for hiding this comment

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

This only applies to ENV_VAR mount type. I would make the change as following:

  1. Mark MountType mount_requirement = 4 [deprecated=true]; as deprecated.
  2. Add:
      oneof MountTarget mount_target {
        MountEnvVar env_var = 5;
        MountFile file = 6;
      }
    
      message MountEnvVar {
        string name = 1;
      }
    
      message MountFile {
        string path = 1;
      }
    
  3. Update secrets webhook to look at the new field first, if it's nil, it should fallback to the old field.
  4. Update flytekit to always set both fields; mount_requirement and mount_target

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I'll try to add it. Could you explain what you mean with 4? Not sure I get what you mean here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added the changes with a working initial version, I'll get to adding a number of tests as well.

Copy link
Owner Author

@gpgn gpgn Jul 8, 2023

Choose a reason for hiding this comment

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

Tests & documentation added, let me know what you think :)

Copy link
Owner Author

@gpgn gpgn changed the title Updates Secret to use env_name field as environment variable name on injection if exists Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection Jul 8, 2023
@gpgn gpgn changed the title Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #minor Jul 8, 2023
@gpgn gpgn closed this Jul 8, 2023
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.

2 participants