Add userid to security context #388

ByronHsu · 2023-04-03T02:59:34Z

TL;DR

Add execution userid to security context

Type

Bug Fix
Feature
Plugin

Tests

Did an e2e test with flyteorg/flyteadmin#549
user_identifier was successfully injected to flyteCRD.

securityContext:
  run_as:
    user_identifier: byhsu

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

flyteorg/flyte#3566

welcome · 2023-04-03T02:59:36Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

codecov · 2023-04-03T03:04:04Z

Codecov Report

Merging #388 (7f58b17) into master (5a3a44f) will increase coverage by 2.37%.
The diff coverage is n/a.

❗ Current head 7f58b17 differs from pull request most recent head 52afed0. Consider uploading reports for the commit 52afed0 to get more accurate results

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   76.11%   78.49%   +2.37%     
==========================================
  Files          18       18              
  Lines        1390     1195     -195     
==========================================
- Hits         1058      938     -120     
+ Misses        280      205      -75     
  Partials       52       52

Flag	Coverage Δ
unittests	`?`

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

see 18 files with indirect coverage changes

wild-endeavor · 2023-04-06T21:43:46Z

protos/flyteidl/core/security.proto

@@ -69,6 +69,9 @@ message Identity {
    // oauth2_client references an oauth2 client. Backend plugins can use this information to impersonate the client when
    // making external calls.
    OAuth2Client oauth2_client = 3;
+
+    // exec_user_id references the id of the user who executes the task.
+    string exec_user_id = 4;


@EngHabu what do you think about this? We've had several requests over the past year and change to add some sort of human-level identity to the wf crd. What do you think about this, just a free-form text? presumably people can make this whatever they want, internal user id, email, jwt token possibly, encrypted encoded bytes, etc.

ic... trying to reason about this... any planned use for it in flyte components?

in our case, we pass the exec_user_id to the flyteplugins because our plugins require this info

i think we should rename this field to just identifier or user_identifier. @EngHabu ?

user_identifier LGTM

How does this work for subworkflows and launch plans triggered by workflows? Will the user identifier be passed down or will flytepropeller's app credentials be passed instead?

Good point. The audit trail should definitely encompass the fact that it came from propeller, but this field should continue to be the original user identifier string. @ByronHsu this is possible with the design we're thinking right? Or were you implying that this should be a list @katrogan ?

I wasn't implying it should be a list but just wanted to understand the implications of hard-coding this to always reference a user if we anticipate app-driven identifiers

i'm okay adding additional fields as they become necessary.

Just to confirm, we will pass this along when Propeller launches LPs from a Workflow Execution...
What will the value be for scheduled workflows? some system/default value?

wild-endeavor

can you update base and regenerate please? also, before merging, can we have and test a sample middleware in the admin repo that will look for the name or email field in the JWT token, and fill this field in? and let's confirm that we see it in the flyteworkflow CRD (kubectl get fly <execid>) but in general looks good i think.

ByronHsu · 2023-04-17T22:37:24Z

@wild-endeavor sure i will test in this pr shortly

wild-endeavor

+1 from me but needs merge from master

wild-endeavor · 2023-04-24T15:20:27Z

did you ever get the time to test the admin change?

EngHabu · 2023-04-24T16:12:53Z

protos/flyteidl/core/security.proto

@@ -69,6 +69,9 @@ message Identity {
    // oauth2_client references an oauth2 client. Backend plugins can use this information to impersonate the client when
    // making external calls.
    OAuth2Client oauth2_client = 3;
+
+    // exec_user_id references the id of the user who executes the task.
+    string exec_user_id = 4;


Just to confirm, we will pass this along when Propeller launches LPs from a Workflow Execution...
What will the value be for scheduled workflows? some system/default value?

Signed-off-by: byhsu <[email protected]>

ByronHsu · 2023-05-02T05:01:02Z

@wild-endeavor e2e test done. Adding in the Tests section in the PR description. Thanks

Signed-off-by: eduardo apolinario <[email protected]>

welcome · 2023-05-06T01:59:05Z

Congrats on merging your first pull request! 🎉

Signed-off-by: byhsu <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: byhsu <[email protected]> Co-authored-by: eduardo apolinario <[email protected]>

* added dynamic_job_spec_uri to dynamic workflow metadata and node execution closure (#360) Signed-off-by: Daniel Rammer <[email protected]> * Use TokenCache in ClientCredentialsTokenSourceProvider (#377) * Init customTokenSource.refreshTime (#381) Signed-off-by: Andrew Dye <[email protected]> * added DataLoadingConfig to K8sPod message (#368) Signed-off-by: Daniel Rammer <[email protected]> * Add Reasons field to TaskExecutionClosure to track time-series of reasons (#382) * added a time-series of reasons to the TaskExecution closure Signed-off-by: Daniel Rammer <[email protected]> * added docs Signed-off-by: Daniel Rammer <[email protected]> * actually finishing docs too Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]> * Create service for runtime metrics (#367) * added span messages Signed-off-by: Daniel Rammer <[email protected]> * added endpoints to service Signed-off-by: Daniel Rammer <[email protected]> * generated mocks Signed-off-by: Daniel Rammer <[email protected]> * removed get task execution metrics rpc Signed-off-by: Daniel Rammer <[email protected]> * added EXECUTION_IDLE category Signed-off-by: Daniel Rammer <[email protected]> * updated PLUGIN_EXECUTION to PLUGIN_RUNTIME Signed-off-by: Daniel Rammer <[email protected]> * removed recorded_at on workflow and node level events Signed-off-by: Daniel Rammer <[email protected]> * added docs for task event reported_at field Signed-off-by: Daniel Rammer <[email protected]> * removed GetNodeExecutionMetrics endpoint - will implement later if necessary Signed-off-by: Daniel Rammer <[email protected]> * updated docs Signed-off-by: Daniel Rammer <[email protected]> * added reported_at for node execution events Signed-off-by: Daniel Rammer <[email protected]> * fixed typo Signed-off-by: Daniel Rammer <[email protected]> * fixed typos and removed dead code Signed-off-by: Daniel Rammer <[email protected]> * updated categories Signed-off-by: Daniel Rammer <[email protected]> * added workflow setup and teardown categories Signed-off-by: Daniel Rammer <[email protected]> * simplified span message and moved to flyteidl.core Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]> * Remove misleading token refresh logic from client credentials token source provider (#383) * Out of core plugin (#378) * Add backend plugin system service Signed-off-by: Kevin Su <[email protected]> * Add backend plugin system service Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * update state Signed-off-by: Kevin Su <[email protected]> * update state Signed-off-by: Kevin Su <[email protected]> * dics Signed-off-by: Kevin Su <[email protected]> * Remove output prefix from get request Signed-off-by: Kevin Su <[email protected]> * update Signed-off-by: Kevin Su <[email protected]> * remove prev state Signed-off-by: Kevin Su <[email protected]> * update proto Signed-off-by: Kevin Su <[email protected]> * remove error message Signed-off-by: Kevin Su <[email protected]> * update comment Signed-off-by: Kevin Su <[email protected]> * make generate Signed-off-by: Kevin Su <[email protected]> * Rename the service Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]> * Feat: Add `ElasticConfig` message type for torch elastic training (#394) * Add elastic config args to pytorch proto Signed-off-by: Fabio Graetz <[email protected]> * Add elastic config message type for torchrun training Signed-off-by: Fabio Graetz <[email protected]> --------- Signed-off-by: Fabio Graetz <[email protected]> Co-authored-by: Fabio Grätz <[email protected]> Co-authored-by: Ketan Umare <[email protected]> * Retract 1.4.x (#397) Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> * Data addresses #minor (#391) Signed-off-by: Yee Hing Tong <[email protected]> * Refactor kf-operator plugins configs and support setting different specs for different replica groups (#386) * refactor kubeflow operators proto Signed-off-by: Yubo Wang <[email protected]> * add back the original proto for backward compatible Signed-off-by: Yubo Wang <[email protected]> * clean up comments Signed-off-by: Yubo Wang <[email protected]> * add kubeflow.rs Signed-off-by: Yubo Wang <[email protected]> * add elastic config Signed-off-by: Yubo Wang <[email protected]> * add command to MPI Signed-off-by: Yubo Wang <[email protected]> * add slots and command to mpi spec Signed-off-by: Yubo Wang <[email protected]> --------- Signed-off-by: Yubo Wang <[email protected]> Co-authored-by: Yubo Wang <[email protected]> * add user_identifier (#388) Signed-off-by: byhsu <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: byhsu <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> * Add envs to execution spec (#400) Signed-off-by: Kevin Su <[email protected]> * Support union and none type in flyteidl (#401) * add support for Union Scalar Signed-off-by: Yubo Wang <[email protected]> * support union type and literals Signed-off-by: Yubo Wang <[email protected]> * change union type extraction Signed-off-by: Yubo Wang <[email protected]> --------- Signed-off-by: Yubo Wang <[email protected]> Co-authored-by: Yubo Wang <[email protected]> Co-authored-by: Kevin Su <[email protected]> * Rename user_identity to execution_identity (#402) Signed-off-by: byhsu <[email protected]> Co-authored-by: byhsu <[email protected]> * make generate Signed-off-by: eduardo apolinario <[email protected]> * Revert "Support union and none type in flyteidl (#401)" This reverts commit 3284f61. Signed-off-by: Eduardo Apolinario <[email protected]> * We should not update flyteidl version in backend components in the case of this branch Signed-off-by: eduardo apolinario <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Andrew Dye <[email protected]> Signed-off-by: Kevin Su <[email protected]> Signed-off-by: Fabio Graetz <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Yubo Wang <[email protected]> Signed-off-by: byhsu <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Dan Rammer <[email protected]> Co-authored-by: Andrew Dye <[email protected]> Co-authored-by: Kevin Su <[email protected]> Co-authored-by: Fabio M. Graetz, Ph.D <[email protected]> Co-authored-by: Fabio Grätz <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> Co-authored-by: Yee Hing Tong <[email protected]> Co-authored-by: Yubo Wang <[email protected]> Co-authored-by: Yubo Wang <[email protected]> Co-authored-by: ByronHsu <[email protected]> Co-authored-by: byhsu <[email protected]>

Signed-off-by: byhsu <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: byhsu <[email protected]> Co-authored-by: eduardo apolinario <[email protected]>

* Adding support for structured dataset (#369) Signed-off-by: pmahindrakar-oss <[email protected]> * added dynamic_job_spec_uri to dynamic workflow metadata and node execution closure (#360) Signed-off-by: Daniel Rammer <[email protected]> * Use TokenCache in ClientCredentialsTokenSourceProvider (#377) * Init customTokenSource.refreshTime (#381) Signed-off-by: Andrew Dye <[email protected]> * added DataLoadingConfig to K8sPod message (#368) Signed-off-by: Daniel Rammer <[email protected]> * Add Reasons field to TaskExecutionClosure to track time-series of reasons (#382) * added a time-series of reasons to the TaskExecution closure Signed-off-by: Daniel Rammer <[email protected]> * added docs Signed-off-by: Daniel Rammer <[email protected]> * actually finishing docs too Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]> * Create service for runtime metrics (#367) * added span messages Signed-off-by: Daniel Rammer <[email protected]> * added endpoints to service Signed-off-by: Daniel Rammer <[email protected]> * generated mocks Signed-off-by: Daniel Rammer <[email protected]> * removed get task execution metrics rpc Signed-off-by: Daniel Rammer <[email protected]> * added EXECUTION_IDLE category Signed-off-by: Daniel Rammer <[email protected]> * updated PLUGIN_EXECUTION to PLUGIN_RUNTIME Signed-off-by: Daniel Rammer <[email protected]> * removed recorded_at on workflow and node level events Signed-off-by: Daniel Rammer <[email protected]> * added docs for task event reported_at field Signed-off-by: Daniel Rammer <[email protected]> * removed GetNodeExecutionMetrics endpoint - will implement later if necessary Signed-off-by: Daniel Rammer <[email protected]> * updated docs Signed-off-by: Daniel Rammer <[email protected]> * added reported_at for node execution events Signed-off-by: Daniel Rammer <[email protected]> * fixed typo Signed-off-by: Daniel Rammer <[email protected]> * fixed typos and removed dead code Signed-off-by: Daniel Rammer <[email protected]> * updated categories Signed-off-by: Daniel Rammer <[email protected]> * added workflow setup and teardown categories Signed-off-by: Daniel Rammer <[email protected]> * simplified span message and moved to flyteidl.core Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]> * Remove misleading token refresh logic from client credentials token source provider (#383) * Out of core plugin (#378) * Add backend plugin system service Signed-off-by: Kevin Su <[email protected]> * Add backend plugin system service Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> * update state Signed-off-by: Kevin Su <[email protected]> * update state Signed-off-by: Kevin Su <[email protected]> * dics Signed-off-by: Kevin Su <[email protected]> * Remove output prefix from get request Signed-off-by: Kevin Su <[email protected]> * update Signed-off-by: Kevin Su <[email protected]> * remove prev state Signed-off-by: Kevin Su <[email protected]> * update proto Signed-off-by: Kevin Su <[email protected]> * remove error message Signed-off-by: Kevin Su <[email protected]> * update comment Signed-off-by: Kevin Su <[email protected]> * make generate Signed-off-by: Kevin Su <[email protected]> * Rename the service Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]> * Feat: Add `ElasticConfig` message type for torch elastic training (#394) * Add elastic config args to pytorch proto Signed-off-by: Fabio Graetz <[email protected]> * Add elastic config message type for torchrun training Signed-off-by: Fabio Graetz <[email protected]> --------- Signed-off-by: Fabio Graetz <[email protected]> Co-authored-by: Fabio Grätz <[email protected]> Co-authored-by: Ketan Umare <[email protected]> * Retract 1.4.x (#397) Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> * Data addresses #minor (#391) Signed-off-by: Yee Hing Tong <[email protected]> * Refactor kf-operator plugins configs and support setting different specs for different replica groups (#386) * refactor kubeflow operators proto Signed-off-by: Yubo Wang <[email protected]> * add back the original proto for backward compatible Signed-off-by: Yubo Wang <[email protected]> * clean up comments Signed-off-by: Yubo Wang <[email protected]> * add kubeflow.rs Signed-off-by: Yubo Wang <[email protected]> * add elastic config Signed-off-by: Yubo Wang <[email protected]> * add command to MPI Signed-off-by: Yubo Wang <[email protected]> * add slots and command to mpi spec Signed-off-by: Yubo Wang <[email protected]> --------- Signed-off-by: Yubo Wang <[email protected]> Co-authored-by: Yubo Wang <[email protected]> * add user_identifier (#388) Signed-off-by: byhsu <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: byhsu <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> * Add envs to execution spec (#400) Signed-off-by: Kevin Su <[email protected]> * Support union and none type in flyteidl (#401) * add support for Union Scalar Signed-off-by: Yubo Wang <[email protected]> * support union type and literals Signed-off-by: Yubo Wang <[email protected]> * change union type extraction Signed-off-by: Yubo Wang <[email protected]> --------- Signed-off-by: Yubo Wang <[email protected]> Co-authored-by: Yubo Wang <[email protected]> Co-authored-by: Kevin Su <[email protected]> * Rename user_identity to execution_identity (#402) Signed-off-by: byhsu <[email protected]> Co-authored-by: byhsu <[email protected]> * Single literal in GetDataResponse (#404) Signed-off-by: Yee Hing Tong <[email protected]> * Add namespace to execution system metadata (#406) Signed-off-by: Katrina Rogan <[email protected]> * Add oauth2 http proxy client (#405) Signed-off-by: byhsu <[email protected]> * Rename externalPluginService to AgentService (#410) * Rename externalPluginService to AgentService Signed-off-by: Kevin Su <[email protected]> * nit Signed-off-by: Kevin Su <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]> * Add external_plugin_service proto back to the idl (#413) * Add external-plugin-service proto back to the idl Signed-off-by: Kevin Su <[email protected]> * update idl Signed-off-by: Kevin Su <[email protected]> * update idll Signed-off-by: Kevin Su <[email protected]> * update idll Signed-off-by: Kevin Su <[email protected]> * AsyncAgentService Signed-off-by: Kevin Su <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]> * Rerun make generate Signed-off-by: eduardo apolinario <[email protected]> --------- Signed-off-by: pmahindrakar-oss <[email protected]> Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Andrew Dye <[email protected]> Signed-off-by: Kevin Su <[email protected]> Signed-off-by: Fabio Graetz <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Yubo Wang <[email protected]> Signed-off-by: byhsu <[email protected]> Signed-off-by: Katrina Rogan <[email protected]> Co-authored-by: pmahindrakar-oss <[email protected]> Co-authored-by: Dan Rammer <[email protected]> Co-authored-by: Andrew Dye <[email protected]> Co-authored-by: Kevin Su <[email protected]> Co-authored-by: Fabio M. Graetz, Ph.D <[email protected]> Co-authored-by: Fabio Grätz <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Co-authored-by: eduardo apolinario <[email protected]> Co-authored-by: Yee Hing Tong <[email protected]> Co-authored-by: Yubo Wang <[email protected]> Co-authored-by: Yubo Wang <[email protected]> Co-authored-by: ByronHsu <[email protected]> Co-authored-by: byhsu <[email protected]> Co-authored-by: Katrina Rogan <[email protected]>

Signed-off-by: byhsu <[email protected]> Signed-off-by: eduardo apolinario <[email protected]> Co-authored-by: byhsu <[email protected]> Co-authored-by: eduardo apolinario <[email protected]>

ByronHsu force-pushed the github/add-userid branch from e9f5fa6 to 50a09d5 Compare April 3, 2023 03:02

wild-endeavor reviewed Apr 6, 2023

View reviewed changes

wild-endeavor approved these changes Apr 13, 2023

View reviewed changes

wild-endeavor approved these changes Apr 24, 2023

View reviewed changes

EngHabu approved these changes Apr 24, 2023

View reviewed changes

ByronHsu force-pushed the github/add-userid branch 2 times, most recently from f1d134d to 75d3a25 Compare May 2, 2023 04:58

add user_identifier

52b79fc

Signed-off-by: byhsu <[email protected]>

ByronHsu force-pushed the github/add-userid branch from 75d3a25 to 52b79fc Compare May 2, 2023 05:00

wild-endeavor approved these changes May 5, 2023

View reviewed changes

eapolinario approved these changes May 5, 2023

View reviewed changes

Merge remote-tracking branch 'origin' into github/add-userid

52afed0

Signed-off-by: eduardo apolinario <[email protected]>

eapolinario enabled auto-merge (squash) May 5, 2023 23:38

eapolinario merged commit 44b6bc6 into flyteorg:master May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add userid to security context #388

Add userid to security context #388

ByronHsu commented Apr 3, 2023 •

edited

Loading

welcome bot commented Apr 3, 2023

codecov bot commented Apr 3, 2023 •

edited

Loading

wild-endeavor Apr 6, 2023

EngHabu Apr 13, 2023

ByronHsu Apr 13, 2023

wild-endeavor Apr 13, 2023

ByronHsu Apr 13, 2023

katrogan Apr 13, 2023

wild-endeavor Apr 14, 2023

katrogan Apr 14, 2023

wild-endeavor Apr 17, 2023

EngHabu Apr 24, 2023

wild-endeavor left a comment

ByronHsu commented Apr 17, 2023

wild-endeavor left a comment •

edited

Loading

wild-endeavor commented Apr 24, 2023

EngHabu Apr 24, 2023

ByronHsu commented May 2, 2023

welcome bot commented May 6, 2023

Add userid to security context #388

Add userid to security context #388

Conversation

ByronHsu commented Apr 3, 2023 • edited Loading

TL;DR

Type

Tests

Are all requirements met?

Complete description

Tracking Issue

welcome bot commented Apr 3, 2023

codecov bot commented Apr 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor left a comment

Choose a reason for hiding this comment

ByronHsu commented Apr 17, 2023

wild-endeavor left a comment • edited Loading

Choose a reason for hiding this comment

wild-endeavor commented Apr 24, 2023

Choose a reason for hiding this comment

ByronHsu commented May 2, 2023

welcome bot commented May 6, 2023

ByronHsu commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

wild-endeavor left a comment •

edited

Loading