Make DNS resolver configurable #1612

imiric · 2020-08-31T12:06:13Z

This introduces a new composite DNS option (--dns, K6_DNS, and dns in JS options) to allow finer-grained control of k6's DNS resolution.

The sub-options are:

ttl: the cache expiration period per hostname. Note that currently k6 doesn't respect the TTL in the DNS record, as that involves a deeper refactoring of the resolver that's currently in progress.
Possible values are:
- inf: hosts are cached indefinitely for the duration of k6 run (previous default).
- 0: the cache is disabled and lookups will be done on each new connection.
- 3000, 30s, 1m, etc.: cache DNS lookups for the specified period. If no unit is provided, milliseconds are assumed.
Default value: 5m.
select: strategy to use when selecting an IP if more than one is returned in the response.
Possible values are:
- first: return the first IP from the response (previous default).
- random: return a random IP from the response.
- roundRobin: rotate the IP returned for each new connection. The current implementation is not stable and could repeat or skip IPs, but this should be good enough in most cases.
Default value: random.
policy: IP version preference.
Possible values are:
- preferIPv4: return an IPv4 address if available, otherwise fall back to IPv6.
- preferIPv6: return an IPv6 address if available, otherwise fall back to IPv4.
- onlyIPv4: only use IPv4 addresses even if an IPv6 address is returned.
- onlyIPv6: only use IPv6 addresses even if an IPv4 address is returned.
- any: no preference, return any address (previous default).
Default value: preferIPv4.

Usage examples

CLI: k6 run --dns='ttl=0,select=roundRobin,policy=onlyIPv4' ...
Environment variable: K6_DNS='ttl=10m,select=first,policy=preferIPv6' k6 run ...

JS options:

export let options = {
  dns: {
    ttl: '1m',
    select: 'random',
    policy: 'any'
  }
}

Breaking changes

As discussed:

The default ttl if unspecified is 5m instead of the previous inf behavior.
The default select if unspecified is random instead of the previous first behavior.

Important note

These resolver settings will only be applied on new HTTP/TCP connections. Since DNS lookups are only done before the connection is established, if the test script and server enable HTTP keep-alives, the script might continue to use the established connection to the "old" host even though the DNS record had changed or the TTL expired. I.e. k6 will not actively close the connection or keep track of the TTL in the background, which it arguably shouldn't anyway.

To circumvent this the script will either have to let the connection close by itself after the idle timeout (should we make http.Transport.IdleConnTimeout configurable?), or enable the noConnectionReuse option to disable keep-alives altogether, which is not ideal...

This PR closes #726 and #738 if used correctly, but is more of a workaround than a proper fix.

lib/options.go

Resolves #1612 (comment)

imiric · 2020-09-03T09:56:58Z

@na-- @mstoykov please take a look at the latest changes.

From manual functional tests both TTL and strategy seem to work OK, but I can't resolve the config consolidation failure. From digging into it it seems that cliConf is modified by fileConf on line 232 here:

https://github.com/loadimpact/k6/blob/5ef753777c5231f4f08d8955ce7432b95db34205/cmd/config.go#L232-L236

So when it's applied again on line 236 it actually applies the modified values, not the original ones that were passed to getConsolidatedConfig().

If I'm not mistaken, to make this work we need a deep copy of the original cliConf which can then override everything else, but I didn't want to open that can of worms here as it can impact potentially everything depending on the current behavior.

Anyway, please test this manually yourselves. The easiest way I've found is changing /etc/hosts since there's no way to mock this currently (and also why the tests I mentioned in the description are useless).

imiric · 2020-09-03T10:00:24Z

lib/options.go

+type NullDNSStrategy struct {
+	DNSStrategy
+	Valid bool
+}


This is awful, but I needed a nullable type to work with the Apply() mechanism... It seems any config item needs to have this logic for the overriding to work. :-/

core/local/local_test.go

js/runner_test.go

lib/netext/dialer.go

lib/options.go

cmd/options.go

Resolves #1612 (comment)

