You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/09/11 03:34:55 UTC
[GitHub] [arrow] westonpace opened a new pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
westonpace opened a new pull request #11136:
URL: https://github.com/apache/arrow/pull/11136
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r712088692
##########
File path: ci/scripts/integration_minio.sh
##########
@@ -0,0 +1,63 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set -e
+
+source_dir=${1}
+build_dir=${2}
+
+mkdir ./minio_integration
+
+# Not ideal but need to run minio server in the background
+# and ensure it is up and running.
+minio server ./minio_integration --address 127.0.0.1:9000 &
+sleep 30
+
+echo '{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}' > ./limited_policy.json
+
+# Create a user with limited permissions
+mc alias set myminio http://127.0.0.1:9000 minioadmin minioadmin
+mc admin policy add myminio/ no-create-buckets ./limited_policy.json
+mc admin user add myminio/ limited limited123
+mc admin policy set myminio no-create-buckets user=limited
+# Create a bucket
+mc mb myminio/existing-bucket
+
+# Run Arrow tests relying on limited permissions user
+
+python -mpytest ${source_dir}/ci/scripts/integration_minio.py
Review comment:
Ah, fair enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r710006645
##########
File path: ci/etc/minio-no-create-bucket-policy.json
##########
@@ -0,0 +1,20 @@
+{
Review comment:
It was intended for an eventual regression test (which I've now added).
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -1530,6 +1529,23 @@ class S3FileSystem::Impl : public std::enable_shared_from_this<S3FileSystem::Imp
}
}
+ // Tests to see if a bucket exists
+ Result<bool> BucketExists(const std::string& bucket) {
+ S3Model::HeadBucketRequest req;
+ req.SetBucket(ToAwsString(bucket));
+
+ auto outcome = client_->HeadBucket(req);
+ if (!outcome.IsSuccess()) {
+ if (!IsNotFound(outcome.GetError())) {
+ return ErrorToStatus(
+ std::forward_as_tuple("When testing for bucket existence '", bucket, "': "),
Review comment:
Fixed up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace closed pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace closed pull request #11136:
URL: https://github.com/apache/arrow/pull/11136
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r707390053
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -87,6 +85,7 @@
#include "arrow/util/optional.h"
#include "arrow/util/task_group.h"
#include "arrow/util/thread_pool.h"
+#include "arrow/util/windows_fixup.h"
Review comment:
I don't know if moving this line is deliberate but it _might_ break Windows builds (or it might no; I honestly have no idea :-)).
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -1530,6 +1529,23 @@ class S3FileSystem::Impl : public std::enable_shared_from_this<S3FileSystem::Imp
}
}
+ // Tests to see if a bucket exists
+ Result<bool> BucketExists(const std::string& bucket) {
+ S3Model::HeadBucketRequest req;
+ req.SetBucket(ToAwsString(bucket));
+
+ auto outcome = client_->HeadBucket(req);
+ if (!outcome.IsSuccess()) {
+ if (!IsNotFound(outcome.GetError())) {
+ return ErrorToStatus(
+ std::forward_as_tuple("When testing for bucket existence '", bucket, "': "),
Review comment:
"for bucket existence 'xxx'" -> "for existence of bucket 'xxx'" perhaps ?
##########
File path: ci/etc/minio-no-create-bucket-policy.json
##########
@@ -0,0 +1,20 @@
+{
Review comment:
Hmm, this file doesn't seem used anywhere. Did you forget to add some other files?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JonnyWaffles commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
JonnyWaffles commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-1062406820
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r714234601
##########
File path: python/pyarrow/tests/test_fs.py
##########
@@ -298,6 +300,99 @@ def subtree_s3fs(request, s3fs):
)
+__minio_limited_policy = """{
Review comment:
Thanks! I changed these to single underscore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r713611373
##########
File path: python/pyarrow/tests/test_fs.py
##########
@@ -298,6 +300,99 @@ def subtree_s3fs(request, s3fs):
)
+__minio_limited_policy = """{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}"""
+
+
+def __run_mc_command(mcdir, *args):
+ full_args = ['mc', '-C', mcdir] + list(args)
+ proc = subprocess.Popen(full_args, stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE, encoding='utf-8')
+ retval = proc.wait(10)
+ cmd_str = ' '.join(full_args)
+ print(f'Cmd: {cmd_str}')
+ print(f' Return: {retval}')
+ print(f' Stdout: {proc.stdout.read()}')
+ print(f' Stderr: {proc.stderr.read()}')
+ if retval != 0:
+ raise ChildProcessError("Could not run mc")
+
+
+def __wait_for_minio_startup(mcdir, address, access_key, secret_key):
+ start = time.time()
+ while time.time() - start < 10:
+ try:
+ __run_mc_command(mcdir, 'alias', 'set', 'myminio',
+ f'http://{address}', access_key, secret_key)
+ return
+ except ChildProcessError:
+ time.sleep(1)
+ raise Exception("mc command could not connect to local minio")
+
+
+def __configure_limited_user(tmpdir, address, access_key, secret_key):
+ """
+ Attempts to use the mc command to configure the minio server
+ with a special user limited:limited123 which does not have
+ permission to create buckets. This mirrors some real life S3
+ configurations where users are given strict permissions.
+
+ Arrow S3 operations should still work in such a configuration
+ (e.g. see ARROW-13685)
+ """
+ try:
+ mcdir = os.path.join(tmpdir, 'mc')
+ os.mkdir(mcdir)
+ policy_path = os.path.join(tmpdir, 'limited-buckets-policy.json')
+ with open(policy_path, mode='w') as policy_file:
+ policy_file.write(__minio_limited_policy)
+ # The s3_server fixture starts the minio process but
+ # it takes a few moments for the process to become available
+ __wait_for_minio_startup(mcdir, address, access_key, secret_key)
+ # These commands create a limited user with a specific
+ # policy and creates a sample bucket for that user to
+ # write to
+ __run_mc_command(mcdir, 'admin', 'policy', 'add',
Review comment:
I wonder, could we use the minio python client instead of subprocess calls to `mc`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r712144497
##########
File path: ci/scripts/integration_minio.sh
##########
@@ -0,0 +1,63 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set -e
+
+source_dir=${1}
+build_dir=${2}
+
+mkdir ./minio_integration
+
+# Not ideal but need to run minio server in the background
+# and ensure it is up and running.
+minio server ./minio_integration --address 127.0.0.1:9000 &
+sleep 30
+
+echo '{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}' > ./limited_policy.json
+
+# Create a user with limited permissions
+mc alias set myminio http://127.0.0.1:9000 minioadmin minioadmin
Review comment:
Any ways to configure the minio server on startup?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r711418122
##########
File path: ci/docker/conda-python-minio.dockerfile
##########
@@ -0,0 +1,22 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+ARG repo
+ARG arch=amd64
+ARG python=3.6
+FROM ${repo}:${arch}-conda-python-${python}
Review comment:
Good catch. I removed this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs edited a comment on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs edited a comment on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-922904334
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](https://github.com/apache/arrow/blob/master/python/pyarrow/tests/conftest.py#L276), but with additional configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r712144024
##########
File path: ci/scripts/integration_minio.py
##########
@@ -0,0 +1,31 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# This file is called from the integration_minio crossbow job
+# It is intended to test S3 integration features that cannot
+# be tested in unit tests against a simple minio setup.
+
+# For example, issues which require a specific configuration
+# of API permissions to reproduce.
+
+import pyarrow.fs as fs
+
+def test_can_create_bucket():
Review comment:
I'd rather see this in the pyarrow testing suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r714224471
##########
File path: python/pyarrow/tests/test_fs.py
##########
@@ -298,6 +300,99 @@ def subtree_s3fs(request, s3fs):
)
+__minio_limited_policy = """{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}"""
+
+
+def __run_mc_command(mcdir, *args):
+ full_args = ['mc', '-C', mcdir] + list(args)
+ proc = subprocess.Popen(full_args, stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE, encoding='utf-8')
+ retval = proc.wait(10)
+ cmd_str = ' '.join(full_args)
+ print(f'Cmd: {cmd_str}')
+ print(f' Return: {retval}')
+ print(f' Stdout: {proc.stdout.read()}')
+ print(f' Stderr: {proc.stderr.read()}')
+ if retval != 0:
+ raise ChildProcessError("Could not run mc")
+
+
+def __wait_for_minio_startup(mcdir, address, access_key, secret_key):
+ start = time.time()
+ while time.time() - start < 10:
+ try:
+ __run_mc_command(mcdir, 'alias', 'set', 'myminio',
+ f'http://{address}', access_key, secret_key)
+ return
+ except ChildProcessError:
+ time.sleep(1)
+ raise Exception("mc command could not connect to local minio")
+
+
+def __configure_limited_user(tmpdir, address, access_key, secret_key):
+ """
+ Attempts to use the mc command to configure the minio server
+ with a special user limited:limited123 which does not have
+ permission to create buckets. This mirrors some real life S3
+ configurations where users are given strict permissions.
+
+ Arrow S3 operations should still work in such a configuration
+ (e.g. see ARROW-13685)
+ """
+ try:
+ mcdir = os.path.join(tmpdir, 'mc')
+ os.mkdir(mcdir)
+ policy_path = os.path.join(tmpdir, 'limited-buckets-policy.json')
+ with open(policy_path, mode='w') as policy_file:
+ policy_file.write(__minio_limited_policy)
+ # The s3_server fixture starts the minio process but
+ # it takes a few moments for the process to become available
+ __wait_for_minio_startup(mcdir, address, access_key, secret_key)
+ # These commands create a limited user with a specific
+ # policy and creates a sample bucket for that user to
+ # write to
+ __run_mc_command(mcdir, 'admin', 'policy', 'add',
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-1062450279
@JonnyWaffles It's probably best to open a new JIRA ticket for that request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-920796166
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r710159406
##########
File path: ci/scripts/integration_minio.py
##########
@@ -0,0 +1,24 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# This file is called from the integration_minio crossbow job
+
+import pyarrow.fs as fs
+
+def test_can_create_bucket():
+ filesystem = fs.S3FileSystem(access_key='limited', secret_key='limited123', endpoint_override='http://127.0.0.1:9000')
+ filesystem.create_dir('existing-bucket/foo') # This line fails without the change
Review comment:
Also, note the comment is a bit cryptic (what is "the change"? there should be probably be a JIRA reference here)
##########
File path: ci/scripts/integration_minio.py
##########
@@ -0,0 +1,24 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# This file is called from the integration_minio crossbow job
+
+import pyarrow.fs as fs
+
+def test_can_create_bucket():
+ filesystem = fs.S3FileSystem(access_key='limited', secret_key='limited123', endpoint_override='http://127.0.0.1:9000')
+ filesystem.create_dir('existing-bucket/foo') # This line fails without the change
Review comment:
These files are probably not linted by `archery lint`, but can you ensure line length doesn't go over 80-85 characters?
##########
File path: ci/scripts/integration_minio.sh
##########
@@ -0,0 +1,63 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set -e
+
+source_dir=${1}
+build_dir=${2}
+
+mkdir ./minio_integration
+
+# Not ideal but need to run minio server in the background
+# and ensure it is up and running.
+minio server ./minio_integration --address 127.0.0.1:9000 &
+sleep 30
+
+echo '{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}' > ./limited_policy.json
+
+# Create a user with limited permissions
+mc alias set myminio http://127.0.0.1:9000 minioadmin minioadmin
+mc admin policy add myminio/ no-create-buckets ./limited_policy.json
+mc admin user add myminio/ limited limited123
+mc admin policy set myminio no-create-buckets user=limited
+# Create a bucket
+mc mb myminio/existing-bucket
+
+# Run Arrow tests relying on limited permissions user
+
+python -mpytest ${source_dir}/ci/scripts/integration_minio.py
Review comment:
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?
##########
File path: ci/docker/conda-python-minio.dockerfile
##########
@@ -0,0 +1,22 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+ARG repo
+ARG arch=amd64
+ARG python=3.6
+FROM ${repo}:${arch}-conda-python-${python}
Review comment:
Is this file useful, since it doesn't seem to add anything to `conda-python.dockerfile`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r711418294
##########
File path: ci/scripts/integration_minio.sh
##########
@@ -0,0 +1,63 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+set -e
+
+source_dir=${1}
+build_dir=${2}
+
+mkdir ./minio_integration
+
+# Not ideal but need to run minio server in the background
+# and ensure it is up and running.
+minio server ./minio_integration --address 127.0.0.1:9000 &
+sleep 30
+
+echo '{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}' > ./limited_policy.json
+
+# Create a user with limited permissions
+mc alias set myminio http://127.0.0.1:9000 minioadmin minioadmin
+mc admin policy add myminio/ no-create-buckets ./limited_policy.json
+mc admin user add myminio/ limited limited123
+mc admin policy set myminio no-create-buckets user=limited
+# Create a bucket
+mc mb myminio/existing-bucket
+
+# Run Arrow tests relying on limited permissions user
+
+python -mpytest ${source_dir}/ci/scripts/integration_minio.py
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r711418182
##########
File path: ci/scripts/integration_minio.py
##########
@@ -0,0 +1,24 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# This file is called from the integration_minio crossbow job
+
+import pyarrow.fs as fs
+
+def test_can_create_bucket():
+ filesystem = fs.S3FileSystem(access_key='limited', secret_key='limited123', endpoint_override='http://127.0.0.1:9000')
+ filesystem.create_dir('existing-bucket/foo') # This line fails without the change
Review comment:
I removed the comment and cleaned up the line lengths.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r710006852
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -87,6 +85,7 @@
#include "arrow/util/optional.h"
#include "arrow/util/task_group.h"
#include "arrow/util/thread_pool.h"
+#include "arrow/util/windows_fixup.h"
Review comment:
I undid this change (over-zealous IDE)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-924544031
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-922851863
Revision: 1c4461ac3c13553a180ec9fb9a007a000a0aefd9
Submitted crossbow builds: [ursacomputing/crossbow @ actions-846](https://github.com/ursacomputing/crossbow/branches/all?query=actions-846)
|Task|Status|
|----|------|
|test-conda-python-minio|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-846-github-test-conda-python-minio)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r713611373
##########
File path: python/pyarrow/tests/test_fs.py
##########
@@ -298,6 +300,99 @@ def subtree_s3fs(request, s3fs):
)
+__minio_limited_policy = """{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Effect": "Allow",
+ "Action": [
+ "s3:ListAllMyBuckets",
+ "s3:PutObject",
+ "s3:GetObject",
+ "s3:ListBucket",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject",
+ "s3:GetObjectVersion"
+ ],
+ "Resource": [
+ "arn:aws:s3:::*"
+ ]
+ }
+ ]
+}"""
+
+
+def __run_mc_command(mcdir, *args):
+ full_args = ['mc', '-C', mcdir] + list(args)
+ proc = subprocess.Popen(full_args, stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE, encoding='utf-8')
+ retval = proc.wait(10)
+ cmd_str = ' '.join(full_args)
+ print(f'Cmd: {cmd_str}')
+ print(f' Return: {retval}')
+ print(f' Stdout: {proc.stdout.read()}')
+ print(f' Stderr: {proc.stderr.read()}')
+ if retval != 0:
+ raise ChildProcessError("Could not run mc")
+
+
+def __wait_for_minio_startup(mcdir, address, access_key, secret_key):
+ start = time.time()
+ while time.time() - start < 10:
+ try:
+ __run_mc_command(mcdir, 'alias', 'set', 'myminio',
+ f'http://{address}', access_key, secret_key)
+ return
+ except ChildProcessError:
+ time.sleep(1)
+ raise Exception("mc command could not connect to local minio")
+
+
+def __configure_limited_user(tmpdir, address, access_key, secret_key):
+ """
+ Attempts to use the mc command to configure the minio server
+ with a special user limited:limited123 which does not have
+ permission to create buckets. This mirrors some real life S3
+ configurations where users are given strict permissions.
+
+ Arrow S3 operations should still work in such a configuration
+ (e.g. see ARROW-13685)
+ """
+ try:
+ mcdir = os.path.join(tmpdir, 'mc')
+ os.mkdir(mcdir)
+ policy_path = os.path.join(tmpdir, 'limited-buckets-policy.json')
+ with open(policy_path, mode='w') as policy_file:
+ policy_file.write(__minio_limited_policy)
+ # The s3_server fixture starts the minio process but
+ # it takes a few moments for the process to become available
+ __wait_for_minio_startup(mcdir, address, access_key, secret_key)
+ # These commands create a limited user with a specific
+ # policy and creates a sample bucket for that user to
+ # write to
+ __run_mc_command(mcdir, 'admin', 'policy', 'add',
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-923212509
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r712125039
##########
File path: ci/scripts/install_minio.sh
##########
@@ -49,4 +49,6 @@ elif [[ ${version} != "latest" ]]; then
fi
wget -nv -P ${prefix}/bin https://dl.min.io/server/minio/release/${platform}-${arch}/minio
+wget -nv -P ${prefix}/bin https://dl.min.io/client/mc/release/${platform}-${arch}/mc
Review comment:
Handy!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-922904334
Couldn't we spin up another properly configured minio server from the pytest test suite and just exercise the regression test on it?
Similarly like [we already do in conftest](https://github.com/apache/arrow/blob/master/python/pyarrow/tests/conftest.py#L276), but with additional configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-920797500
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-922851333
@github-actions crossbow submit test-conda-python-minio
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-918122454
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#discussion_r713611102
##########
File path: python/pyarrow/tests/test_fs.py
##########
@@ -298,6 +300,99 @@ def subtree_s3fs(request, s3fs):
)
+__minio_limited_policy = """{
Review comment:
Single leading underscore should be sufficient for all of the "protected" variables and functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-917330704
https://issues.apache.org/jira/browse/ARROW-13685
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #11136: ARROW-13685: [C++] Cannot write dataset to S3FileSystem if bucket already exists
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11136:
URL: https://github.com/apache/arrow/pull/11136#issuecomment-931246618
I kind of forgot about this. I rebased and fixed a lint error. Assuming CI passes I will merge this tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org