Fix broken sdb.get_or_set_hash for Hashicorp Vault

[driskell]

What does this PR do?

When using Hashicorp Vault and SDB it's not possible to use sdb.get_or_set_hash because the get returns an Exception rather than None, which get_or_set_hash expects. Additionally, sdb.get returns a CommandExecutionError when it would be expected to return None.

Example response from sdb.get_or_set_hash:

Exception occurred in runner sdb.get_or_set_hash: Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/salt/client/mixins.py", line 377, in low
    data['return'] = func(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/salt/runners/sdb.py", line 83, in get_or_set_hash
    return salt.utils.sdb.sdb_get_or_set_hash(uri, __opts__, length, chars, __utils__)
  File "/usr/lib/python2.7/site-packages/salt/utils/sdb.py", line 127, in sdb_get_or_set_hash
    ret = sdb_get(uri, opts, utils=utils)
  File "/usr/lib/python2.7/site-packages/salt/utils/sdb.py", line 46, in sdb_get
    return loaded_db[fun](query, profile=profile)
  File "/usr/lib/python2.7/site-packages/salt/sdb/vault.py", line 114, in get
    raise salt.exceptions.CommandExecutionError(e)
CommandExecutionError: 404 Client Error: Not Found

This PR fixes this by fixing sdb.get

This PR does make the assumption that the correct behaviour for get when a value does not exist is to return None rather than a CommandExecutionError. I assume this because CommandExecutionError is an error-case - whereas the non-existence of a SDB URI is a perfectly valid state, and should be handled as a successful result, albeit with no found value. Currently, as it stands, because of this execution error it isn't possible to handle inside a Jinja template the non-existence of an SDB value in Vault.

What issues does this PR fix or reference?

N/A

Previous Behavior

CommandExecutionError is thrown for SDB get and SDB get_or_set_hash if the value does not exist in Vault. This renders get_or_set_hash useless.

New Behavior

SDB get returns None if Vault does not have a value. SDB get_or_set_hash now functions as documented, setting a hash where one does not exist.

Tests written?

Yes

Was unable to verify tests run locally due to abort problem (see comments).

Commits signed with GPG?

No

[twangboy]

@driskell Could we get a test for this?

[driskell]

@twangboy I can try, I’m not entirely sure how. Is there a guide or a folder to reference ? Will save me some time trying to work out how to do it.

I did skim the docs but there’s nothing about writing or running tests. Seems to imply the tests already exist? I’ll check the linting bit too since I guess I should get that working if writing more.
https://docs.saltstack.com/en/latest/topics/development/contributing.html

[driskell]

Found something, I’ll read up - hopefully a straight forward test! https://docs.saltstack.com/en/latest/topics/development/tests/index.html

[driskell]

Ok so I will use as a reference the only SDB test: https://github.com/saltstack/salt/blob/develop/tests/unit/sdb/test_yaml.py

I’ll try cover all functions and code paths in the vault module in a new one.

[driskell]

Odd - the PR had all the needed bits but when I filled it in it had no template - maybe GitHub doesn’t show that when you do PR from editing a file :/ Sorry!

[cmcmarrow]

@driskell

Setup

'salt/requirements' pip install -r tests.txt

Run test

'salt/tests' python runtests.py --run-destructive -n unit.modules.test_win_iis

Help

https://docs.python.org/3/library/unittest.mock.html

Example

https://github.com/saltstack/salt/pull/54069/files

Note

We only ask you to write test for your changes

[cmcmarrow]

ping me if you need any help

[dwoz]

@driskell Can you re-base this PR against master please?

[driskell]

This is still an issue. @dwoz @cmcmarrow @twangboy @gtmanfred Is this still waiting on my time?
I just upgraded SaltStack and it looks like this is still broken and get_or_set_hash is completely unusable with Vault.

[driskell]

Maybe I should've raised a bug ticket alongside?

[driskell]

I just can't get tests to run on my MacBook.

After battling for 30minutes trying to find missing packages not included by the requirements, finding the test documentation refers to python27.txt that doesn't exist. Struggling with import error on pytestsalt which needs installing as pytest-salt, I can now get it to run but it just aborts.

zsh: abort      python tests/runtests.py --run-destructive -n unit.sdb.test_vault

I'll push the test though. Best I can do.

[driskell]

Rebased to master and changed target merge to master.

[driskell]

Found out that the tests were broken, but passing only because of the implicit MagicMock functionality. I've changed them to properly check the full return.

Also fixed another issue found today, in that if the path exists within Vault, but the key requested does not exist at that path, it fails with CommandExecutionError again due to KeyError. This now properly checks again that the value exists in the dictionary and returns None if not, so that get_or_set_hash works properly again for multi-value paths inside Vault. Added a test too.

Still unable to get tests to run so I am working on them blind at the moment. Hopefully the CI passes

[driskell]

@cmcmarrow What's left to do here to get things ready to merge? I saw the label for Merge Ready was removed. Maybe I'm not understanding the process but if there's anything else left to do I'm keen to merge this as we keep having to patch it manually every time we update.

Thanks!

[DmitryKuzmenko]

@driskell this looks good. Let's wait for test re-run.

[driskell]

I can see 3 test failures. I'll try to get them evaluated.
I've never been able to get the test suite to run locally on my mac which is difficult but hopefully there's enough info in the Jenkins to push a fix!

[driskell]

So far so good!