Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[master] S3 Pillar Pagination Fixes #55694

Merged
merged 5 commits into from
Jan 8, 2020

Conversation

garethgreenaway
Copy link
Contributor

What does this PR do?

Fixing S3 pillar when the keys in a bucket exceed 1000, ensure we are handling pagination properly

What issues does this PR fix or reference?

#55662

Tests written?

[NOTICE] Bug fixes or features added to Salt require tests.
Please review the test documentation for details on how to implement tests into Salt's test suite.

Not Yet.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@garethgreenaway garethgreenaway requested a review from a team as a code owner December 18, 2019 21:07
@ghost ghost requested a review from Akm0d December 18, 2019 21:07
@garethgreenaway garethgreenaway force-pushed the 55662_s3_pillar_fixes branch 2 times, most recently from 9c18b2f to 08d2055 Compare December 18, 2019 21:21
@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #55694 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #55694      +/-   ##
==========================================
- Coverage    18.8%   18.79%   -0.01%     
==========================================
  Files         818      817       -1     
  Lines      175220   175144      -76     
  Branches    37597    37580      -17     
==========================================
- Hits        32937    32905      -32     
+ Misses     139601   139560      -41     
+ Partials     2682     2679       -3
Flag Coverage Δ
#archlts 18.07% <ø> (-0.01%) ⬇️
#centos7 23.69% <ø> (-0.02%) ⬇️
#proxy 23.72% <ø> (-0.02%) ⬇️
#py2 18.59% <ø> (-0.01%) ⬇️
#py3 18.42% <ø> (-0.01%) ⬇️
#runtests 18.79% <ø> (-0.01%) ⬇️
#ubuntu1604 23.67% <ø> (-0.02%) ⬇️
#zeromq 18.79% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
salt/serializers/msgpack.py 25.38% <0%> (-1.89%) ⬇️
salt/payload.py 28.89% <0%> (-1.5%) ⬇️
salt/log/handlers/fluent_mod.py 21.54% <0%> (-0.57%) ⬇️
salt/modules/winrepo.py 38.71% <0%> (-0.27%) ⬇️
salt/states/pkg.py 6.21% <0%> (-0.01%) ⬇️
salt/states/file.py 5.09% <0%> (ø) ⬆️
salt/returners/local_cache.py 20.13% <0%> (ø) ⬆️
salt/utils/cloud.py 9.32% <0%> (ø) ⬆️
salt/state.py 23.17% <0%> (ø) ⬆️
salt/modules/state.py 19.98% <0%> (ø) ⬆️
... and 10 more

@@ -312,6 +324,14 @@ def __get_pillar_environments(files):
if s3_meta:
bucket_files[bucket] = __get_pillar_files_from_s3_meta(s3_meta)

# Check if we have a NextContinuationToken and loop until we don't
while True:
continuation_token = __get_continuation_token(s3_meta)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be rewritten as an iterator?

for continuation_token in __get_continuation_tokens(s3_meta):
    ...

I think the function would just be:

for item in s3_meta:
    if item.get('NextContinuationToken'):
        yield item('NextContinuationToken')

Or something to that effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that approach will work. There is only ever one NextContinuationToken in s3_meta, but it could be different when s3_meta is updated after subsequent calls to __get_s3_meta. Once it's not longer in s3_meta then it should exist the loop since there are no more results to pull down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooooh. I see!

What about something like this?


    # helper s3 query function
    def __get_s3_meta():
        params = {'prefix': prefix, 'list-type': 2}
        while True:
            result = __utils__['s3.query'](
                key=creds.key,
                keyid=creds.keyid,
                kms_keyid=creds.kms_keyid,
                bucket=creds.bucket,
                service_url=creds.service_url,
                verify_ssl=creds.verify_ssl,
                location=creds.location,
                return_bin=False,
                params={'prefix': prefix},
                path_style=creds.path_style,
                https_enable=creds.https_enable)
            yield result
            continuation_token = next((item.get('NextContinuationToken')
                                       for item in s3_meta
                                       if item.get('NextContinuationToken')),
                                      None)
            if continuation_token is None:
                break
            else:
                params['continuation-token'] = continuation_token

Then it could simply be

for s3_meta in __get_s3_meta():
    ...

Would that work?

@dwoz dwoz merged commit 3523004 into saltstack:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants