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

Allow differentiating blobstores by buckets #376

Closed
wants to merge 1 commit into from

Conversation

timuralp
Copy link
Collaborator

Allows differentiating the backend blobstores by configured buckets.
The commit allows using the same s3proxy credentials across the
different backends and relies on the bucket names to disambiguate them.

The "default" blobstore is the one that occurs first.

Fixes: #371

@@ -66,6 +66,9 @@
"s3proxy.max-single-part-object-size";
public static final String PROPERTY_V4_MAX_NON_CHUNKED_REQUEST_SIZE =
"s3proxy.v4-max-non-chunked-request-size";
/** Used to locate blobstores by specified bucket names. */
public static final String PROPERTY_BUCKET_LOCATOR =
"s3proxy.bucket-locator";
Copy link
Owner

Choose a reason for hiding this comment

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

I am unclear how to use this feature -- can you add some more documentation? Does each properties file have a unique identifier? Is it used for something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Expanded the description. The bucket-locator property specifies the bucket names associated with the backend specified in the properties file. Different properties files can specify different buckets, allowing a client to use the same S3Proxy credentials, but have the requests route to different backends.

localCredential, blobStore));
Map.Entry<String, String> key = Maps.immutableEntry(
localIdentity, null);
if (!parsedLocations.contains(key)) {
Copy link
Owner

Choose a reason for hiding this comment

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

You can use the return value from parsedLocations.add.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@gaul
Copy link
Owner

gaul commented Sep 17, 2021

@berlinsaint @jiffysrc there seems to be some overlap between #372, #374, and this PR. Can you guys sort out which approach is the best?

@timuralp timuralp force-pushed the feature/bucket-locator branch from 917fc68 to 3fce2d3 Compare September 17, 2021 18:19
Allows differentiating the backend blobstores by configured buckets.
The commit allows using the same s3proxy credentials across the
different backends and relies on the bucket names to disambiguate them.

The "default" blobstore is the one that occurs first.

Fixes: gaul#371
@timuralp timuralp force-pushed the feature/bucket-locator branch from 3fce2d3 to 9be15e1 Compare September 17, 2021 18:25
@timuralp
Copy link
Collaborator Author

I think #374 and this PR are related, but orthogonal: adding external authorization callback vs being able to select a backend by the bucket name. I'm struggling to understand the intent of #372, aside from introducing alternate stores for credentials.

@whybeyoung
Copy link

I think #374 and this PR are related, but orthogonal: adding external authorization callback vs being able to select a backend by the bucket name. I'm struggling to understand the intent of #372, aside from introducing alternate stores for credentials.

agree

localCredential, blobStore));
}
}
if (!buckets.isEmpty()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do you need to check whether buckets is empty? The foreach loop should handle this already.

@gaul
Copy link
Owner

gaul commented Sep 25, 2021

Tested this with two local filesystem configs and verified that objects go where they should. Also tried some duplicate buckets and the error message is comprehensible.

@gaul
Copy link
Owner

gaul commented Oct 20, 2021

Superseded by #378.

@gaul gaul closed this Oct 20, 2021
@timuralp timuralp deleted the feature/bucket-locator branch October 23, 2021 19:05
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.

Use container names to disambiguate backend stores
3 participants