pipelines icon indicating copy to clipboard operation
pipelines copied to clipboard

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

Open DharmitD opened this issue 1 year ago • 12 comments

Description of your changes: Resolves https://github.com/kubeflow/pipelines/issues/10914

Attempting to pass the secret_name parameter in the dsl compile this way:

from kfp import dsl
from kfp import kubernetes

@dsl.component(base_image="python:3.10")
def print_secret():
    with open('/mnt/my_vol') as f:
        print(f.read())

@dsl.pipeline
def pipeline(secret_name: str = 'my-secret'):
    task = print_secret()
    kubernetes.use_secret_as_volume(task,
                                    secret_name=secret_name,
                                    mount_path='/mnt/my_vol')

results in this error:

File "/home/ddalvi/PycharmProjects/pythonProject2/main.py", line 23, in pipeline
    kubernetes.use_secret_as_volume(task,
  File "/home/ddalvi/miniconda3/envs/kfp_pipe_secret_env/lib/python3.11/site-packages/kfp/kubernetes/secret.py", line 81, in use_secret_as_volume
    secret_as_vol = pb.SecretAsVolume(
                    ^^^^^^^^^^^^^^^^^^
TypeError: bad argument type for built-in operation

The secret_name parameter was originally expected to be a string (str). However, in this case, it was being provided as a an object of the PipelineParameterChannel class , leading to a type mismatch error when the code tried to use it as a string. Updated secret_name to accept either a str or PipelineParameterChannel using Union[str, PipelineParameterChannel] and added a check to convert PipelineParameterChannel to its string representation.

Furthermore,

  • Implemented support for dynamically specifying secret names in the use_secret_as_volume function.
  • Updated driver code to handle secret name substitution at runtime based on input parameters.
  • Introduced a {{secret_name}} template string representation for secret names in the compiled DSL.
  • Added a test to validate secret name template creation in IR.

Refer to this comment to understand why the driver code change is required.

Please refer to a PoC wherein we've listed out different test cases - four different pairs of DSL compile code and resulting pipeline yaml IRs with which we tested this update.

Collaborated with @gregsheremeta for this contribution, it was a joint effort. Thanks Greg for your contributions! :)

Checklist:

DharmitD avatar Jul 25 '24 03:07 DharmitD

/lgtm

I also verified it, so /verified :)

@chensun mind taking a quick look?

gregsheremeta avatar Jul 25 '24 13:07 gregsheremeta

Please create a test for this scenario

good call

gregsheremeta avatar Jul 25 '24 13:07 gregsheremeta

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amadhusu Once this PR has been reviewed and has the lgtm label, please assign chensun for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

google-oss-prow[bot] avatar Aug 14 '24 15:08 google-oss-prow[bot]

@chensun could you please take a look and let us know if this templating method looks good to you? Thanks!

DharmitD avatar Aug 15 '24 13:08 DharmitD

/rerun-all

DharmitD avatar Aug 15 '24 13:08 DharmitD

/lgtm

diegolovison avatar Aug 16 '24 13:08 diegolovison

@chensun could you please take a look? Thanks!

DharmitD avatar Aug 16 '24 20:08 DharmitD

New changes are detected. LGTM label has been removed.

google-oss-prow[bot] avatar Aug 19 '24 04:08 google-oss-prow[bot]

/rerun-all

DharmitD avatar Aug 19 '24 22:08 DharmitD

@chensun could you please take a look and let us know if this templating method looks good to you? Thanks!

DharmitD avatar Aug 22 '24 17:08 DharmitD

@chensun bumping :)

cc: @gregsheremeta @rimolive

DharmitD avatar Aug 23 '24 22:08 DharmitD

I've tested the implementation and it works as intended, however I think it's worth getting input from @chensun on the approach, I've left a comment here as well.

HumairAK avatar Aug 28 '24 13:08 HumairAK

closing in lieu of https://github.com/kubeflow/pipelines/pull/11621

HumairAK avatar Feb 12 '25 16:02 HumairAK