Commit 6f5af7e5 authored by Tomeu Vizoso's avatar Tomeu Vizoso

ci-fairy: Fix minio cp command when there's no list permissions

Before this patch, in order to upload files we were listing all buckets
and also all objects in the destination bucket.

The user may not necessarily have access for listing even if the upload
would have been fine, thus preventing the copy to be performed.

Perform the listings lazily so we can still copy.

Squashed are several fixes and test improvements by Benjamin Tissoires.
Signed-off-by: default avatarTomeu Vizoso <tomeu.vizoso@collabora.com>
Signed-off-by: Benjamin Tissoires's avatarBenjamin Tissoires <benjamin.tissoires@gmail.com>
parent 52dd4a94
......@@ -97,7 +97,7 @@ stages:
before_script:
- curl https://bootstrap.pypa.io/get-pip.py -o /root/get-pip.py
- python3 /root/get-pip.py
- dnf install -y git-core
- dnf install -y git-core gcc
sanity check:
extends: .pip_install
......
......@@ -41,7 +41,7 @@ stages:
before_script:
- curl https://bootstrap.pypa.io/get-pip.py -o /root/get-pip.py
- python3 /root/get-pip.py
- dnf install -y git-core
- dnf install -y git-core gcc
sanity check:
extends: .pip_install
......
#!/usr/bin/env python3
import boto3
import botocore
import click
import colored
import functools
......@@ -323,16 +324,23 @@ class S3Remote(S3Object):
if creds is None:
raise KeyError(f"host '{host}' not found in credentials, please use 'ci-fairy minio login' first")
s3 = boto3.resource('s3',
endpoint_url=creds['endpoint_url'],
aws_access_key_id=creds['AccessKeyId'],
aws_secret_access_key=creds['SecretAccessKey'],
aws_session_token=creds['SessionToken'],
config=Config(signature_version='s3v4'),
region_name='us-east-1')
self._s3 = boto3.resource('s3',
endpoint_url=creds['endpoint_url'],
aws_access_key_id=creds['AccessKeyId'],
aws_secret_access_key=creds['SecretAccessKey'],
aws_session_token=creds['SessionToken'],
config=Config(signature_version='s3v4'),
region_name='us-east-1')
self._name = host
self.bucket_names = [b.name for b in s3.buckets.all()]
self.buckets = {b: s3.Bucket(b) for b in self.bucket_names}
self._buckets = None
@property
def buckets(self):
if self._buckets is None:
bucket_names = [b.name for b in self._s3.buckets.all()]
self._buckets = {b: self._s3.Bucket(b) for b in bucket_names}
return self._buckets
@property
def name(self):
......@@ -355,7 +363,7 @@ class S3Remote(S3Object):
@property
def children(self):
return [S3Bucket(b) for b in self.buckets.values()]
return [S3Bucket(b, self) for b in self.buckets.values()]
def get(self, path):
# Remove the host from the URL
......@@ -371,22 +379,17 @@ class S3Remote(S3Object):
if '/' in path:
bucket_name, key = path.split('/', 1)
try:
bucket = self.buckets[bucket_name]
except KeyError:
# minio://host/bucket_that_doesn_t_exist
raise FileNotFoundError(f"bucket '{bucket_name}' doesn't exist on {self.name}")
return S3Bucket(bucket).get(key)
return S3Bucket(self._s3.Bucket(bucket_name), self).get(key)
class S3Bucket(S3Object):
'''
A remote S3 bucket
'''
def __init__(self, bucket):
def __init__(self, bucket, remote):
super().__init__('/')
self._bucket = bucket
self._remote = remote
@property
def exists(self):
......@@ -402,6 +405,10 @@ class S3Bucket(S3Object):
@property
def children(self):
if self.name not in self._remote.buckets:
# minio://host/bucket_that_doesn_t_exist
raise FileNotFoundError(f"bucket '{self.name}' doesn't exist on {self._remote.name}")
objs = [o.key for o in self._bucket.objects.all()]
children = []
......@@ -468,42 +475,9 @@ class S3RemoteObject(S3Object):
def __init__(self, bucket, key):
super().__init__(key)
self._bucket = bucket
self._is_dir = False
self._children = []
objs = [o.key for o in bucket.objects if o.key.startswith(key)]
# find if the object exists already (full key matches)
self._exists = key in objs
# special case for directories
if not objs:
# nothing in the remote matches, check if we
# have a terminating '/'
self._is_dir = key.endswith('/')
elif not self._exists:
# at least one remote object starts with our key,
# check if we have a parent of a remote object, or
# just if the path and name starts with the same key
# build the list of all parents of all objects
parents = [p for o in objs for p in Path(o).parents]
self._is_dir = self._exists = self.path in parents
# compute the list of files or dir immediately below the
# current dir
if self._is_dir:
for o in objs:
path = Path(o)
for parent in path.parents:
if parent == self.path:
self._children.append(path)
break
path = parent
self._is_dir = key.endswith('/')
self._children = None
self._exists = False
@property
def is_dir(self):
......@@ -519,6 +493,41 @@ class S3RemoteObject(S3Object):
@property
def children(self):
if self._children is None:
self._children = []
objs = [o.key for o in self._bucket.objects if o.key.startswith(self.key)]
# find if the object exists already (full key matches)
self._exists = self.key in objs
# special case for directories
if not objs:
# nothing in the remote matches, check if we
# have a terminating '/'
self._is_dir = self.key.endswith('/')
elif not self._exists:
# at least one remote object starts with our key,
# check if we have a parent of a remote object, or
# just if the path and name starts with the same key
# build the list of all parents of all objects
parents = [p for o in objs for p in Path(o).parents]
self._is_dir = self._exists = self.path in parents
# compute the list of files or dir immediately below the
# current dir
if self._is_dir:
for o in objs:
path = Path(o)
for parent in path.parents:
if parent == self.path:
self._children.append(path)
break
path = parent
return self._children
def copy_from(self, other):
......@@ -653,10 +662,19 @@ def login(credentials, endpoint_url, token):
def ls(ctx, credentials, path):
try:
s3_obj = S3.s3(path, credentials)
except FileNotFoundError as e:
ctx.fail(e)
except KeyError as e:
ctx.fail(e)
except botocore.exceptions.ClientError as e:
ctx.fail(e)
# for ls, we need to actually query the object to check if it exists
# we can not rely on the assumption it does exist
# calling children lazily evaluate the object and we sync ourself
# with the remote
try:
children = s3_obj.children
except FileNotFoundError as e:
ctx.fail(e)
if not s3_obj.exists:
ctx.fail(f"file '{path}' does not exist")
......@@ -664,7 +682,7 @@ def ls(ctx, credentials, path):
if not s3_obj.is_dir:
print(s3_obj.name)
else:
for o in s3_obj.children:
for o in children:
print(o.name)
......@@ -676,19 +694,15 @@ def ls(ctx, credentials, path):
def cp(ctx, credentials, src, dst):
try:
src = S3.s3(src, credentials)
except FileNotFoundError as e:
ctx.fail(e)
except KeyError as e:
ctx.fail(e)
# src doesn't exist
if not src.exists:
if not src.exists and src.is_local:
ctx.fail(f"source file '{src.path}' does not exist")
try:
dst = S3.s3(dst, credentials)
except FileNotFoundError as e:
ctx.fail(e)
except KeyError as e:
ctx.fail(e)
......@@ -698,6 +712,10 @@ def cp(ctx, credentials, src, dst):
ctx.fail(e)
except IsADirectoryError as e:
ctx.fail(e)
except boto3.exceptions.S3UploadFailedError as e:
ctx.fail(e)
except botocore.exceptions.ClientError as e:
ctx.fail(e)
@ci_fairy.command()
......
......@@ -2,6 +2,7 @@
from click.testing import CliRunner
from unittest.mock import patch, MagicMock
import botocore
import git
import json
import pytest
......@@ -817,6 +818,8 @@ def test_minio_login(session, json_dump, caplog):
def mock_minio(minio):
error_response = {'Error': {'Code': 404, 'Message': 'Not Found'}}
_404 = botocore.exceptions.ClientError(error_response, 'HeadObject')
buckets = []
for i in range(3):
bucket = MagicMock()
......@@ -839,6 +842,8 @@ def mock_minio(minio):
files.append(f)
def download_file(src, dst):
if src not in [f.key for f in files]:
raise _404
with open(dst, 'w') as f:
f.write('Hello World!')
......@@ -859,6 +864,9 @@ def mock_minio(minio):
bucket = MagicMock()
bucket.name = arg
bucket.objects.all = MagicMock(return_value=[])
bucket.upload_file.side_effect = _404
bucket.download_file.side_effect = _404
return bucket
ctx.Bucket.side_effect = bucket_side_effect
......@@ -1025,10 +1033,10 @@ def test_minio_ls_no_creds(minio, caplog):
(['local_file', 'an_other_local_file'], "Error: source file 'local_file' does not exist"),
# source file does not exist
([f'minio://{MINIO_TEST_SERVER}/bucket_that_does_not_exist/file', 'an_other_local_file'], "Error: bucket 'bucket_that_does_not_exist' doesn't exist"),
([f'minio://{MINIO_TEST_SERVER}/bucket_that_does_not_exist/file', 'an_other_local_file'], "404"),
# source file does not exist
([f'minio://{MINIO_TEST_SERVER}/bucket1/file', 'an_other_local_file'], "Error: source file 'file' does not exist"),
([f'minio://{MINIO_TEST_SERVER}/bucket1/file', 'an_other_local_file'], "404"),
# source file is a local dir
(['.', 'local_file'], 'Error: cannot do recursive cp'),
......@@ -1037,7 +1045,7 @@ def test_minio_ls_no_creds(minio, caplog):
([f'minio://{MINIO_TEST_SERVER}/bucket0/', 'local_file'], 'Error: cannot do recursive cp'),
# source file is a remote dir
([f'minio://{MINIO_TEST_SERVER}/bucket1/dir-0', 'local_file'], 'Error: cannot do recursive cp'),
([f'minio://{MINIO_TEST_SERVER}/bucket1/dir-0', 'local_file'], '404'),
# source file is a remote dir
([f'minio://{MINIO_TEST_SERVER}/bucket1/dir-1/', 'local_file'], 'Error: cannot do recursive cp'),
......@@ -1058,10 +1066,10 @@ def test_minio_ls_no_creds(minio, caplog):
(['hello.txt', 'minio://WRONG_MINIO_TEST_SERVER/'], 'Error: "host \'WRONG_MINIO_TEST_SERVER\' not found in credentials'),
# destination is invalid (no bucket)
(['hello.txt', f'minio://{MINIO_TEST_SERVER}//dir-0/'], "Error: bucket 'dir-0' doesn't exist"),
(['hello.txt', f'minio://{MINIO_TEST_SERVER}//dir-0/'], '404'),
# destination is invalid (wrong bucket)
(['hello.txt', f'minio://{MINIO_TEST_SERVER}/this_bucket_doesnt_exist/dir-0/'], "Error: bucket 'this_bucket_doesnt_exist' doesn't exist"),
(['hello.txt', f'minio://{MINIO_TEST_SERVER}/this_bucket_doesnt_exist/dir-0/'], '404'),
# destination is invalid (new dir)
([f'minio://{MINIO_TEST_SERVER}/bucket1/dir-0/file-1', 'new_dir/'], "Error: directory 'new_dir' does not exist"),
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment