Skip to content
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

apiclient: update tokio-tungstenite to v0.16.1+ #1947

Closed
etungsten opened this issue Feb 7, 2022 · 2 comments · Fixed by #1948
Closed

apiclient: update tokio-tungstenite to v0.16.1+ #1947

etungsten opened this issue Feb 7, 2022 · 2 comments · Fixed by #1948
Labels
area/core Issues core to the OS (variant independent) status/in-progress This issue is currently being worked on type/bug Something isn't working

Comments

@etungsten
Copy link
Contributor

etungsten commented Feb 7, 2022

In v0.16.0 of tungstenite, the closing frame received by the client gets echoed back and returned to the user. Before it always returned a closing frame with a normal close code and empty string for the reason. With this behavior change we've seen some instances where apiclient exec would return non-zero exit codes regardless of the command result due to the receiving the closing frame with a disallowed code indicating protocol violation. We need to look into apiserver and figure what's going on there to fix the issue more permanently.

@etungsten etungsten added type/bug Something isn't working priority/p1 area/core Issues core to the OS (variant independent) labels Feb 7, 2022
@etungsten
Copy link
Contributor Author

etungsten commented Feb 7, 2022

The reason why this was happening is because apiserver sends the "exec'd" process's exit code inside the close frame as the close code. See:

/// Sends the process return code to the client inside a Close message.
fn handle(&mut self, msg: message::ProcessReturn, ctx: &mut Self::Context) -> Self::Result {
info!("exec process returned {}", msg.code);
// nix deals with i32 (c_int) return codes, but we know they're never negative; really,
// they're just a u8. If that assumption breaks for some reason, we don't have a
// reasonable code to send to the user, so just give a 0.
let code = u16::try_from(msg.code).unwrap_or(0);
stop(ctx, None::<String>, ws::CloseCode::Other(code));
}

But on the client side, tungstenite doesn't allow any close code except a select few, otherwise it treats it as a protocol violation. See:
https://github.com/snapview/tungstenite-rs/blob/d079090031b8628fd33bca6e4622af156648c02c/src/protocol/frame/coding.rs#L190-L195
and
https://github.com/snapview/tungstenite-rs/blob/5ad8cef6ffb7edc372343717b6f86be16805e08c/src/protocol/mod.rs#L560-L568

This is problematic because any closing frame with a close code that's not allowed by tungstenite will be overwritten with a closing frame that just indicates a generic protocol violation. What we really want is for it to pass the closing frame directly to apiclient without modification. I'm currently not sure if this is a websocket specification thing but it doesn't quite make sense to me why it wouldn't just pass the closing frame as is...

To fix this issue with v0.16 of tungstenite, we need apiserver to set the close code to Normal (1000) when the exec'd process returns exit code0. If the exec'd process errors out, then apiserver would return Bad with the close code set to the exit status of the invoked process. But tungstenite will overwrite the closing frame with a protocol violation closing frame, hopefully that changes eventually.
There would be no changes on the apiclient side since it's handling the closing frame close code correctly as far as I can tell. Due to the problem I describe above, it would just return exit status code 1 for failed apiclient exec calls like we would expect.

@etungsten
Copy link
Contributor Author

etungsten commented Feb 7, 2022

I guess to completely work around the protocol violation warning message when apicient exec commands do fail, apiserver would need to send a closing frame with closing code Normal regardless of the exec'd process exit status code and just store the exit code in the reason message. apiclient would then extract the exit code out of the closing frame reason message and return accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core Issues core to the OS (variant independent) status/in-progress This issue is currently being worked on type/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant