QOL-8612 make non-current S3 objects private #75

ThrawnCA · 2022-02-04T04:03:11Z

Older objects, with different filenames, will still be accessible via the orig_download URL, but will not be directly visible and cacheable.

- this makes it easier to wipe the 'all objects' cache when uploading a new object

duttonw

looking good, would like some improvements for optional flags so others can use this and have all resources still public if they want.

as well as ability for the system to clean up dead resource files which are not linked anymore from s3 which in turn speeds up the system.

duttonw · 2022-02-04T06:30:47Z

ckanext/s3filestore/uploader.py

-            is_public_read = self.is_key_public(upload['Key'])
+            upload_key = upload['Key']
+            log.debug("Setting visibility for key [%s], current object is [%s]", upload_key, current_key)
+            if upload_key == current_key:


Can we put this behind a config flag
ckanext.s3filestore.hideOldResourceObject = true/false #default value: true
if true if not same as db key then set private
if false, same as original functionality (all is same as active file)

another option for a 'cleaned up' s3 is

ckanext.s3filestore.deleteNonLinkedResource = true/false #default value: false
ckanext.s3filestore.deleteNonLinkedResourceAge = <number of days old> #default value: 90

if a file is found that does not match database and it is older than deleteNonLinkedResourceAge, then delete file so it is treated like it was overwritten with possible backup window. (Age might be hard to do as i don't think you can record acl changes and tag key/values are immutable unless the object is 'refreshed/re-uploaded'

for our purposes we would have it set to true. As we use s3 versioning we have 90+ days to recover the file if requested.

This keeps our blob storage size down and only list what is still active.

seems you can't 'modify' objects to have a marker, so deleteNonLinkedResourceAge can't be done if true it will purge dead data.

I've added a config value to control the ACLs of non-current files. Default is 'private'.

I'm unclear, though, on why deleting old objects would require modifying tags. Couldn't we just rely on the built-in "Last Modified" metadata?

ok, thats true, if its fresh it is kept for x days and then deleted which will give it more grace period.

duttonw · 2022-02-04T06:32:07Z

ckanext/s3filestore/uploader.py

+                    Bucket=self.bucket_name, Key=upload_key, ACL=acl)
+                self._cache_delete(upload_key)
+                self._cache_put(upload_key + VISIBILITY_CACHE_PATH, acl)
+        self._cache_put(current_key + VISIBILITY_CACHE_PATH + '/all', target_acl)


is it possible to set a longer window for /all and for /visibility keys, 1 hour may not be long enough for the performance improvements and with these items being so small, i think 30 days should be ok, similar for public files, they should be left in cache far longer than when they are private and expire.

eg update signature of _cache_put to
def _cache_put(self, key, value, ttl=self.signed_url_cache_window):
so default is from cache_window but for acl targets, they can be 30 days for max speed and reduce s3 'hits'

Hmm...since the cache is automatically cleared when a resource is uploaded, I suppose it is reasonable to have a fairly long expiry on it if that hasn't happened.

…d ACL if any

- ACLs can have a very long expiry, since they should only change when a resource or dataset is changed, will will override the cache settings anyway.

duttonw · 2022-02-07T01:26:13Z

ckanext/s3filestore/uploader.py

@@ -128,14 +129,21 @@ def _cache_get(self, key):
            cache_value = six.ensure_text(cache_value)
        return cache_value

-    def _cache_put(self, key, value):
-        ''' Set a value in the cache, if enabled, with an appropriate expiry.
+    def _cache_put_url(self, key, value):


unless we will be doing other things (like appending /acl or visability or all etc), lets keep it simple and let this just take a ttl value param and have the default as self.signed_url_cache_window if None is passed in, so that the ttl can be injected when doing acl stuff.

Well, there's already a logical split based on whether signed URL caching is enabled, and I've just added another to automatically distinguish signed from unsigned URLs.

- unsigned URLs don't expire so they can be cached much longer

- when updating object visibility, if deletion age is configured and non-current object is older than configured age, then delete instead of updating

duttonw · 2022-02-07T03:53:54Z

README.rst

+    ckanext.s3filestore.acl_cache_window = 2592000
+
+    # If set, then prior objects uploaded for a resource may be deleted
+    # after the specified number of days. If less than zero, nothing


Please update this that this is about when resource was uploaded. Not when delete function was called.
i.e. if uploaded over 90 days ago, it will be purged if set to 90. if it was uploaded yesterday, it will be kept for 89 days. Then it is up to the configuration of the s3 bucket on how long deleted items are kept.

I'm not sure which part of the documentation is unclear about that.

duttonw · 2022-02-07T04:03:01Z

ckanext/s3filestore/uploader.py

@@ -308,7 +327,7 @@ def get_signed_url_to_key(self, key, extra_params={}):
        if is_public_read:
            url = url.split('?')[0] + '?ETag=' + metadata['ETag']

-        self._cache_put(key, url)
+        self._cache_put_url(key, url)


can you look at doing a refactor on this method def get_signed_url_to_key(self, key, extra_params={}):

to check cache at the start before doing the s3 head calls etc.
And if it is in the cache, return it. saves as doing so many checks when its still the same.

We also saw that if redis/cache_put fails, it stops the url being returned to the customer. Can you wrap the redis put with a try, log error continue as its a nice to have not a show stopper for getting the url.

Also on redis get, do similar that if it throws exception that None is return per it not finding a cached key.

…d-gov-au/ckanext-s3filestore into QOL-8612-manage-s3-visibility

…estore into QOL-8612-manage-s3-visibility

- return without contacting S3 API if we have a cached URL - add error handling so a Redis failure doesn't break the whole system - make Flake8 happy

…ier to reuse

ThrawnCA added 11 commits February 4, 2022 11:16

[QOL-8612] make non-current S3 objects private

3ba849d

[QOL-8612] increase test debugging

1d851c4

[QOL-8612] increase test debugging

4d4e398

[QOL-8612] increase test debugging

67f3bec

[QOL-8612] increase debugging

0a01e52

[QOL-8612] increase debugging

bb77dff

[QOL-8612] make test filenames match URLs

ea1f474

[QOL-8612] clear cache of object visibility when updating resource

86e1fbd

[QOL-8612] fix visibility cache key names

fa684a0

[QOL-8612] base visibility cache on path, not resource ID

fa24291

- this makes it easier to wipe the 'all objects' cache when uploading a new object

[QOL-8612] revert logging levels

8f169fa

ThrawnCA requested review from duttonw, asifaminb, chris-randall-qol, devonpis, kundupz and RossWebsterWork February 4, 2022 04:05

duttonw requested changes Feb 4, 2022

View reviewed changes

ThrawnCA added 4 commits February 7, 2022 10:24

[QOL-8612] add test to check that non-current objects match configure…

382cd3f

…d ACL if any

[QOL-8612] use config flag to control visibility of non-current objects

5982862

[QOL-8612] set non-current visibility to match current if flag is 'auto'

8f7edac

[QOL-8612] split cache expiry settings vs URLs vs ACLs

04393ce

- ACLs can have a very long expiry, since they should only change when a resource or dataset is changed, will will override the cache settings anyway.

duttonw requested changes Feb 7, 2022

View reviewed changes

ThrawnCA added 3 commits February 7, 2022 11:30

[QOL-8612] split unsigned URL expiry settings vs signed ones

613ff11

- unsigned URLs don't expire so they can be cached much longer

[QOL-8612] add deletion of non-current files

2fe8065

- when updating object visibility, if deletion age is configured and non-current object is older than configured age, then delete instead of updating

[QOL-8612] specify time zone for 'now' so we can do date arithmetic

2cd2877

duttonw reviewed Feb 7, 2022

View reviewed changes

duttonw and others added 2 commits February 7, 2022 14:42

[QOL-8612] update readme to better explain resource cleanup

cabdf53

[QOL-8612] oops expect private test URL by default

e64d5ec

ThrawnCA and others added 9 commits February 7, 2022 14:56

[QOL-8612] wrap boto error in toolkit error

a78e3d0

Merge branch 'QOL-8612-manage-s3-visibility' of https://github.com/ql…

6714a9b

…d-gov-au/ckanext-s3filestore into QOL-8612-manage-s3-visibility

Merge branch 'develop' of https://github.com/qld-gov-au/ckanext-s3fil…

7be468f

…estore into QOL-8612-manage-s3-visibility

[QOL-8612] cleanup

f49f49c

- return without contacting S3 API if we have a cached URL - add error handling so a Redis failure doesn't break the whole system - make Flake8 happy

[QOL-8612] drop duplicate resource loading

6bb0655

[QOL-8612] extract Redis functions to a separate class so they're eas…

fa019d9

…ier to reuse

[QOL-8612] only update object visibility if package visibility changes

65e15c5

[QOL-8612] oops check new privacy status even if old doesn't exist

3b87820

[QOL-8612] fix logic

9d541c6

duttonw approved these changes Feb 7, 2022

View reviewed changes

[QOL-8612] use fully path to redis helper class

c50c8df

duttonw merged commit 70dbb38 into develop Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QOL-8612 make non-current S3 objects private #75

QOL-8612 make non-current S3 objects private #75

ThrawnCA commented Feb 4, 2022 •

edited

Loading

duttonw left a comment

duttonw Feb 4, 2022 •

edited

Loading

duttonw Feb 4, 2022

ThrawnCA Feb 7, 2022

duttonw Feb 7, 2022

duttonw Feb 4, 2022

duttonw Feb 4, 2022

ThrawnCA Feb 7, 2022

duttonw Feb 7, 2022

ThrawnCA Feb 7, 2022

duttonw Feb 7, 2022

ThrawnCA Feb 7, 2022

duttonw Feb 7, 2022 •

edited

Loading

QOL-8612 make non-current S3 objects private #75

QOL-8612 make non-current S3 objects private #75

Conversation

ThrawnCA commented Feb 4, 2022 • edited Loading

duttonw left a comment

Choose a reason for hiding this comment

duttonw Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duttonw Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

ThrawnCA commented Feb 4, 2022 •

edited

Loading

duttonw Feb 4, 2022 •

edited

Loading

duttonw Feb 7, 2022 •

edited

Loading