[KFP] Obscuring KFP env credentials #6370

adamdelman · 2024-09-19T11:22:57Z

Modify the compiled YAML on the server side to include references to kubernetes secrets instead of plain text values

https://iguazio.atlassian.net/browse/ML-7735

pipeline specs, instead set them on creation in the server side

…mlrun#6363) * [Nuclio] Raise error on multiple triggers + explicit ack + older nuclio [ML-7763](https://iguazio.atlassian.net/browse/ML-7763) * Update minimal nuclio version * [Nuclio] Raise error on multiple triggers + explicit ack + old nuclio [ML-7763](https://iguazio.atlassian.net/browse/ML-7763) * Remove redundant code

liranbg

some suggestions and fixes, overall - looking very well!

tests/api/api/test_pipelines.py

server/api/crud/pipelines.py

server/api/api/utils.py

pipeline-adapters/mlrun-pipelines-kfp-v1-8/src/mlrun_pipelines/patcher.py

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/models.py

theSaarco

Looks good, but there are some changes needed, and I'm also not so sure about some of the changes done and why they're needed.
Also, the original bug shows there are 2 locations that need cleaning - the env vars, and the spec of the function when a deploy_function is done. In this PR I can only see you handling the 1st case, not the 2nd. Are you planning to do that in a separate PR?

mlrun/projects/pipelines.py

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

pipeline-adapters/mlrun-pipelines-kfp-v1-8/src/mlrun_pipelines/patcher.py

server/api/api/utils.py

server/api/crud/pipelines.py

moranbental

Looks good! some suggestions

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/models.py

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

alonmr

Very nice! small comments

pipeline-adapters/mlrun-pipelines-kfp-v1-8/src/mlrun_pipelines/patcher.py

tests/api/api/test_pipelines.py

mlrun/projects/pipelines.py

tests/api/conftest.py

mlrun/common/schemas/workflow.py

theSaarco

Looks very good. Have some relatively minor comments.
Also - still can't see any reference to replacing secrets from function spec in the deploy-function case. Will this be treated in a different PR?

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

mlrun/projects/pipelines.py

liranbg

up

mlrun/projects/pipelines.py

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

…d/ML-7735

alonmr

Looks very well! Some comments/questions

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

server/api/crud/pipelines.py

tests/api/api/test_pipelines.py

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

theSaarco

Looks good - I'm approving as functionality seems complete, but have some suggestions for improvements that would make this cleaner.

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py

theSaarco · 2024-09-30T14:45:22Z

tests/api/api/test_pipelines.py

A thought that could improve this test - since the kfp_client_mock mocks all calls to the KFP Client, you could check the call actually being generated and verify that the contents of the pipeline really has the secrets obfuscated. This is a more direct and complete check than verifying the secrets-mock has this auth secret being created.
It's not a must, but if it's simple enough to do, would try to add that here.

The base branch was changed.

adamdelman and others added 5 commits September 19, 2024 11:02

[KFP] Prevent mlrun secrets from being set on KFP

93c1ac1

pipeline specs, instead set them on creation in the server side

[K8s] Retry paginated list when token is expired (mlrun#6360)

43beab8

Revert "[Artifacts] Limit the artifact body size when logging artifac…

400973b

…ts" (mlrun#6364)

Merge branch 'development' of github.com:mlrun/mlrun into adamd/ML-7735

841b5db

adamdelman force-pushed the adamd/ML-7735 branch 5 times, most recently from d432378 to 4464604 Compare September 19, 2024 17:23

liranbg requested changes Sep 20, 2024

View reviewed changes

theSaarco requested changes Sep 22, 2024

View reviewed changes

moranbental requested changes Sep 22, 2024

View reviewed changes

adamdelman force-pushed the adamd/ML-7735 branch 4 times, most recently from 61aa4f1 to e5aa2b1 Compare September 23, 2024 14:12

liranbg reviewed Sep 23, 2024

View reviewed changes

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py Outdated Show resolved Hide resolved

alonmr reviewed Sep 24, 2024

View reviewed changes

pipeline-adapters/mlrun-pipelines-kfp-v1-8/src/mlrun_pipelines/patcher.py Outdated Show resolved Hide resolved

tests/api/api/test_pipelines.py Outdated Show resolved Hide resolved

mlrun/projects/pipelines.py Outdated Show resolved Hide resolved

alonmr reviewed Sep 24, 2024

View reviewed changes

tests/api/conftest.py Outdated Show resolved Hide resolved

adamdelman force-pushed the adamd/ML-7735 branch from e5aa2b1 to 662ce51 Compare September 24, 2024 07:57

alonmr reviewed Sep 24, 2024

View reviewed changes

mlrun/common/schemas/workflow.py Outdated Show resolved Hide resolved

adamdelman force-pushed the adamd/ML-7735 branch from 662ce51 to 0237a5d Compare September 24, 2024 08:32

Merge branch 'development' of github.com:mlrun/mlrun into adamd/ML-7735

5824e43

adamdelman force-pushed the adamd/ML-7735 branch from 0237a5d to 5824e43 Compare September 24, 2024 08:35

theSaarco requested changes Sep 24, 2024

View reviewed changes

pipeline-adapters/mlrun-pipelines-kfp-common/src/mlrun_pipelines/common/ops.py Outdated Show resolved Hide resolved

mlrun/projects/pipelines.py Outdated Show resolved Hide resolved

Merge branch 'development' of github.com:mlrun/mlrun into adamd/ML-7735

c8879c4

adamdelman force-pushed the adamd/ML-7735 branch from 202301c to c52e8bd Compare September 25, 2024 09:20

liranbg requested changes Sep 25, 2024

View reviewed changes

Merge branch 'adamd/ML-7735' of github.com:adamdelman/mlrun into adam…

1c64c49

…d/ML-7735

adamdelman force-pushed the adamd/ML-7735 branch from e6545a3 to 1c64c49 Compare September 29, 2024 08:48

Merge branch 'development' of github.com:mlrun/mlrun into adamd/ML-7735

ae38529

adamdelman force-pushed the adamd/ML-7735 branch 4 times, most recently from d4de188 to ad7f44a Compare September 30, 2024 13:16

alonmr reviewed Sep 30, 2024

View reviewed changes

adamdelman force-pushed the adamd/ML-7735 branch 2 times, most recently from 54f644e to 6c74452 Compare September 30, 2024 14:31

theSaarco previously approved these changes Sep 30, 2024

View reviewed changes

Merge branch 'development' of github.com:mlrun/mlrun into adamd/ML-7735

db2e34b

adamdelman force-pushed the adamd/ML-7735 branch from 6c74452 to db2e34b Compare September 30, 2024 15:14

adamdelman requested a review from liranbg September 30, 2024 15:15

Merge branch 'development' into adamd/ML-7735

afac06a

liranbg previously approved these changes Sep 30, 2024

View reviewed changes

liranbg changed the title ~~[KFP] Prevent mlrun secrets from being set on KFP pipeline specs, instead set them on pipeline creation in the server side~~ [KFP] Obscuring KFP env credentials Sep 30, 2024

liranbg changed the base branch from development to 1.7.x September 30, 2024 20:18

liranbg changed the base branch from 1.7.x to development September 30, 2024 20:19

liranbg merged commit 78730c3 into mlrun:development Sep 30, 2024

liranbg pushed a commit to liranbg/mlrun that referenced this pull request Sep 30, 2024

[KFP] Obscuring KFP env credentials (mlrun#6370)

61784af

liranbg mentioned this pull request Sep 30, 2024

[KFP] Obscuring KFP env credentials [1.7.x] #6458

Merged

adamdelman deleted the adamd/ML-7735 branch May 27, 2025 14:58

adamdelman mentioned this pull request Nov 5, 2025

[Kaniko] Add S3/MinIO Build Context Support and Improve Builder Environment Handling #8849

Merged

1 task

[KFP] Obscuring KFP env credentials #6370

[KFP] Obscuring KFP env credentials #6370

Conversation

adamdelman commented Sep 19, 2024 • edited by liranbg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liranbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theSaarco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moranbental left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alonmr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theSaarco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liranbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alonmr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theSaarco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theSaarco Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

adamdelman commented Sep 19, 2024 •

edited by liranbg

Loading

theSaarco Sep 30, 2024 •

edited

Loading