ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets

[wjones127]

BREAKING CHANGE: modifies S3FileSystem to not allow creating or deleting buckets by default. Two new options are added: allow_bucket_creation, which enables creating buckets, and allow_bucket_deletion, which enables deleting buckets.

Outside of tests, most use cases will not want to create or delete buckets, and doing so accidentally is not desirable either. Buckets have governance controls like permissions and cost-tracking tags.

To make it easy for users to transition, the FromUri method also supports these arguments:

frompyarrow.fsimportFileSystemuri="s3://minioadmin:minioadmin@?scheme=http&endpoint_override=localhost%3A9000"fs, path=FileSystem.from_uri(uri)
fs.create_dir("test")
# Traceback (most recent call last):# File "<stdin>", line 1, in <module># File "pyarrow/_fs.pyx", line 463, in pyarrow._fs.FileSystem.create_dir# check_status(self.fs.CreateDir(directory, recursive=recursive))# File "pyarrow/error.pxi", line 115, in pyarrow.lib.check_status# raise IOError(message)# OSError: Bucket 'test' not found. To create buckets, enable the allow_bucket_creation option.uri=uri+"&allow_bucket_creation=True&allow_bucket_deletion=True"fs, path=FileSystem.from_uri(uri)
fs.create_dir("test")
fs.delete_dir("test")

fs<-FileSystem$from_uri("s3://minioadmin:minioadmin@?scheme=http&endpoint_override=localhost%3A9000")$fsfs$CreateDir("test")
#> Error: IOError: Bucket 'test' not found. To create buckets, enable the allow_bucket_creation option.fs<-FileSystem$from_uri(
paste0("s3://minioadmin:minioadmin@?scheme=http&endpoint_override=localhost%3A9000",
"&allow_bucket_creation=TRUE&allow_bucket_deletion=TRUE")
)$fsfs$CreateDir("test")
fs$DeleteDir("test")

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15906

[kou]

Could you also update S3Options::FromUri() to support this?

[wjones127]

Good idea. I've added that.

[pitrou]

Let's make it two options for flexibility (creation and deletion).

Also wrt. naming, should this be allow_bucket_creation? @lidavidm

[lidavidm]

Perhaps create_buckets and delete_buckets? That reads more consistently with background_writes to me. Or allow_creating_buckets

[wjones127]

I liked create_buckets but since there is a setter method and I want it named the same thing as the property (for clear error hints), I felt like allow_bucket_creation was the better choice.

[kou]

Could you send an e-mail to dev@ to confirm whether this default behavior change is acceptable? I can't decide this because I haven't used Apache Arrow to access S3 yet.

[kou]

It seems that "deletion" depends on allow_create_buckets confuses users. How about renaming allow_create_buckets or create one more option?

BTW, why should we disable "deletion" by default?

[wjones127]

We probably don't have to. Buckets can only be deleted if already empty, so hard to accidentally delete an important one I suppose.

[wjones127]

Could you send an e-mail to dev@ to confirm whether this default behavior change is acceptable? I can't decide this because I haven't used Apache Arrow to access S3 yet.

Sure, I've done that here: https://lists.apache.org/thread/2wnmq72trrtcxyvxzw261gq0t6grq18g

[pitrou]

Let's make it two options for flexibility (creation and deletion).

Also wrt. naming, should this be allow_bucket_creation? @lidavidm

[pitrou]

Hmm, I think we should be a bit stricter and not blindly interpret any other value as "false".
How about allowing the following values:

• "0", "false" (case-insensitive) -> boolean false
• "1", "true" (case-insensitive) -> boolean true
• any other value -> raise Status::Invalid

[wjones127]

Sounds good. I didn't see any existing utility function to do this, so maybe I will create one.

[pitrou]

These trivial setters are not needed as people can access the attributes directly.

[wjones127]

options() returns a const reference; they could access but I don't think they could mutate right?

Being able to change this setting later felt important when I didn't support in FromURI, but maybe it's not as important now.

[wjones127]

These trivial setters are not needed as people can access the attributes directly.

I'm not so sure that's true. This fails:

auto options = fs_->options();
options.allow_bucket_creation = true;
options.allow_bucket_deletion = true;
ASSERT_EQ(fs_->options().allow_bucket_creation, true);
ASSERT_EQ(fs_->options().allow_bucket_deletion, true);

[pitrou]

Hmm, this is not really supported, though. You should set options before creating the filesystem.

[wjones127]

Okay, I think I'm fine at this point with only allowing setting them during construction, since we've added them to the URI parsing.

[pitrou]

We should probably use the same docstring for constructor parameters and the associated properties.
Personally, I like "Whether to allow CreateDir at the bucket-level."

[pitrou]

Thanks a lot @wjones127 . Here are some more comments needing addressing.

[pitrou]

Can you also add tests in test_s3_options?

[pitrou]

@thisisnic@dragosmg Can one of you perhaps review the R changes?

[dragosmg]

LGTM, but I'd like the opinion of someone more experienced.

[thisisnic]

Generally LGTM, just one small comment/suggestions on the R tests.

[thisisnic]

I wonder if these tests might be better without the regexp parameter, given that the specific phrasing of them comes from the C++ layer, and we tend to avoid testing that component of the error message?

[wjones127]

Sure, I'll rewrite it so these messages are tested in C++ instead.

[pitrou]

Thanks @wjones127 !