Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add support to disable logging from bucket #390

Merged
merged 6 commits into from Jun 25, 2020
Merged

feat: Add support to disable logging from bucket #390

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05b175f
feat: implement disabling logging api
athakor Jun 22, 2020
72ef594
feat: add support to disable bucket logging
athakor Jun 23, 2020
b3fc860
feat: modified tests
athakor Jun 23, 2020
51c1c59
feat: addressed review changes
athakor Jun 24, 2020
6e32236
feat: use alternative solution
athakor Jun 25, 2020
b3aab02
feat: use or instead of and
athakor Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
View file
Open in desktop
Expand Up @@ -1310,7 +1310,7 @@ public Builder setIamConfiguration(IamConfiguration iamConfiguration) {

@Override
public Builder setLogging(Logging logging) {
this.logging = logging;
this.logging = logging != null ? logging : null;
Copy link
Member

Choose a reason for hiding this comment

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

Instead do:

this.logging = logging != null ? logging : Data.nullOf(Bucket.Logging.class);

Copy link
Contributor Author

@athakor athakor Jun 24, 2020 •
edited

Choose a reason for hiding this comment

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

@frankyn thanks for the quick review I think we have to go with the null instead of Data.nullOf(com.google.cloud.storage.Bucket.Logging.class) because if we do as you said then the following error occurs : unable to create new instance of class com.google.cloud.storage.BucketInfo$Logging because it has no accessible default constructor

return this;
}

Expand Down Expand Up @@ -1758,9 +1758,8 @@ public Rule apply(LifecycleRule lifecycleRule) {
if (iamConfiguration != null) {
bucketPb.setIamConfiguration(iamConfiguration.toPb());
}
if (logging != null) {
bucketPb.setLogging(logging.toPb());
}
Bucket.Logging loggingPb = logging != null ? logging.toPb() : Data.nullOf(Bucket.Logging.class);
Copy link
Member

Choose a reason for hiding this comment

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

Revert this change:

 Bucket.Logging loggingPb = logging != null ? logging.toPb() : Data.nullOf(Bucket.Logging.class);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we also have to keep this check because when the object of logging is null, the logging.toPb() will throw the nullPointer exception.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/googleapis/java-storage/compare/qlogics?expand=1

Here's alternative solution. If you have concerns with it always open to discuss further.

I'd like to address the issue with logging field is not selected in Fields. In which case it comes back null and this PR can corrupt the logging field when not selected.

For example this will corrupt logging metadata:

      BucketInfo.Logging logging =
          BucketInfo.Logging.newBuilder()
              .setLogBucket(logsBucket)
              .setLogObjectPrefix("test-logs")
              .build();
      Bucket bucket =
          storage.create(
              BucketInfo.newBuilder(loggingBucket).setLocation("us").setLogging(logging).build());
      // Perform Get Request without selecting logging field
      Bucket remoteBucket = storage.get(loggingBucket);
      // Don't update logging and only perform an update.
      Bucket updatedBucket = bucket.toBuilder().build().update();
      // Logging is now null.

Additionally could you remove setting IAM policy to allow writes from allAuthenticatedUsers. This is a security risk:

          storage.setIamPolicy(
              logsBucket,
              policy
                  .toBuilder()
                  .addIdentity(StorageRoles.legacyBucketWriter(), Identity.allAuthenticatedUsers())
                  .build()));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankyn good catch and thanks for the detailed description.

Both the comments have been addressed PTAL when get chance.

bucketPb.setLogging(loggingPb);
return bucketPb;
}

Expand Down
1 change: 1 addition & 0 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java
View file
Open in desktop
Expand Up @@ -229,6 +229,7 @@ public void testToPbAndFromPb() {
BucketInfo.newBuilder("b")
.setDeleteRules(DELETE_RULES)
.setLifecycleRules(LIFECYCLE_RULES)
.setLogging(LOGGING)
.build();
compareBuckets(bucketInfo, BucketInfo.fromPb(bucketInfo.toPb()));
}
Expand Down
22 changes: 22 additions & 0 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/BucketTest.java
View file
Open in desktop
Expand Up @@ -863,4 +863,26 @@ public void testDeleteLifecycleRules() {
Bucket actualUpdatedBucket = updatedBucket.update();
assertThat(actualUpdatedBucket.getLifecycleRules()).hasSize(0);
}

@Test
public void testUpdateBucketLogging() {
initializeExpectedBucket(6);
BucketInfo.Logging logging =
BucketInfo.Logging.newBuilder()
.setLogBucket("logs-bucket")
.setLogObjectPrefix("test-logs")
.build();
BucketInfo bucketInfo = BucketInfo.newBuilder("b").setLogging(logging).build();
Bucket bucket = new Bucket(serviceMockReturnsOptions, new BucketInfo.BuilderImpl(bucketInfo));
assertEquals("logs-bucket", bucket.getLogging().getLogBucket());
assertEquals("test-logs", bucket.getLogging().getLogObjectPrefix());
Bucket expectedUpdatedBucket = bucket.toBuilder().setLogging(null).build();
expect(storage.getOptions()).andReturn(mockOptions).times(2);
expect(storage.update(expectedUpdatedBucket)).andReturn(expectedUpdatedBucket);
replay(storage);
initializeBucket();
Bucket updatedBucket = new Bucket(storage, new BucketInfo.BuilderImpl(expectedUpdatedBucket));
Bucket actualUpdatedBucket = updatedBucket.update();
assertThat(actualUpdatedBucket.getLogging()).isNull();
}
}
5 changes: 5 additions & 0 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java
View file
Open in desktop
Expand Up @@ -3244,6 +3244,11 @@ public void testBucketLogging() throws ExecutionException, InterruptedException
BucketInfo.newBuilder(loggingBucket).setLocation("us").setLogging(logging).build());
assertEquals(logsBucket, bucket.getLogging().getLogBucket());
assertEquals("test-logs", bucket.getLogging().getLogObjectPrefix());

// Disable bucket logging.
Bucket updatedBucket = bucket.toBuilder().setLogging(null).build().update();
assertThat(updatedBucket.getLogging()).isNull();

} finally {
RemoteStorageHelper.forceDelete(storage, logsBucket, 5, TimeUnit.SECONDS);
RemoteStorageHelper.forceDelete(storage, loggingBucket, 5, TimeUnit.SECONDS);
Expand Down