Gcs remote data #121

honnix · 2020-09-01T08:52:13Z

TL;DR

Add support of signed GCS URL.
This is a continuation of #81 because CI doesn't seem to work in forked repo.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Right now only AWS/S3 presigned URL is support. This PR adds support for GCS URL.

The signing part looks a bit funky, mainly due to GCP lib does not support signing URL without
materialised service account key. So this PR uses https://cloud.google.com/iam/docs/reference/credentials/rest/v1/projects.serviceAccounts/signBlob directly.

SigningPrincipal refers to the service account that signs the URL, as well as reads object size. Service account associated with flyteadmin instance needs to have the role on the principal.

Tracking Issue

Follow-up issue

NA

Although this doesn't seem to be required, it is better to do things according to API doc.

This will make sure only the `signingPrincipal` needs readonly permission on the object instead of the service account associated with flyteadmin.

This is because of introducing of google.golang.org/protobuf

codecov-commenter · 2020-09-01T13:06:26Z

Codecov Report

Merging #121 into master will decrease coverage by 0.15%.
The diff coverage is 52.43%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   62.39%   62.24%   -0.16%     
==========================================
  Files         104      105       +1     
  Lines        7752     7840      +88     
==========================================
+ Hits         4837     4880      +43     
- Misses       2345     2385      +40     
- Partials      570      575       +5

Flag	Coverage Δ
#unittests	`62.24% <52.43%> (-0.16%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/data/implementations/gcp_remote_url.go	`52.43% <52.43%> (ø)`
pkg/repositories/config/migrations.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf159e5...02c9eae. Read the comment docs.

pkg/data/implementations/gcp_remote_url.go

honnix · 2020-09-01T15:39:03Z

pkg/manager/impl/util/digests_test.go

@@ -46,8 +46,8 @@ var compiledTaskDigest = []byte{
 	0xa, 0x22, 0x80, 0xb1, 0x8, 0x44, 0x53, 0xf3, 0xca, 0x60, 0x4, 0xf7, 0x6f}

 var compiledWorkflowDigest = []byte{
-	0x1c, 0x66, 0x45, 0x89, 0x8f, 0x5, 0xa5, 0x4f, 0xf5, 0xba, 0x6f, 0xd9, 0xd, 0x70, 0xf6, 0x86, 0xf, 0x8e, 0x7e, 0x1b,
-	0x80, 0x1d, 0xb9, 0x59, 0xe, 0x3, 0x50, 0x4d, 0x64, 0xeb, 0x13, 0xa2}
+	0xeb, 0x66, 0x44, 0xe8, 0x1c, 0xa8, 0x51, 0x7d, 0x3f, 0x33, 0xf0, 0x77, 0x95, 0x24, 0x84, 0xc2, 0xbe, 0x79, 0xcd,


This is due to introducing of google.golang.org/protobuf as transitive dependency.

interesting, @katrogan will this affect all existing digests to be invalidated, should be fine, but we might cause some re-registrations?

This is where I most concerned about as well. I can dig a bit further to see whether this can be avoided.

oh interesting! this could be a problem because some folks will get a warning about the digest changing for a workflow with the same identifier, so they will have to change the version parameter to reregister - which is unfortunate

I managed to revert this change by downgrading a few deps, in latest commit. Still we will need to face this thing later: #121 (comment)

kumare3 · 2020-09-02T16:14:35Z

pkg/data/factory.go

@@ -45,6 +46,12 @@ func GetRemoteDataHandler(cfg RemoteDataHandlerConfig) RemoteDataHandler {
 		return &remoteDataHandler{
 			remoteURL: implementations.NewAWSRemoteURL(awsConfig, presignedURLDuration),
 		}
+	case common.GCP:


@katrogan / @honnix with the new change to return the entire data as part of the getData API, we should probably think of making signing optional (we have to keep it around to ensure that very large datasets can still be served). By optional I mean, that if it is not specified, the signing should just be skipped.

I do not think my comment needs to be added as part of this commit, but as a follow up? @katrogan what do you think?

yeah that was what i had in the original idl pr - adding a mode to specify what data we want returned and making only the signed url a non-default option. returning the signed url data optionally sounds good to me

kumare3

One comment, but LGTM

katrogan · 2020-09-02T16:51:19Z

pkg/data/implementations/gcp_remote_url.go

+			codes.Internal, "failed to get object size for %s with %v", uri, err)
+	}
+
+	// The second return argument here is the GetObjectOutput, which we don't use below.


nit: is this comment relevant?

Oops, sorry. Copy pasta. Will fix.

katrogan · 2020-09-02T17:16:32Z

pkg/data/implementations/gcp_remote_url.go

+}
+
+type gcsClientWrapper struct {
+	delegate *gcs.Client


hey @honnix I'm not super familiar with the delegate pattern, what's the reason for using it here?

is this so you can mock out the bucket like you commented below?

Yes. That is the only way I could figure out how to test this fluid API.

katrogan · 2020-09-02T17:17:49Z

pkg/data/implementations/gcp_remote_url.go

+		SignBytes: func(b []byte) ([]byte, error) {
+			req := &credentialspb.SignBlobRequest{
+				Payload: b,
+				Name:    "projects/-/serviceAccounts/" + g.signingPrincipal,


what's the meaning of this string prefix?

It's a GCP resource name: https://cloud.google.com/iam/docs/reference/rest/v1/projects.serviceAccounts/signBlob

Basically, it means the service account in some GCP project (-) that doesn't matter.

honnix · 2020-09-02T19:06:45Z

go.mod

@@ -3,6 +3,8 @@ module github.com/lyft/flyteadmin
 go 1.13

 require (
+	cloud.google.com/go v0.56.0


This is the last version staying on protobuf v1.3.5. Since v1.4.0, github.com/golang/protobuf started to depend on google.golang.org/protobuf: https://github.com/golang/protobuf/blob/v1.4.0/ptypes/timestamp/timestamp.pb.go#L9

So sooner or later we will need to face this issue again.

oh this is awesome, thank you for finding it

* master: Gcs remote data (#121) Do not depend on GOPATH to locate test data (#122) Add index to optimize for list task executions for node execution (#120) Grpc health checking (#118) Allow random cluster selection when no override (#117)

* signed GCS URL Impersonate the signingPrincipal to get object attributes and sign the GCS URL This will make sure only the `signingPrincipal` needs readonly permission on the object instead of the service account associated with flyteadmin.

honnix added 5 commits September 1, 2020 09:46

[wip] signed GCS URL

e41ea37

fix lint error

c08ced5

go mod tidy

c787862

Upgrade modules

d485760

Use proper resource name

32cd3e5

Although this doesn't seem to be required, it is better to do things according to API doc.

honnix requested review from anandswaminathan, EngHabu, katrogan, kumare3, matthewphsmith and wild-endeavor as code owners September 1, 2020 08:52

honnix mentioned this pull request Sep 1, 2020

[wip] signed GCS URL #81

Closed

8 tasks

honnix added 7 commits September 1, 2020 12:16

Impersonate the signingPrincipal to get object attributes

dc6dfce

This will make sure only the `signingPrincipal` needs readonly permission on the object instead of the service account associated with flyteadmin.

Implementation sounds better

ab7a74b

Test factory

371e463

Start unit test

bd81d84

Fix digest change

41cd0e5

This is because of introducing of google.golang.org/protobuf

Extract interfaces

efc0c1e

This requires ADC

2e5bd3d

honnix commented Sep 1, 2020

View reviewed changes

pkg/data/implementations/gcp_remote_url.go Show resolved Hide resolved

Unit test it

07c3c1c

honnix commented Sep 1, 2020

View reviewed changes

honnix added 2 commits September 1, 2020 17:43

Fix copy pasta

b3ca5ff

Test more

93d8794

kumare3 reviewed Sep 2, 2020

View reviewed changes

kumare3 previously approved these changes Sep 2, 2020

View reviewed changes

katrogan reviewed Sep 2, 2020

View reviewed changes

honnix added 2 commits September 2, 2020 20:18

Remove misleading comment

5ae7ee4

Downgrade deps

9d62d5e

honnix dismissed kumare3’s stale review via 9d62d5e September 2, 2020 19:04

honnix commented Sep 2, 2020

View reviewed changes

Fix lint

02c9eae

katrogan approved these changes Sep 2, 2020

View reviewed changes

honnix merged commit 6e2c136 into master Sep 3, 2020

honnix deleted the gcs-remote-data branch September 3, 2020 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gcs remote data #121

Gcs remote data #121

honnix commented Sep 1, 2020 •

edited

Loading

codecov-commenter commented Sep 1, 2020 •

edited

Loading

honnix Sep 1, 2020

kumare3 Sep 2, 2020

honnix Sep 2, 2020

katrogan Sep 2, 2020

honnix Sep 2, 2020

kumare3 Sep 2, 2020

katrogan Sep 2, 2020

kumare3 left a comment

katrogan Sep 2, 2020

honnix Sep 2, 2020

katrogan Sep 2, 2020

katrogan Sep 2, 2020

honnix Sep 2, 2020

katrogan Sep 2, 2020

honnix Sep 2, 2020

honnix Sep 2, 2020

katrogan Sep 2, 2020

Gcs remote data #121

Gcs remote data #121

Conversation

honnix commented Sep 1, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov-commenter commented Sep 1, 2020 • 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

kumare3 left a 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

Choose a reason for hiding this comment

honnix commented Sep 1, 2020 •

edited

Loading

codecov-commenter commented Sep 1, 2020 •

edited

Loading