feat: Add support to disable logging from bucket #390
Conversation
googlebot
added
the
cla: yes
athakor
changed the title
Codecov Report
@@ Coverage Diff @@
## master #390 +/- ##
============================================
+ Coverage 63.13% 63.27% +0.13%
- Complexity 601 609 +8
============================================
Files 32 32
Lines 5078 5113 +35
Branches 481 487 +6
============================================
+ Hits 3206 3235 +29
- Misses 1708 1713 +5
- Partials 164 165 +1
Continue to review full report at Codecov.
|
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
| @@ -1310,7 +1310,7 @@ public Builder setIamConfiguration(IamConfiguration iamConfiguration) { | |||
|
|
|||
| @Override | |||
| public Builder setLogging(Logging logging) { | |||
| this.logging = logging; | |||
| this.logging = logging != null ? logging : null; | |||
There was a problem hiding this comment.
Instead do:
this.logging = logging != null ? logging : Data.nullOf(Bucket.Logging.class);
There was a problem hiding this comment.
@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
| if (logging != null) { | ||
| bucketPb.setLogging(logging.toPb()); | ||
| } | ||
| Bucket.Logging loggingPb = logging != null ? logging.toPb() : Data.nullOf(Bucket.Logging.class); |
There was a problem hiding this comment.
Revert this change:
Bucket.Logging loggingPb = logging != null ? logging.toPb() : Data.nullOf(Bucket.Logging.class);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()));
There was a problem hiding this comment.
@frankyn good catch and thanks for the detailed description.
Both the comments have been addressed PTAL when get chance.
| logging.setLogBucket(logBucket); | ||
| logging.setLogObjectPrefix(logObjectPrefix); | ||
| Bucket.Logging logging; | ||
| if (logBucket != null && logObjectPrefix != null) { |
Fixes #389