Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

SEP: Expose utility functions to states, but not to CLI or template context #70

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

SEP: Expose utility functions to states, but not to CLI or template context #70

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
910e742
SEP: Expose utility functions to states, but not to CLI or template c…
terminalmage Jun 7, 2023
9a9f3eb
Add PR and issue links
terminalmage Jun 7, 2023
591d0d5
Clarification
terminalmage Jun 7, 2023
05cc38f
Fix anchors
terminalmage Jun 7, 2023
46e1653
Add note about how docs may be affected
terminalmage Jun 7, 2023
372497d
spelling
terminalmage Jun 7, 2023
89c7078
Update SEP to remove obsolete concerns
Jul 31, 2023
f0fc71b
Add link to patch
Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions expose-utility-functions-to-states.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
- Feature Name: Expose utility functions to states, but not to the CLI or template context
- Start Date: 2023-06-06
- SEP Status: Draft
- SEP PR: https://github.com/saltstack/salt-enhancement-proposals/pull/70
- Salt Issue: https://github.com/saltstack/salt/pull/64287#pullrequestreview-1451646261

# Summary
[summary]: #summary

Provide a means for the loader to access utility functions, but keep them from
being added to the CLI or template context.

# Motivation
[motivation]: #motivation

It is a policy of the Salt core team not to add any new utility functions to
the `__salt__` loader (thus exposing them to the Salt CLI and the template
context). The core team's proposed alternative, however, is to do an OS check
in the state module, then a late import of the salt execution module, and
finally to invoke the utility function directly. While this option technically
works, it is unequivocally a bad idea, with several complications:

1. The state modules were never supposed to include platform-specific code. I
was even [advised to
remove](https://github.com/saltstack/salt/pull/3019#issuecomment-11680999)
grains checks from a state module some 10 years ago.
Copy link

Choose a reason for hiding this comment

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

Let me start by pointing out that 10 years is a long time ago, things have changed. The main idea, never supposed to include platform-specific code is still valid, when possible.

Copy link

Choose a reason for hiding this comment

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

When not possible, there are still ways to achieve it without code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then name them?

Copy link

Choose a reason for hiding this comment

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

It was named

#70 (comment)


2. Doing an OS check in a state module duplicates work already done in the
execution module's `__virtual__` function. If the conditions in that
`__virtual__` function are changed at a later date, all ad-hoc duplications
of this logic in state modules must be located and updated. It is only a
matter of when, not if, this will cause bugs to arise.
Comment on lines +28 to +32
Copy link

Choose a reason for hiding this comment

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

There are other ways to maintain consistency on __virtual__ checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then name them?

Copy link

Choose a reason for hiding this comment

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

It was named

#70 (comment)


3. For execution modules which are virtual (e.g. `service`, `pkg`, etc.),
additional care needs to be taken to craft these ad-hoc duplications, in
order to catch minions which use the
[providers](https://docs.saltproject.io/en/latest/ref/configuration/minion.html#providers)
config option to manually assign a module as the virtual provider. If this
very obscure and specific case isn't manually accounted for in one's ad-hoc
platform checks,
[providers](https://docs.saltproject.io/en/latest/ref/configuration/minion.html#providers)
functionality is broken.

In the past, complications `2` and `3` above were unnecessary, as we would
simply add a function to the platform-specific execution module, and then use
loader membership (e.g. `if "pkg.somefunc" in __salt__`) to determine whether
or not to run them. The `pkg` state module is littered with such loader checks
to handle the various idiosyncrasies of different package managers. This method
has the benefit of automatically supporting minions which use
[providers](https://docs.saltproject.io/en/latest/ref/configuration/minion.html#providers),
and keeps duplicated platform checks out of state modules, at the expense of
exposing dubiously-useful functions to the CLI and template context.

# Design
[design]: #detailed-design

## Option 1: Augmentation of LazyLoader

An optional module-level dunder could be added to remote-execution modules, to
suppress availability of utility functions. For example:

```python
__utility_funcs__ = ('foo', 'bar')
```

Any function name found in `__utility_funcs__` would be ignored by the loader
by default, but they could be conditionally loaded by passing an argument when
instantiating the `LazyLoader` class. This would allow `salt.loader.states()`
to instantiate its own copy of the `minion_mods` loader that contains the
utility functions, while the CLI and template context get a copy with them
absent, preventing them from being exposed to end users.

## Option 2: Move the utility functions to a new loader type

This solution is far less elegant and more intrusive, but I'm including it as
an alternative. It was my first idea for solving this problem, but I believe
that [Option 1](#option-1-augmentation-of-lazyloader) is the superior solution.

Utility functions would be moved to a different module, which would be in a
directory processed by a loader instance. That loader would be added to the
dunders which get packed onto the state modules. Meanwhile, the execution
modules can import these utility functions directly.

The obvious first choice would be to use `__utils__` for this purpose, but I
noticed there is already [a SEP to remove the `__utils__`
loader](https://github.com/saltstack/salt-enhancement-proposals/pull/66). So,
a different directory then? Assuming this directory is called `foo`, then you
would for example have a `salt/modules/aptpkg.py` and a `salt/foo/aptpkg.py`.

Splitting the code into separate modules for remote-execution and utility
functions would appear at first to result in a duplicated `__virtual__`
function, but objects beginning and ending in double-underscores are not
name-mangled and thus can be imported. So, the `__virtual__` can be defined in
the utility module and imported into the remote-execution module.
Comment on lines +97 to +98
Copy link

Choose a reason for hiding this comment

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

So, the __virtual__ can be defined in the utility module and imported into the remote-execution module

Precisely, this is the way to avoid duplication.
Shouldn't use any salt dunders, or, if the function needs access to them, pass them in directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it doesn't avoid duplication when it comes to invoking imported code in the state module. As I've mentioned both in the original Salt issue and in this SEP, you end up needing to duplicate the logic from the __virtual__ everywhere you want to use this imported code.

Copy link

Choose a reason for hiding this comment

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

You import the util module that implements the virtual logic, and then, on each of the module needing that check, you call that function.

We're not arguing about adding import statements are we?

Copy link

@s0undt3ch s0undt3ch Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

And we're also not arguing about adding a function call in each of the virtual functions right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No and no. What is at issue here is the duplication of OS checks in state modules. Yes, it is a 10-year-old opinion. Pointing out that it's an old opinion is not a refutation of the opinion, it's just a non sequitur. There was a reason behind it, and that reason is that the loader is the right solution for running code specific to a given platform.

There are cases where simply importing a function from another module is a viable solution. Simpler state modules for which the execution module counterpart is not a virtual module are a great candidate for simply importing a function and running it. But pkg (and to a lesser extent, pkgrepo) are very different. There are more than 20 virtual pkg modules, representing almost as many different package managers. Many have functionality that is unique to them, which the pkg states have to be able to support.

Since the very early days of Salt, the way we have accounted for that is to use the loader. If a specific piece of functionality is only applicable to a single platform, Salt just checks if that function is in the loader. I gave several examples of this technique in this post, only to be told that the way Salt has been doing this since time immemorial is wrong, and should be considered "technical debt that needs to be fixed".

To be clear, the method required to "fix" this code in the pkg and pkgrepo states requires replacing loader membership checks with a reproduction of the same logic from the execution module's __virtual__ method, and also including a check in the minion opts to see if the pkg virtual has been assigned using providers. For example, that means replacing this:

if "pkg.check_db" not in __salt__:

with this:

import salt.modules.ebuildpkg

if not (
    (salt.modules.ebuildpkg.HAS_PORTAGE and __grains__["os"] == "Gentoo")
    or __opts__.get('providers', {}).get('pkg') == 'ebuildpkg'
):

That is the alternative to a loader membership check. That is what you are arguing in favor of. You have to reproduce everything that the loader is already doing for you.

Loader membership checks are the right way to handle these cases. This SEP is an attempt to find a way to balance this truth with the desire of the project not to further expose unnecessary functions to the loader.

Copy link

Choose a reason for hiding this comment

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

To be clear, you will always be able to do:

if "pkg.check_db" not in __salt__:

This was never in question, unless check_db is a function which serves no user facing usefulness, which, in that case, should be moved to a utils module.

You can still create a utility module with all the required checks to reduce the verboseness of:

import salt.modules.ebuildpkg

if not (
    (salt.modules.ebuildpkg.HAS_PORTAGE and __grains__["os"] == "Gentoo")
    or __opts__.get('providers', {}).get('pkg') == 'ebuildpkg'
):

What I'm arguing for is not adding yet another complexity layer which will have to be maintained by the core team.
My position regarding this should be expected as the submitter of #66

Your counter-argument is that "virtual" state modules do a lot of checks, and switching that to use imported utilities will make it more complex.
You are correct, it will be more complex, for virtual modules. But it will be way less complex for non virtual modules.
The virtual state modules vs non virtual state modules ratio is quite low, so, extra work there is acceptable, if the overall complexity lowers.

This is my point of view.

Copy link
Contributor Author

@terminalmage terminalmage Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To be clear, you will always be able to do:

if "pkg.check_db" not in __salt__:

This was never in question, unless check_db is a function which serves no user facing usefulness, which, in that case, should be moved to a utils module.

You can still create a utility module with all the required checks to reduce the verboseness of:

import salt.modules.ebuildpkg

if not (
    (salt.modules.ebuildpkg.HAS_PORTAGE and __grains__["os"] == "Gentoo")
    or __opts__.get('providers', {}).get('pkg') == 'ebuildpkg'
):

You'll still need to reproduce all the work the loader is already doing to detect OS and use of providers. Moving this code to a utils module doesn't do anything to fix the code duplication, it just hides it.

Your counter-argument is that "virtual" state modules do a lot of checks, and switching that to use imported utilities will make it more complex. You are correct, it will be more complex, for virtual modules. But it will be way less complex for non virtual modules. The virtual state modules vs non virtual state modules ratio is quite low, so, extra work there is acceptable, if the overall complexity lowers.

It's not "extra work". It's duplicating code, and then hoping that you remember to update that duplicated logic in the future if/when the code in the execution module drifts. Adding an attribute lookup to the loader is a less complex and more robust solution.

Arguing that there are a lot more non-virtual modules than virtual modules is another non-sequitur. They won't have the attribute, and will act the same way as they did before. The __virtual_aliases__ attribute in execution modules has been used only a few times, such as when transitioning a newer (i.e. "ng") implementation of a module to replace the original module (e.g. dockerng.py becoming dockermod.py), and allowing the old "ng" module name to continue to work for a few releases to allow people to transition.

The ratio of modules that have used __virtual_aliases__ to those that have not is even smaller than the ratio of virtual modules to non-virtual modules. That fact didn't make it a bad idea to add __virtual_aliases__, and likewise the ratio of virtual to non-virtual modules is not an argument against this proposed augmentation of the loader.

What I'm arguing for is not adding yet another complexity layer which will have to be maintained by the core team.

Am I unqualified to help maintain Salt? I was a core team member before I was employed by SaltStack, and I didn't lose my ability or inclination to contribute when I stopped being employed by SaltStack.

EDIT: Additionally, who if not the core team will have to keep in sync all the code duplication that will be added if this SEP is not implemented?

Copy link

Choose a reason for hiding this comment

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

You'll still need to reproduce all the work the loader is already doing to detect OS and use of providers. Moving this code to a utils module doesn't do anything to fix the code duplication, it just hides it.

Not quite, there's no duplication on the utils module.

It's not "extra work". It's duplicating code, and then hoping that you remember to update that duplicated logic in the future if/when the code in the execution module drifts. Adding an attribute lookup to the loader is a less complex and more robust solution.

Not if you just call a utils function in the virtual functions. You update logic in one place, it works on all places using that function.

Arguing that there are a lot more non-virtual modules than virtual modules is another non-sequitur. They won't have the attribute, and will act the same way as they did before. The virtual_aliases attribute in execution modules has been used only a few times, such as when transitioning a newer (i.e. "ng") implementation of a module to replace the original module (e.g. dockerng.py becoming dockermod.py), and allowing the old "ng" module name to continue to work for a few releases to allow people to transition.

The ratio of modules that have used virtual_aliases to those that have not is even smaller than the ratio of virtual modules to non-virtual modules. That fact didn't make it a bad idea to add virtual_aliases, and likewise the ratio of virtual to non-virtual modules is not an argument against this proposed augmentation of the loader.

__virtual_aliases__ does not add to the current maintenance burden of Salt.

Am I unqualified to help maintain Salt? I was a core team member before I was employed by SaltStack, and I didn't lose my ability or inclination to contribute when I stopped being employed by SaltStack.

Erik, seriously?
If that's what you understood, I'm sorry, but I'll clarify.

Me, as paid maintainer of Salt, have the obvious obligation to fix any issues that come in.
Any Salt contributor, former employee or not, does not have the obligation to fix issues. As always, any fixes contributed by the community are more than welcomed by the core team, and, in fact, community contributed fixes increase the bug fix count allowing the core team to address other issues.

Copy link
Contributor Author

@terminalmage terminalmage Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You'll still need to reproduce all the work the loader is already doing to detect OS and use of providers. Moving this code to a utils module doesn't do anything to fix the code duplication, it just hides it.

Not quite, there's no duplication on the utils module.

It's not "extra work". It's duplicating code, and then hoping that you remember to update that duplicated logic in the future if/when the code in the execution module drifts. Adding an attribute lookup to the loader is a less complex and more robust solution.

Not if you just call a utils function in the virtual functions. You update logic in one place, it works on all places using that function.

The issue is is not that the __virtual__ needs to do the same logic that something in a utils module would do, it's that a state module would need to duplicate the work the loader is already doing (i.e. OS checks, minion opts checks) to decide whether or not it should execute an imported function (irrespective of whether it is imported from the execution module or from a utils module).

And the example I showed was really just a best-case scenario. The functions in lowpkg for RPM serve no purpose on the CLI and would be candidates for moving to a utils module as part of the effort to "reduce technical debt". To decide whether or not to call them from the state module, you'd need to create an if statement that encompasses OS checks for RedHat and SUSE, as well as checking if lowpkg has been explicitly set to either yumpkg or zypper via providers.

Unless, of course, you have a solution for removing these OS and minion opts checks from the state module?

Arguing that there are a lot more non-virtual modules than virtual modules is another non-sequitur. They won't have the attribute, and will act the same way as they did before. The virtual_aliases attribute in execution modules has been used only a few times, such as when transitioning a newer (i.e. "ng") implementation of a module to replace the original module (e.g. dockerng.py becoming dockermod.py), and allowing the old "ng" module name to continue to work for a few releases to allow people to transition.
The ratio of modules that have used virtual_aliases to those that have not is even smaller than the ratio of virtual modules to non-virtual modules. That fact didn't make it a bad idea to add virtual_aliases, and likewise the ratio of virtual to non-virtual modules is not an argument against this proposed augmentation of the loader.

__virtual_aliases__ does not add to the current maintenance burden of Salt.

This is just moving the goalposts.

There will be an additional maintenance burden to maintain the additional OS and opts checks in the state modules and keep them in sync.

Let's assume your best-case scenario, in which the OS check exists in one place, within a utils module, where it is invoked by the execution module's __virtual__ and the state module (let's ignore for now that if #66 is implemented, the utils modules won't have access to the grains used for OS checks). You still have to check providers, in order to catch those edge cases where the virtual module is manually being assigned. That check still needs to be in the state module. It "adds to the maintenance burden of salt", so to speak.

By any measure, augmenting the loader is the less-complex solution. It doesn't require code duplication in the state modules. It automatically responds to changes in the execution modules. And it lets us continue to use loader membership to decide whether or not to run platform-specific code.

Am I unqualified to help maintain Salt? I was a core team member before I was employed by SaltStack, and I didn't lose my ability or inclination to contribute when I stopped being employed by SaltStack.

Erik, seriously? If that's what you understood, I'm sorry, but I'll clarify.

Me, as paid maintainer of Salt, have the obvious obligation to fix any issues that come in. Any Salt contributor, former employee or not, does not have the obligation to fix issues. As always, any fixes contributed by the community are more than welcomed by the core team, and, in fact, community contributed fixes increase the bug fix count allowing the core team to address other issues.

Any SEP will result in some future maintenance burden. In this case, not doing the SEP will entail its own, even uglier maintenance burden.

The excuse that the core team will have an additional maintenance burden can be used to suppress any SEP. If that's the case, then what are we even doing here?

Copy link

Choose a reason for hiding this comment

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

There will be an additional maintenance burden to maintain the additional OS and opts checks in the state modules and keep them in sync.

Let's assume your best-case scenario, in which the OS check exists in one place, within a utils module, where it is invoked by the execution module's virtual and the state module (let's ignore for now that if #66 is implemented, the utils modules won't have access to the grains used for OS checks). You still have to check providers, in order to catch those edge cases where the virtual module is manually being assigned. That check still needs to be in the state module. It "adds to the maintenance burden of salt", so to speak.

A proper utils functions can do that check, and if it needs access to __opts__ or any other salt dunder, they should be explicitly passed.
The utility functions alone, should be enough.

Any SEP will result in some future maintenance burden. In this case, not doing the SEP will entail its own, even uglier maintenance burden.

Depends on the perspective. Might be ugly, but less magical, and thus, more predictable, testable, maintainable.

The excuse that the core team will have an additional maintenance burden can be used to suppress any SEP. If that's the case, then what are we even doing here?

I can't reject a SEP alone, and I'm not talking on behalf of the whole core team.


## Alternatives
[alternatives]: #alternatives

This SEP already contains two potential implementations. The alternative to
implementing this SEP would be to duplicate the logic from the `__virtual__` at
every location where utility functions need to be used, which is an open
invitation for the two (or more) copies of that logic to drift from one
another.
Comment on lines +101 to +107
Copy link

Choose a reason for hiding this comment

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

Suggested change
[alternatives]: #alternatives
This SEP already contains two potential implementations. The alternative to
implementing this SEP would be to duplicate the logic from the `__virtual__` at
every location where utility functions need to be used, which is an open
invitation for the two (or more) copies of that logic to drift from one
another.
[alternatives]: #alternatives
This SEP already contains two potential implementations. The alternative to
implementing this SEP would be to duplicate the logic from the `__virtual__` at
every location where utility functions need to be used, which is an open
invitation for the two (or more) copies of that logic to drift from one
another.

This is not an alternative. I've pointed out how it should be done in the previous comment.


## Unresolved questions
[unresolved]: #unresolved-questions

- For [Option 1](#option-1-augmentation-of-lazyloader), Salt once had a test
which parsed all docstrings in every execution module, ensuring that CLI
examples were included. I'm not sure if this test still exists, but utility
functions would not need CLI examples and this test case (if still present)
would need to be updated to account for this fact.
Copy link

Choose a reason for hiding this comment

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

Suggested change
- For [Option 1](#option-1-augmentation-of-lazyloader), Salt once had a test
which parsed all docstrings in every execution module, ensuring that CLI
examples were included. I'm not sure if this test still exists, but utility
functions would not need CLI examples and this test case (if still present)
would need to be updated to account for this fact.
- For [Option 1](#option-1-augmentation-of-lazyloader), Salt once had a test
which parsed all docstrings in every execution module, ensuring that CLI
examples were included. I'm not sure if this test still exists, but utility
functions would not need CLI examples and this test case (if still present)
would need to be updated to account for this fact.

There isn't a test case, but there is a pre-commit hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer a concern with my proposed implementation from #70 (comment), since the utility functions now remain prefixed with underscores.


- Also for [Option 1](#option-1-augmentation-of-lazyloader), we may need to add
something to prevent Sphinx from including utility functions in the docs.

- As noted above for [Option
2](#option-2-move-the-utility-functions-to-a-new-loader-type), with the fate
of `__utils__` in question, the actual name of the hypothetical loader type
is unknown.

# Drawbacks
[drawbacks]: #drawbacks

- For [Option 1](#option-1-augmentation-of-lazyloader), this means that
`states` loader instances would no longer be re-using the existing
`minion_mods` loader instance that is used by the CLI and template context,
and would need to generate its own to use as its `__salt__` dunder. The
performance impact of this should be minimal, however.

- For [Option 2](#option-2-move-the-utility-functions-to-a-new-loader-type),
adding a new loader type is a non-trivial undertaking, so this would be a
much less-attractive solution.