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

salt.loader: add error logging when whitelist lookup fails #47710

Merged
merged 4 commits into from May 18, 2018

Conversation

Projects
None yet
4 participants
@terminalmage
Member

terminalmage commented May 17, 2018

Rather than just raising a bare KeyError with no additional information, this raises a proper KeyError with the key that failed to be looked up. It also logs the key that failed to load, as well as the whitelist, to aid in troubleshooting.

Additionally, this resolves failed lazydict lookups in the tests due to improperly-set whitelists.

terminalmage added some commits May 17, 2018

Add error logging when whitelist lookup fails
Rather than just raising a bare KeyError with no additional information,
this raises a proper KeyError with the key that failed to be looked up.
It also logs the key that failed to load, as well as the whitelist, to
aid in troubleshooting.
Fix loader whitelists in unit tests
This resolves failed lazydict lookups in the tests due to improperly-set
whitelists.

@terminalmage terminalmage requested a review from saltstack/team-core as a code owner May 17, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse May 17, 2018

@@ -1629,7 +1629,11 @@ def _load(self, key):
return True
# if the modulename isn't in the whitelist, don't bother
if self.whitelist and mod_name not in self.whitelist:
raise KeyError
log.error(
'Failed to load key %s because its module (%s) is not in '

This comment has been minimized.

@isbm

isbm May 18, 2018

Contributor

If someone reading the log won't start panicking about SSH pr SSL keys... Maybe we could rephrase this to something else?

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

Sure, that is a good point.

This comment has been minimized.

@terminalmage
@rallytime

This comment has been minimized.

Contributor

rallytime commented May 18, 2018

@terminalmage The dockermod tests are failing on this change. Can you take a look?

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-cent7-py3/5009/

terminalmage added some commits May 18, 2018

Change key -> function to make log message more clear
Leaving it as "key" would potentially confuse people into thinking there
was a problem with an SSH/SSL/TLS/etc. key.
@terminalmage

This comment has been minimized.

Member

terminalmage commented May 18, 2018

@rallytime Should be fixed now. I thought I ran all the dockermod unit tests after editing the whitelist, but it looks like I only ran a single test.

@rallytime rallytime merged commit 22807ac into saltstack:2018.3 May 18, 2018

4 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5029 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19086 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22963 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9999 — FAILURE
Details
default Build started sha1 is merged.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25219 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17316 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21946 — SUCCESS
Details

@terminalmage terminalmage deleted the terminalmage:fix-loader-whitelist branch May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment