[3006.x] Fix x509_v2 state without changes can trigger file module to run in test mode #64269
Conversation
salt-project-bot-prod-environment
bot
changed the title
lkubb
force-pushed
the
x509v2-test-mode-interaction
branch
from
ba63d03 to
d87eb20
Compare
salt-project-bot-prod-environment
bot
changed the title
lkubb
changed the title
lkubb
force-pushed
the
x509v2-test-mode-interaction
branch
from
d87eb20 to
417f9d8
Compare
lkubb
changed the title
This is cosmetic in the current state since only in non-test mode is file.managed actually called without the workaround.
| return res[next(iter(res))] | ||
| file_managed = __states__["file.managed"] | ||
| if test: | ||
| # calls via __salt__["state.single"](..., test=test) |
There was a problem hiding this comment.
Shouldn't the fix be in state.single if that call is what is causing the overwrite in __opts__?
There was a problem hiding this comment.
I suspect the problem is somewhere in the loader, though I'm neither qualified nor motivated to find out tbh. I have tried to debug a very similar problem with overriding __opts__ not being reversed in the Vault runner before. Also not sure if this workaround can be applied to state.single easily.
This specific bug breaks the x509_v2 state module pretty hard though and I suspect it will take a bit of time until someone figures out the root cause, if at all (since it's not a huge deal for most of Salt).
Edit: Note that in addition, by avoiding state.single this should run quite a bit more performant.
There was a problem hiding this comment.
@Ch3LL can we merge it as is? It's a huge issue in 3006.x since there is a problem installing mcrypto to onedir and the only other way to use x509 state is to use x509_v2 which has that issue.
There was a problem hiding this comment.
@lkubb Mind checking the file.directory issue reported here #64195 (comment) ?
There was a problem hiding this comment.
@Oloremo Without having much to go on regarding the report, I fail to see how file.directory would behave differently with this patch atm. If anything, I would expect it the other way around (test to not be respected). Using file.directory instead of file.managed for the reproduction test in this PR succeeds as well. So my current assumption is that it's an unrelated issue or the patch was not applied correctly, barring any future (and more detailed) reports.
There was a problem hiding this comment.
This bug is nicely distilled in this issue #62590. I think this should be addressed between salt/state.py and salt/loader/lazy.py which would fix it for all cases where we could run into this, not just x509_v2.
-
I've done some preliminary investigation and think this is something we can address in a 3006.x maintenance release.
-
We want all of Salt's dunders to be managed by the loader. Using
func_globals_injectshould be considered an anti-pattern. -
We'd like to get to a place where opts is no longer mutable. This means in the long run we'll have to figure how a better way to handle
__opts__["test"]. Currently the loader adds__opts__to the modules it imports (at import time). The loader adds a copy of opts, not opts itself. This means updating__opts__["test"]orloader.optsafter a loader has imported a module won't have any affect.
There was a problem hiding this comment.
This bug is nicely distilled in this issue #62590.
Do you mean that bug has the same source as the sticky __opts__ causing this issue with state.single or fixing that bug will remove the necessity for using state.single instead of __states__ here in the first place?
The problem is that the x509_v2 state module also overrides test for early checks to avoid signing certificates that are discarded directly after, e.g. by a missing parent dir without makedirs. So if it does not have the same cause, a restructuring of the x509_v2 state module would still be necessary (not the end of the world though).
There was a problem hiding this comment.
This bug is nicely distilled in this issue #62590.
fixing that bug will remove the necessity for using
state.singleinstead of__states__here in the first place?
I don't think we should need to refactor x509_v2 after fixing the bug. I will pull in your tests to the PR where this gets fixed so we can validate the issue is resolved.
There was a problem hiding this comment.
@dwoz
It's been a month, any updates on this issue? I believe it block literally everyone who wants to use 3006.x and x509
There was a problem hiding this comment.
#64960 addresses this issue. I think this PR can be closed.
|
Superseded by #64960. |
What does this PR do?
In some cases, the
x509_v2state module can cause subsequent calls to thefilemodules to erroneously run in test mode.What issues does this PR fix or reference?
Fixes: #64195
Previous Behavior
The
filemodule sometimes runs in test mode.New Behavior
The
filemodule always runs in the expected mode.Merge requirements satisfied?
Commits signed with GPG?
Yes