get signed URLs for GCS objects by ChengyuanZhao · Pull Request #261 · spring-attic/spring-cloud-gcp

ChengyuanZhao · 2017-12-14T23:03:33Z

fixes #251

meltsufin · 2017-12-15T02:46:25Z

 	}

+	@Test
+	public void nullSignedUrlForNullBlob() throws IOException {


I think we need more tests to fully cover the method.

IOException when privateKeyPath is empty.

Verify that this.storage.signUrl is called in the happy path.

Will be not be nice to test, because env vars are not great to test with unit tests. Remember a few months ago we had issues with the CI stuff and tests that altered env vars. The GOOGLE_APPLICATION_CREDENTIALS env var is very widespread and set in stone, so I think this is the right way to access that private key JSON file. I think maybe we can just skip testing it. Even if this test would have failed, it's not a catastrophic or mysterious failure anyway.

Will do.

Why can't we rely on spring.cloud.gcp.storage.credentials.location, which is easy to mock in the test?

Isn't that one only in the starter project? Also, other than a unit test of minimal (if any) value, what's the purpose? GOOGLE_APPLICATION_CREDENTIALS is much more widely known, and is mentioned directly in the documentation in this repo (not just the starter projects as the spring.cloud.gcp.xxxxx.credentials.location props).

meltsufin · 2017-12-15T02:50:38Z

 to obtain a https://github.com/GoogleCloudPlatform/google-cloud-java/blob/master/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java[`Blob`].
 This type represents a GCS file, which has associated https://cloud.google.com/storage/docs/gsutil/addlhelp/WorkingWithObjectMetadata[metadata], such as content-type, that can be set.
-
+The `getSignedUrl` method can also be used to obtain signed URLs for GCS objects.


getSignedUrl -> createSignedUrl
Also, link to the documentation about signed URLs: https://cloud.google.com/storage/docs/access-control/signed-urls

meltsufin · 2017-12-15T02:51:03Z

 can be cast as a `GoogleStorageResourceObject` and the `getGoogleStorageObject` method can be called
 to obtain a https://github.com/GoogleCloudPlatform/google-cloud-java/blob/master/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java[`Blob`].
 This type represents a GCS file, which has associated https://cloud.google.com/storage/docs/gsutil/addlhelp/WorkingWithObjectMetadata[metadata], such as content-type, that can be set.
+The `getSignedUrl` method can also be used to obtain signed URLs for GCS objects.


Same comment as above.

meltsufin · 2017-12-15T02:51:41Z

 	}

+	/**
+	 * Gets a signed URL to an object if it exists.


Gets -> Creates

meltsufin · 2017-12-15T02:51:52Z

+	 * @return the URL if the object exists, and null if it does not.
+	 * @throws IOException
+	 */
+	public URL getSignedUrl(TimeUnit timeUnit, int timePeriods) throws IOException {


createSignedUrl

meltsufin · 2017-12-15T02:56:16Z

+		if (blob == null) {
+			return null;
+		}
+		String privateKeyPath = System.getenv(


Can we use the auto-configured CredentialsProvider to get the private key instead? @joaoandremartins

Doesn't look like there's a way to get a private key from com.google.auth.Credentials. I'm not a security guy, but perhaps it's intentional that the only copy of the private key you're supposed to have is from the JSON file you download just once when creating the service account in the UI.

What about the GcpStorageProperties.credentials.location? We already have a mechanism for specifying the private key file. Why use the environment variable instead?

See my other comment. The prop you're thinking of is actually only in the starter projects, and infact the larger repo-level docs here specify using the env var. The env var is also standard across any GCP code, so I don't feel it's out of place (it's also mentioned to be set in this repo's docs anyway).

Actually I could make this path a member that we set in the constructor (like the auto-create setting last time), and that it would just default to GOOGLE_APPLICATION_CREDENTIALS. Still, we would have this env var involved, but we could at least use the path from the starter instead if it was provided.

meltsufin · 2017-12-15T15:09:38Z

@ChengyuanZhao Also, I noticed that this PR does not add support for creating signed URLs for buckets. One way to fix it would be to just go back to having a single GoogleStorageResource class and represent a GCS resource and deal with bucket vs. object internally, as I suggested previously.

joaoandremartins · 2017-12-15T15:24:50Z

+		String privateKeyPath = getGcpPrivateKeyPath();
+		return this.storage.signUrl(
+				BlobInfo.newBuilder(blob.getBucket(), blob.getName()).build(),
+				timePeriods, timeUnit, getSignUrlOptionForPrivateKey(privateKeyPath));


Why are you passing credentials here at all?
this.storage already contains the correct credentials, no need to pass them here again.

Plus, if for whatever reason they needed to be passed here again, you would depend on GoogleCredentials, not get the credentials on your own.

What's needed is a ServiceAccountCredentials, which is a child of GoogleCredentials, and I'll try looking for if that is available at all, but it might not be, since none of the existing code requires specifically a service account credentials to work.

I see what you mean.

I think the best way forward here is to add a GoogleCredentials argument to this method and then checking if it's an instance of ServiceAccountCredentials and, if so, cast it.

If it isn't, we should fail with clear instructions for the user (e.g., createSignedUrl only works if you're using service account creds).

Makes sense, will do.

joaoandremartins · 2017-12-15T15:29:29Z

My understanding of #251 was that when a blob is created, we should return (e.g., by logging) the GCS URL to users. Although now that I'm re-reading #251 I'm not totally sure what it's about.

Maybe I'd just add a log statement to GoogleStorageResourceObject#createBlob() logging the URL of the newly created object (same for buckets).

joaoandremartins · 2017-12-15T15:34:31Z

On a second thought, I may have misinterpreted the meaning of signed URL.
So maybe disregard my comment about logging the signed URL on blob creation.

ChengyuanZhao · 2017-12-15T15:35:32Z

@meltsufin

I must say I feel completely the opposite. We had a previous PR that exposes setting metadata, which only makes sense for an object ( not a bucket), and that was elegantly handled by having the two separate types.

Also ,the function for getting signed url is from Storage, and only works for objects, not buckets, so again I feel our current setup with 2 classes is very fitting. I definitely don't think we should be implementing our own url building functionn for buckets especially since Storage only provides one for objects. not buckets.

ChengyuanZhao · 2017-12-15T15:38:12Z

@meltsufin

Ultimately it's because while a gs://bucket and gs://bucket/object path look very similar to a user, they represent very different things and have different functionality even from the user perspective, so I really don't think we should be hiding both behind a single type and instead warning the user at runtime that one operation is supported or not. For example, setting meta data or even getting a write stream.

joaoandremartins · 2017-12-15T15:43:36Z

I'm also OK with having the two types vs single type and internal ifs (and I've read something on this recently in Effective Java where multiple types are encouraged vs internal ifs, Item 20 - Prefer class hierarchies to tagged classes).

