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

wjones127 · 2022-05-20T19:36:29Z

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:

from pyarrow.fs import FileSystem

uri = "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")$fs
fs$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")
    )$fs
fs$CreateDir("test")
fs$DeleteDir("test")

github-actions · 2022-05-20T19:36:51Z

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

kou · 2022-05-21T20:13:18Z

cpp/src/arrow/filesystem/s3fs.h

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

Good idea. I've added that.

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

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

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

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 · 2022-05-24T21:07:28Z

cpp/src/arrow/filesystem/s3fs.h

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?

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 · 2022-05-24T22:42:44Z

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

python/pyarrow/tests/test_dataset.py

pitrou · 2022-05-30T15:39:05Z

cpp/src/arrow/filesystem/s3fs.h

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

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

pitrou · 2022-06-01T09:51:52Z

cpp/src/arrow/filesystem/s3fs.cc

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

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

pitrou · 2022-06-01T09:52:24Z

cpp/src/arrow/filesystem/s3fs.h

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

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.

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);

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

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.

cpp/src/arrow/filesystem/s3fs.h

pitrou · 2022-06-01T09:57:36Z

python/pyarrow/_s3fs.pyx

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."

python/pyarrow/includes/libarrow_fs.pxd

r/R/filesystem.R

pitrou

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

cpp/src/arrow/filesystem/s3fs.cc

python/pyarrow/_s3fs.pyx

pitrou · 2022-06-06T12:24:04Z

python/pyarrow/tests/test_fs.py

+        allow_bucket_creation=True,
+        allow_bucket_deletion=True
    )
    fs.create_dir(bucket)


Can you also add tests in test_s3_options?

python/pyarrow/tests/test_fs.py

pitrou · 2022-06-06T12:25:17Z

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

dragosmg · 2022-06-06T14:39:10Z

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

thisisnic

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

thisisnic · 2022-06-06T15:58:59Z

r/tests/testthat/test-s3-minio.R

+    test_that("CreateDir fails on bucket if allow_bucket_creation=False", {
+      now_tmp <- paste0(now, "-test-fail-delete")
+      fs$CreateDir(now_tmp)
+
+      expect_error(
+        limited_fs$CreateDir("should-fail"),
+        regexp = "Bucket does not exist"
+      )
+      expect_error(
+        limited_fs$DeleteDir(now_tmp),
+        regexp = "Would delete bucket"
+      )
+    })


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?

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

pitrou

Thanks @wjones127 !

