api docs: document stream format #19696
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
github-actions
bot
added
the
kind/api-change
| // ### Stream format | ||
| // | ||
| // When the TTY setting is disabled for the container, | ||
| // the HTTP Content-Type header is set to application/vnd.docker.multiplexed-stream |
There was a problem hiding this comment.
I am unable to find track down where this is being set in Podman. I see it in vendor.
There was a problem hiding this comment.
Good catch, yes looks like we always set application/vnd.docker.raw-stream regardless of tty or not.
I guess nobody really cares about the headers, I will see if I can fix it in the code. Feels weird to me that we even use these docker headers in the libpod API but it is what it is.
There was a problem hiding this comment.
ah looks like docker just added this type with api 1.42, https://github.com/moby/moby/blob/331854a126674175dde199c6516174d15862cf1c/api/server/router/container/container_routes.go#L160-L162
| // After the headers and two new lines, the TCP connection can now be used | ||
| // for raw, bidirectional communication between the client and server. | ||
| // | ||
| // To hint potential proxies about connection hijacking, the client |
There was a problem hiding this comment.
"To inform" instead of "To hint" maybe?
There was a problem hiding this comment.
Sure this was mostly copied from the docker docs, I just removed the docker daemon references.
| // | ||
| // The header contains the information which the stream writes (`stdout` or | ||
| // `stderr`). It also contains the size of the associated frame encoded in | ||
| // the last four bytes (`uint32`). |
There was a problem hiding this comment.
Should mention byte-order of the u32 here. Not clear if we're sending little or big endian in this.
There was a problem hiding this comment.
I see that this is mentioned a few paragraphs down... Maybe unify that paragraph into here?
| @@ -1319,7 +1410,95 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error { | |||
| // tags: | |||
| // - containers | |||
| // summary: Attach to a container | |||
| // description: Hijacks the connection to forward the container's standard streams to the client. | |||
| // description: | | |||
| // Attach to a container to read its output or send it input. You can attach | |||
There was a problem hiding this comment.
Maybe just mention that the format is identical to Compat Attach?
There was a problem hiding this comment.
Properly better to do it the other way around? I mean I expect only a small number of people to looks at out compat docs. I would assume most people would use the docker docs or the libpod specific endpoint docs. SO having it here sounds better to me.
Document the attach, exec and logs output stream format. We use the same format as docker. Fixes containers#19280 Signed-off-by: Paul Holzinger <[email protected]>
Move SupportedVersion() and IsLibpodRequest() to separate package to avoid import cycle when using it in libpod. Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
api-stream-format
branch
from
a9a2b96 to
b2d6bf7
Compare
Luap99
changed the title
The attach API used to always return the Content-Type `vnd.docker.raw-stream`, however docker api v1.42 added the `vnd.docker.multiplexed-stream` type when no tty was used. Follow suit and return the same header for docker api v1.42 and libpod v4.7.0. This technically allows clients to make a small optimization as they no longer need to inspect the container to see if they get a raw or multiplexed stream. Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
api-stream-format
branch
from
b2d6bf7 to
7c9c969
Compare
|
PTAL again, I removed the duplicated docs and added code the return the new docker header. I decided to do for libpod endpoint as well as it will allow optimizations on the client side. |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Document the attach, exec and logs output stream format. We use the same format as docker.
Fixes #19280
Does this PR introduce a user-facing change?