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.

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

As of 2022-02-22 Automatic detection of requesterPays buckets is failing #824

Closed
BenWhitehead opened this issue Feb 22, 2022 · 3 comments · Fixed by #841
Closed

As of 2022-02-22 Automatic detection of requesterPays buckets is failing #824

BenWhitehead opened this issue Feb 22, 2022 · 3 comments · Fixed by #841
Assignees
Labels
api: storage Issues related to the googleapis/java-storage-nio API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@BenWhitehead
Copy link
Collaborator

First observed on PR #823 and visible in the integration tests related to Requester Pays (build results)

Environment details

  1. this library
  2. OS type and version: N/A
  3. Java version: N/A
  4. version(s): all released prior to 2022-02-22

Code example

public void testAutodetectWhenRequesterPays() throws IOException {
CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(true, project);
Assert.assertEquals(
"Autodetect should have detected the RP bucket",
testRPBucket.config().userProject(),
project);
}

External references such as API reference guides

It seems the error message response changed subtly

- Bucket is requester pays bucket but no user project provided.
+ Bucket is a requester pays bucket but no user project provided.

According to public documentation, we should expect the reason to be userProjectMissing, however it is required as can be seen in

{
  "error": {
    "code": 400,
    "message": "Bucket is a requester pays bucket but no user project provided.",
    "errors": [
      {
        "message": "Bucket is a requester pays bucket but no user project provided.",
        "domain": "global",
        "reason": "required"
      }
    ]
  }
}

Related from NodeJS https://github.com/googleapis/nodejs-storage/pull/1791/files

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage-nio API. label Feb 22, 2022
@BenWhitehead BenWhitehead added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Feb 22, 2022
BenWhitehead added a commit that referenced this issue Feb 22, 2022
BenWhitehead added a commit that referenced this issue Feb 22, 2022
@lbergelson
Copy link
Contributor

I think the underlying issue is here.

public boolean requesterPays(String bucketName) {
initStorage();
try {
// instead of true/false, this method returns true/null.
Boolean isRP = storage.get(bucketName).requesterPays();
return isRP != null && isRP.booleanValue();
} catch (StorageException ex) {
if (ex.getCode() == 400 && ex.getMessage().contains("Bucket is requester pays")) {
return true;
}
throw ex;
}
}

It always falls into the catch block which then tries to match against that changed string. I think it shouldn't be falling into the catch block and instead the call to get(bucketName) should include an option specifying the project name which should keep it from throwing an exception. It seems like a bug that was previously covered up by the catch clause.
Boolean isRP = storage.get(bucketName).requesterPays();

@lbergelson
Copy link
Contributor

Ignore that. It seems that throwing the exception is necessary because it's attempting to find requester pays status without providing the user account to avoid access charges in the case where it wasn't requester pays.

It seems like there needs to be a more robust mechanism of determining if this is a requester pays bucket than scraping the exception message after trying to connect. This mechanism also causes problems because it requires the somewhat unusual storage.buckets.get permission which is not necessary for reading the actual content of the bucket. So users need more permissions than necessary when trying to read from a requester pays bucket because the test to see if it IS a requester pays bucket requires additional permissions than actually accessing the file they want. This has caused continual confusion for our users.

Is there either:
A) a way to query the requester pays status of a bucket that doesn't require the user to be authenticated in the case where it is requester pays
B) a way to make this ignore querying requester pays status and just falling back to connecting with a user account when we get back an error that says it is requester pays?

@droazen
Copy link
Contributor

droazen commented Mar 1, 2022

This issue is unfortunately breaking large numbers of our production workflows. I hate to suggest this, but perhaps for now we could grit our teeth and add the missing "a" into the message it's looking for to get it working again, and open a separate ticket to research / implement a more robust mechanism for detecting requester pays status. Unless someone already knows of a more robust way?

Once a patch is in for this issue, would it be possible to get a release of the library with the fix?

lbergelson added a commit to lbergelson/java-storage-nio that referenced this issue Mar 1, 2022
* Fix the expected error message that is tested when checking for requester pays buckets.
* The error message recieved when querying for requester pays status changed to include an 'a' which caused strict matching to fail.
* This fixes and standardizes the error message to use the same string for both the code and the tests.
lbergelson added a commit to lbergelson/java-storage-nio that referenced this issue Mar 1, 2022
* Fix the expected error message that is tested when checking for requester pays buckets.
* The error message recieved when querying for requester pays status changed to include an 'a' which caused strict matching to fail.
* This fixes and standardizes the error message to use the same string for both the code and the tests.
lbergelson added a commit to lbergelson/java-storage-nio that referenced this issue Mar 1, 2022
* Fix the expected error message that is tested when checking for requester pays buckets.
* The error message recieved when querying for requester pays status changed to include an 'a' which caused strict matching to fail.
* This fixes and standardizes the error message to use the same string for both the code and the tests.
lbergelson added a commit to lbergelson/java-storage-nio that referenced this issue Mar 1, 2022
The error message recieved when querying for requester pays status changed to include an 'a' which caused strict matching to fail.
This fixes and standardizes the error message to use the same string for both the code and the tests.

Refs: googleapis#824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage-nio API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
4 participants