Repository: moby/moby (language: go)

Issue:
Attaching websocket (/containers/{name:.*}/attach/ws) not configurable as stated in the docs
<!--
If you are reporting a new issue, make sure that we do not have any duplicates
already open. You can ensure this by searching the issue list for this
repository. If there is a duplicate, please close your issue and add a comment
to the existing issue instead.

If you suspect your issue is a bug, please edit your issue description to
include the BUG REPORT INFORMATION shown below. If you fail to provide this
information within 7 days, we cannot debug your issue and will close it. We
will, however, reopen it if you later provide the information.

For more information about reporting issues, see
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#reporting-other-issues

---------------------------------------------------
GENERAL SUPPORT INFORMATION
---------------------------------------------------

The GitHub issue tracker is for bug reports and feature requests.
General support for **docker** can be found at the following locations:

- Docker Support Forums - https://forums.docker.com
- Slack - community.docker.com #general channel
- Post a question on StackOverflow, using the Docker tag

General support for **moby** can be found at the following locations:

- Moby Project Forums - https://forums.mobyproject.org
- Slack - community.docker.com #moby-project channel
- Post a question on StackOverflow, using the Moby tag

---------------------------------------------------
BUG REPORT INFORMATION
---------------------------------------------------
Use the commands below to provide key information from your environment:
You do NOT have to include this information if this is a FEATURE REQUEST
-->

**Description**

The [docs](https://docs.docker.com/engine/api/v1.41/#operation/ContainerAttachWebsocket) describe, that `ContainerAttachWebsocket`  is configurable for stdin/stdout/stderr, but stdin and stderr are always merged, independently of the actual params.

Possible, the reason is here:
If I followed the code correctly, 
https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/api/server/router/container/container.go#L45
links to the function wsContainersAttach
https://github.com/moby/moby/blob/9e7bbdb9ba53440cfc67e2e1de89e35e44c43ddf/api/server/router/container/container_routes.go#L640
which sets stdin, stdout and stderr to true and actually ignores the params:
https://github.com/moby/moby/blob/9e7bbdb9ba53440cfc67e2e1de89e35e44c43ddf/api/server/router/container/container_routes.go#L676-L685

**Steps to reproduce the issue:**

Just have a container with some stdout:
```bash
docker run ubuntu bash -c "echo 'Hello World' && sleep 1000"
```

Connecting to the container using python docker api:
```py
import docker

api_client = docker.APIClient('yourdockersocket')
# Uses /containers/{name:.*}/attach/ws (Websocket Stream)
sock = api_client.attach_socket('test_container_id', params={'stderr': 1, 'stream': 1, 'logs': 1}, ws=True)
print(sock1.read(11))
```
will output stdout as well, although only stderr is desired. On the other hand:
```py
import docker

api_client = docker.APIClient('yourdockersocket')
# Uses /containers/{name:.*}/attach (TCP stream)
sock = api_client.attach_socket('test_container_id', params={'stderr': 1, 'stream': 1, 'logs': 1})
print(sock1.read(11))
```
will correctly, probably using
https://github.com/moby/moby/blob/9e7bbdb9ba53440cfc67e2e1de89e35e44c43ddf/api/server/router/container/container_routes.go#L613-L622


**Describe the results you received:**
As explained above, stderr and stdout merged stream is returned in the websocket endpoint, although not specified.

**Describe the results you expected:**
As explained above, expected result would be the stderr stream and stdout stream separately, but they are merged for the websocket request.


**Output of `docker version`:**


```
Client: Docker Engine - Community
 Version:           20.10.12
 API version:       1.41
 Go version:        go1.16.12
 Git commit:        e91ed57
 Built:             Mon Dec 13 11:45:33 2021
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server: Docker Engine - Community
 Engine:
  Version:          20.10.12
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.16.12
  Git commit:       459d0df
  Built:            Mon Dec 13 11:43:42 2021
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.4.12
  GitCommit:        7b11cfaabd73bb80907dd23182b9347b4245eb5d
 runc:
  Version:          1.0.2
  GitCommit:        v1.0.2-0-g52b36a2
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0
```

**Output of `docker info`:**

```
Client:
 Context:    default
 Debug Mode: false
 Plugins:
  app: Docker App (Docker Inc., v0.9.1-beta3)
  buildx: Docker Buildx (Docker Inc., v0.7.1-docker)
  scan: Docker Scan (Docker Inc., v0.12.0)

Server:
 Containers: 1
  Running: 1
  Paused: 0
  Stopped: 0
 Images: 25
 Server Version: 20.10.12
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 1
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 7b11cfaabd73bb80907dd23182b9347b4245eb5d
 runc version: v1.0.2-0-g52b36a2
 init version: de40ad0
 Security Options:
  seccomp
   Profile: default
 Kernel Version: 5.10.60.1-microsoft-standard-WSL2
 Operating System: Ubuntu 20.04.3 LTS
 OSType: linux
 Architecture: x86_64
 CPUs: 8
 Total Memory: 12.32GiB
 Name: DEAGM03731
 ID: 4GVK:G7FV:GK5N:W5KE:2X7M:44FA:26XI:B5MW:DTLG:NLQV:7XLV:FK5L
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Registry Mirrors:
  https://docker.deagxartifactory.sickcn.net/
 Live Restore Enabled: false

WARNING: API is accessible on http://127.0.0.1:2375 without encryption.
         Access to the remote API is equivalent to root access on the host. Refer
         to the 'Docker daemon attack surface' section in the documentation for
         more information: https://docs.docker.com/go/attack-surface/
WARNING: No blkio throttle.read_bps_device support
WARNING: No blkio throttle.write_bps_device support
WARNING: No blkio throttle.read_iops_device support
WARNING: No blkio throttle.write_iops_device support
```

**Additional environment details (AWS, VirtualBox, physical, etc.):**
Running on WSL2.


Discussion / hints:
Thanks for reporting; having a look in git history, I _suspect_ that this is the commit where this changed; https://github.com/moby/moby/commit/c44f513248a8b40b1b2221726c7441881383e919 (https://github.com/moby/moby/pull/12120)

Or at least, I see some of the booleans being removed there, and don't see a direct replacement for those 🤔 
@ndeloof  perhaps this is something to look into as well (after your other PR was merged)

Fix patch:
diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go
index b66bdac847b79..287611ad866fe 100644
--- a/api/server/router/container/container_routes.go
+++ b/api/server/router/container/container_routes.go
@@ -693,15 +693,22 @@ func (s *containerRouter) wsContainersAttach(ctx context.Context, w http.Respons
 		return conn, conn, conn, nil
 	}
 
+	useStdin, useStdout, useStderr := true, true, true
+	if versions.GreaterThanOrEqualTo(version, "1.42") {
+		useStdin = httputils.BoolValue(r, "stdin")
+		useStdout = httputils.BoolValue(r, "stdout")
+		useStderr = httputils.BoolValue(r, "stderr")
+	}
+
 	attachConfig := &backend.ContainerAttachConfig{
 		GetStreams: setupStreams,
+		UseStdin:   useStdin,
+		UseStdout:  useStdout,
+		UseStderr:  useStderr,
 		Logs:       httputils.BoolValue(r, "logs"),
 		Stream:     httputils.BoolValue(r, "stream"),
 		DetachKeys: detachKeys,
-		UseStdin:   true,
-		UseStdout:  true,
-		UseStderr:  true,
-		MuxStreams: false, // TODO: this should be true since it's a single stream for both stdout and stderr
+		MuxStreams: false, // never multiplex, as we rely on websocket to manage distinct streams
 	}
 
 	err = s.backend.ContainerAttach(containerName, attachConfig)
diff --git a/api/types/backend/backend.go b/api/types/backend/backend.go
index db7a8d9011fef..50d203c0bbf30 100644
--- a/api/types/backend/backend.go
+++ b/api/types/backend/backend.go
@@ -17,11 +17,8 @@ type ContainerAttachConfig struct {
 	Logs       bool
 	Stream     bool
 	DetachKeys string
-
-	// Used to signify that streams are multiplexed and therefore need a StdWriter to encode stdout/stderr messages accordingly.
-	// TODO @cpuguy83: This shouldn't be needed. It was only added so that http and websocket endpoints can use the same function, and the websocket function was not using a stdwriter prior to this change...
-	// HOWEVER, the websocket endpoint is using a single stream and SHOULD be encoded with stdout/stderr as is done for HTTP since it is still just a single stream.
-	// Since such a change is an API change unrelated to the current changeset we'll keep it as is here and change separately.
+	// Used to signify that streams must be multiplexed by producer as endpoint can't manage multiple streams.
+	// This is typically set by HTTP endpoint, while websocket can transport raw streams
 	MuxStreams bool
 }
 
diff --git a/docs/api/version-history.md b/docs/api/version-history.md
index dea3a82fc408a..7e4807e05a6f8 100644
--- a/docs/api/version-history.md
+++ b/docs/api/version-history.md
@@ -87,6 +87,8 @@ keywords: "API, Docker, rcli, REST, documentation"
 * The `Volume` type, as returned by `Added new `ClusterVolume` fields 
 * Added a new `PUT /volumes{name}` endpoint to update cluster volumes (CNI).
   Cluster volumes are only supported if the daemon is a Swarm manager.
+* `/containers/{name}/attach/ws` endpoint only attach to configured streams
+  according to `stdin`, `stdout` and `stderr` parameters.
 
 ## v1.41 API changes
 


Test patch: