fix: minio error while creating bucket if already exists

[achettyiitr]

Description

• Since for every upload, we are trying to create the bucket. In case of the bucket already exists, we try to check if the bucket exists or not using minioClient.BucketExists but this is returning a permission error.
• We particularly don't need an API call to verify if the bucket exists or not. We can suppress the error from the error code itself when we get a bucket creation error.

Notion Ticket

https://www.notion.so/rudderstacks/Minio-data-retention-object-storage-validations-is-failing-b1c961c67337476a82566a43eaed243e?pvs=4

Security

• The code changed/added as part of this pull request won't create any security issues with how the software is being used.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (7b6fa35) 53.69% compared to head (5aac17b) 53.70%.

❗ Current head 5aac17b differs from pull request most recent head f5f34c7. Consider uploading reports for the commit f5f34c7 to get more accurate results

Additional details and impacted files

@@              Coverage Diff               @@
##           release/1.7.x    #3109   +/-   ##
==============================================
  Coverage          53.69%   53.70%           
==============================================
  Files                350      351    +1     
  Lines              54456    54459    +3     
==============================================
+ Hits               29240    29247    +7     
+ Misses             23563    23551   -12     
- Partials            1653     1661    +8     

Impacted Files	Coverage Δ
services/filemanager/azureBlobStoragemanager.go	87.76% <ø> (-0.40%)	⬇️
services/filemanager/errors.go	100.00% <100.00%> (ø)
services/filemanager/miniomanager.go	79.24% <100.00%> (ø)

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[lvrach]

[minor] not sure if wrapping error handling in a separate method is really necessary. The errors that we are ignoring here are specific to bucket creation, not sure if the function should be reused elsewhere

[saurav-malani]

Instead of directly trying to create a bucket and then handling the error, why don't we 1st check if the bucket exist or not. And only create if it doesn't exist?
We can do it by calling BucketExists() func in minio and ContainerExists() in azure.

[achettyiitr]

Instead of directly trying to create a bucket and then handling the error, why don't we 1st check if the bucket exist or not. And only create if it doesn't exist? We can do it by calling BucketExists() func in minio and ContainerExists() in azure.

Yeah, But it will still require 1 API call anyways. Why not just use the Create APi and suppress it in case of already exists?

[saurav-malani]

Yeah, But it will still require 1 API call anyways. Why not just use the Create APi and suppress it in case of already exists?

Well, I believe preventing error in first place is better then handling it.
Also, according to me creating of a bucket is like a write operation where as checking if it exists or not is like a read operation. And, read would have less overhead in comparison to write in terms of internal implementation. In short, the overhead of both the api call would be quite different.

[achettyiitr]

Yeah, But it will still require 1 API call anyways. Why not just use the Create APi and suppress it in case of already exists?

Well, I believe preventing error in first place is better then handling it. Also, according to me creating of a bucket is like a write operation where as checking if it exists or not is like a read operation. And, read would have less overhead in comparison to write in terms of internal implementation. In short, the overhead of both the api call would be quite different.

Make sense.

[achettyiitr]

What if we don't create a bucket at all? Ideally, it should be the customer's responsibility to create the bucket.

[lvrach]

Both approaches sound good to me, I personally prefer the ask for forgiveness than permission. Being optimistic that something would work, and handle the unexpected outcome.

Having a single call is better safeguard against race conditions (trying to create the bucket at the same time).

Checking if bucket exists from an implicit call makes our service more robust against error code changes.

[saurav-malani]

What if we don't create a bucket at all? Ideally, it should be the customer's responsibility to create the bucket.

well, I am not sure if we have clearly mentioned it somewhere that they themself need to create a bucket. But, if that is the case then your point make sense. But, still I believe at the end of the it will escalate to some issue when upload would fail and then we will be notified and CS team will get in touch with them to create it. So, I personally think creating on our own if it doesn't exist would be best for us. 😄

[achettyiitr]

I have reverted the Azure changes since it requires additional logic to check if the container exists or not using storage.GetContainerReferenceFromSASURI(sasUri).Exists() which requires a SASUri instead of an existing containerUri.

[achettyiitr]

Also, it was not in the scope of the PR.