Skip to content
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

Refactor publish func in master/masterapi to use check_authentication #43703

Conversation

rallytime
Copy link
Contributor

@rallytime rallytime commented Sep 22, 2017

What does this PR do?

Refactors the publish function in master.py and masterapi.py to use salt.auth.check_authentication and reworks the publish function in each file to be more concise.

In a previous PR, the runner and wheel functions in master.py and masterapi.py were refactored to reduce common code via the salt.auth.check_authentication method.

The publish function also can utilize the check_authentication function in master.py and masterapi.py.

Consolidation of this code will help us be able to differentiate between authorization and authentication errors in the future.

What issues does this PR fix or reference?

Builds on the work done in PR #43203.

This is necessary to fix (eventually) #6420, #34750, and #35160.

Tests written?

Yes

Please review Salt's Contributing Guide for best practices.

@ghost
Copy link

ghost commented Sep 22, 2017

@rallytime, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DmitryKuzmenko, @terminalmage and @thatch45 to be potential reviewers.

@rallytime
Copy link
Contributor Author

Ah, this isn't quite right. I need to look at this a little bit more.

@rallytime rallytime changed the title Refactor publish func in master/masterapi to use check_authentication [WIP] Refactor publish func in master/masterapi to use check_authentication Sep 22, 2017
@rallytime rallytime added the pending-changes The pull request needs additional changes before it can be merged label Sep 22, 2017
@rallytime rallytime force-pushed the publish_refactor_use_check_authentication branch from f298d54 to 1b33b03 Compare September 26, 2017 20:25
@rallytime rallytime changed the title [WIP] Refactor publish func in master/masterapi to use check_authentication Refactor publish func in master/masterapi to use check_authentication Sep 26, 2017
@rallytime rallytime removed the pending-changes The pull request needs additional changes before it can be merged label Sep 26, 2017
@rallytime
Copy link
Contributor Author

Ok, this should be ready to review now, pending tests.

Copy link
Member

@thatch45 thatch45 left a comment

Choose a reason for hiding this comment

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

This looks good!

In a previous PR, the runner and wheel functions in master.py and masterapi.py
were refactored to reduce common code via the salt.auth.check_authentication
method.

The publish function also can utilize the check_authentication function in
master.py and masterapi.py.

Consolidation of this code will help us be able to differentiate between
authorization and authentication errors in the future.
@rallytime rallytime force-pushed the publish_refactor_use_check_authentication branch from f78e8b7 to 74acaad Compare October 17, 2017 16:40
@rallytime
Copy link
Contributor Author

re-run py3

Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

Choose a reason for hiding this comment

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

I like this approach!

@rallytime
Copy link
Contributor Author

Test failures are not related.

@cachedout
Copy link
Contributor

re-run py3

@rallytime rallytime merged commit af1b460 into saltstack:develop Oct 30, 2017
@rallytime rallytime deleted the publish_refactor_use_check_authentication branch October 30, 2017 21:32
@keesbos
Copy link
Contributor

keesbos commented Nov 8, 2017

See salt/scripts.py:

salt/scripts.py:

86 def salt_master():
87     '''
88     Start the salt master.
89     '''
90      import salt.cli.daemons
91      master = salt.cli.daemons.Master()
92      master.start()

This is the normal flow (salt.minion effectively imported at line 92):

  File "/usr/local/src/tests/develop/bin/salt-master", line 22, in <module>
    salt_master()
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/scripts.py", line 92, in salt_master
    master.start()
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/cli/daemons.py", line 205, in start
    super(Master, self).start()
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/parsers.py", line 1044, in start
    self.prepare()
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/cli/daemons.py", line 185, in prepare
    import salt.master
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/master.py", line 52, in <module>
    import salt.pillar
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/pillar/__init__.py", line 21, in <module>
    import salt.minion
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/minion.py", line 115, in <module>

This is the wrong flow (salt.minion effectively imported at line 90):

  File "/usr/local/src/tests/develop/bin/salt-master", line 22, in <module>
    salt_master()
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/scripts.py", line 90, in salt_master
    import salt.cli.daemons
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/cli/daemons.py", line 48, in <module>
    import salt.utils.parsers
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/parsers.py", line 28, in <module>
    import salt.config as config
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/config/__init__.py", line 100, in <module>
    _DFLT_IPC_WBUFFER = _gather_buffer_space() * .5
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/config/__init__.py", line 90, in _gather_buffer_space
    import salt.grains.core
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/grains/core.py", line 46, in <module>
    import salt.utils.dns
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/dns.py", line 31, in <module>
    import salt.modules.cmdmod
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/modules/cmdmod.py", line 34, in <module>
    import salt.utils.templates
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/templates.py", line 41, in <module>
    import salt.utils.jinja
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/jinja.py", line 30, in <module>
    import salt.fileclient
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/fileclient.py", line 21, in <module>
    import salt.client
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/client/__init__.py", line 39, in <module>
    import salt.utils.minions
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/minions.py", line 23, in <module>
    import salt.auth.ldap
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/auth/__init__.py", line 32, in <module>
    import salt.utils.master
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/utils/master.py", line 21, in <module>
    import salt.pillar
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/pillar/__init__.py", line 21, in <module>
    import salt.minion
  File "/usr/local/src/tests/develop/local/lib/python2.7/site-packages/salt/minion.py", line 115, in <module>
    from salt.config import DEFAULT_MINION_OPTS
ImportError: cannot import name DEFAULT_MINION_OPTS

Delayed import of salt.utils.master to just before auth_list = salt.utils.master.get_values_of_matching_keys (only place where salt.utils.master is used) will fix this. But I'm not sure that that's the right way to go.

@rallytime
Copy link
Contributor Author

@keesbos Thanks for adding that information. I think you're on the right track. I will dig into this more right now and submit a fix.

rallytime pushed a commit to rallytime/salt that referenced this pull request Nov 8, 2017
Fixes saltstack#44435

PR saltstack#43703 inadvertently moved a "late import" statement up to the
top of the auth file, which caused the problem reported in the PR
itself and issue saltstack#44435.

Moving the salt.utils.master import lower down in the file fixes
the issue.
@rallytime
Copy link
Contributor Author

@keesbos You're absolutely right that we need to move that import down. There was already a "late import" declaration in the original code, but I missed it during the refactor. I've moved this back down in PR #44445. Thanks again for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants