From 910e74265181acae89392cf92a3042d49546a2e9 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 6 Jun 2023 22:23:36 -0500 Subject: [PATCH 1/8] SEP: Expose utility functions to states, but not to CLI or template context --- expose-utility-functions-to-states.md | 129 ++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 expose-utility-functions-to-states.md diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md new file mode 100644 index 0000000..9301189 --- /dev/null +++ b/expose-utility-functions-to-states.md @@ -0,0 +1,129 @@ +- 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: +- Salt Issue: + +# 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 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. + +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. + +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 +[option-1]: #option-1 + +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 +[option-2]: #option-2 + +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) 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. + +## 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. + +## Unresolved questions +[unresolved]: #unresolved-questions + +- For [Option 1](#option-1), 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. + +- As noted above for [Option 2](#option-2), 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), 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 tempalte 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), adding a new loader type is a non-trivial + undertaking, so this would be a much less-attractive solution. From 9a9f3ebe957f780754e3364a3d741ef4fcb08d57 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 6 Jun 2023 22:28:44 -0500 Subject: [PATCH 2/8] Add PR and issue links --- expose-utility-functions-to-states.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index 9301189..643bc6d 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -1,8 +1,8 @@ - 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: -- Salt Issue: +- 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 From 591d0d566ab0a19bde50088360ddd0732eb24faa Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 6 Jun 2023 22:31:29 -0500 Subject: [PATCH 3/8] Clarification --- expose-utility-functions-to-states.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index 643bc6d..4eac2c9 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -13,9 +13,9 @@ 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 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 +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: From 05cc38f2ab817b8af4b346b942eaf98848e36770 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 6 Jun 2023 22:39:29 -0500 Subject: [PATCH 4/8] Fix anchors --- expose-utility-functions-to-states.md | 35 ++++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index 4eac2c9..361966e 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -55,7 +55,6 @@ exposing dubiously-useful functions to the CLI and template context. [design]: #detailed-design ## Option 1: Augmentation of LazyLoader -[option-1]: #option-1 An optional module-level dunder could be added to remote-execution modules, to suppress availability of utility functions. For example: @@ -72,11 +71,10 @@ 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 -[option-2]: #option-2 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) is the superior solution. +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 @@ -107,23 +105,26 @@ another. ## Unresolved questions [unresolved]: #unresolved-questions -- For [Option 1](#option-1), 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. -- As noted above for [Option 2](#option-2), with the fate of `__utils__` in - question, the actual name of the hypothetical loader type is unknown. +- 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), 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 tempalte 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 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 tempalte 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), adding a new loader type is a non-trivial - undertaking, so this would be a much less-attractive solution. +- 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. From 46e1653a463ca2799ce008210e590d744373455a Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Wed, 7 Jun 2023 13:12:56 -0500 Subject: [PATCH 5/8] Add note about how docs may be affected --- expose-utility-functions-to-states.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index 361966e..10bbd4c 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -111,6 +111,9 @@ another. functions would not need CLI examples and this test case (if still present) would need to be updated to account for this fact. +- 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 From 372497d314605cc5a2202f032c3f3c33dc0296f3 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Wed, 7 Jun 2023 13:28:06 -0500 Subject: [PATCH 6/8] spelling --- expose-utility-functions-to-states.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index 10bbd4c..876d849 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -124,7 +124,7 @@ another. - 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 tempalte context, + `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. From 89c70781aebd59c3195163ab3452d716996dff36 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Mon, 31 Jul 2023 14:38:45 -0500 Subject: [PATCH 7/8] Update SEP to remove obsolete concerns --- expose-utility-functions-to-states.md | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index 876d849..c1505c3 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -60,15 +60,15 @@ 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') +__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. +This dunder would be used to specify private utility functions which can +conditionally be added to the loader 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 @@ -105,15 +105,6 @@ another. ## 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. - -- 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 From f0fc71b2590da8bd33437d9b1e1fb1adfd950a29 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Mon, 31 Jul 2023 14:51:40 -0500 Subject: [PATCH 8/8] Add link to patch --- expose-utility-functions-to-states.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/expose-utility-functions-to-states.md b/expose-utility-functions-to-states.md index c1505c3..d5d3411 100644 --- a/expose-utility-functions-to-states.md +++ b/expose-utility-functions-to-states.md @@ -70,6 +70,10 @@ 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. +See +[here](https://github.com/saltstack/salt-enhancement-proposals/pull/70#issuecomment-1625554331) +for a patch that implements this. + ## 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