-
Notifications
You must be signed in to change notification settings - Fork 80
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.
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
feat: Add support to disable logging from bucket #390
Conversation
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.
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);
There was a problem hiding this comment.
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
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.
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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()));
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more change but looks good to me overall, thanks!
logging.setLogBucket(logBucket); | ||
logging.setLogObjectPrefix(logObjectPrefix); | ||
Bucket.Logging logging; | ||
if (logBucket != null && logObjectPrefix != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an || an or.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Fixes #389