feat: TRR2 resolver #846

DecFox · 2022-07-22T04:59:05Z

Checklist

I have read the contribution guidelines
reference issue for this pull request: engine: add TRR2 resolver to measurexlite probe#2185
if you changed anything related how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request:

Description

This intoduces a TRR2 resolver to measurexlite allowing us to opportunistcally test for DoH and make use of it whenever we can. The resolver uses the system resolver (getaddrinfo) as a fallback when DoH fails

This intoduces a TRR2 resolver to measurexlite allowing us to opportunistcally test for DoH and make use of it whenever we can. The resolver uses the system resolver (getaddrinfo) as a fallback when DoH fails

bassosimone

I like the work you did so far. As I mention to you in private, we probably need to figure out some design details around this functionality, which won't cause dramatic changes, so we can discuss them in parallel with the code review proper. I highlighted a bunch of places in which I think you should tweak/refactor your code for improved robustness.

Additionally, I've seen quite a large coverage drop (-0.5%) which most likely means that we have added a bit too much code that we're not testing. I would recommend for you to check which are the new functions that we're not testing and add tests.

bassosimone · 2022-07-28T11:57:44Z

internal/measurexlite/dns.go

@@ -64,6 +64,20 @@ func (r *resolverTrace) LookupNS(ctx context.Context, domain string) ([]*net.NS,
 	return r.r.LookupNS(netxlite.ContextWithTrace(ctx, r.tx), domain)
 }

+// NewTrustedRecursiveResolver2 returns a trace-aware TRR2 resolver


Suggested change

// NewTrustedRecursiveResolver2 returns a trace-aware TRR2 resolver

// NewTrustedRecursiveResolver2 returns a trace-aware TRR2 resolver.

bassosimone · 2022-07-28T12:01:57Z

internal/measurexlite/resolver.go

+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+)
+
+// NewTrustedRecursiveResolver2 returns a new trusted recursive resolver


Suggested change

// NewTrustedRecursiveResolver2 returns a new trusted recursive resolver

// NewTrustedRecursiveResolver2 returns a new trusted recursive resolver.

You should document that logger is MANDATORY while URL is OPTIONAL and you should say what happens if you provide an empty URL. You should probably say, for future robustness, that we're going to choose a suitable URL (or you could also be specific, up to you).

bassosimone · 2022-07-28T12:02:35Z

internal/measurexlite/resolver.go

+	}
+}
+
+// TrustedRecursiveResolver2 emulates Firefox's TRR2 mode


Suggested change

// TrustedRecursiveResolver2 emulates Firefox's TRR2 mode

// TrustedRecursiveResolver2 emulates Firefox's TRR2 mode.

bassosimone · 2022-07-28T12:04:08Z

internal/measurexlite/resolver.go

+	Logger model.Logger
+	URL    string


