-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[BUG] After upgrade to 3005.3, failure to fetch latest pillar data after being updated in git pillar repo branch #65467
Comments
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
3005.2 changed how git pillars were cached to fix a security issue. Having to clear the cache after upgrading is probably expected. |
This does NOT only affect the pillar cache across upgrades. With a running 3006.3 master, the master will NOT refresh a git_pillar branch after the first time that branch is fetched. Steps to test:
|
Are there any chances we may get a fix for this soon? It affects both 3006 and 3005 and it was introduced after the CVE patches so it makes sense to have it fixed within both versions. This kind of issues make it harder to upgrade salt version from the versions with security issues to avoid having breaking issues and keep everything working as desired. |
@cmcmarrow do you have any insight into this? I think this regression happened as a result of your CVE patch 6120bca |
Theres been no github activity on @cmcmarrow 's account since sept. Safe to say hes no longer involved on the project |
I would like to add another detail: We tried to use the new released version 3005.5 which contain additional CVE patches, and we noticed that the pillar refresh issue is partially fixed and happens only on branches containing |
I had wondered if |
@Ikramromdhani Can you upgrade to latest Salt 3006 and retry, Salt 3005 has passed it's bug fix (last August 2023) and it about to run out of CVE support on the Feb 25th 2024, 24 days from now. At this stage nothing will be getting done to Salt 3005 given that it is dead in 24 days, hardly time to debug/test/fix/release cycle, esp. since would have to build classic packages and there is a couple of days in just getting that done. |
@Ikramromdhani Rereading this I remember Chad talking about how caching is changed and how the cache is now only updated correctly, rather than as it was where the cache was needlessly getting updated due to other commands being run, need to go back and find the actual commit etc., it was last year - Autumn time. And yup, Chad is gone, moved on to greener pastures, so I am reading up on GitFS, etc. to get fully familiar (having to do more areas since less people now on the core team), so will update next week with code lines that changed, but I expect you were relying on a side-effect of an operation that has since been fixed (namely incorrectly updating the cache), but will get the full details for you. That said, doesn't mean there isn't an error too. |
We are also observing this error after recently upgrading from
We have observed that when we use
However, if we create a new branch named (This evidence conflicts with #65467 (comment), although the commenter had not actually confirmed it themselves) Note:
|
@donkopotamus , that is what I was about to confirm. Tested today with 3006.6 using pygit2 and still have issues pulling changes for pillars when relying on pillarenv branch having |
I observed that the cache directory organizes cached branches by foldername=branchname, and that branches with For example, with three pillar branches:
If this turns out to be the actual issue, then the obvious solution would appear to be escaping the
or
or
|
@Ikramromdhani Won't make it for Salt 3006.7, that is close to release, waiting on clean tests in the pipeline. Possibly for 3007, but on my backlog, dealing with #65816, which may be related (similar code changes for pillarenv went in at same time for gitfs locks), writing tests for that fix now. |
Not sure if it helps, but it looks like the CVE fix in 3005.2 and then #65017 in 3005.3 that mitigated a negative effect of that fix, probably contributed to this issue |
Yup, the set of changes that Chad did and had to be adjusted, applied to both the gitfs changes he did along the pillarenv and cache changes, etc. That is why I expect some linkage to the gitfs changes. |
Just a heads up that we tested 3006.7 and the problem is still there. Any chances to get a fix for this on salt 3007? This problem is holding us for performing salt upgrades. |
@dmurphy18 since the issue fix did not make it to 3007. Just would like to ask if there are specific version to target? Thank you |
@Ikramromdhani Working on fix for Git FS and the Git Pillar is right after it, fix for Git FS is done but have to refractor a new test after PR review (#65937), after that then work on Git Pillar since it will be possibly affected by GitFS changes. |
Do we have a timeline on when we are getting this fix released? We were trying to upgrade since last year and this is the only issue holding us back. Checked the release notes for 3006.9 and did not see it included there so was wondering if this is going to be included in 3006.10 and if yes, do we know when is it going to be released? This way we can plan our upgrade accordingly. |
@Ikramromdhani Actually working this at the moment, would help to just check still occurs on 3006.9, attempting to setup similar environment. |
I'm seeing it in 3006.9, yes. |
Thank you for confirming @mdschmitt. @dmurphy18 would the fix be part of 3006.10? and if yes do we have timeline for when the release will take place? I tried to look on the documentation and was not able to find a release calendar or announcements |
@Ikramromdhani Sorry but this is on the back-burner and I have to get back to it, given the reduced size of the team, some fireballs got thrown which had to be handled. Be assured it is on the list, and the fix is half done, but tests to be written if memory recall is correct. Apologies but only so much time in the day, with a smaller team, and a laptop rebuild didn't help either. |
Interestly running steps 1-6 above gave the following which could explain why removing
using simplified
test_repo.igt has
|
Given this appears to be an issue with '/' in the name of the field, I think changes from PR 65017 to salt/utils/gitfs.py, could be the culprit, the added
whereas previously just did the decode and replace of '/' with '_'. Here the now presented name is not getting sanitized, and functions such as os.listdir will interpret the '/' as a path separator. This is just a thought that I wanted to record and not forget, not saying its the problem, just a probable cause |
@Ikramromdhani Working away on this but wanting to make sure I am addressing everything.
Unfortunately the contents of that Slack discussion are no more, and I was wondering if you have them saved off anywhere that could be shared with me. Writing tests and want to make sure I have all the bases covered. Well,m found an archive and nothing much from it apart from this bug. |
Description of Issue
A minion/master is unable to get the new pillar data from an alternative branch in git after it was updated on remote git pillar repository. This was working with 3005.1, but broke after upgrade to 3005.3. It also seem to be the case for those using 3006.3. Please refer to community discussion: https://saltstackcommunity.slack.com/archives/C7K04SEJC/p1697576330136199
Setup
Amazon Linux 2 salt master, configured with git repository for states and pillar data.
Steps to Reproduce Issue
Master config
Push an update to an existant pillar called test in repo.git on branch testpillar. On a minion, run salt-call pillar.get test pillarenv=testpillar
Expected behavior
The new value of pillar test should be visible in the output of the salt call on the minion.
Actual behavior
The minion get the old data of pillar test. Please note that when executing the same comand after deleting
/var/cache/salt/master/git_pillar
, the minion get the correct valueVersions Report
The text was updated successfully, but these errors were encountered: