Skip to content

[PLUGIN-1945] Skip bucket creation if exist (copy/move action) #1596

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

Open
psainics wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

[PLUGIN-1945] Skip bucket creation if exist (copy/move action) #1596

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a201e32
Skip bucket creation if exist (copy/move action)
psainics Feb 17, 2026
22a2c5f
Fix GCS bucket error message
psainics Feb 18, 2026
646a7da
Improve createBucketIfNotExists to handle StorageException gracefully
psainics Feb 18, 2026
a2d98a4
Change log level to trace for existing bucket check
psainics Feb 18, 2026
0d0ee70
Improve error handling for bucket access in StorageClient
psainics Feb 18, 2026
ddc9534
Enhance logging for bucket existence check by including exception det…
psainics Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/main/java/io/cdap/plugin/gcp/gcs/StorageClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,18 @@ public void mapMetaDataForAllBlobs(String path, Consumer<Map<String, String>> fu
* @param cmekKeyName the name of the cmek key
*/
public void createBucketIfNotExists(GCSPath path, @Nullable String location, @Nullable CryptoKeyName cmekKeyName) {
// Skip bucket creation if bucket already exists.
try {
if (storage.get(path.getBucket()) != null) {
Copy link
Contributor

@itsankit-google itsankit-google Feb 18, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@itsankit-google itsankit-google Feb 18, 2026

Choose a reason for hiding this comment

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

or may be we could just ignore 403 with storage.get and move forward for backward compatibility.

Copy link
Contributor Author

@psainics psainics Feb 18, 2026

Choose a reason for hiding this comment

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

storage.buckets.get permission is required for this plugin to work, we use it to traverse the bucket in function pairTraverse.

Copy link
Contributor

@itsankit-google itsankit-google Feb 18, 2026

Choose a reason for hiding this comment

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

pairTraverse function might be only getting called in GCS Move / Copy plugin but StorageClient is used in all other GCS plugins as well.

Copy link
Contributor Author

@psainics psainics Feb 18, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@itsankit-google itsankit-google Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

As I mentioned in the reply, we can use storage.get but just ignore 403 with storage.get and move forward for backward compatibility.

Copy link
Contributor Author

@psainics psainics Feb 18, 2026

Choose a reason for hiding this comment

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

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

LOG.trace("Bucket {} already exists, skipping creation.", path.getBucket());
return;
}
} 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(), e);
}
// Fallback to bucket creations when get returns null or throws exception.
try {
GCPUtils.createBucket(storage, path.getBucket(), location, cmekKeyName);
LOG.info("Bucket {} has been created successfully", path.getBucket());
Expand All @@ -157,7 +169,7 @@ public void createBucketIfNotExists(GCSPath path, @Nullable String location, @Nu
e.getMessage(), path.getUri());
} else {
String errorReason =
String.format("Unable to create bucket %s. Ensure you entered the correct bucket path and " +
String.format("Unable to create or get bucket %s. Ensure you entered the correct bucket path and " +
"have permissions for it.", path.getBucket());
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.UNKNOWN,
true, GCPUtils.GCS_SUPPORTED_DOC_URL);
Expand Down
Loading