Enable setting DNS options through global nerdctl config #4378
Conversation
|
Hey @swagatbora90 Noting that: Different behaviors on this ^. Appreciate your PR is not about that, but maybe, if you wish, this PR could fix that discrepancy as well since it is touching that part? |
| nerdtest.Setup() | ||
|
|
||
| testCase := &test.Case{ | ||
| Env: map[string]string{"NERDCTL_TOML": tomlPath}, |
There was a problem hiding this comment.
Tigron has a helper for that:
Just add this to your testCase instead of the Env:
Config: test.WithConfig(nerdtest.NerdctlToml, configContent),
(also takes care of writing the file in a temp location for you, and make it easier to retrieve)
| { | ||
| Description: "Global DNS settings are used when command line options are not provided", | ||
| Command: func(data test.Data, helpers test.Helpers) test.TestableCommand { | ||
| nerdctlTomlPath := os.Getenv("NERDCTL_TOML") |
There was a problem hiding this comment.
Is this going to work as you think?
os.Getenv points to global env, not the test / commands env.
If you use the config from above ^^^
helpers.Read(nerdtest.NerdctlToml)
Should give you what you want?
There was a problem hiding this comment.
If I read it correctly, the Env var should be passed to all Subtest. I need all the Subtest to use the global env, so it does work.
However, your suggestion to use Config should work too. Let me change it like you suggested.
There was a problem hiding this comment.
Yes, testCase.Env is propagated to subtests, but this is not the global os.Environ (otherwise, you would pollute other unrelated tests), so os.Getenv will not give you the test environment...
There was a problem hiding this comment.
Not completely sure I was clear, so, in a shell:
- do not rely on os.Getenv or os.Setenv - that will cross pollute other tests and is specifically frowned-upon
- the testCase Env is private to the test, but it is inherited by subtests
- every test has a custom nerdctl.toml, regardless of the fact you want one explicitly or not (this is to guarantee that we are isolated from a possible, pre-existing, host
nerdctl.toml)
There was a problem hiding this comment.
Okay, I see your point. I did check that the NERDCTL_TOML config values are correct when running the test. Anyways, I have changed to use the Config attribute like you suggested.
| cmd := helpers.Command("run", "--rm", testutil.CommonImage, "cat", "/etc/resolv.conf") | ||
| return cmd | ||
| }, | ||
| Expected: func(data test.Data, helpers test.Helpers) *test.Expected { |
There was a problem hiding this comment.
More compact, using the test.Expects helper and comparators instead of naked asserts (better debugging info):
Expected: test.Expects(expect.ExitCodeSuccess, nil, expect.All(
expect.Contains("foo"),
expect.Contains("bla"),
)),
|
@swagatbora90 left a few comments on tests. Tigron testing has some (semblance of) documentation here: https://github.com/containerd/nerdctl/blob/main/docs/testing/tools.md Very much work in progress, so, it is what it is... but then, am always happy to cosplay "testing-helper-AI-bot" until the doc and API are settled :-) (also, feedback on Tigron is always welcome) |
Looks like there is no validation today on the DNS servers and domain names that are being passed. I can add a follow up PR to add validation, I would prefer to keep this PR only for adding the global config. |
Fair. |
swagatbora90
force-pushed
the
add-global-dns-config
branch
2 times, most recently
from
0c8bade to
7e07e0d
Compare
Signed-off-by: Swagat Bora <sbora@amazon.com>
swagatbora90
force-pushed
the
add-global-dns-config
branch
from
c5bba54 to
1028d42
Compare
This PR adds support for configuring DNS settings globally through the nerdctl configuration file (nerdctl.toml), allowing users to set default DNS servers, search domains, and options that apply to all containers unless explicitly overridden by command-line flags.
This is for a usecase where I want all containers to use a specific dns server instead of system default. Currently, I have to use
--dnsflag with every container create/run commands. This functionality is available in docker which allows setting global dns config value in the daemon config file.Testing
Added a end to end test TestDNSWithGlobalConfig() that checks 4 scenarios: