[PLUGIN-1945] Skip bucket creation if exist (copy/move action)#1596
[PLUGIN-1945] Skip bucket creation if exist (copy/move action)#1596psainics wants to merge 6 commits intodata-integrations:developfrom
Conversation
| */ | ||
| public void createBucketIfNotExists(GCSPath path, @Nullable String location, @Nullable CryptoKeyName cmekKeyName) { | ||
| try { | ||
| if (storage.get(path.getBucket()) != null) { |
There was a problem hiding this comment.
This adds an additional permission storage.buckets.get, do we do storage.get anywhere else?
I wonder if we should do storage.list instead which we do already later on?
There was a problem hiding this comment.
or may be we could just ignore 403 with storage.get and move forward for backward compatibility.
There was a problem hiding this comment.
storage.buckets.get permission is required for this plugin to work, we use it to traverse the bucket in function pairTraverse.
There was a problem hiding this comment.
pairTraverse function might be only getting called in GCS Move / Copy plugin but StorageClient is used in all other GCS plugins as well.
There was a problem hiding this comment.
As of now createBucketIfNotExists is only being used in gcs copy/move action.
From gcs docs it looks like list permission is for listing all buckets in a project, much slower than get, and less secure.
Let me know what do you think.
There was a problem hiding this comment.
As I mentioned in the reply, we can use storage.get but just ignore 403 with storage.get and move forward for backward compatibility.
There was a problem hiding this comment.
Updated
// Skip bucket creation if bucket already exists.
try {
if (storage.get(path.getBucket()) != null) {
LOG.info("Bucket {} already exists, skipping creation.", path.getBucket());
return;
}
} catch (StorageException e) {
int errorCode = e.getCode();
if (errorCode == 403) {
LOG.warn(
"Getting 403 Forbidden: {} You may not have permission to access bucket {}. Attempting to create bucket.",
e.getMessage(), path.getUri());
} else {
LOG.warn("Getting unexpected error code {}: {} when checking if bucket {} exists. Attempting to create bucket.",
errorCode, e.getMessage(), path.getBucket());
}
}
// Fallback to bucket creations when get returns null or throws exception.
// ... Old Flow- Add try-catch around bucket existence check to handle permission errors - Log warning messages for 403 Forbidden and other unexpected errors - Fallback to bucket creation when existence check fails - Separate bucket check and creation into distinct try-catch blocks
| } catch (StorageException e) { | ||
| // do not throw error if unable to access bucket for backward compatibility. | ||
| LOG.warn("Getting unexpected error code {}: {} when checking if bucket {} exists. Attempting to create bucket.", | ||
| e.getCode(), e.getMessage(), path.getBucket()); |
There was a problem hiding this comment.
please also add the exception in the log.
LOG.warn(....., e);
4 participants
Skip bucket creation if exist (copy/move action)
Jira : PLUGIN-1945
Description
if the user does not have
storage.buckets.createthe flow fails with 403 error, bucket creation should be skipped if the bucket already exists.Code change
StorageClient.java