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

Replace method IsCustomerManagedKeyError for a more generic IsAccessDeniedErr on the bucket interface. #71

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

alanprot
Copy link
Contributor

This PR replace the IsCustomerManagedKeyError method for a more generic IsAccessDeniedErr method and implement it multiples provides.

This method can be useful for the projects consuming this lib to identify AccessDenied errors from other types of errors and for, for example, avoid retries.

I think is safe to remove the IsCustomerManagedKeyError for now as is a very recent change and is being used only in cortex for now.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Remove the IsCustomerManagedKeyError method from the objstore interface
  • Add IsAccessDeniedErr method on the objstore interface and implement it on multiples providers.

@alanprot alanprot changed the title Implementing IsAccessDeniedErr method on the objstore Replace method IsCustomerManagedKeyError for a more generic IsAccessDeniedErr on the bucket interface. Aug 16, 2023
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks. I think this change makes sense overall.
The only concern I have is that for different object store implementation, whether the current way of checking access deny is correct or not.
But since the method is only used in Cortex, not in Thanos at all, maybe it is fine. We can iterate on this and fix accordingly

@yeya24 yeya24 merged commit 20395bf into thanos-io:main Aug 16, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants