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

decompress response body from websocket error #6650

Merged
merged 3 commits into from
Nov 8, 2019
Merged

Conversation

drewbailey
Copy link
Contributor

@drewbailey drewbailey commented Nov 7, 2019

If a websocket connection errors we currently return the error with a
copy of the response body. The response body from the websocket can
often times be gzipped, so check if the content-encoding was gzipped
and decompress.

Example error responses that were getting appended:

 �JK��IMQ(�W�M,*�H�QH���M�KQ��+�W�*�ϳR��P��W04��'�~�-

which was the result of the following response body

 Reader: (*bytes.Reader)(0xc00014a540)({
  s: ([]uint8) (len=69 cap=1024) {
   00000000  1f 8b 08 00 00 00 00 00  00 ff 4a 4b cc cc 49 4d  |..........JK..IM|
   00000010  51 28 c9 57 c8 4d 2c 2a  ce 48 cc 51 48 ce cf cd  |Q(.W.M,*.H.QH...|
   00000020  4d cc 4b 51 c8 cc 2b c9  57 c8 2a ce cf b3 52 c8  |M.KQ..+.W.*...R.|
   00000030  cf 50 c8 cb 57 30 34 00  04 00 00 ff ff 27 ea 7e  |.P..W04......'.~|
   00000040  f4 2d 00 00 00                                    |.-...|
  },

fixes #6545

api/api.go Outdated Show resolved Hide resolved
@drewbailey drewbailey changed the title Remove response body from websocket error un-gzip response body from websocket error Nov 7, 2019
Copy link
Contributor

@cgbaker cgbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch - thanks!

api/allocations_test.go Outdated Show resolved Hide resolved
// ensure the error is what we expect
_, err := a.Exec(context.Background(), alloc, "bar", false, []string{"command"}, os.Stdin, os.Stdout, os.Stderr, sizeCh, nil)
require.Contains(t, err.Error(), "Unexpected response code: 301")
require.Contains(t, err.Error(), "Moved Permanently")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your change but I find it very sad that we return redirect to /ui if an /v1/ endpoint isn't found.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is so strange. Filed an issue: #6656

If a websocket connection errors we currently return the error with a
copy of the response body. The response body from the websocket can
often times be completely illegible so remove it from the error string.

make alloc id empty for more reliable failure

un-gzip if content encoding header present
@drewbailey drewbailey changed the title un-gzip response body from websocket error decompress response body from websocket error Nov 8, 2019
@drewbailey drewbailey merged commit 5e117ec into master Nov 8, 2019
@drewbailey drewbailey deleted the b-better-ws-error branch November 8, 2019 15:31
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad error message on alloc exec when streaming through ELB
4 participants