-
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
[3007.x] transport: tcp check that pull_path exists before chmod #66931
base: 3007.x
Are you sure you want to change the base?
[3007.x] transport: tcp check that pull_path exists before chmod #66931
Conversation
@dwoz You did the initial permission fixup. You're probably the best one to review. |
Could we get a changelog and a test for this, please? That means you'll probably need to create an issue to link to. |
cf8455f
to
a21cc46
Compare
I'm not actually sure the best way to test. There's no functional issues. The async task just cancels right before it was going to exit anyway, but the other tasks it spawns are successful. In the codepath that causes the traceback the server is binding to a TCP port and thus chmod is also inconsequential. I /could/ make a specific test for TCP that the "publisher" method is successful, but that seems a little silly to make one for a specific transport. Ideally all transports have the same "interface". Unfortunately making a generic one doesn't make sense either as each transport treats "publisher" differently (which maybe should be normalized?). For example WebSocket will hang indefinitely (on purpose) in that function and at the moment ZeroMQ publisher is "broken" (see #66751). Do you have any suggestions? |
What does this PR do?
Check that pull_path is used before attempting to chmod. This was an oversight in the patch to open up the socket permissions. I'm not entirely sure how this doesn't break tests / functionality.. but whatever. The fix removes a traceback, which is good I suppose.
I'm not actually sure what kind of tests would be needed ... since the current tests seem not to have caught this?
What issues does this PR fix or reference?
Fixes
Previous Behavior
I noticed this traceback in some testing artifacts:
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.