Improve consul_pillar configuration parsing #49903
Conversation
ghost
self-requested a review
salt/pillar/consul_pillar.py
Outdated
| @@ -267,7 +267,7 @@ def fetch_tree(client, path, expand_keys): | |||
| are folders. Take the remaining data and send it to be formatted | |||
| in such a way as to be used as pillar data. | |||
| ''' | |||
| _, items = consul_fetch(client, path) | |||
| _, items = consul_fetch(client, path + '/') | |||
There was a problem hiding this comment.
The function isn't hidden. So I'd maybe rather check if path has training slash.
salt/pillar/consul_pillar.py
Outdated
| @@ -267,7 +267,8 @@ def fetch_tree(client, path, expand_keys): | |||
| are folders. Take the remaining data and send it to be formatted | |||
| in such a way as to be used as pillar data. | |||
| ''' | |||
| _, items = consul_fetch(client, path) | |||
| absolute_path = path.rstrip('/') + '/' | |||
| _, items = consul_fetch(client, absolute_path) | |||
There was a problem hiding this comment.
Nooo, inside the consul_fetch. It should look on its own if the path is right or not. If you reuse it elsewhere, you don't know if trailing slash is needed or not, so you will do it every time you call it, and thus duplicate the code.
Please move that rstrip+/ inside the function. 😉
salt/pillar/consul_pillar.py
Outdated
| absolute_path = path | ||
| else: | ||
| absolute_path = path.rstrip('/') + '/' | ||
| return client.kv.get(absolute_path, recurse=True) |
There was a problem hiding this comment.
Almost. 😉 If you are not sure path may be None, it might still crash. So:
return client.kv.get(os.path.join((path or '').rstrip(os.path.sep), ''),
recurse=True)Otherwise just .rstrip it.
There was a problem hiding this comment.
Ah, right for back-slash. Still can be much less coding:
return client.kv.get((path or '').rstrip('/') + '/', recurse=True)
salt/pillar/consul_pillar.py
Outdated
| absolute_path = path | ||
| else: | ||
| absolute_path = path.rstrip('/') + '/' | ||
| return client.kv.get(absolute_path, recurse=True) |
There was a problem hiding this comment.
Ah, right for back-slash. Still can be much less coding:
return client.kv.get((path or '').rstrip('/') + '/', recurse=True)
|
@isbm Re-review requested. |
[master] Porting #49903 to master
What does this PR do?
Ensure consistency with how the pillar paths are parsed from the consul_pillar configuration.
What issues does this PR fix or reference?
Because of how the regex was, it was possible to get the wrong value for
rootifpillar_rootwas first in the list.A user could be using a trailing slash or not in the config parameters and consul_pillar would perform differently. The documentation does not use the trailing slash, but the test cases do. This especially matters in the API call to Consul because if that call does not have a trailing slash, Consul returns all data for keys that the given key is a substring of.
Keys in Consul that contained special characters would get processed as regex ie
data.yamlTests written?
I moved
pillar_rootto the beginning of the config intest_pillar_nestto cover case #1.test_pillar_datashould cover case #2. I also removed an unnecessary assertion from thetest_pillar_nesttest.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.