Thoughts on moving the template to its own file and embedding it into the program?

Done ... although I'm not really sure it's an improvement, now it's easier to miss changes to members of ResolvConf that are used by the template (because they're not searchable-for in the same file), and those don't show up until the template's used.

Aside from wrapping a call to rc.Generate(), this method is only tangentially related to the ResolvConf type.

I put it in the package, wrapping Generate(), for a couple of reasons ...

The old exported package functions use it - so, putting it outside the package would have meant duplicating the generate and write.

But, mostly, sandbox_dns_unix code depends on not-writing the hash file after it's been set up for the internal nameserver ... after that happens, it's never updated. (If it was, it'd break if the container was later connected to the default network.) That wasn't at all obvious, so I wanted to make it explicit by using a write method that requires a missing hash-file path - rather than just a missing update.

Perhaps the mistake was exporting Generate(). For now, I've un-exported it - but that can change if we want to make the package internal and remove the old methods.

The old exported package functions use it - so, putting it outside the package would have meant duplicating the generate and write.

So? What's the big deal writing a half-dozen lines of similar code in two places?

contents, err := rc.Generate(true)
if err != nil { return err }
if err := somepkg.WriteFile(contents, path, hashPath, 0o644); err != nil {
    return err
}

sandbox_dns_unix code depends on

This isn't sandbox_dns_unix code. It is a distinct exported package. Code that is not sandbox_dns_unix may have no need for writing a digest of a resolv.conf file to disk. It may not even want to write the generated resolv.conf to disk at all! Maybe it wants to write it into a tarball it's writing, or return it in an RPC response.

I would be much less picky about the shape of the API if this code becomes exclusively an implementation detail of sandbox_dns_unix such that we would be free to make breaking changes at any time to type ResolvConf, its methods, or related functions.

Iterative string concatenation is O(n^2). Why not make other a []string and concatenate when rendering the template?

Done.

The default is equivalent to ndots:1, so should we treat an explicit ndots:1 the same as not setting the option at all?

Any value in the host's resolv.conf acts as an override for our ndots:0. So, treating ndots:1.

sb.ndotsSet is used in the resolver here ...

... which looks like a mistake.

The comment says If the user sets ndots > 0 explicitly but the test is r.backend.NdotsSet(), which is true for ndots:0.

It's another thing for a follow-up fix.

You shouldn't have to supply a custom Comparer unless you need to customize the definition of equality to be something other than == (a.k.a. (reflect.Value).Equal)

Without it ...

    resolvconf_unix_test.go:1143: assertion failed: cannot handle unexported field at {[]resolvconf.ExtDNSEntry}[0].Addr.addr:
        	"net/netip".Addr
        consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported

Oops, my bad, of course DeepEqual can't use direct == comparison as it wouldn't be able to make a field-by-field diff without recursively comparing fields. Setting a custom comparer for ExtDNSEntry would also prevent it from doing a field-by-field diff of its fields. The documented right answer is cmpopts.EquateComparable(netip.Addr{})

Ahh - that's handy, thank you ... it's new in go-cmp 0.6.0 - even more handily, that's in master as of last week. So, I'll rebase and do that.

I was hoping that we'd get syntax highlighting if this was moved into its own file... but Go templates is not one of the supported syntaxes in https://github.com/github-linguist/linguist so we won't get highlighting on GitHub regardless of file name.

You made a good point about locality. I phrased my review comment as a question because I genuinely was unsure whether it was the right pick and wanted to start a conversation. I support you if you want to move the template back inline.

Ok, yes - I've put it back inline.

Done.

I'd really like to refactor stringly-typed IP addresses out of libnetwork. I figure we can accomplish this iteratively by writing new code to accept only pre-parsed addresses and parsing at the boundary between existing code and new code.

Good plan, done.

Is the daemon log necessary, given that a comment will be injected into the rendered resolv.conf with essentially the same message?

The log message is from the original resolvconf, and it'll still appear for FilterResolvDNS - which doesn't include comments in its output. So, I think it's best to leave it in.

https://go.dev/wiki/CodeReviewComments#error-strings

Removed the capital letters.

It's unfortunate the standard library does not have a generic slices.Map function. (Yet. See https://go.dev/issue/61898) So I whipped up #47209 for you.

var a2s = sliceutil.Mapper(netip.Addr.String)

Nice! Done.

That's useful; don't comment it out!

It was useful for grabbing the expected output when writing the test - but it'll show up in the DeepEqual output if the test fails ... done anyway.

What are you doing? The package doc comment is externally visible! Documenting implementation details makes them part of the API contract!

Oh, right. Removed.

Avoid renaming imports except to avoid a name collision. There is nothing else named resolvconf in this scope.

Ok, I thought referring to (other) resolvconf in resolvconf might be confusing. But, done.

You could apply strings.TrimSpace to result.Content to make the tests not be a change-detector for differences in leading and trailing whitespace.

Done - rejigged that test as a list of testcases.

Have resolvconf.Path() return the value of a magic environment variable if set and start a test daemon up with that env var set? Or (this might be too clever to be practical) add an env var to set a "sysroot" path which gets prepended to all hardcoded host-absolute paths which would need to be overridden in tests. I stole this idea from GCC and clang; it's part of their strategy for supporting cross-compilation without the hassle of chroots.

Obviously, the environment variable would be for integration-testing purposes only. We have prior art for using environment variables this way.

Another possible approach would be to unshare a mount namespace, bind-mount over /etc/resolv.conf and run a test daemon in that namespace. But the environment variable override idea is easier to implement and reason about.

Done.

This might be a good place to use https://pkg.go.dev/gotest.tools/v3@v3.5.1/golden instead of having to update the expected string manually whenever the template is changed.

For this single integration test, I think it's probably more fiddly to work out how to run the test with "-update" to generate the expected output than to just replace the text.

But I have now used it for the 40-ish unit tests in libnetwork/internal/resolvconf.

Done.

Done.

Even if you're avoiding gotest.tools in this file for a reason, you could still call https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff directly to be nice to whoever has to diagnose test failures.

Yikes! Things could go very wrong if an unsuspecting developer tried to run the integration suite outside of a container. If no better way is implemented, could you add a sanity check to skip this test if it's not run from inside a container?

_, err := os.Stat("/.dockerenv")
skip.If(t, err == nil, "outside a container")

Yes, it's grim - but, not my idea! It was already happening in a few tests ...

So, I'll do the env-var thing, and fix those too.

Eek, that's really scary! Thank you!

I deleted test "TestResolvConf" - as I described in now-deleted comments, it wasn't clear what it was trying to test. And, whatever it was trying to test is covered elsewhere.

I tidied up the other two by turning them into a table-driven test ... they didn't need to overwrite the host's resolv.conf or use the new env-var, they could just use libnetwork.OptionOriginResolvConfPath.

How? defaultPath is const.

resolvconf_test.go has tests which depend on global state set up by other tests. We can't have interdependent unit tests.

I don't think so - nothing's sharing the instance of the daemon that's started by the test, which does this...

	d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_RESOLV_CONF_PATH="+etchostsPath))
	d.StartWithBusybox(ctx, t, "--experimental", "--ip6tables")
	defer d.Stop(t)

Did you reply to the right comment? You quoted my review comment in a reply to this unrelated inline comment

Oh... right - I didn't notice I'd accidentally left in that comment, so your "how" question looked related to the your comment about global state.

I thought defaultPath was the state you were referring to. But, it can't affect other tests, because the daemon's started with the env var set just for this test. Which probably means I missed the point, what global state were you looking at?

(Initially I tried to read the env-var here, to initialise pathAfterSystemdDetection to whatever it said. But, I'd forgotten the path is overridden at daemon level via libnetwork options. So, I put the env-var read there instead, and failed to delete that comment. I'll do that now.)

what global state were you looking at?

golden.NormalizeCRLFToLF

(Initially I tried to read the env-var here, to initialise pathAfterSystemdDetection to whatever it said. But, I'd forgotten the path is overridden at daemon level via libnetwork options. So, I put the env-var read there instead, and failed to delete that comment. I'll do that now.)

Perfect! I was confused by the orphaned comment.