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

Use binary frame for websocket attach endpoint #30460

Merged
merged 1 commit into from Feb 2, 2017

Conversation

Projects
None yet
7 participants
@yongtang
Member

yongtang commented Jan 26, 2017

- What I did
This fix tries to address the issue raised in #28176 where text frame was used in websocket attach endpoint. In case the data send out contains non utf8 data, the connection will be closed in certain browsers, e.g., Safari.

- How I did it

This fix address the issue by change PayloadType to BinaryFrame.

- How to verify it

This fix is tested manually with Safari. The docker daemon is inside a Linux Virtual Machine.

Create a container with:

docker run -itd --name websocket busybox sh -c "while true; do echo -e 'he\\xc3\\x28o'; sleep 5; done"

Use the following url (172.16.66.128:2375 is the tcp address of the daemon):

file:///websocket.html?url=ws://172.16.66.128:2375/containers/websocket/attach/ws?logs=1&stderr=1&stdout=1&stream=1&stdin=1

and the following websocket.html:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="utf-8">
  <title>Websocket</title>
  <script type="text/javascript">
    function DockerWebSocket() {
      if ("WebSocket" in window) {
        console.log("WebSocket is supported by Browser...")
        // Remove '?url=' prefix
        url = window.location.search.replace(/^(\?url=)/,"");
        console.log("URL ["+url+"]...");
        var ws = new WebSocket(url);
        ws.onopen = function() {
          console.log("Connection is opened...");
        };
        ws.onclose = function() {
          console.log("Connection is closed...");
        };
        ws.onmessage = function (e) {
          if (typeof e.data === "string") {
            alert("WebSocket received text message ["+e.data+"]!")
          } else {
            console.log("Message is received...")
            var blobReader = new FileReader();
            blobReader.onload = function(event) {
              console.log(JSON.stringify(blobReader.result))
            };
            blobReader.readAsText(e.data)
            console.log("Message complete...")
          }
        };
      } else {
        alert("WebSocket is not supported by Browser!");
      }
    }
  </script>
</head>
<body>
  <div>
    <a href="javascript:DockerWebSocket()">Run DockerWebSocket</a>
  </div>
</body>
</html>

The following error is returned without fix:

[Error] WebSocket connection to 'ws://172.16.66.129:2375/containers/websocket/attach/ws?logs=1&stderr=1&stdout=1&stream=1&stdin=1' failed: Could not decode a text frame as UTF-8.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cutest_kitten_ever

This fix fixes #28176.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@LK4D4

LK4D4 approved these changes Jan 31, 2017

LGTM

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 31, 2017

Contributor

ping @docker/core-engine-maintainers

Contributor

LK4D4 commented Jan 31, 2017

ping @docker/core-engine-maintainers

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 31, 2017

Member

@yongtang can you add a note to the API change history? https://github.com/docker/docker/blob/master/docs/api/version-history.md

IIUC, users of this API endpoint may have to make changes for this, so it's important to mention this change

Member

thaJeztah commented Jan 31, 2017

@yongtang can you add a note to the API change history? https://github.com/docker/docker/blob/master/docs/api/version-history.md

IIUC, users of this API endpoint may have to make changes for this, so it's important to mention this change

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Jan 31, 2017

Contributor

Will sending a binary frame actually break older clients?

Contributor

stevvooe commented Jan 31, 2017

Will sending a binary frame actually break older clients?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 31, 2017

Member

@stevvooe apparently it does; #28176 (comment)

Member

thaJeztah commented Jan 31, 2017

@stevvooe apparently it does; #28176 (comment)

Use binary frame for websocket attach endpoint
This fix tries to address the issue raised in 28176 where
text frame was used in websocket attach endpoint. In case
the data send out contains non utf8 data, the connection
will be closed in certain browsers, e.g., Safari.

This fix address the issue by change `PayloadType` to `BinaryFrame`.

This fix is tested manually with Safari. The docker daemon is inside a Linux Virtual Machine.

Create a container with:
```
docker run -itd --name websocket busybox sh -c "while true; do echo -e 'he\\xc3\\x28o'; sleep 5; done"
```

Use the following url (172.16.66.128:2375 is the tcp address of the daemon):
```
file:///websocket.html?url=ws://172.16.66.128:2375/v1.25/containers/websocket/attach/ws?logs=1&stderr=1&stdout=1&stream=1&stdin=1
```

and the following html:
```
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="utf-8">
  <title>Websocket</title>
  <script type="text/javascript">
    function DockerWebSocket() {
      if ("WebSocket" in window) {
        console.log("WebSocket is supported by Browser...")
        // Remove '?url=' prefix
        url = window.location.search.replace(/^(\?url=)/,"");
        console.log("URL ["+url+"]...");
        var ws = new WebSocket(url);
        ws.onopen = function() {
          console.log("Connection is opened...");
        };
        ws.onclose = function() {
          console.log("Connection is closed...");
        };
        ws.onmessage = function (e) {
          if (typeof e.data === "string") {
            alert("WebSocket received text message ["+e.data+"]!")
          } else {
            console.log("Message is received...")
            var blobReader = new FileReader();
            blobReader.onload = function(event) {
              console.log(JSON.stringify(blobReader.result))
            };
            blobReader.readAsText(e.data)
            console.log("Message complete...")
          }
        };
      } else {
        alert("WebSocket is not supported by Browser!");
      }
    }
  </script>
</head>
<body>
  <div>
    <a href="javascript:DockerWebSocket()">Run DockerWebSocket</a>
  </div>
</body>
</html>
```

This fix fixes 28176.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 31, 2017

Member

@thaJeztah the PR has been updated with API change history docs. Please take a look.

@stevvooe It depends on the client library. Golang is fine. But for JavaScript client has to use typeof e.data === "string" or e.data instanceof Blob or e.data instanceof ArrayBuffer to inspect the WebSocket frame type, and process the data accordingly.

Member

yongtang commented Jan 31, 2017

@thaJeztah the PR has been updated with API change history docs. Please take a look.

@stevvooe It depends on the client library. Golang is fine. But for JavaScript client has to use typeof e.data === "string" or e.data instanceof Blob or e.data instanceof ArrayBuffer to inspect the WebSocket frame type, and process the data accordingly.

@thaJeztah

LGTM

@estesp estesp merged commit f0089a8 into moby:master Feb 2, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30246 has succeeded
Details
janky Jenkins build Docker-PRs 38788 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9817 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 2, 2017

@yongtang yongtang deleted the yongtang:28176-attach-binary-frame-websocket branch Feb 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment