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

[DharmitD]

Description of your changes:
Resolves #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:

• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

[gregsheremeta]

/lgtm

I also verified it, so
/verified
:)

@chensun mind taking a quick look?

[diegolovison]

Please create a test for this scenario

[hbelmiro]

/lgtm

[gregsheremeta]

Please create a test for this scenario

good call

[amadhusu]

Looks good to me !

[diegolovison]

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

[DharmitD]

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

[gregsheremeta]

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

[gregsheremeta]

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.

[google-oss-prow]

[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:

• backend/OWNERS
• kubernetes_platform/OWNERS

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

[DharmitD]

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

[DharmitD]

/rerun-all

[diegolovison]

/lgtm

[DharmitD]

@chensun could you please take a look? Thanks!

[google-oss-prow]

New changes are detected. LGTM label has been removed.

[DharmitD]

/rerun-all

[DharmitD]

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

[DharmitD]

@chensun bumping :)

cc: @gregsheremeta @rimolive

[HumairAK]

I am uncertain about this approach, we're essentially implementing templating, but what maybe considered somewhat hacky. Have we considered other approaches?

For example, for names retrieved via parameterschannels, we can include a new field for SecretAsEnv called "secret_name_parameter".

Based on last community call @chensun had a proposal for an alternative approach as well, so I would be interested in hearing about this as well.

[gregsheremeta]

we can include a new field for SecretAsEnv called "secret_name_parameter".

that does feel cleaner to me

[HumairAK]

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.

[chensun]

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