Improve error handling/messaging around bucket exist check

[o-nikolas]

This PR was brought about by the confusion caused by the current code (see thread here).
S3Hook.check_for_bucket() method uses the boto3 s3 client method head_bucket (docs) to check for bucket existence. This client method does not work like most boto3 APIs, it only returns a small subset of error codes with very little context.

Manual examples/testing of the updated code:

In [1]: from  airflow.providers.amazon.aws.hooks.s3 import S3Hook
<omitted airflow init logging>

In [2]: s3hook = S3Hook()

In [3]: s3hook.check_for_bucket('test')  # Exists but not owned by this account

[2022-08-18 14:01:15,677] {s3.py:215} ERROR - Access to bucket "test" is forbidden or there was an error with the request
[2022-08-18 14:01:15,677] {s3.py:218} ERROR - An error occurred (403) when calling the HeadBucket operation: Forbidden
Out[3]: False

In [4]: s3hook.check_for_bucket('asdfasdfgfdsagfdsfaasdlkjlkdsajflsad')  # Does not exist

[2022-08-18 14:01:39,237] {s3.py:213} ERROR - Bucket "asdfasdfgfdsagfdsfaasdlkjlkdsajflsad" does not exist
Out[4]: False

In [5]: s3hook.check_for_bucket('env6a7fbbff-athena-bucket')  # Exists in the account
Out[5]: True

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[uranusjr]

Should something still be printed if the return code is 200?

[o-nikolas]

Should something still be printed if the return code is 200?

Thanks for the review TP!

I'm generally not a huge fan of success case logging, usually it's superfluous IMHO. I think maintaining the behaviour of no logging on success makes the most sense here and is less invasive. We could perhaps put a DEBUG level message for the success case if people really want something to trace in that case?

[potiuk]

Should something still be printed if the return code is 200?

Thanks for the review TP!

I'm generally not a huge fan of success case logging, usually it's superfluous IMHO. I think maintaining the behaviour of no logging on success makes the most sense here and is less invasive. We could perhaps put a DEBUG level message for the success case if people really want something to trace in that case?

Agree. not to print anything. I think 200 is not an interesting one and printing it in the logs adds no value. I think debug is not really needed - if someone is interested in seeing such details for DEBUG, they are likely better off by enabling some debug levels in boto ?

[o-nikolas]

If anything, I might be inclined to make the log message for the 404/does-not-exist case to be INFO level or remove it altogether. It is a perfectly reasonable state for that thing to be in. I.e. we're checking if a bucket exists, if it doesn't, that's not really an error or Exception IMHO. Seeing a scary red error message for the bucket not existing could mislead people.