Refactor vault

[toydarian]

SUMMARY

This is a refactoring-only PR for vault.

I plan to add some quality-of-life improvements and features to vault, but before doing so, I wanted to split up the code from one 1.2k-lines file into several files and make it a little more modern.

Those are the major changes in this PR:

• all classes and most functions have been grouped by use
• old '%'-style format-strings have been updated to "fancy" formatted string-literals
• fixed formatting and typos in doc-strings, comments and messages

ISSUE TYPE

• Feature Pull Request

or more like

• Refactoring Pull Request

COMPONENT NAME

vault

ADDITIONAL INFORMATION

Most classes and functions are not available on the package ansible.parsing.vault anymore, but some that are used in other components are imported in __init__.py to avoid making this PR even larger than it already is.

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

test/units/parsing/vault/test_vault.py:659:65: E127: continuation line over-indented for visual indent
test/units/parsing/vault/test_vault.py:660:65: E127: continuation line over-indented for visual indent
test/units/parsing/vault/test_vault.py:661:65: E127: continuation line over-indented for visual indent
test/units/parsing/yaml/test_dumper.py:62:74: E251: unexpected spaces around keyword / parameter equals

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/parsing/vault/editor.py:71:20: disallowed-name: Disallowed name "_"
lib/ansible/parsing/vault/editor.py:77:24: disallowed-name: Disallowed name "_"
test/sanity/ignore.txt:115:1: ansible-test: Ignoring 'disallowed-name' on 'lib/ansible/parsing/vault/__init__.py' is unnecessary

click here for bot help

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/parsing/yaml/test_dumper.py:62:74: E251: unexpected spaces around keyword / parameter equals

click here for bot help

[s-hertel]

I've done an initial review and it looks like all changes are cosmetic and generally looks good to me 👍 This will need a thorough review every time it needs to be rebased, so if you can remove unnecessary changes that aren't an explicit goal of this PR (for example, modified function signatures/variable names/whitespace), that would make reviewing code parity easier.

It looks like a bunch of class methods have been turned into staticmethods, but none of them are called without a class instance. Until that's needed for some reason, those changes can be removed.

Since lib/ansible/parsing/vault/__init__.py is internal API, the backwards incompatible changes to the classes/methods available may not matter, but I'd like to mention it was easy to find some things in the wild that would break from that. Some examples: https://engineering.adjust.com/post/secrets_management_with_ansible_vault/#vault-python-api, https://github.com/nuagenetworks/nuage-metroae/blob/master/encrypt_credentials.py#L8, https://github.com/arenadata/adcm/blob/develop/python/cm/adcm_config/ansible.py#L14

[toydarian]

I've done an initial review and it looks like all changes are cosmetic and generally looks good to me +1 This will need a thorough review every time it needs to be rebased, so if you can remove unnecessary changes that aren't an explicit goal of this PR (for example, modified function signatures/variable names/whitespace), that would make reviewing code parity easier.

The few changes to variable names, signatures etc. are meant to make the code easier to understand. I already tried to keep those to a minimum.

It looks like a bunch of class methods have been turned into staticmethods, but none of them are called without a class instance. Until that's needed for some reason, those changes can be removed.

I changed a few functions that didn't use self into static methods (@staticmethod), but I don't think I changed any @classmethod to @staticmethod.

Since lib/ansible/parsing/vault/__init__.py is internal API, the backwards incompatible changes to the classes/methods available may not matter, but I'd like to mention it was easy to find some things in the wild that would break from that. Some examples: https://engineering.adjust.com/post/secrets_management_with_ansible_vault/#vault-python-api, https://github.com/nuagenetworks/nuage-metroae/blob/master/encrypt_credentials.py#L8, https://github.com/arenadata/adcm/blob/develop/python/cm/adcm_config/ansible.py#L14

Do you suggest, I import every class and metod in __init__.py to preserve backwards-compatibility?

[s-hertel]

Thanks for the quick update.

The few changes to variable names, signatures etc. are meant to make the code easier to understand. I already tried to keep those to a minimum.

I didn't want to make a comprehensive list of these because they're all very minor, but there are quite a few. To keep this general:

• The class methods (not to be confused with classmethod) that were decorated with staticmethod fall into this category. They're still being called via class instances, so the change is unnecessary and I don't think it's an improvement to readability. I'd also question whether some of them would be better as standalone functions, if they needed to be called without a class instance.
• vaulttext was inconsistently renamed to vault_text in variable and function names, but it wasn't changed everywhere and none of the occurrences of ciphertext or plaintext were modified. Variables named consistently makes it easier to find/remember them, so I don't love this change even though I can see the argument that the double t is less readable without the underscore separating them.
• Some otherwise unchanged lines add or remove line breaks.

Ideally to me, most of these would be removed so it's easier to see that yes, this function was just moved from file1 to file2 with no actual changes other than format strings.

Do you suggest, I import every class and metod in init.py to preserve backwards-compatibility?

I'm not sure, maybe. You could wait for others to weigh in on this.

[toydarian]

The class methods (not to be confused with classmethod) that were decorated with staticmethod fall into this category. They're still being called via class instances, so the change is unnecessary and I don't think it's an improvement to readability. I'd also question whether some of them would be better as standalone functions, if they needed to be called without a class instance.

I have a strong opinion about this. Methods, that don't use self should be either static or standalone. When I call a method with object.method() I expect it to depend on the object.
I changed the methods that I made static earlier to standalone functions and all usages of those. They were only used in the vault module or in tests, so I think that makes the most sense.

vaulttext was inconsistently renamed to vault_text in variable and function names, but it wasn't changed everywhere and none of the occurrences of ciphertext or plaintext were modified. Variables named consistently makes it easier to find/remember them, so I don't love this change even though I can see the argument that the double t is less readable without the underscore separating them.

I'm in slight favor of separating words for readability, but I guess there are enough changes in this PR, so I reverted all vault_text to vaulttext.

Some otherwise unchanged lines add or remove line breaks.

Those should mostly be docstrings and comments. The starting quotes were inconsistent, sometimes on a separate line, and sometimes not. That itched me a little and I find it more optically pleasing if they have their own lines. As I like to have those things consistent, I formatted them all the same way.
I also dislike the first line of a class or function being empty and if there is an empty line after each line of code.

Do you suggest, I import every class and method in __init__.py to preserve backwards-compatibility?

I'm not sure, maybe. You could wait for others to weigh in on this.

What is the best way forward in this case? Should I post to a mailing-list?

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/parsing/vault/ciphers.py:53:1: E303: too many blank lines (3)
lib/ansible/parsing/vault/ciphers.py:79:1: E302: expected 2 blank lines, found 1
lib/ansible/parsing/vault/editor.py:257:1: E302: expected 2 blank lines, found 1
lib/ansible/parsing/vault/lib.py:329:1: W391: blank line at end of file

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/parsing/vault/lib.py:329:0: trailing-newlines: Trailing newlines

click here for bot help

[s-hertel]

Thanks for reducing some of the extraneous changes. Still LGTM in general, but there are a few more minor changes that could be tidied up.

[s-hertel]

Suggested change

	b_data = to_bytes(to_text(data, encoding='ascii', errors='strict', nonstring='strict'), encoding='ascii',
	errors='strict')
	b_data = to_bytes(to_text(data, encoding='ascii', errors='strict', nonstring='strict'), encoding='ascii', errors='strict')

[toydarian]

this would cause tests to fail, as the line is too long

[s-hertel]

It shouldn't. My suggestion was the original line, which is < 160 characters.

[toydarian]

you're right, my editor is set to 120 characters

[s-hertel]

The git blame with -C looks great now. I don't think there's a way to see from the UI, so here's the current state of things squashed to a single commit: https://gist.github.com/s-hertel/6f7038adf383a7746269da94943dcf0e.

[s-hertel]

Suggested change

	@staticmethod
	def confirm(b_vault_pass_1, b_vault_pass_2):
	def confirm(self, b_vault_pass_1, b_vault_pass_2):

[s-hertel]

Suggested change

	f'Re-keyed file "{to_text(filename)}" (decrypted with vault id "{to_text(vault_id_used)}") was encrypted '
	f'Rekeyed file "{to_text(filename)}" (decrypted with vault id "{to_text(vault_id_used)}") was encrypted '

[s-hertel]

I don't think rewrites/refactoring usually warrant a copyright addition (same for the other files). Adding your name would be more applicable on a PR containing feature work.

[s-hertel]

It shouldn't. My suggestion was the original line, which is < 160 characters.

[toydarian]

Hello @s-hertel, do you have a suggestion on how I can get attention to this PR? I posted on the chat a few weeks back, but to no success.

[nitzmahone]

(needs review, major changes to vault coming)

[toydarian]

(needs review, major changes to vault coming)

@nitzmahone is this going to get attention? I had given up on it, but if it is going to be picked up, I'm going to rebase and resolve conflicts

[bcoca]

@toydarian no, the point is that this PR might need to be redone or even made irrelevant with upcoming changes. We have kept it open waiting for the relevant core members having some time to do a deep review and comparison to make a final decision.