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.

Sign up for GitHub

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

WIP: fix(Backend+SDK): Update kubernetes.use_secret_as_volume to accept secret name dynamically at runtime #11039

Closed
wants to merge 3 commits into from
Closed

WIP: fix(Backend+SDK): Update kubernetes.use_secret_as_volume to accept secret name dynamically at runtime #11039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6df1dd1
Update kubernetes.use_secret_as_volume to accept secret name as a par…
DharmitD Jul 25, 2024
686a7b9
Add check to ensure use_secret_as_volume accepts secret name as param…
DharmitD Jul 31, 2024
6aa7591
Add dynamic secret name support in use_secret_as_volume function
DharmitD Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions backend/src/v2/driver/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ import (
"context"
"encoding/json"
"fmt"
"github.com/kubeflow/pipelines/backend/src/v2/objectstore"
"strconv"
"strings"
"time"

"github.com/kubeflow/pipelines/backend/src/v2/objectstore"

"github.com/golang/glog"
"github.com/golang/protobuf/ptypes/timestamp"
"github.com/google/uuid"
Expand Down Expand Up @@ -536,14 +538,40 @@ func extendPodSpecPatch(
// Get secret mount information
for _, secretAsVolume := range kubernetesExecutorConfig.GetSecretAsVolume() {
optional := secretAsVolume.Optional != nil && *secretAsVolume.Optional

secretName := secretAsVolume.GetSecretName()

if strings.HasPrefix(secretName, "{{") && strings.HasSuffix(secretName, "}}") {
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in the community meeting, we suggest follow the "inject inputs" pattern to accept dynamic runtime value.
Example PRs on "injecting inputs":
#10435
#5183

// Strip the braces
key := secretName[2 : len(secretName)-2]

// Check if the key exists in the parameter inputs map
inputParams, _, err := dag.Execution.GetParameters()
if err != nil {
return fmt.Errorf("failed to get input parameters: %v", err)
}

val, ok := inputParams[key]
if !ok {
return fmt.Errorf("dynamic secret name key '%s' not found in input parameters", key)
}

secretName = val.GetStringValue()
if secretName == "" {
return fmt.Errorf("secret name for key '%s' is empty", key)
}
} else if strings.TrimSpace(secretName) == "" {
return fmt.Errorf("secret name is empty or invalid")
}

secretVolume := k8score.Volume{
Name: secretAsVolume.GetSecretName(),
Name: secretName,
VolumeSource: k8score.VolumeSource{
Secret: &k8score.SecretVolumeSource{SecretName: secretAsVolume.GetSecretName(), Optional: &optional},
Secret: &k8score.SecretVolumeSource{SecretName: secretName, Optional: &optional},
},
}
secretVolumeMount := k8score.VolumeMount{
Name: secretAsVolume.GetSecretName(),
Name: secretName,
MountPath: secretAsVolume.GetMountPath(),
}
podSpec.Volumes = append(podSpec.Volumes, secretVolume)
Expand Down
14 changes: 10 additions & 4 deletions kubernetes_platform/python/kfp/kubernetes/secret.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
from kfp.dsl import PipelineTask
from kfp.kubernetes import common
from kfp.kubernetes import kubernetes_executor_config_pb2 as pb

from typing import Union
from kfp.dsl.pipeline_channel import PipelineParameterChannel

def use_secret_as_env(
task: PipelineTask,
Expand Down Expand Up @@ -59,7 +60,7 @@ def use_secret_as_env(

def use_secret_as_volume(
task: PipelineTask,
secret_name: str,
secret_name: Union[str, PipelineParameterChannel],
mount_path: str,
optional: bool = False,
) -> PipelineTask:
Expand All @@ -75,11 +76,16 @@ def use_secret_as_volume(
Returns:
Task object with updated secret configuration.
"""

msg = common.get_existing_kubernetes_config_as_message(task)

val = secret_name
# if secret_name is a PipelineParameterChannel, then we don't know what secret to mount until RUNTIME
# so, treat is as a map KEY instead of a secret name
if isinstance(secret_name, PipelineParameterChannel):
val = "{{" + secret_name.name + "}}"
HumairAK marked this conversation as resolved.
Show resolved Hide resolved

secret_as_vol = pb.SecretAsVolume(
secret_name=secret_name,
secret_name=val,
mount_path=mount_path,
optional=optional,
)
Expand Down
28 changes: 28 additions & 0 deletions kubernetes_platform/python/test/unit/test_secret.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,34 @@ def my_pipeline():
}
}

def test_with_secret_name_param(self):
@dsl.pipeline
def my_pipeline(secret_name: str = 'my-secret'):
Copy link
Contributor

Choose a reason for hiding this comment

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

It is missing a test that assert from the value my-secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is the pipeline spec generated from this compile step has the name stored in as a variable under the roots section.

root:
  dag:
    tasks:
      print-secret:
        cachingOptions:
          enableCache: true
        componentRef:
          name: comp-print-secret
        taskInfo:
          name: print-secret
  inputDefinitions:
    parameters:
      secret_name:
        defaultValue: my-secret
        isOptional: true
        parameterType: STRING
schemaVersion: 2.1.0
sdkVersion: kfp-2.8.0
---
platforms:
  kubernetes:
    deploymentSpec:
      executors:
        exec-print-secret:
          secretAsVolume:
          - mountPath: /mnt/my_vol
            optional: false
            secretName: secret_name

I tried a bunch of combinations to have the defaultValue field validated, so we know the name is being passed correctly, but the structure of this tests is such that if I pass in the entire pipelineSpec in the assert it fails:
https://github.com/kubeflow/pipelines/actions/runs/10191867165/job/28193820249#step:4:886

The tests only work when platform_spec is extracted and matched.

Note that the assert statement still verifies that the secret_name variable is getting passed into the secretAsVolume, I believe that's a good enough preliminary test at this point. Wdyt? @diegolovison @gregsheremeta

Copy link
Contributor

Choose a reason for hiding this comment

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

for posterity

The problem here is the pipeline spec generated from this compile step has the name stored in as a variable under the roots section.

we discovered today that that's not how this works. secretName (at the very bottom of the platforms spec) has to say secretName: my-secret and not secretName: secret_name

Copy link
Contributor

Choose a reason for hiding this comment

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

also for posterity, the above comment is also incorrect 😅

secretName: secret_name actually needs to be a key to the input parameter map, so we can grab the parameter by name at runtime. Newly added changes to driver.go show how we do that.

task = comp()
kubernetes.use_secret_as_volume(
task,
secret_name=secret_name,
mount_path='secretpath',
)

assert json_format.MessageToDict(my_pipeline.platform_spec) == {
'platforms': {
'kubernetes': {
'deploymentSpec': {
'executors': {
'exec-comp': {
'secretAsVolume': [{
'secretName': '{{secret_name}}',
'mountPath': 'secretpath',
'optional': False
}]
}
}
}
}
}
}

def test_preserves_secret_as_env(self):
# checks that use_secret_as_volume respects previously set secrets as env

Expand Down
Loading