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.

Sign up for GitHub

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

Jump to bottom

Allow uid:gid to be specified during container creation #363

Merged
merged 2 commits into from
Oct 22, 2021
Merged

Allow uid:gid to be specified during container creation #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2db996f
support uid:gid specification
Oct 4, 2021
d69086e
uid tests
Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ type ContainerRequest struct {
Privileged bool // for starting privileged container
Networks []string // for specifying network names
NetworkAliases map[string][]string // for specifying network aliases
User string // for specifying uid:gid
SkipReaper bool // indicates whether we skip setting up a reaper for this
ReaperImage string // alternative reaper image
AutoRemove bool // if set to true, the container will be removed from the host when stopped
Expand Down
1 change: 1 addition & 0 deletions docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,7 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque
Labels: req.Labels,
Cmd: req.Cmd,
Hostname: req.Hostname,
User: req.User,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would happen if the req.User is empty? Will the docker client understand that as root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User is a string field, so it appears that an empty string equates to not setting a UID at all:

Confirmed with these tests:

package main

import (
	"bytes"
	"context"
	"fmt"

	"github.com/testcontainers/testcontainers-go"
	"github.com/testcontainers/testcontainers-go/wait"
)

func main() {
	tests := []testcontainers.ContainerRequest{
		{
			Image:      "debian:latest",
			Cmd:        []string{"sh", "-c", "id -u"},
			WaitingFor: wait.ForExit(),
		},
		{
			Image:      "debian:latest",
			User:       "60125",
			Cmd:        []string{"sh", "-c", "id -u"},
			WaitingFor: wait.ForExit(),
		},
	}

	for _, test := range tests {
		ctx := context.Background()
		c, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
			ContainerRequest: test,
			Started:          true,
		})
		if err != nil {
			panic(err)
		}
		buf := new(bytes.Buffer)
		r, _ := c.Logs(ctx)
		buf.ReadFrom(r)
		fmt.Printf("User: expected: %s; got: %s\n", test.User, buf.String())
	}
}

Output:

2021/10/05 08:17:34 Starting container id: fa62829446d6 image: quay.io/testcontainers/ryuk:0.2.3
2021/10/05 08:17:35 Waiting for container id fa62829446d6 image: quay.io/testcontainers/ryuk:0.2.3
2021/10/05 08:17:35 Container is ready id: fa62829446d6 image: quay.io/testcontainers/ryuk:0.2.3
2021/10/05 08:17:35 Starting container id: 1b75176f40da image: debian:latest
2021/10/05 08:17:35 Waiting for container id 1b75176f40da image: debian:latest
2021/10/05 08:17:35 Container is ready id: 1b75176f40da image: debian:latest
User: expected: ; got: 0

2021/10/05 08:17:35 Starting container id: 063145d38a97 image: debian:latest
2021/10/05 08:17:36 Waiting for container id 063145d38a97 image: debian:latest
2021/10/05 08:17:36 Container is ready id: 063145d38a97 image: debian:latest
User: expected: 60125; got: 60125

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding two tests in your PR: one for adding the User, and another one without it?

Besides that, we could only set the User field if req.User is not empty

Copy link
Contributor Author

@bamsammich bamsammich Oct 5, 2021 •
edited

Choose a reason for hiding this comment

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

@mdelapenya sure thing! Tests have been added.

}

// prepare mounts
Expand Down
63 changes: 62 additions & 1 deletion docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,19 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/stretchr/testify/assert"
"io/ioutil"
"math/rand"
"net/http"
"os"
"os/exec"
"path/filepath"
"regexp"
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"

"github.com/docker/docker/errdefs"

"github.com/docker/docker/api/types/volume"
Expand Down Expand Up @@ -1580,6 +1582,65 @@ func TestContainerWithReaperNetwork(t *testing.T) {
assert.NotNil(t, cnt.NetworkSettings.Networks[networks[1]])
}

func TestContainerWithUserID(t *testing.T) {
ctx := context.Background()
req := ContainerRequest{
Image: "alpine:latest",
User: "60125",
Cmd: []string{"sh", "-c", "id -u"},
WaitingFor: wait.ForExit(),
}
container, err := GenericContainer(ctx, GenericContainerRequest{
ContainerRequest: req,
Started: true,
})
if err != nil {
t.Fatal(err)
}
Comment on lines +1597 to +1599
Copy link

Choose a reason for hiding this comment

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

Nitpick: these if-statements can be replaced by calls to require.NoError(t, err) if you import github.com/stretchr/testify/require

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I used t.Fatal(err) here to match other tests for this package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you agree, would you mind sending the changes related to the require.NoError, or assert.Nil, in a follow up PR?

defer container.Terminate(ctx)

r, err := container.Logs(ctx)
if err != nil {
t.Fatal(err)
}
defer r.Close()
b, err := ioutil.ReadAll(r)
if err != nil {
t.Fatal(err)
}
actual := regexp.MustCompile(`\D+`).ReplaceAllString(string(b), "")
assert.Equal(t, req.User, actual)
}

func TestContainerWithNoUserID(t *testing.T) {
ctx := context.Background()
req := ContainerRequest{
Image: "alpine:latest",
Cmd: []string{"sh", "-c", "id -u"},
WaitingFor: wait.ForExit(),
}
container, err := GenericContainer(ctx, GenericContainerRequest{
ContainerRequest: req,
Started: true,
})
if err != nil {
t.Fatal(err)
}
defer container.Terminate(ctx)

r, err := container.Logs(ctx)
if err != nil {
t.Fatal(err)
}
defer r.Close()
b, err := ioutil.ReadAll(r)
if err != nil {
t.Fatal(err)
}
actual := regexp.MustCompile(`\D+`).ReplaceAllString(string(b), "")
assert.Equal(t, "0", actual)
}

func TestGetGatewayIP(t *testing.T) {
// When using docker-compose with DinD mode, and using host port or http wait strategy
// It's need to invoke GetGatewayIP for get the host
Expand Down