Add testutil daemon.WithResolvConf by robmry · Pull Request #49132 · moby/moby

robmry · 2024-12-19T10:08:01Z

- What I did

Add daemon.WithResolvConf to tidy up tests that were setting env var DOCKER_TEST_RESOLV_CONF_PATH.

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2024-12-19T12:21:38Z

+	d := daemon.New(t,
+		daemon.WithResolvConf(network.WriteTempResolvConf(t, "127.0.0.1")),
+		daemon.WithExperimental(),
+	)


Heh, this now makes it really tempting to combine those (not sure if we should), as it looks like WriteTempResolvConf is always used in combination with daemon.WithResolvConf

daemon.WithResolver(t, "127.0.0.1")

There is some special handling in that utility for permissions;

moby/integration/internal/network/dns.go

Lines 20 to 24 in f95e4b3

// Not using t.TempDir() here because in rootless mode, while the temporary

// directory gets mode 0777, it's a subdir of an 0700 directory owned by root.

// So, it's not accessible by the daemon.

f, err := os.CreateTemp("", "resolv.conf")

assert.NilError(t, err)

I wonder if (as we're passing the Daemon struct to these options) if putting it somewhere inside the daemon's storage location would work for that; in that case, we'd keep all things related to that daemon instance together;

moby/testutil/daemon/daemon.go

Lines 200 to 204 in f95e4b3

dest := os.Getenv("DOCKER_INTEGRATION_DAEMON_DEST")

if dest == "" {

dest = os.Getenv("DEST")

}

dest = filepath.Join(dest, t.Name())

moby/testutil/daemon/daemon.go

Lines 106 to 122 in f95e4b3

func NewDaemon(workingDir string, ops ...Option) (*Daemon, error) {

storageDriver := os.Getenv("DOCKER_GRAPHDRIVER")

if err := os.MkdirAll(SockRoot, 0o700); err != nil {

return nil, errors.Wrapf(err, "failed to create daemon socket root %q", SockRoot)

}

id := "d" + stringid.TruncateID(stringid.GenerateRandomID())

dir := filepath.Join(workingDir, id)

daemonFolder, err := filepath.Abs(dir)

if err != nil {

return nil, err

}

daemonRoot := filepath.Join(daemonFolder, "root")

if err := os.MkdirAll(daemonRoot, 0o755); err != nil {

return nil, errors.Wrapf(err, "failed to create daemon root %q", daemonRoot)

}

Heh, this now makes it really tempting to combine those (not sure if we should), as it looks like WriteTempResolvConf is always used in combination with daemon.WithResolvConf

That'd take away the possibility of giving the daemon a more complicated resolv.conf (or a deliberately broken one).

I thought about having StartDaftDNS write its own resolv.conf, returning the filename. Then, in four of the five cases, it'd be ... daemon.WithResolvConf(network.StartDaftDNS(t, "127.0.0.1")). (In the other case, no dummy resolver gets started, the test just needs its own resolv.conf.)

But, same thing there really - it'd limit the resolv.conf, without buying much.

I wonder if (as we're passing the Daemon struct to these options) if putting it somewhere inside the daemon's storage location would work for that; in that case, we'd keep all things related to that daemon instance together;

I suppose WithResolvConf could take a []byte to write to a resolv.conf in the daemon's Folder, instead of the location of a file (so the file doesn't need to be created before the NewDaemon has created Folder). I'll try that...

I suppose WithResolvConf could take a []byte to write to a resolv.conf in the daemon's Folder, instead of the location of a file (so the file doesn't need to be created before the NewDaemon has created Folder). I'll try that...

Oh, that might work yes!

And, yes, I was also considering "would we need more complex ones?" so yeah, I can agree.

It's mostly brainstorming a bit here, knowing that this is not a critical/urgent PR, so just looking what shape it could take to make it clean and flexible enough 😄 ❤️

I've had a go at that ... I think it's better.

robmry · 2024-12-19T12:22:07Z

Unrelated Windows test failures ...

=== FAIL: github.com/docker/docker/integration/container TestWaitRestartedContainer/not-running (60.38s)
    wait_test.go:205: assertion failed: error is not nil: error during connect: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.48/containers/create": EOF

=== FAIL: github.com/docker/docker/integration/build TestBuildWithRemoveAndForceRemove/successful_build_with_no_removal (70.35s)
    build_test.go:114: assertion failed: error is not nil: unexpected EOF

thaJeztah

LGTM - left one more suggestion in case you think it makes sense (but not a blocker)

thaJeztah · 2025-01-03T09:34:14Z


-	tmpFileName := net.WriteTempResolvConf(t, "127.0.0.1")
-	d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_RESOLV_CONF_PATH="+tmpFileName))
+	d := daemon.New(t, daemon.WithResolvConf(net.GenResolvConf(t, "127.0.0.1")))


Heh, very unrelated, but we should probably avoid using net as alias, as it would be clashing with Go's net package;

net "github.com/docker/docker/integration/internal/network"

thaJeztah · 2025-01-03T09:37:21Z

+func WithResolvConf(content []byte) Option {
+	return func(d *Daemon) {
+		d.resolvConfContent = content


Not a blocker; wondering if string as arguments (and converting to []byte internally) would make these utilities easier to use. Or at least, I often find myself creating a string, and then having to convert it to []byte, because strings tend to be easier to produce 😅

Signed-off-by: Rob Murray <[email protected]>

thaJeztah

LGTM!

robmry added area/networking Networking area/testing kind/refactor PR's that refactor, or clean-up code area/networking/dns Networking labels Dec 19, 2024

robmry added this to the 28.0.0 milestone Dec 19, 2024

robmry self-assigned this Dec 19, 2024

thaJeztah added the status/2-code-review label Dec 19, 2024

thaJeztah reviewed Dec 19, 2024

View reviewed changes

robmry marked this pull request as ready for review December 19, 2024 12:22

robmry force-pushed the withresolvconf branch from 2195841 to 138b2fb Compare January 2, 2025 19:04

akerouanton approved these changes Jan 3, 2025

View reviewed changes

thaJeztah approved these changes Jan 3, 2025

View reviewed changes

Add testutil daemon.WithResolvConf

a079f62

Signed-off-by: Rob Murray <[email protected]>

robmry force-pushed the withresolvconf branch from 138b2fb to a079f62 Compare January 3, 2025 12:42

thaJeztah approved these changes Jan 3, 2025

View reviewed changes

thaJeztah merged commit 0cd495c into moby:master Jan 3, 2025

robmry deleted the withresolvconf branch January 28, 2025 11:48

thaJeztah mentioned this pull request Oct 6, 2025

flaky test: TestBuildWithRemoveAndForceRemove (on Windows) #51112

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testutil daemon.WithResolvConf#49132

Add testutil daemon.WithResolvConf#49132
thaJeztah merged 1 commit intomoby:masterfrom
robmry:withresolvconf

robmry commented Dec 19, 2024

Uh oh!

thaJeztah Dec 19, 2024

Uh oh!

robmry Dec 19, 2024

Uh oh!

thaJeztah Dec 19, 2024

Uh oh!

robmry Jan 2, 2025

Uh oh!

robmry commented Dec 19, 2024

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Jan 3, 2025

Uh oh!

thaJeztah Jan 3, 2025

Uh oh!

robmry Jan 3, 2025

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// Not using t.TempDir() here because in rootless mode, while the temporary
	// directory gets mode 0777, it's a subdir of an 0700 directory owned by root.
	// So, it's not accessible by the daemon.
	f, err := os.CreateTemp("", "resolv.conf")
	assert.NilError(t, err)

	dest := os.Getenv("DOCKER_INTEGRATION_DAEMON_DEST")
	if dest == "" {
	dest = os.Getenv("DEST")
	}
	dest = filepath.Join(dest, t.Name())

	func NewDaemon(workingDir string, ops ...Option) (*Daemon, error) {
	storageDriver := os.Getenv("DOCKER_GRAPHDRIVER")

	if err := os.MkdirAll(SockRoot, 0o700); err != nil {
	return nil, errors.Wrapf(err, "failed to create daemon socket root %q", SockRoot)
	}

	id := "d" + stringid.TruncateID(stringid.GenerateRandomID())
	dir := filepath.Join(workingDir, id)
	daemonFolder, err := filepath.Abs(dir)
	if err != nil {
	return nil, err
	}
	daemonRoot := filepath.Join(daemonFolder, "root")
	if err := os.MkdirAll(daemonRoot, 0o755); err != nil {
	return nil, errors.Wrapf(err, "failed to create daemon root %q", daemonRoot)
	}

Conversation

robmry commented Dec 19, 2024

Uh oh!

thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

robmry Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

robmry Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

robmry commented Dec 19, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

robmry Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants