ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists by westonpace · Pull Request #11136 · apache/arrow

westonpace · 2021-09-11T03:34:47Z

I still need to add a regression test. I've been able to test by configuring my server with minio client. I think it'd probably be easiest to create a crossbow test for this situation. Current steps:

mc alias set myminio http://localhost:9000 minioadmin minioadmin
mc admin policy add myminio/ no-create-buckets ci/etc/minio-no-create-bucket-policy.json
mc admin user add myminio/ limited limited123
mc admin policy set myminio no-create-buckets user=limited
mc mb myminio/existing-bucket

Then, in python:

import pyarrow.fs as fs
filesystem = fs.S3FileSystem(access_key='limited', secret_key='limited123', endpoint_override='http://localhost:9000')
filesystem.create_dir('existing-bucket/foo') # This line fails without the change

github-actions · 2021-09-11T03:35:06Z

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

lidavidm · 2021-09-13T12:03:11Z

The tests do spawn minio - maybe it would be possible to also ensure/check if mc is on the path and run the test that way?

westonpace · 2021-09-16T10:55:22Z

@kszucs Any chance you'd be able to take a look at this integration test? It works but I'm not sure if you have any suggestions for doing it better.

westonpace · 2021-09-16T10:57:31Z

@lidavidm I considered running mc programmatically but since I have to run several different commands and there is quite a bit of boilerplate in s3_test_util.h I worried it would end up being more complex than a solution like this (using a nightly build job). Also, it would add one more step for anyone wanting to run tests for local development.

pitrou

Thanks for doing this. The fix looks fine. Just a couple questions about the new CI tests.

pitrou · 2021-09-16T14:12:59Z

I am not entirely sure an integration is deserved for this (after all we're just checking a single regression), though I have no strong opinion. @jorisvandenbossche What do you think?

I agree it is a bit of overhead for a single regression. My thinking is that it will be a good starting point that can be extended in the future in case we run into future issues that either require running against a real S3 instance / configuration or require specific permissions.

Ah, fair enough.

pitrou · 2021-09-20T11:41:09Z

@github-actions crossbow submit test-conda-python-minio

github-actions · 2021-09-20T11:42:05Z

Revision: 1c4461ac3c13553a180ec9fb9a007a000a0aefd9

Submitted crossbow builds: ursacomputing/crossbow @ actions-846

Task	Status
test-conda-python-minio

pitrou

+1, thank you.

kszucs · 2021-09-20T13:00:10Z

Couldn't we spin up another properly configured minio server from the pytest test suite and just exercise the regression test on it? That way we would always run that test, so no need for additional scripts, docker-compose service and crossbow task.

Similarly like we already do in conftest, but with additional configuration.

westonpace · 2021-09-20T19:22:35Z

Couldn't we spin up another properly configured minio server from the pytest test suite and just exercise the regression test on it? That way we would always run that test, so no need for additional scripts, docker-compose service and crossbow task.

@kszucs

Yes, this was David's point too. My initial reluctance was that it would require anyone that wants to run the test to have the mc binary in addition to the minio binary but, in retrospect, that may not be too much of an additional burden. I'll try spinning up a version that works this way.

westonpace · 2021-09-22T03:07:20Z

Per everyone's suggestions I have moved the test from a standalone test to a builtin test that is skipped if mc is not present.

kszucs · 2021-09-22T05:50:45Z

Single leading underscore should be sufficient for all of the "protected" variables and functions.

Thanks! I changed these to single underscore.

kszucs · 2021-09-22T05:51:27Z

I wonder, could we use the minio python client instead of subprocess calls to mc?
https://docs.min.io/docs/python-client-api-reference.html

I don't think the python client exposes admin operations like adding policies or users (at least, not that I can tell). Unfortunately, set_bucket_policy is not sufficient because the s3:CreateBucket operation only makes sense as a user permission.

…y instead of relying on CreateBucket to do so as CreateBucket can fail for permission denied reasons

…ssions

…y tasks

…st suite instead of a dedicated integration test.

westonpace · 2021-09-30T11:44:49Z

I kind of forgot about this. I rebased and fixed a lint error. Assuming CI passes I will merge this tomorrow.

…s not to be confused with mc.exe which is the Windows message compiler

…eady exists I still need to add a regression test. I've been able to test by configuring my server with minio client. I think it'd probably be easiest to create a crossbow test for this situation. Current steps: ``` mc alias set myminio http://localhost:9000 minioadmin minioadmin mc admin policy add myminio/ no-create-buckets ci/etc/minio-no-create-bucket-policy.json mc admin user add myminio/ limited limited123 mc admin policy set myminio no-create-buckets user=limited mc mb myminio/existing-bucket ``` Then, in python: ``` import pyarrow.fs as fs filesystem = fs.S3FileSystem(access_key='limited', secret_key='limited123', endpoint_override='http://localhost:9000') filesystem.create_dir('existing-bucket/foo') # This line fails without the change ``` Closesapache#11136 from westonpace/bugfix/ARROW-13685-cannot-write-to-s3-if-bucket-exists Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

JonnyWaffles · 2022-03-08T23:57:48Z

Hi team, @westonpace is it possible to open this thread back up? Our environment is tightly locked down and I cannot grant my application the necessary s3:ListBucket resource permissions. Is it possible to add a keyword argument to skip this check behavior entirely, so I can write my data similar to if I use the pq.write_table functionality?

westonpace · 2022-03-09T01:10:06Z

@JonnyWaffles It's probably best to open a new JIRA ticket for that request.

westonpace marked this pull request as draft September 11, 2021 03:34

github-actionsBot added the Component: C++ label Sep 11, 2021

jorisvandenbossche requested a review from pitrou September 13, 2021 14:08

pitrou reviewed Sep 13, 2021
View reviewed changes

Comment threadcpp/src/arrow/filesystem/s3fs.cc Outdated
Comment threadcpp/src/arrow/filesystem/s3fs.cc Outdated
Comment threadci/etc/minio-no-create-bucket-policy.json Outdated

westonpace marked this pull request as ready for review September 16, 2021 10:54

westonpace requested a review from pitrou September 16, 2021 10:54

pitrou reviewed Sep 16, 2021
View reviewed changes

pitrou approved these changes Sep 20, 2021
View reviewed changes

kszucs reviewed Sep 20, 2021
View reviewed changes

Comment threadci/scripts/install_minio.sh Outdated

kszucs reviewed Sep 20, 2021
View reviewed changes

Comment threadci/scripts/integration_minio.py Outdated

kszucs reviewed Sep 20, 2021
View reviewed changes

Comment threadci/scripts/integration_minio.sh Outdated

westonpace force-pushed the bugfix/ARROW-13685-cannot-write-to-s3-if-bucket-exists branch from 1c4461a to a9d2287Compare September 21, 2021 23:26

github-actionsBot added the Component: Python label Sep 21, 2021

westonpace requested review from kszucs and pitrou September 22, 2021 03:07

kszucs reviewed Sep 22, 2021
View reviewed changes

westonpace added 4 commits September 30, 2021 01:42

ARROW-13685: Changed CreateDir to test for bucket existence explicitl…
496e89b
…y instead of relying on CreateBucket to do so as CreateBucket can fail for permission denied reasons

ARROW-13685: Created an integration test for minio with limited permi…
8911f3f
…ssions

ARROW-13685: Reverted header ordering change
2eea041

ARROW-13685: Touched up error per PR review
06deba4

westonpace added 7 commits September 30, 2021 01:42

ARROW-13685: Removed needless minio dockerfile. Added minio to nightl…
bc43ad1
…y tasks

ARROW-13685: Updated comments and cleaned up test
54c6aa5

ARROW-13685: Forgot to remove unneeded dockerfile from the repo
23e56a2

ARROW-13685: Moved mc-based integration tests into the main python te…
765a2c0
…st suite instead of a dedicated integration test.

ARROW-13685: Forgot to revert change to tasks.yml
c83c166

ARROW-13685: Convert leading __ to leading _
b28f415

ARROW-13685: Lint errors
249a499

westonpace force-pushed the bugfix/ARROW-13685-cannot-write-to-s3-if-bucket-exists branch from a173cba to 249a499Compare September 30, 2021 11:42

ARROW-13685: Proactively disabling tests requiring mc on Windows so a…
6b8a57c
…s not to be confused with mc.exe which is the Windows message compiler

westonpace closed this in 700ac1eOct 1, 2021

westonpace deleted the bugfix/ARROW-13685-cannot-write-to-s3-if-bucket-exists branch January 6, 2022 08:15

This was referenced Mar 9, 2022
[C++] Cannot write dataset to S3FileSystem if bucket already exists #29321
Closed
[C++] Dataset APIs require s3:ListBucket Permissions #20147
Closed

Uh oh!

Conversation

westonpace commented Sep 11, 2021

Uh oh!

github-actionsBot commented Sep 11, 2021

Uh oh!

lidavidm commented Sep 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonpace commented Sep 16, 2021

Uh oh!

westonpace commented Sep 16, 2021

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitrouSep 16, 2021

Choose a reason for hiding this comment

Uh oh!

westonpaceSep 18, 2021

Choose a reason for hiding this comment

Uh oh!

pitrouSep 20, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou commented Sep 20, 2021

Uh oh!

github-actionsBot commented Sep 20, 2021

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kszucs commented Sep 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonpace commented Sep 20, 2021

Uh oh!

westonpace commented Sep 22, 2021

Uh oh!

kszucsSep 22, 2021

Choose a reason for hiding this comment

Uh oh!

westonpaceSep 22, 2021

Choose a reason for hiding this comment

Uh oh!

kszucsSep 22, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonpaceSep 22, 2021

Choose a reason for hiding this comment

Uh oh!

westonpace commented Sep 30, 2021

Uh oh!

JonnyWaffles commented Mar 8, 2022

Uh oh!

westonpace commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kszucs commented Sep 20, 2021•
edited
Loading

kszucsSep 22, 2021•
edited
Loading