Please, document Logger and URL (and mention they're MANDATORY)

bassosimone · 2022-07-28T12:04:29Z

internal/measurexlite/resolver.go

+// we can't return a decisive address here since we don't know if LookupHost
+// succeeds with the DoH resolver or the system resolver
+func (trr *TrustedRecursiveResolver2) Address() string {
+	return ""


How about returning the URL here?

bassosimone · 2022-07-28T12:11:23Z

internal/measurexlite/resolver.go

+
+//
+// Trusted Recursive Resolver
+//


Either call the file trr2.go and keep this comment or call this file resolver.go and say it contains resolvers

bassosimone · 2022-07-28T12:12:40Z

internal/measurexlite/resolver_test.go

+)
+
+func TestNewTrustedRecursiveResolver(t *testing.T) {
+	var called bool


Please, put suitable copies of this variable in the narrowest possible scope to avoid mistakes caused by the variable in a subtest using a value previously computed by another subtest.

(I see how in the code you only use it once, but I'd like to recommend you probably just create the resolver you need each time rather than having too much shared state for tests. The issue with shared state is that it's not robust to refactoring, so I'd rather just initialize each time the specific calls I need and leave others as nil)

bassosimone · 2022-07-28T12:21:08Z

internal/measurexlite/resolver_test.go

+		resolver := NewTrustedRecursiveResolver2(&mocks.Logger{}, "")
+		resolvert := resolver.(*TrustedRecursiveResolver2)


Suggested change

resolver := NewTrustedRecursiveResolver2(&mocks.Logger{}, "")

resolvert := resolver.(*TrustedRecursiveResolver2)

resolver := NewTrustedRecursiveResolver2(&mocks.Logger{}, "").(*TrustedRecursiveResolver2)

This should save you a variable.

You are right. Thanks!

bassosimone · 2022-07-28T12:21:47Z

internal/measurexlite/resolver_test.go

+		resolvert := resolver.(*TrustedRecursiveResolver2)
+		resolvert.NewParallelDNSOverHTTPSResolverFn = newResolver
+		ctx := context.Background()
+		t.Run("LookupHost", func(t *testing.T) {


Suggested change

t.Run("LookupHost", func(t *testing.T) {

t.Run("LookupHost", func(t *testing.T) {

Please always add spaces around t.Run invocations. This allows for easy navigation of blocks using vi's { and } commands.

bassosimone · 2022-07-28T12:25:52Z

internal/engine/probeservices/measurementmeta_test.go

@@ -52,7 +52,7 @@ func TestGetMeasurementMetaWorkingAsIntended(t *testing.T) {
 	if mmeta.Input == nil || *mmeta.Input != config.Input {
 		t.Fatal("unexpected input value")
 	}
-	if mmeta.MeasurementStartTime.String() != "2020-12-09 05:22:25 +0000 UTC" {
+	if mmeta.MeasurementStartTime.String() != "2020-12-09 04:22:25 +0000 UTC" {


I don't fully understand why this change is necessary and the commit does not seem to explain in detail why this was behaving in a flaky way. Would you mind clarifying?

There was a bug in the data pipeline (as I had informed in our 1:1). I have reverted this back since the bug has been fixed now: f71c4e2

bassosimone

Additional issues that I discovered while writing a design document for this feature

bassosimone · 2022-07-28T13:17:11Z

internal/measurexlite/resolver.go

+	if reso == nil {
+		reso = netxlite.NewParallelDNSOverHTTPSResolver(trr.Logger, trr.URL)
+	}
+	addrs, err := reso.LookupHost(ctx, hostname)


I forgot to mention: if we want to implement TRR2 correctly, this lookup should be strictly time bounded (i.e., we should timeout the lookup after N seconds have elapsed)

I have added a timeout to the DoH resolver here: 286c78f#diff-4c58fee97f07a0f47542cb08705a81d5f43f76be1d6235538ceb3e4c51725e72R33,
this is currently configured to use Firefox's default timeout value of 1.5s

bassosimone · 2022-07-28T13:18:10Z

internal/measurexlite/resolver.go

+	}
+	out, err := reso.LookupHTTPS(ctx, domain)
+	if err != nil {
+		out, err = trr.ResolverSystem.LookupHTTPS(ctx, domain)


Because the system resolver only implements LookupHost, we should consider implementing model.SimpleResolver rather than model.Resolver.

@hellais

We discussed this @hellais and it was a bug in the data pipeline which seems to be have been fixed. We can therefore revert back to the original implementation.

DecFox · 2022-08-02T06:29:42Z

I like the work you did so far. As I mention to you in private, we probably need to figure out some design details around this functionality, which won't cause dramatic changes, so we can discuss them in parallel with the code review proper. I highlighted a bunch of places in which I think you should tweak/refactor your code for improved robustness.

Additionally, I've seen quite a large coverage drop (-0.5%) which most likely means that we have added a bit too much code that we're not testing. I would recommend for you to check which are the new functions that we're not testing and add tests.

Thank you for the feedback! I have made some changes based on the review. I have also added more tests, but the coverage still seems to be decreased. So we probably need to write some more.

decfox added 3 commits July 20, 2022 15:32

feat: A TRR2 resolver

67f092e

This intoduces a TRR2 resolver to measurexlite allowing us to opportunistcally test for DoH and make use of it whenever we can. The resolver uses the system resolver (getaddrinfo) as a fallback when DoH fails

fix: flaky test, measurement StartTime

a11d3f1

fix: unrequired fmt instance removed

e5b3a93

bassosimone suggested changes Jul 28, 2022

View reviewed changes

decfox added 2 commits August 2, 2022 11:02

refix: flaky test

f71c4e2

We discussed this @hellais and it was a bug in the data pipeline which seems to be have been fixed. We can therefore revert back to the original implementation.

fix: some changes based on the review

286c78f

decfox added 3 commits August 4, 2022 13:56

feat: add simple resolver functionality in measurexlite

0a72b15

testing: add test for simple resolver

8c594c2

more tests for coverage

1360712

DecFox marked this pull request as ready for review August 8, 2022 05:37

DecFox requested a review from hellais as a code owner August 8, 2022 05:37

DecFox mentioned this pull request Sep 9, 2022

GSoC 2022: Designing the middlebox and port-filtering experiments ooni/probe#2272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: TRR2 resolver #846

feat: TRR2 resolver #846

DecFox commented Jul 22, 2022

bassosimone left a comment

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

DecFox Aug 2, 2022

bassosimone Jul 28, 2022

bassosimone Jul 28, 2022

DecFox Aug 2, 2022

bassosimone left a comment

bassosimone Jul 28, 2022

DecFox Aug 2, 2022

bassosimone Jul 28, 2022

DecFox commented Aug 2, 2022 •

edited

Loading

	// NewTrustedRecursiveResolver2 returns a trace-aware TRR2 resolver
	// NewTrustedRecursiveResolver2 returns a trace-aware TRR2 resolver.

	// TrustedRecursiveResolver2 emulates Firefox's TRR2 mode
	// TrustedRecursiveResolver2 emulates Firefox's TRR2 mode.

		resolver := NewTrustedRecursiveResolver2(&mocks.Logger{}, "")
		resolvert := resolver.(*TrustedRecursiveResolver2)

	t.Run("LookupHost", func(t *testing.T) {

	t.Run("LookupHost", func(t *testing.T) {

feat: TRR2 resolver #846

Are you sure you want to change the base?

feat: TRR2 resolver #846

Conversation

DecFox commented Jul 22, 2022

Checklist

Description

bassosimone 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

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

Choose a reason for hiding this comment

bassosimone 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

DecFox commented Aug 2, 2022 • edited Loading

DecFox commented Aug 2, 2022 •

edited

Loading