Add env_name to Secrets #6160

thomasjpfan · 2025-01-10T20:00:05Z

Tracking issue

Why are the changes needed?

This PR adds an env_name to the Secrets IDL, which has the follow behavior:

If mount_requirement is ENV_VAR, then we set an environment variable named env_name to the value of the secret.
If mount_requirement is FILE, then we set an environment variable named env_name to the path of the mounted secret.

What changes were proposed in this pull request?

This PR adds env_name to the Secrets IDL. This makes it easy to configure a secret in a Flyte task. For example, one can easily set a hugging face secret:

@task(secret_requests=[Secret(..., env_name="HF_TOKEN", mount_requirement=Secret.MountType.ENV_VAR)
def hello():
    ...

Or for secrets that require a file:

@task(secret_requests=[Secret(..., env_name="HF_TOKEN_PATH", mount_requirement=Secret.MountType.FILE)
def hello():
    ...

How was this patch tested?

I ran the following to try the two different modes:

from flytekit import task, Secret, ImageSpec
from typing import Optional
import os

image = ImageSpec(
    apt_packages=["git"],
    packages=[
        "git+https://github.com/thomasjpfan/flytekit.git@615499eae5539036abf70d960beb93621f7f9a29",
    ],
    registry="localhost:30000",
    commands=[
        "uv pip install git+https://github.com/thomasjpfan/flyte.git@ef360e831f26471160c2b76a5e9635e77caab2ae#subdirectory=flyteidl"
    ],
)


@task(
    container_image=image,
    secret_requests=[
        Secret(
            key="token",
            group="my-fun-group",
            mount_requirement=Secret.MountType.ENV_VAR,
            env_name="HELLO_WORLD",
        )
    ],
)
def get_secret_env_var() -> Optional[str]:
    return os.getenv("HELLO_WORLD")


@task(
    container_image=image,
    secret_requests=[
        Secret(
            key="token",
            group="my-fun-group",
            mount_requirement=Secret.MountType.FILE,
            env_name="HELLO_WORLD",
        )
    ],
)
def get_secret_file() -> str:
    with open(os.getenv("HELLO_WORLD"), "r") as f:
        return f.read()

Summary by Bito

This PR implements custom environment variable naming for Flyte secrets by adding an env_name field to the Secret message type. The enhancement supports both ENV_VAR and FILE mount types, allowing users to specify custom environment variable names for secret values and file paths. The implementation includes protobuf definition updates and generated code across multiple languages (Go, TypeScript, JavaScript, Python, Rust).

Unit tests added: True

Estimated effort to review (1-5, lower is better): 3

Signed-off-by: Thomas J. Fan <[email protected]>

flyte-bot · 2025-01-10T20:00:29Z

Code Review Agent Run #40b3bc

Actionable Suggestions - 2

flytepropeller/pkg/webhook/k8s_secrets.go - 1
- Consider extracting duplicate env var logic · Line 90-96
flytepropeller/pkg/webhook/utils.go - 1
- Consider path.Join for URL paths · Line 73-73

Review Details

Files reviewed - 11 · Commit Range: dd7a215..ef360e8
- flyteidl/gen/pb-es/flyteidl/core/security_pb.ts
- flyteidl/gen/pb-go/flyteidl/core/security.pb.go
- flyteidl/gen/pb-js/flyteidl.d.ts
- flyteidl/gen/pb-js/flyteidl.js
- flyteidl/gen/pb_python/flyteidl/core/security_pb2.py
- flyteidl/gen/pb_python/flyteidl/core/security_pb2.pyi
- flyteidl/gen/pb_rust/flyteidl.core.rs
- flyteidl/protos/flyteidl/core/security.proto
- flytepropeller/pkg/webhook/k8s_secrets.go
- flytepropeller/pkg/webhook/k8s_secrets_test.go
- flytepropeller/pkg/webhook/utils.go
Files skipped - 4
- flyteidl/clients/go/assets/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/agent.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/external_plugin_service.swagger.json - Reason: Filter setting
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-10T20:06:44Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Add env_name field to Secret IDL	- `security.proto` - Added env_name field to Secret message with documentation - `security.pb.go` - Generated Go code for env_name field in Secret struct - `security_pb.ts` - Generated TypeScript code for env_name field - `flyteidl.d.ts` - Added TypeScript definitions for env_name field - `flyteidl.js` - Generated JavaScript code for env_name field - `security_pb2.py` - Generated Python code for env_name field - `security_pb2.pyi` - Added Python type hints for env_name field - `flyteidl.core.rs` - Generated Rust code for env_name field
Other Improvements - Implement env_name support in K8s Secret Injector	- `k8s_secrets.go` - Added logic to handle custom environment variable names for secrets
Feature Improvement - Add env_name field to Secret IDL	- `security.proto` - Added env_name field to Secret message with documentation - `security.pb.go` - Generated Go code for env_name field in Secret struct - `security_pb.ts` - Generated TypeScript code for env_name field - `flyteidl.d.ts` - Added TypeScript definitions for env_name field - `flyteidl.js` - Generated JavaScript code for env_name field - `security_pb2.py` - Generated Python code for env_name field - `security_pb2.pyi` - Added Python type hints for env_name field - `flyteidl.core.rs` - Generated Rust code for env_name field
Testing - Add tests for custom environment variable support	- `k8s_secrets_test.go` - Added test cases for custom environment variable names in both ENV_VAR and FILE mount types - `utils.go` - Added utility function to create environment variables with custom names for secrets

flyte-bot · 2025-01-10T20:06:46Z

flytepropeller/pkg/webhook/k8s_secrets.go

+		if secret.GetEnvName() != "" {
+			extraEnvVar := *envVar.DeepCopy()
+			extraEnvVar.Name = secret.GetEnvName()
+
+			p.Spec.InitContainers = AppendEnvVars(p.Spec.InitContainers, extraEnvVar)
+			p.Spec.Containers = AppendEnvVars(p.Spec.Containers, extraEnvVar)
+		}


Consider extracting duplicate env var logic

Consider extracting the duplicate environment variable injection logic into a helper function. The same pattern of copying and injecting environment variables appears in multiple places. A similar issue was also found in flytepropeller/pkg/webhook/k8s_secrets.go (line 79-82).

Code suggestion

Check the AI-generated fix before applying

Suggested change

if secret.GetEnvName() != "" {

extraEnvVar := *envVar.DeepCopy()

extraEnvVar.Name = secret.GetEnvName()

p.Spec.InitContainers = AppendEnvVars(p.Spec.InitContainers, extraEnvVar)

p.Spec.Containers = AppendEnvVars(p.Spec.Containers, extraEnvVar)

}

if secret.GetEnvName() != "" {

injectEnvVarToContainers(p, envVar, secret.GetEnvName())

}

Code Review Run #40b3bc

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flyte-bot · 2025-01-10T20:06:47Z

flytepropeller/pkg/webhook/utils.go

+func CreateVolumeMountEnvVarForSecretWithEnvName(secret *core.Secret) corev1.EnvVar {
+	return corev1.EnvVar{
+		Name:  secret.GetEnvName(),
+		Value: filepath.Join(filepath.Join(K8sSecretPathPrefix...), strings.ToLower(secret.GetGroup()), strings.ToLower(secret.GetKey())),


Consider path.Join for URL paths

Consider using path.Join() instead of filepath.Join() for URL-like paths. The filepath.Join() uses OS-specific path separators which may cause issues if the path is used in URLs or container environments.

Code suggestion

Check the AI-generated fix before applying

Suggested change

Value: filepath.Join(filepath.Join(K8sSecretPathPrefix...), strings.ToLower(secret.GetGroup()), strings.ToLower(secret.GetKey())),

Value: path.Join(path.Join(K8sSecretPathPrefix...), strings.ToLower(secret.GetGroup()), strings.ToLower(secret.GetKey())),

Code Review Run #40b3bc

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

codecov · 2025-01-10T20:20:18Z

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.

Project coverage is 37.03%. Comparing base (b010747) to head (05d128f).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
flyteidl/gen/pb-go/flyteidl/core/security.pb.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6160   +/-   ##
=======================================
  Coverage   37.02%   37.03%           
=======================================
  Files        1317     1317           
  Lines      132523   132557   +34     
=======================================
+ Hits        49066    49086   +20     
- Misses      79211    79225   +14     
  Partials     4246     4246

Flag	Coverage Δ
unittests-datacatalog	`51.58% <ø> (ø)`
unittests-flyteadmin	`54.25% <ø> (ø)`
unittests-flytecopilot	`30.99% <ø> (ø)`
unittests-flytectl	`62.29% <ø> (ø)`
unittests-flyteidl	`7.23% <0.00%> (-0.01%)`	⬇️
unittests-flyteplugins	`53.85% <ø> (ø)`
unittests-flytepropeller	`42.67% <100.00%> (+0.03%)`	⬆️
unittests-flytestdlib	`55.11% <ø> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flyteidl/protos/flyteidl/core/security.proto

Signed-off-by: Thomas J. Fan <[email protected]>

flyte-bot · 2025-01-11T15:51:16Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - We encountered technical difficulties while attempting to generate code feedback. Please try again or contact [email protected].

thomasjpfan added 2 commits January 10, 2025 13:13

Generate idl

dd7a215

Signed-off-by: Thomas J. Fan <[email protected]>

Add env_name to Secrets

ef360e8

Signed-off-by: Thomas J. Fan <[email protected]>

thomasjpfan added the added Merged changes that add new functionality label Jan 10, 2025

thomasjpfan mentioned this pull request Jan 10, 2025

Adds secret env_var flyteorg/flytekit#3048

Open

flyte-bot reviewed Jan 10, 2025

View reviewed changes

pingsutw reviewed Jan 10, 2025

View reviewed changes

flyteidl/protos/flyteidl/core/security.proto Outdated Show resolved Hide resolved

Uses +optional

05d128f

Signed-off-by: Thomas J. Fan <[email protected]>

pingsutw approved these changes Jan 14, 2025

View reviewed changes

thomasjpfan merged commit 91d0383 into flyteorg:master Jan 15, 2025
53 checks passed

thomasjpfan mentioned this pull request Jan 16, 2025

Rename Secret.env_name to Secret.env_var #6176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add env_name to Secrets #6160

Add env_name to Secrets #6160

thomasjpfan commented Jan 10, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 10, 2025 •

edited

Loading

Code Review Agent Run #40b3bc

flyte-bot commented Jan 10, 2025

Changelist by Bito

flyte-bot Jan 10, 2025

flyte-bot Jan 10, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

flyte-bot commented Jan 11, 2025 •

edited

Loading

	Value: filepath.Join(filepath.Join(K8sSecretPathPrefix...), strings.ToLower(secret.GetGroup()), strings.ToLower(secret.GetKey())),
	Value: path.Join(path.Join(K8sSecretPathPrefix...), strings.ToLower(secret.GetGroup()), strings.ToLower(secret.GetKey())),

Add env_name to Secrets #6160

Add env_name to Secrets #6160

Conversation

thomasjpfan commented Jan 10, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Summary by Bito

flyte-bot commented Jan 10, 2025 • edited Loading

Code Review Agent Run #40b3bc

flyte-bot commented Jan 10, 2025

Changelist by Bito

flyte-bot Jan 10, 2025

Choose a reason for hiding this comment

flyte-bot Jan 10, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

flyte-bot commented Jan 11, 2025 • edited Loading

Code Review Agent Run Status

thomasjpfan commented Jan 10, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

flyte-bot commented Jan 11, 2025 •

edited

Loading