…Hub issue numbers (#34260) Rewrite the Jira issue numbers to the GitHub issue numbers, so that the GitHub issue numbers are automatically linked to the issues by pkgdown's auto-linking feature. Issue numbers have been rewritten based on the following correspondence. Also, the pkgdown settings have been changed and updated to link to GitHub. I generated the Changelog page using the `pkgdown::build_news()` function and verified that the links work correctly. --- ARROW-6338 #5198 ARROW-6364 #5201 ARROW-6323 #5169 ARROW-6278 #5141 ARROW-6360 #5329 ARROW-6533 #5450 ARROW-6348 #5223 ARROW-6337 #5399 ARROW-10850 #9128 ARROW-10624 #9092 ARROW-10386 #8549 ARROW-6994 #23308 ARROW-12774 #10320 ARROW-12670 #10287 ARROW-16828 #13484 ARROW-14989 #13482 ARROW-16977 #13514 ARROW-13404 #10999 ARROW-16887 #13601 ARROW-15906 #13206 ARROW-15280 #13171 ARROW-16144 #13183 ARROW-16511 #13105 ARROW-16085 #13088 ARROW-16715 #13555 ARROW-16268 #13550 ARROW-16700 #13518 ARROW-16807 #13583 ARROW-16871 #13517 ARROW-16415 #13190 ARROW-14821 #12154 ARROW-16439 #13174 ARROW-16394 #13118 ARROW-16516 #13163 ARROW-16395 #13627 ARROW-14848 #12589 ARROW-16407 #13196 ARROW-16653 #13506 ARROW-14575 #13160 ARROW-15271 #13170 ARROW-16703 #13650 ARROW-16444 #13397 ARROW-15016 #13541 ARROW-16776 #13563 ARROW-15622 #13090 ARROW-18131 #14484 ARROW-18305 #14581 ARROW-18285 #14615 * Closes: #33631 Authored-by: SHIMA Tatsuya <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: C++ Component: Python Component: R labels May 20, 2022

wjones127 changed the title ~~ARROW-15906: [C++][Python][R] By default, don't create or delete buckets~~ ARROW-15906: [C++][Python][R] By default, don't create or delete S3 buckets May 20, 2022

wjones127 marked this pull request as ready for review May 20, 2022 22:12

kou reviewed May 21, 2022

View reviewed changes

wjones127 force-pushed the ARROW-15906-no-new-bucket branch from 4409d3d to 06b10bf Compare May 23, 2022 16:55

wjones127 requested a review from kou May 24, 2022 17:06

kou reviewed May 24, 2022

View reviewed changes

jorisvandenbossche reviewed May 25, 2022

View reviewed changes

python/pyarrow/tests/test_dataset.py Outdated Show resolved Hide resolved

pitrou requested changes May 30, 2022

View reviewed changes

pitrou reviewed Jun 1, 2022

View reviewed changes

wjones127 force-pushed the ARROW-15906-no-new-bucket branch from 3703808 to b25cedf Compare June 2, 2022 17:44

wjones127 added 14 commits June 3, 2022 07:43

Add option to not create buckets

94e6166

Test deletion control

1e65e1d

Add support for option in Python interface

300c915

Add R changes

9a8486c

Make it easier to change the create bucket behavior

cd979a1

Document methods

3e319e2

Format

0a65bb8

Adjust test expectations

c52568f

feat: allow_create_buckets supported in S3Options::FromURI()

25adf4a

doc: Guide users to create buckets some other way

b961c26

Add back original test

b1bb3bc

chore: change to two separate settings

749300b

feat: Make boolean parsing stricter

31abe7c

feat: Just allow configuration before construction

02a1db0

wjones127 force-pushed the ARROW-15906-no-new-bucket branch from b25cedf to 02a1db0 Compare June 3, 2022 14:44

fix: remove mention of deletion on creation option

76addc5

wjones127 requested a review from pitrou June 3, 2022 19:46

pitrou requested changes Jun 6, 2022

View reviewed changes

fix: Make sure new options can be pickled

367efd3

thisisnic reviewed Jun 6, 2022

View reviewed changes

wjones127 added 2 commits June 6, 2022 14:12

chore: push error msg test down to C++

4164e11

fix: correct error match statement

deb99f3

wjones127 requested review from pitrou and thisisnic June 13, 2022 08:04

thisisnic approved these changes Jun 15, 2022

View reviewed changes

pitrou approved these changes Jun 15, 2022

View reviewed changes

pitrou merged commit c72f84a into apache:master Jun 15, 2022

wjones127 deleted the ARROW-15906-no-new-bucket branch June 15, 2022 10:10

asfimport mentioned this pull request Nov 23, 2022

[C++] S3Filesystem shouldn't create new buckets by default #31336

Closed

eitsupi mentioned this pull request Feb 19, 2023

GH-33631: [R] Rewrite Jira ticket numbers in pkgdown documents to GitHub issue numbers #34260

Merged

AlenkaF mentioned this pull request Mar 18, 2024

GH-39958: [Python][CI] Remove upper pin on pytest #40487

Merged

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

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

Uh oh!

Conversation

wjones127 commented May 20, 2022 • edited by ianmcook Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjones127 commented May 24, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Jun 6, 2022

Uh oh!

dragosmg commented Jun 6, 2022

Uh oh!

thisisnic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wjones127 commented May 20, 2022 •

edited by ianmcook

Loading