Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

[Bigquery] Add support for impersonation of GSA bound to task's KSA #355

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

jeevb
Copy link
Contributor

@jeevb jeevb commented Jun 2, 2023

Addresses: flyteorg/flyte#3736

Incorporates lost changes from #161 with some some modern bits.

Smoke tested on GCP test cluster with flytesnacks BQ workflow. In the second snapshot, the correct GSA, userflyterole, is used.
Screenshot 2023-06-02 at 12 13 22 PM
Screenshot 2023-06-02 at 12 13 54 PM

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #355 (fa0f9fd) into master (c499a48) will increase coverage by 1.08%.
The diff coverage is 27.86%.

❗ Current head fa0f9fd differs from pull request most recent head 0f509a7. Consider uploading reports for the commit 0f509a7 to get more accurate results

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   62.80%   63.89%   +1.08%     
==========================================
  Files         148      152       +4     
  Lines       12701    10355    -2346     
==========================================
- Hits         7977     6616    -1361     
+ Misses       4112     3126     -986     
- Partials      612      613       +1     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
go/tasks/pluginmachinery/google/config.go 0.00% <ø> (ø)
...inmachinery/google/default_token_source_factory.go 0.00% <0.00%> (ø)
...sks/pluginmachinery/google/token_source_factory.go 0.00% <0.00%> (ø)
...gke_task_workload_identity_token_source_factory.go 34.00% <34.00%> (ø)

... and 131 files with indirect coverage changes

…uery plugin

Signed-off-by: Jeev B <jeevb@users.noreply.github.com>
Signed-off-by: Jeev B <jeevb@users.noreply.github.com>
@jeevb jeevb marked this pull request as ready for review June 3, 2023 04:46
Copy link

@ctso ctso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks pretty reasonable to me. We should make sure we update the docs to call out this new gke-task-workload-identity token source. We should be sure to mention that the KSA that (I think...) flytepropeller uses must have enough permissions to read ServiceAccount objects from the k8s API.

@jeevb
Copy link
Contributor Author

jeevb commented Jun 6, 2023

And also that the GSA bound to Flytepropeller's KSA must be able to impersonate GSAs bound to the respective task KSAs.

Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome.

@jeevb jeevb merged commit aa1ed67 into master Jun 8, 2023
@jeevb jeevb deleted the jeev/bq-gke-task-wi branch June 8, 2023 15:58
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
…355)

* Support impersonation of GKE task workload identity bound GSA in BigQuery plugin

Signed-off-by: Jeev B <jeevb@users.noreply.github.com>

* Fix linting

Signed-off-by: Jeev B <jeevb@users.noreply.github.com>

---------

Signed-off-by: Jeev B <jeevb@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants