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

Mine minion acl master #55760

Merged
merged 26 commits into from Jan 8, 2020
Merged

Mine minion acl master #55760

merged 26 commits into from Jan 8, 2020

Conversation

github-abcde
Copy link
Contributor

@github-abcde github-abcde commented Dec 30, 2019

Master port of #54100

What does this PR do?

It adds a new format for defining mine_functions that is equal to the format used for module.run.
It adds deprecation warnings for the old style format until version Aluminium.
It adds minion-side ACL when sending items to the mine, or updating the mine, which uses allow_tgt and allow_tgt_type to determine the list of minions that is allowed to retrieve this function from the salt mine. Minions that do not have access get an empty result, just as if the requested function was not present.
Also a slight refactor has been performed, cleaning up code in salt.modules.mine significantly.

What issues does this PR fix or reference?

I have mentioned the minion-side ACL as a possible solution for #45882. Whether or not this will resolve that specific issue is up for debate though :)
#6437

Previous Behavior

All mine functions (and their data) are retrievable by all minions.

New Behavior

Mine functions (and their data) can be made available to targeted minions only.

Tests written?

Yes, plenty. There were none for the salt mine in salt.modules.mine to begin with.

Commits signed with GPG?

Yes

github-abcde added 18 commits Dec 30, 2019
…s.mine) and replaced it by the _mine_get in salt.daemons.masterapi.
…y request a mine function that they do not have access to.
Using kwargs is misleading as only the keyword func_args would be extracted from kwargs.
Updated function docstring.
Altered call_function to accept *args and **kwargs instead of just kwargs with a magic keyword that contained the args.
Removed unused continue. Removed unused variables na_type, kw_type. Renamed variables for more readability.
…ests to use self.assert* instead of test.fail.
@github-abcde github-abcde requested a review from as a code owner Dec 30, 2019
@ghost ghost requested a review from cmcmarrow Dec 30, 2019
dwoz
dwoz approved these changes Jan 2, 2020
@dwoz dwoz self-requested a review Jan 2, 2020
@dwoz dwoz mentioned this pull request Jan 2, 2020
function_kwargs = {}
minion_acl = {}
if isinstance(function_definition, dict):
salt.utils.versions.warn_until(
Copy link
Member

@s0undt3ch s0undt3ch Jan 2, 2020

Choose a reason for hiding this comment

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

Salt has taken a strict approach when changes might eventually break people's states, which was the case with the new module.run syntax and also this case.

It's ok to support both old and new syntax.
It's not ok to deprecate the old syntax.

Please remove any deprecation information about the old syntax, it won't be dropped, and update(if needed) the code to support both syntaxes.

Copy link
Contributor Author

@github-abcde github-abcde Jan 3, 2020

Choose a reason for hiding this comment

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

Removed deprecation warnings in 4f4c562

.. versionchanged:: Sodium

The format to define mine_functions has been changed to allow the same format
as used for module.run. The old format (above) will still be supported until
Copy link
Member

@s0undt3ch s0undt3ch Jan 3, 2020

Choose a reason for hiding this comment

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

The old format will still be supported. Please remove the until.... bit.

Copy link
Contributor Author

@github-abcde github-abcde Jan 3, 2020

Choose a reason for hiding this comment

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

Updated in bfee1c8

Syntax update
-------------

The syntax for defining salt functions in config or pillar files has changed to
Copy link
Member

@s0undt3ch s0undt3ch Jan 3, 2020

Choose a reason for hiding this comment

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

Also needs updating

Copy link
Contributor Author

@github-abcde github-abcde Jan 3, 2020

Choose a reason for hiding this comment

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

Updated in bfee1c8

github-abcde and others added 2 commits Jan 3, 2020
to indicate the current formats for describing mine_functions will still be supported
Copy link
Member

@s0undt3ch s0undt3ch left a comment

Thank You!

@dwoz
Copy link
Contributor

@dwoz dwoz commented Jan 5, 2020

@github-abcde Can you take a look at the merge conflicts please?

@github-abcde
Copy link
Contributor Author

@github-abcde github-abcde commented Jan 5, 2020

Ok, looks like I forgot to add a few pylint-disable statements. Will do that tomorrow :)

github-abcde and others added 3 commits Jan 6, 2020
as these calls are not expected to raise any exceptions anyway.
Also rewrite some assert statements to use self.assertEqual, more in line with the testsuite.
@dwoz dwoz merged commit 5a6ed7c into saltstack:master Jan 8, 2020
49 checks passed
@github-abcde github-abcde deleted the mine-minion-acl-master branch Jan 9, 2020
@mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Jan 16, 2020

@dwoz and @github-abcde looks like the versionadded and release notes reference sodium, but it looks like this is in Neon. Should that be updated?

@OrangeDog
Copy link
Collaborator

@OrangeDog OrangeDog commented Apr 29, 2020

Turns out this was a breaking change, as the arg func was renamed name.

See the examples here, that use func as a kwarg.
https://docs.saltstack.com/en/3000/ref/states/all/salt.states.x509.html

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

5 participants