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

S3 client believes every object exists. #2445

Closed
manarth opened this issue May 8, 2022 · 7 comments
Closed

S3 client believes every object exists. #2445

manarth opened this issue May 8, 2022 · 7 comments
Assignees
Labels
bug This issue is a bug. closed-for-staleness response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@manarth
Copy link

manarth commented May 8, 2022

Describe the bug

When invoking 'doesObjectExist()` with the S3 client, the API returns TRUE for non-existing objects.

Expected Behavior

The API should return TRUE if the specified bucket and key exists.
The API should return FALSE if the specified bucket does not exist, or if the specified bucket and key do not exist.

Current Behavior

The API always returns true.

Reproduction Steps

$params = [
  'Bucket' => 'my-bucket-which-exists',
  // Non-existent key name.
  'Key' => 'sdsgfdsgregerdfdgdd',
];
$result = $s3Client->doesObjectExist($params['Bucket'], $params['Key']);

Possible Solution

The method checkExistenceWithCommand() in the S3ClientTrait executes a defined command.
If the command does not throw an exception, TRUE is returned.

    /**
     * Determines whether or not a resource exists using a command
     *
     * @param CommandInterface $command Command used to poll for the resource
     *
     * @return bool
     * @throws S3Exception|\Exception if there is an unhandled exception
     */
    private function checkExistenceWithCommand(CommandInterface $command)
    {
        try {
            $result = $this->execute($command);
            return true;
        } catch (S3Exception $e) {
            if ($e->getAwsErrorCode() == 'AccessDenied') {
                return true;
            }
            if ($e->getStatusCode() >= 500) {
                throw $e;
            }
            return false;
        }
    }

The doesObjectExist() method uses a HeadObject command to check if the bucket exists.

    /**
     * @see S3ClientInterface::doesObjectExist()
     */
    public function doesObjectExist($bucket, $key, array $options = [])
    {
        return $this->checkExistenceWithCommand(
            $this->getCommand('HeadObject', [
                    'Bucket' => $bucket,
                    'Key'    => $key
                ] + $options)
        );
    }

When executing a HeadObject command for a non-existing key, the command does not throw an Exception.
Instead, it returns the 404 response as an Aws\Result object.

class Aws\Result#2316 (2) {
  private $data =>
  array(1) {
    '@metadata' =>
    array(4) {
      'statusCode' =>
      int(404)
      'effectiveUri' =>
      string(55) "https://<redacted>.s3.amazonaws.com/sdsgfdsgregerdfdgdd"
      'headers' =>
      array(5) {
        ...
      }
      'transferStats' =>
      array(1) {
        ...
      }
    }
  }
  private $monitoringEvents =>
  array(0) {
  }
}

Additional Information/Context

The most significant end-user impact is that using mkdir() with the S3 StreamWrapper always fails, because it always believes the (pseudo)-directory exists.

Solution options may include:

  • Within checkExistenceWithCommand(), test for a 404 result (in addition to detecting an exception).
  • Within the HeadObject command, throw an Exception for non-existing objects.

SDK version used

3.222.6

Environment details (Version of PHP (php -v)? OS name and version, etc.)

PHP 7.4.29 (cli), on Ubuntu Bionic 18.04.

@manarth manarth added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 8, 2022
@manarth manarth changed the title S3 multiregion client believes every object exists. S3 client believes every object exists. May 8, 2022
@stobrien89
Copy link
Member

stobrien89 commented May 9, 2022

Hi @manarth,

Thanks for raising this. We recently implemented doesObjectExistV2 and doesBucketExistV2, which address some of the faulty assumptions made by the first implementations. I did take into consideration the customizations that rely on the original implementations, but realized changing them could be a breaking change for customers and did not know the extent to which customers were being impacted.

I think the most sensible thing to do at this point would be to add an optional argument for usage of the newer existence methods. I'll shoot to have a PR up for this within a week.

@manarth
Copy link
Author

manarth commented May 9, 2022

Hi @stobrien89 – the unexpected behaviour also currently exists for the doesObjectExistV2 method (I haven't been able to trace it to a specific change yet though).

@manarth
Copy link
Author

manarth commented May 9, 2022

My current local patch checks for a 404 response:

    /**
     * Determines whether or not a resource exists using a command
     *
     * @param CommandInterface $command Command used to poll for the resource
     *
     * @return bool
     * @throws S3Exception|\Exception if there is an unhandled exception
     */
    private function checkExistenceWithCommand(CommandInterface $command)
    {
        try {
            $result = $this->execute($command);
            return $result->get('@metadata')['statusCode'] !== 404;
        } catch (S3Exception $e) {

This works for me locally for now, although I'm not sure whether this aligns with the SDK design philosophy.

@stobrien89 stobrien89 removed the needs-triage This issue or PR still needs to be triaged. label May 9, 2022
@stobrien89 stobrien89 self-assigned this May 9, 2022
@stobrien89
Copy link
Member

Hi @manarth,

Could you describe the unexpected behavior that's occurring when you use doesObjectExistV2 ?

@manarth
Copy link
Author

manarth commented May 9, 2022

The same as above: checking for a non-existing key, in an existing bucket, returns TRUE – i.e. it believes the object exists.

$result = $s3Client->doesObjectExistV2("bucket-which-exists", "dsbhjbsdhjdbfjhs");
var_dump($result);
// bool(true)

@stobrien89
Copy link
Member

Hi @manarth,

Thanks for confirming. I think it'd be helpful for us to take a look at the debug logs here— we definitely want to correct this if there's something wrong with the implementation. Could you post the S3 response from the debug logs after adding 'debug' => true to your client configuration? Please obscure any sensitive information such as keys or account numbers (if applicable). Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 9, 2022
@github-actions
Copy link

This issue has not recieved a response in 1 week. If you want to keep this issue open, please just leave a comment below and auto-close will be canceled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. closed-for-staleness response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants