wsContainersAttach attach to stdin/out/err streams as requested#43322
wsContainersAttach attach to stdin/out/err streams as requested#43322thaJeztah merged 1 commit intomoby:masterfrom
Conversation
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
While the change is the correct behavior, it breaks older clients completely.
I put the TODO in there originally because it had already been years of the existing broken behavior.
I'd also expect us to take advantage of ws channels rather than doing the muxing on the data.
|
@cpuguy83 I replaced TODO with a comment, but still multiplex is set to |
|
Thanks for clarifying, I see that now. Don't know why I didn't see that before. |
|
I think we should keep the existing behavior for old API versions. |
|
Yes, if this breaks current clients, we should probably make the (default) behavior depend on the API version. (Haven't looked in depth what/how things break) Perhaps would be good to;
(Happy to help with rephrasing / wording if needed) |
ndeloof
force-pushed
the
websocket_streams
branch
from
68439c8 to
91807e8
Compare
|
I've checked endpoint history, and it actually never supported streams to be selected. Initial implementation used a distinct backend function for HTTP and WS attachment, which was simplified into a single one by a77b7dd (#18943).
|
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
websocket_streams
branch
from
91807e8 to
ea67601
Compare
Interesting; so it didn't work before c44f513 (#12120) either? #43272 (comment) |
|
So, looks like the documentation for existing API versions is incorrect, as it mentions the
|
| if versions.GreaterThanOrEqualTo(version, "1.42") { | ||
| useStdin = httputils.BoolValue(r, "stdin") | ||
| useStdout = httputils.BoolValue(r, "stdout") | ||
| useStderr = httputils.BoolValue(r, "stderr") | ||
| } |
There was a problem hiding this comment.
Trying to think if there's a way to make the change less disruptive, but not sure how to;
- API < 1.41 enabled these by default (which didn't match the docs (see my other comment)), so I'd consider that to be a bug
- But has been around for a long time, so we probably shouldn't change for existing API versions
So in the old API versions; if I didn't look at the API docs, and forgot to set ?stdin=1&stdout=1&stderr=1, then I would still have all three attached.
In the new API version; I MUST set all three to preserve the old behavior; which matches what was documented, but (see above) "just worked" before.
So, yeah, I don't see a clear way to migrate between one and the other, other than calling it out explicitly in the release notes 🤔 🤷♂️
| * 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 |
There was a problem hiding this comment.
nit;
| * `/containers/{name}/attach/ws` endpoint only attach to configured streams | |
| * `GET /containers/{name}/attach/ws` endpoint only attach to configured streams |
(we can fix that later, not a blocker)
3 participants
- What I did
attach/wsAPI applies stdlin/stdout/stderr parameters to select streams to attach to (as documented)closes #43272
- How I did it
set parameters on
ContainerAttachConfig. Also aligned struct initialization withpostContainersAttachfor consistency.- How to verify it
curl -v -H 'Connection: upgrade' -H 'upgrade: websocket' -H 'Sec-Websocket-Key:hello' -H 'Sec-Websocket-Version: 13'tp://localhost:2375/v1.41/containers/$ID/attach/ws?stream=1&stdout=1" --output -
- Description for the changelog
containers/{ID}/attach/wsonly attach to streams according bystdin,stdoutandstderrparameters.- A picture of a cute animal (not mandatory but encouraged)
