Skip to content

Fixed the skip_files_list_verify in archive.extracted state#55975

Merged
dwoz merged 1 commit intosaltstack:masterfrom
Oloremo:55443-archive-hash-trust-fix
May 18, 2020
Merged

Fixed the skip_files_list_verify in archive.extracted state#55975
dwoz merged 1 commit intosaltstack:masterfrom
Oloremo:55443-archive-hash-trust-fix

Conversation

@Oloremo
Copy link
Copy Markdown
Contributor

@Oloremo Oloremo commented Jan 25, 2020

What does this PR do?

It fixes the skip_files_list_verify logic in archive.extractedstate for the case if keep_source is set to False.

What issues does this PR fix or reference?

#55443

Previous Behavior

Before that this logic only worked if we keep the source which is not how I originally intended to implement this. See #55443

New Behavior

Now, this logic works for both cases of keep_source, yet there is a bunch of limitations that has to be taken into account.

Implementation details

I don't fancy current implementation. There is a lot of magic going on in this state, especially regarding caching.

Also, there is a huge edge case that I highlighted in the warning section of the skip_files_list_verify argument. To properly fix that we need to introduce some smart Minion cache controls, see #34369

Tests written?

Yes

Commits signed with GPG?

Yes

@Oloremo Oloremo requested a review from a team as a code owner January 25, 2020 20:52
@ghost ghost requested a review from cmcmarrow January 25, 2020 20:52
Comment thread salt/states/archive.py Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't like it, but I alternative was to change the approach for this whole caching thing in the state which sounds scary for me. Right now it's a bit magical like this archive cache extrn_files is only exists in a single line in the state and no other states or modules aware of it. So I have to mimic that logic to generate the expected path to the file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is required due to cache limitation. Right now any test with skip_files_list_verify=True and source_hash_update=True will create a hash file in minion cache and other tests will use it when they shouldn't.

The alternative was to pass a different source for each test which sounds worse.

@Oloremo
Copy link
Copy Markdown
Contributor Author

Oloremo commented Jan 26, 2020

re-run full ubuntu1604-py2-m2crypto

@Oloremo
Copy link
Copy Markdown
Contributor Author

Oloremo commented Jan 26, 2020

re-run full centos7-py2

cmcmarrow
cmcmarrow previously approved these changes Jan 31, 2020
@Oloremo
Copy link
Copy Markdown
Contributor Author

Oloremo commented Apr 30, 2020

@cmcmarrow Mind to look at it one more time? No changes were made, just rebased to be up to date with the current master.

@Oloremo
Copy link
Copy Markdown
Contributor Author

Oloremo commented May 4, 2020

re-run full fedora32-py3

@Oloremo
Copy link
Copy Markdown
Contributor Author

Oloremo commented May 14, 2020

re-run full fedora32-py3

@dwoz
Copy link
Copy Markdown
Contributor

dwoz commented May 14, 2020

re-run fedora32

@Oloremo Oloremo force-pushed the 55443-archive-hash-trust-fix branch from 6fa549e to a0b5828 Compare May 17, 2020 11:45
@Oloremo Oloremo force-pushed the 55443-archive-hash-trust-fix branch from a0b5828 to 04c51e9 Compare May 17, 2020 13:58
@dwoz dwoz merged commit eb89407 into saltstack:master May 18, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants