Skip to content

Fixed condition, when local dns lookup should happen. #1272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+89 −2
Open

Fixed condition, when local dns lookup should happen. #1272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80182b3
Fixed condition, when local dns lookup should happen.
ulikl Jul 25, 2024
9d42375
Added tests, rebased, linted
ulikl May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion prober/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@
targetPort := targetURL.Port()

var ip *net.IPAddr
if !module.HTTP.SkipResolvePhaseWithProxy || module.HTTP.HTTPClientConfig.ProxyURL.URL == nil || module.HTTP.HTTPClientConfig.ProxyFromEnvironment {
if !module.HTTP.SkipResolvePhaseWithProxy || (module.HTTP.HTTPClientConfig.ProxyConfig.ProxyURL.URL == nil && !module.HTTP.HTTPClientConfig.ProxyConfig.ProxyFromEnvironment) {

Check failure on line 393 in prober/http.go

View workflow job for this annotation

GitHub Actions / lint 

QF1008: could remove embedded field "ProxyConfig" from selector (staticcheck)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !module.HTTP.SkipResolvePhaseWithProxy || (module.HTTP.HTTPClientConfig.ProxyConfig.ProxyURL.URL == nil && !module.HTTP.HTTPClientConfig.ProxyConfig.ProxyFromEnvironment) {
if !module.HTTP.SkipResolvePhaseWithProxy || (module.HTTP.HTTPClientConfig.ProxyURL.URL == nil && !module.HTTP.HTTPClientConfig.ProxyFromEnvironment) {

ProxyConfig can be dropped, Golang will handle it correctly without it as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this condition is getting crowded and hard to reason about. let's refactor this into something better and easy to read.

possible ways to rewrite this:

	proxyNotUsed := module.HTTP.HTTPClientConfig.ProxyConfig.ProxyURL.URL == nil || module.HTTP.HTTPClientConfig.ProxyConfig.ProxyFromEnvironment == false
	// resolve if skip_resolve_phase_with_proxy is false or proxy is not set, we need to resolve the address.
	if !module.HTTP.SkipResolvePhaseWithProxy || proxyNotUsed {
	    // do DNS resolution
	}

OR something like this, that's even more clear to read.

proxySet := module.HTTP.HTTPClientConfig.ProxyConfig.ProxyURL.URL != nil || module.HTTP.HTTPClientConfig.ProxyConfig.ProxyFromEnvironment
skipResolve := module.HTTP.SkipResolvePhaseWithProxy && proxySet

if !skipResolve {
    // do DNS resolution
}

or you can turn it into a function so we can test this logic as a unit test and ensure it works as expected.

func shouldResolveDNS(httpProbe config.HTTPProbe) bool {
    proxySet := httpProbe.HTTPClientConfig.ProxyConfig.ProxyURL.URL != nil || httpProbe.HTTPClientConfig.ProxyConfig.ProxyFromEnvironment
    return !(httpProbe.SkipResolvePhaseWithProxy && proxySet)
}

// Usage:
if shouldResolveDNS(module.HTTP) {
    // do DNS resolution
}

I personally prefer the function route because it makes it easy to write a tiny and contained table test for this logic.

var lookupTime float64
ip, lookupTime, err = chooseProtocol(ctx, module.HTTP.IPProtocol, module.HTTP.IPProtocolFallback, targetHost, registry, logger)
durationGaugeVec.WithLabelValues("resolve").Add(lookupTime)
Expand Down
89 changes: 88 additions & 1 deletion prober/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,8 @@ func TestSkipResolvePhase(t *testing.T) {
}))
defer ts.Close()

// Note: just tested, if local resolve is done or not via metrics. No proxy with diffent resolving available.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we reword or remove this comment? I am not sure what this comment is trying to say?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better?
// Note: just testing via metrics, if local DNS resolve is executed or not. We have in the test setup no proxy with different DNS resolution available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not sure what is comment is saying, maybe we can drop this and add a more local comment inside the test or drop this comment altogether.


t.Run("Without Proxy", func(t *testing.T) {
registry := prometheus.NewRegistry()
testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Expand Down Expand Up @@ -1686,7 +1688,39 @@ func TestSkipResolvePhase(t *testing.T) {

checkMetrics(expectedMetrics, mfs, t)
})
t.Run("With Proxy", func(t *testing.T) {
t.Run("With Proxy and resolve", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this test name correct? I see the SkipResolvePhaseWithProxy: false is false? same for the With Proxy and without resolve test case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see what should be wrong for "With Proxy and resolve".
SkipResolvePhaseWithProxy: false means resolve is used.
The default is with resolve.

Here the grep on the new versions for all tests with the resolve settings. They look of for me.
$ git grep -n -i resolve prober/http_test.go prober/http_test.go:1612: "resolve": {}, prober/http_test.go:1653:func TestSkipResolvePhase(t *testing.T) { prober/http_test.go:1662: // Note: just tested, if local resolve is done or not via metrics. No proxy with diffent resolving available. prober/http_test.go:1669: config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: pconfig.DefaultHTTPClientConfig, SkipResolvePhaseWithProxy: true}}, registry, promslog.NewNopLogger()) prober/http_test.go:1682: "resolve": {}, prober/http_test.go:1691: t.Run("With Proxy and resolve", func(t *testing.T) { prober/http_test.go:1704: config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: false}}, registry, promslog.NewNopLogger()) prober/http_test.go:1714: "resolve": {}, prober/http_test.go:1723: t.Run("With Proxy and without resolve", func(t *testing.T) { prober/http_test.go:1736: config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: true}}, registry, promslog.NewNopLogger()) prober/http_test.go:1754: t.Run("With Proxy from env var and without resolve", func(t *testing.T) { prober/http_test.go:1762: config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: true}}, registry, promslog.NewNopLogger()) prober/http_test.go:1780: t.Run("With Proxy from env var and with resolve", func(t *testing.T) { prober/http_test.go:1788: config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: false}}, registry, promslog.NewNopLogger()) prober/http_test.go:1798: "resolve": {},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, that double negation is making it hard to read and reason about.

let's simplify the chained OR and AND condition where negation on already negated config SkipResolvePhaseWithProxy

registry := prometheus.NewRegistry()
testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
httpCfg := pconfig.DefaultHTTPClientConfig
u, err := url.Parse("http://127.0.0.1:3128")
if err != nil {
t.Fatal(err.Error())
}
httpCfg.ProxyURL = pconfig.URL{
URL: u,
}
ProbeHTTP(testCTX, ts.URL,
config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: false}}, registry, promslog.NewNopLogger())
mfs, err := registry.Gather()
if err != nil {
t.Fatal(err)
}
expectedMetrics := map[string]map[string]map[string]struct{}{
"probe_http_duration_seconds": {
"phase": {
"connect": {},
"processing": {},
"resolve": {},
"transfer": {},
"tls": {},
},
},
}

checkMetrics(expectedMetrics, mfs, t)
})
t.Run("With Proxy and without resolve", func(t *testing.T) {
registry := prometheus.NewRegistry()
testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
Expand Down Expand Up @@ -1715,6 +1749,59 @@ func TestSkipResolvePhase(t *testing.T) {
},
}

checkMetrics(expectedMetrics, mfs, t)
})
t.Run("With Proxy from env var and without resolve", func(t *testing.T) {
registry := prometheus.NewRegistry()
testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
t.Setenv("HTTP_PROXY", "http://127.0.0.1:3128")
httpCfg := pconfig.DefaultHTTPClientConfig
httpCfg.ProxyFromEnvironment = true
ProbeHTTP(testCTX, ts.URL,
config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: true}}, registry, promslog.NewNopLogger())
mfs, err := registry.Gather()
if err != nil {
t.Fatal(err)
}
expectedMetrics := map[string]map[string]map[string]struct{}{
"probe_http_duration_seconds": {
"phase": {
"connect": {},
"processing": {},
"transfer": {},
"tls": {},
},
},
}

checkMetrics(expectedMetrics, mfs, t)
})
t.Run("With Proxy from env var and with resolve", func(t *testing.T) {
registry := prometheus.NewRegistry()
testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
t.Setenv("HTTP_PROXY", "http://127.0.0.1:3128")
httpCfg := pconfig.DefaultHTTPClientConfig
httpCfg.ProxyFromEnvironment = true
ProbeHTTP(testCTX, ts.URL,
config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, HTTPClientConfig: httpCfg, SkipResolvePhaseWithProxy: false}}, registry, promslog.NewNopLogger())
mfs, err := registry.Gather()
if err != nil {
t.Fatal(err)
}
expectedMetrics := map[string]map[string]map[string]struct{}{
"probe_http_duration_seconds": {
"phase": {
"connect": {},
"processing": {},
"resolve": {},
"transfer": {},
"tls": {},
},
},
}

checkMetrics(expectedMetrics, mfs, t)
})
}
Expand Down
Loading