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

fix: do not prepend garbage in the container.Exec response #624

Merged
merged 5 commits into from Nov 16, 2022

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Nov 16, 2022

What does this PR do?

This PR provides a why to extract the multiplexed Docker response when executing a command in a container, adding a variadic argument holding the options that a user would need to process the Docker response. In this PR, we are adding just one option for the processor: Multiplexed, which uses Docker's built-in stdcopy.StdCopy to extract the simple response from the combined response that Docker generates.

For further reference on how this multiplexed response is obtained:

We are adding two unit tests demonstrating the bug, running an Nginx container, and executing ls /usr/share/nginx, which should return just one directory: html. We are covering both use cases: executing a command in the container with multiplexing the response (the output is sanitized) and without multiplexing it (output contains garbage at the beginning).

As expected, the tests fail before the fix and pass after it.

Given the original function returned a Reader, pushing the responsibility of processing it to the client code, we did not want to update the signature breaking the API, therefore, this solution is adding the ability to request the multiplexed response. This is achieved by replacing the original Reader with the result of Docker's stdcopy. Please notice the stderr Reader is not used at all. In future versions of this library we probably offer a deprecation path to return a struct containing both readers.

Why is it important?

There is a bug when using the container.Exec function, as it prepends garbage to the reponse bytes returned by the Docker client, caused by how the command is created at the library level.

With this fix, it's possible to choose the response format, simplifying how client code is using the Exec function.

Related issues

Follow-ups

Let's create an issue to offer a deprecation path for the current Exec function, which returns a struct containing stdout and stderr, something like what we already do for the Docker Compose module: https://github.com/testcontainers/testcontainers-go/blob/main/compose_local.go#L245

@mdelapenya mdelapenya requested a review from a team as a code owner November 16, 2022 09:57
@mdelapenya mdelapenya added the bug An issue with the library label Nov 16, 2022
@mdelapenya mdelapenya self-assigned this Nov 16, 2022
@mdelapenya mdelapenya requested a review from a team November 16, 2022 09:57
docker.go Outdated Show resolved Hide resolved
cyberbeast
cyberbeast previously approved these changes Nov 16, 2022
Copy link

@cyberbeast cyberbeast left a comment

Choose a reason for hiding this comment

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

Thank you for taking a look at it. Looks good to me.

@mdelapenya
Copy link
Collaborator Author

Thank you for taking a look at it. Looks good to me.

@cyberbeast we are going to propose another solution for the fix, keeping the test: we verified that Docker defines a protocol to stream responses: https://docs.docker.com/engine/api/v1.37/#tag/Container/operation/ContainerAttach

This protocol is implemented in Docker Java here: https://github.com/docker-java/docker-java/blob/f6f9d676d9244d6048fdf85c5db6f2c4813d5c23/docker-java-core/src/main/java/com/github/dockerjava/core/FramedInputStreamConsumer.java#L28, and in Docker.DotNet here: https://github.com/dotnet/Docker.DotNet/blob/30181cc146076ee8dc7ab839535ae89128672531/src/Docker.DotNet/MultiplexedStream.cs#L82

In Go we are simply returning the Reader, but it could be interesting to hide the streaming protocol implementation detail from the consumers, wrapping the returned reader demultiplexing the response.

kiview
kiview approved these changes Nov 16, 2022
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

I think it is the right approach to delegate github.com/docker/docker/pkg/stdcopy and the ProcessOption interface allows us to be open for future required changes, if they are necessary.

If we merge the PR like this, the PR description should be updated to reflect the new approach of the PR, else it is quite misleading (it still references the TTY approach).

Do we plan to introduce an abstraction later, that makes accessing stdout and stderr distinctively easier?

@mdelapenya
Copy link
Collaborator Author

I think it is the right approach to delegate github.com/docker/docker/pkg/stdcopy and the ProcessOption interface allows us to be open for future required changes, if they are necessary.

If we merge the PR like this, the PR description should be updated to reflect the new approach of the PR, else it is quite misleading (it still references the TTY approach).

Do we plan to introduce an abstraction later, that makes accessing stdout and stderr distinctively easier?

Yes, need to update the description of the PR.

Indeed, accessing the stderr (stdout is already available in the original reader) would be desired, and I already thought about it but wanted to check this solution first. Maybe logging stderr would be a thing to consider to avoid changing the signature of the Exec function.

@kiview
Copy link
Member

kiview commented Nov 16, 2022

Then I'd suggest merging this PR as-is to solve the issue at hand and think about approaches for programmatic access to stderr.

Sometimes stderr behaves quite unexpected. I remember in the specific case of PostgreSQL, that the database process like to output very normal startup information on stderr. So intuitively assuming we can log stderr as an error might not be the right approach.

@mdelapenya mdelapenya merged commit 4c4def7 into testcontainers:main Nov 16, 2022
12 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Nov 17, 2022
* main:
  docs: update method to `nginxC.Terminate` (testcontainers#627)
  fix: do not prepend garbage in the container.Exec response (testcontainers#624)
  chore: retire podman pipeline (testcontainers#625)
  update gotest.md - fix errors in the example (testcontainers#623)
  chore: sync governance files (testcontainers#622)
@mdelapenya mdelapenya deleted the fix-reader branch November 18, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Random characters in the output of io.Reader returned in Exec(ctx context.Context, cmd []string)
4 participants