lib/testutils/resolver.go

js/runner.go

cmd/config_consolidation_test.go

lib/netext/dialer.go

Resolves: - #1612 (comment) - #1612 (comment)

imiric · 2020-09-09T18:06:04Z

All major issues with the configuration and tests should now be resolved, and the current test failure is because of the global netext.LookupIP override I'm doing. This was the simplest way to mock the stdlib call in tests, as all other attempts I tried involved changing the netext.Resolver interface. I don't like this approach as it changes the implementation too much simply for testing purposes, while mocking Runner.Resolver is messy, so suggestions are welcome. :) A possible approach could be to pass the LookupIP function to NewResolver() or another constructor, but that could also be problematic.

And apparently TestDNSResolver is flaky, judging by the AppVeyor failure. 😞

lib/options.go

js/runner_test.go

na--

Haven't completely finished looking through the code, will mention anything else as individual comments

lib/options.go

na-- · 2020-09-10T08:20:49Z

lib/options.go

+	if string(text) == DefaultDNSConfigText {
+		*c = DefaultDNSConfig()
+		return nil
+	}


This is a very premature and unnecessary optimization 😅 And what's stopping me from reversing the order of params, strategy=first,ttl=inf? 😄

It wasn't meant as an optimization, but a way to avoid more complex logic for setting default values. I'd argue this shouldn't even be in UnmarshalText(), but it was one of those quick workarounds for when the exact default string is used, so the order didn't matter. I'll see if I can get rid of it.

Yeah, so I wasn't able to remove this easily, as it causes TestConfigConsolidation to fail 😭 Try it for yourself, comment out this block, run the test and see all the issues... (I have an intense hatred for that specific test 😆, having seen it fail so many times. It's huge and makes it difficult to parse the errors and go to the source.)

To fix it I'd need to manually handle the default values for both fields plus their Valid values, which is tricky to get right without messing up some fallback logic. So I'd chalk this one up as another TODO for #883. Let me know if you want a comment for it or have another suggestion.

lib/options.go

lib/netext/resolver.go

Resolves #1612 (comment)

Resolves... #1612 (comment)

Resolves #1612 (comment)

na--

LGTM! 🎉 I am not sure if local.TestDNSResolver wouldn't prove to be flaky in the future, but 🤞, we'll address it then

na-- · 2020-10-16T07:45:00Z

cmd/options.go

+		"for a persistent cache, '0' to disable the cache,\nor a positive duration, e.g. '1s', '1m', etc. "+
+		"Milliseconds are assumed if no unit is provided.\n"+
+		"Possible select values to return a single IP are: 'first', 'random' or 'round-robin'.\n"+
+		"Possible policy values are: 'preferIPv4', 'preferIPv6', 'onlyIPv4', 'onlyIPv6' or 'any'.\n")


Why not simply rename round-robin to roundRobin to be in line with the other options? And if we make the identifiers case-insensitive, then it'd be even better. I see the new enumer version seems to support that.

mstoykov

LGTM, apart from my worries that the change to ParseExtentedDuration will break scripts.

I haven't tested it, but will try to get to it today.

lib/netext/resolver.go

mstoykov · 2020-10-16T10:28:42Z

lib/types/types.go

@@ -95,8 +95,12 @@ func (d Duration) String() string {
 }

 // ParseExtendedDuration is a helper function that allows for string duration
-// values containing days.
+// values containing days. If no units are provided, milliseconds are assumed.


This also applies to other configurations such as:

setup/teardownTimeout

gracefulStop/rampdown

timeunit

duration

starttime
... more
I am not saying this is bad idea, I am just pointing out this is breaking change even if it's likely a welcome one
Also ... as far as I can see ... this is not used consistantly so some options will be unmarshalled from JSON to this but not from the cli flags for example min-iteration-duration will use time.ParseDuration as this is what pflag says or maybe .. not, not certain on whether there is somethign that will check for UnmarshalText before that.

But I would argue this should've been another function in this PR and have this merging be another PR in which this is more tested and looked into
cc @na--

hmm I think it's not going to be a big issue and we have to address #1305 eventually... @mstoykov keep in mind that this won't actually be used for integer JS/JSON values because of https://github.com/loadimpact/k6/blob/caf82ab98ec3b5dc581519a5269c276ca05a2306/lib/types/types.go#L154-L160
So I'm not sure how much effect this will actually have

but yeah, maybe we shouldn't make half-measures... so I'm fine if we use a separate function this time, with a TODO to switch completely to it and address #1305 in another PR

Looking in the code that you just linked ... I am even more of the opinion that this should be a separate functio now ... as this seems to make things ... worse :D

fair enough - I can't think of an example how to break it, but it'd definitely be cleaner, so 👍 from me

Hey, thanks for the heads up, I agree that it's safer to use a different function. I changed it in the recent push, along with some squashing since I had to fix the merge conflicts anyway.

Resolves #1612 (comment)

na--

LGTM 🎉

imiric requested review from mstoykov and na-- August 31, 2020 12:06

na-- reviewed Sep 2, 2020

View reviewed changes

lib/options.go Outdated Show resolved Hide resolved

imiric pushed a commit that referenced this pull request Sep 3, 2020

Implement TextUnmarshaler for DNSConfig instead of Decoder

5ef7537

Resolves #1612 (comment)

imiric requested a review from na-- September 3, 2020 09:45

imiric commented Sep 3, 2020

View reviewed changes

mstoykov reviewed Sep 4, 2020

View reviewed changes

cmd/options.go Outdated Show resolved Hide resolved

imiric pushed a commit that referenced this pull request Sep 8, 2020

Make DefaultDNSConfig a generator function

7578a41

Resolves #1612 (comment)

imiric commented Sep 8, 2020

View reviewed changes

lib/testutils/resolver.go Outdated Show resolved Hide resolved

na-- reviewed Sep 8, 2020

View reviewed changes

js/runner.go Outdated Show resolved Hide resolved

na-- reviewed Sep 8, 2020

View reviewed changes

cmd/config_consolidation_test.go Outdated Show resolved Hide resolved

na-- reviewed Sep 8, 2020

View reviewed changes

lib/netext/dialer.go Outdated Show resolved Hide resolved

imiric mentioned this pull request Sep 9, 2020

Unexpected DNS caching #726

Closed

imiric pushed a commit that referenced this pull request Sep 9, 2020

Treat unitless values as ms, add test

d0582b8

Resolves: - #1612 (comment) - #1612 (comment)

imiric force-pushed the fix/726-738-dns-cache-refresh branch from 5275d5f to f280b60 Compare September 9, 2020 17:27

imiric changed the title ~~WIP Make DNS cache configurable~~ Make DNS resolver configurable Sep 9, 2020

imiric commented Sep 9, 2020

View reviewed changes

lib/options.go Outdated Show resolved Hide resolved

imiric commented Sep 9, 2020

View reviewed changes

lib/options.go Outdated Show resolved Hide resolved

imiric commented Sep 9, 2020

View reviewed changes

js/runner_test.go Outdated Show resolved Hide resolved

na-- reviewed Sep 10, 2020

View reviewed changes

lib/netext/resolver.go Outdated Show resolved Hide resolved

imiric pushed a commit that referenced this pull request Sep 10, 2020

Simplify NewResolver, pass ttl as time.Duration

cf9bbb2

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Sep 10, 2020

Add DNSConfig.String() method

53c94b9

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Sep 10, 2020

Refactor DNSConfig.unmarshal(), return error on unknown fields

acdf402

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Sep 10, 2020

Use internal random generator for resolver

f80540f

Resolves... #1612 (comment)

imiric pushed a commit that referenced this pull request Sep 10, 2020

Unlock cache mutex before lookup, smaller lock period

6c4d8fd

Resolves #1612 (comment)

imiric force-pushed the fix/726-738-dns-cache-refresh branch from ed65b6f to 60797be Compare September 10, 2020 16:10

imiric pushed a commit that referenced this pull request Oct 15, 2020

Simplify DNSConfig.String()

401961f

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Oct 15, 2020

Remove custom DNSConfig.MarshalJSON method

54290d8

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Oct 15, 2020

Simplify resolver.applyPolicy

e048040

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Oct 15, 2020

Move DNS config types to lib/types

caf82ab

Resolves #1612 (comment)

na-- previously approved these changes Oct 16, 2020

View reviewed changes

mstoykov previously approved these changes Oct 16, 2020

View reviewed changes

Ivan Mirić added 2 commits October 19, 2020 16:16

Update outdated comment

6745390

Make DNS resolver configurable, drop dnscache dependency

5c97ddb

imiric pushed a commit that referenced this pull request Oct 19, 2020

Move DNS config types to lib/types

2343394

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Oct 19, 2020

Change DNSSelect options to camel case to align with DNSPolicy

9fb4366

Resolves #1612 (comment)

imiric pushed a commit that referenced this pull request Oct 19, 2020

Treat unitless duration values as milliseconds

37de2a1

Resolves #1612 (comment)

imiric dismissed stale reviews from mstoykov and na-- via 37de2a1 October 19, 2020 14:37

imiric force-pushed the fix/726-738-dns-cache-refresh branch from caf82ab to 37de2a1 Compare October 19, 2020 14:37

Ivan Mirić added 4 commits October 19, 2020 17:12

Add DNS policy option

b6f595c

Move DNS config types to lib/types

a0bc139

Resolves #1612 (comment)

Change DNSSelect options to camel case to align with DNSPolicy

5dbb0cb

Resolves #1612 (comment)

Treat unitless duration values as milliseconds

6cb1a23

Resolves #1612 (comment)

imiric force-pushed the fix/726-738-dns-cache-refresh branch from 37de2a1 to 6cb1a23 Compare October 19, 2020 15:20

imiric requested review from mstoykov and na-- October 19, 2020 15:25

na-- approved these changes Oct 20, 2020

View reviewed changes

mstoykov approved these changes Oct 20, 2020

View reviewed changes

imiric merged commit 0e39d45 into master Oct 20, 2020

imiric deleted the fix/726-738-dns-cache-refresh branch October 20, 2020 07:55

mstoykov mentioned this pull request Nov 20, 2020

IPv4 and IPv6 selection compatibility between --local-ips and --dns #1733

Open

imiric mentioned this pull request Dec 2, 2020

New DNS resolver #1751

Open

mstoykov mentioned this pull request Jul 6, 2021

Client Configured with IPv6 Address not able to Connect to Server(Configured with IPv6 Address) Using Hostname #2084

Closed

mstoykov mentioned this pull request Nov 18, 2021

Support multiple IPs in options.hosts #2246

Open

na-- mentioned this pull request Mar 8, 2023

Add support for reporting DNS resolution times #2961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DNS resolver configurable #1612

Make DNS resolver configurable #1612

imiric commented Aug 31, 2020 •

edited

Loading

imiric commented Sep 3, 2020

imiric Sep 3, 2020

imiric commented Sep 9, 2020 •

edited

Loading

na-- left a comment

na-- Sep 10, 2020

imiric Sep 10, 2020

imiric Sep 10, 2020

na-- left a comment

na-- Oct 16, 2020

mstoykov left a comment

mstoykov Oct 16, 2020

na-- Oct 16, 2020

na-- Oct 16, 2020

mstoykov Oct 16, 2020

na-- Oct 16, 2020

imiric Oct 19, 2020

na-- left a comment

Make DNS resolver configurable #1612

Make DNS resolver configurable #1612

Conversation

imiric commented Aug 31, 2020 • edited Loading

Usage examples

Breaking changes

Important note

imiric commented Sep 3, 2020

Choose a reason for hiding this comment

imiric commented Sep 9, 2020 • edited Loading

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

imiric commented Aug 31, 2020 •

edited

Loading

imiric commented Sep 9, 2020 •

edited

Loading