[Feat] add user annotations to k8s objects

[ttitsworth-lila]

Why are the changes needed?

Presently when submitting workflows via Flyte, it's hard to know who submitted what. This PR addresses one of two concerns:

1. I can't filter by users in the Flyte Console
2. I can't see a user identifier in k8s

Why do I/we care about 2? Let's say you have two teams (A & B), if team A has access to 10 resources and team B has access to 1000 resources and we know who is in each team (exclusively). One can create further admission controls for any user in team A that requests more resources than are allotted to that team. Furthermore, this can be tracked properly with Kueue.

Today when I submit a Flyte workflow I have no way to actually know who submitted what from a k8s perspective because principal information is not passed to the downstream k8s flyteworkflow object.

What changes were proposed in this pull request?

This PR adds a parameter that enables a new annotation to be injected into each flyteworkflow object that contains the user principal, which usually carries oauth information. Additionally, it allows customization of the annotation prefix.

How was this patch tested?

I added some unit tests, and I also tested in production with the flyte-core chart on EKS and using Keycloak:

$ kubectl describe flyteworkflow -n flyte-workspace ax5hwpvbmwnkhdkmq67m | grep flyte.ai
Annotations:  flyte.ai/user: [email protected]

Labels

Please add one or more of the following labels to categorize your PR:

• added: For new features.
• changed: For changes in existing functionality.
• deprecated: For soon-to-be-removed features.
• removed: For features being removed.
• fixed: For any bug fixed.
• security: In case of vulnerabilities

This is important to improve the readability of release notes.

Setup process

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

Summary by Bito

• This pull request introduces a feature for injecting user annotations into Flyte workflows, enhancing visibility and accountability in multi-team environments, while also addressing potential risks related to user data handling in Kubernetes.
• It modifies the execution manager to handle user identity more effectively, ensuring correct functionality and potentially impacting resource management.
• The changes include updates to the application configuration to support new identity annotation fields and customization options for annotation prefixes.
• Improvements to the execution manager for user identity handling, updates to configuration checksums, and shared secrets for better resource management and security in Kubernetes are also included.
• Unit tests have been added to ensure the functionality works as intended, validating the new feature.
• Overall, this pull request introduces new features related to user annotations, execution management, and resource management in Kubernetes.

[welcome]

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.76%. Comparing base (27c5f12) to head (3689269).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6710      +/-   ##
==========================================
+ Coverage   59.72%   59.76%   +0.03%     
==========================================
  Files         929      929              
  Lines       57995    58051      +56     
==========================================
+ Hits        34638    34694      +56     
  Misses      20205    20205              
  Partials     3152     3152              

Flag	Coverage Δ
unittests-datacatalog	60.30% <ø> (ø)
unittests-flyteadmin	58.04% <100.00%> (+0.13%)	⬆️
unittests-flytecopilot	43.16% <ø> (ø)
unittests-flytectl	65.31% <ø> (ø)
unittests-flyteidl	78.64% <ø> (ø)
unittests-flyteplugins	62.02% <ø> (ø)
unittests-flytepropeller	55.56% <ø> (ø)
unittests-flytestdlib	64.02% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ttitsworth-lila]

Hey @davidmirror-ops it looks like there's an issue with pulling helm on a bunch of the tests, is this expected?

I'm working on updating the code coverage to get things green.

[davidmirror-ops]

@ttitsworth-lila yes, that's still a limitation on the current version of CI. Could you run make helm locally and push the changes?

[ttitsworth-lila]

@davidmirror-ops no problem, done.

[Sovietaced]

We do something similar internally since the principal is not passed from the control plane to the data plane, and thus can't be rendered into the k8s manifests in propeller. I will review this more deeply later.

[ttitsworth-lila]

We do something similar internally since the principal is not passed from the control plane to the data plane, and thus can't be rendered into the k8s manifests in propeller. I will review this more deeply later.

@Sovietaced Thanks for the comment. I'd love to understand more about what you mean. Presently I'm testing this in a multi-cluster setup where my above method is able to pass that information. What IDP provider do you use internally?

[Sovietaced]

@Sovietaced Thanks for the comment. I'd love to understand more about what you mean. Presently I'm testing this in a multi-cluster setup where my above method is able to pass that information. What IDP provider do you use internally?

Not sure what you mean with regards to IDP. What I'm trying to say is that flyte admin (the control plane) has the identity information when the user calls the API to create an execution. None of that information is passed down into the workflow resource and as a result is unusable by the flyte propeller (the data plane). So like you've done here we inject some of that information into the workflow annotations which isn't super elegant, but it works.

Arguably the workflow spec should embed some information about the user identity in a more structured way.

[pmahindrakar-oss]

So like you've done here we inject some of that information into the workflow annotations which isn't super elegant, but it works.
Arguably the workflow spec should embed some information about the user identity in a more structured way.

Like your suggestion @Sovietaced , but would require a lot more changes across the stack to include in the spec but the current PR can be considered incremental and adds a benefit for having identity info in the dataplane.

[ttitsworth-lila]

Thanks for the review @pmahindrakar-oss, I just pushed a change to improve the coverage. I'm having some issues running helm make:

Save error occurred:  could not find : chart docker-registry not found in https://helm.twun.io/: looks like "https://helm.twun.io/" is not a valid chart repository or cannot be reached: Get "https://helm.twun.io/index.yaml": dial tcp: lookup helm.twun.io on 10.255.255.254:53: no such host

Hope this is acceptable, and would appreciate some help with landing this PR :)

[Sovietaced]

Thanks for the review @pmahindrakar-oss, I just pushed a change to improve the coverage. I'm having some issues running helm make:

Save error occurred:  could not find : chart docker-registry not found in https://helm.twun.io/: looks like "https://helm.twun.io/" is not a valid chart repository or cannot be reached: Get "https://helm.twun.io/index.yaml": dial tcp: lookup helm.twun.io on 10.255.255.254:53: no such host

Hope this is acceptable, and would appreciate some help with landing this PR :)

The DNS issue looks like possible company VPN / firewall not resolving approved domains

[pingsutw]

The DNS issue looks like possible company VPN / firewall not resolving approved domains

fixed it here #6726

[Sovietaced]

Got some conflicts

[flyte-bot]

Bito Review Skipped - No Changes Detected

Bito didn't review this pull request because we did not detect any changes in the pull request to review.