In this case, this comes at the price of having a duplicated createSignedUrl(), which is OK for now. If duplication grows, we might think of doing something else.

meltsufin · 2017-12-15T15:58:47Z

If it's true that you can't create a signed URL for a bucket, then this makes total sense and two classes are good.

joaoandremartins · 2017-12-15T16:30:31Z

+		if (blob == null) {
+			return null;
+		}
+		return this.storage.signUrl(


Have you tested this change without using application default credentials?

From reading the Storage#signUrl() documentation, I think we do need to pass it a SignUrlOptions object containing the desired service account credentials.
In other words, I don't think the Storage object is smart enough to use the GoogleCredentials that it was passed in its constructor, but I'd be curious to test if this is true.

I did try it just now and it did make a working URL for me. This part of the doc makes it sound like it is aware of the credentials with which it was made:

If the credentials passed to

{@link StorageOptions} do not implement {@link ServiceAccountSigner} (this is the case, for

instance, for Compute Engine credentials and Google Cloud SDK credentials) then {@code signUrl}

will throw an {@link IllegalStateException} unless an implementation of

{@link ServiceAccountSigner} is passed using the

{@link SignUrlOption#signWith(ServiceAccountSigner)} option.

joaoandremartins · 2017-12-15T16:35:56Z

+		GoogleStorageResourceObject resource = new GoogleStorageResourceObject(storage, location);
+		Mockito.when(storage.get(Mockito.any(BlobId.class))).thenReturn(blob);
+		resource.createSignedUrl(TimeUnit.DAYS, 1L);
+		Mockito.verify(storage, Mockito.times(1))


nit: we've been using static imports in tests, you might want to do the same here for consistency.

joaoandremartins

LGTM modulo the test static imports nit

get signed URLs for GCS objects

1cffb53

ChengyuanZhao self-assigned this Dec 14, 2017

ChengyuanZhao requested review from artembilan, joaoandremartins, meltsufin and saturnism December 14, 2017 23:03

meltsufin suggested changes Dec 15, 2017

View reviewed changes

added unit test to verify signUrl called

31a7e6a

joaoandremartins reviewed Dec 15, 2017

View reviewed changes

Do not explicity provide service account credentials

7d5d734

joaoandremartins reviewed Dec 15, 2017

View reviewed changes

Added static imports for Mockito stuff

e233c5b

joaoandremartins approved these changes Dec 15, 2017

View reviewed changes

meltsufin approved these changes Dec 15, 2017

View reviewed changes

ChengyuanZhao merged commit 9fe3d1c into master Dec 15, 2017

ChengyuanZhao deleted the signed-urls branch December 15, 2017 21:34

elefeint pushed a commit that referenced this pull request May 28, 2021

Remove empty buildAndTest.yaml workflow (#261)

dd78355

Conversation

ChengyuanZhao commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meltsufin commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaoandremartins commented Dec 15, 2017

Uh oh!

joaoandremartins commented Dec 15, 2017

Uh oh!

ChengyuanZhao commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChengyuanZhao commented Dec 15, 2017

Uh oh!

joaoandremartins commented Dec 15, 2017

Uh oh!

meltsufin commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaoandremartins left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

ChengyuanZhao commented Dec 15, 2017 •

edited

Loading