Fixed condition, when local dns lookup should happen. #1272
Conversation
ulikl
force-pushed
the
fix-SkipResolvePhaseWithProxy
branch
2 times, most recently
from
0412f5a to
54ea96b
Compare
ulikl
force-pushed
the
fix-SkipResolvePhaseWithProxy
branch
from
827c1a4 to
94f8094
Compare
Bug was introduced with option proy_from_enviroment with release V0.24. Signed-off-by: Ulrike Klusik <[email protected]>
Signed-off-by: Urike Klusik <[email protected]>
ulikl
force-pushed
the
fix-SkipResolvePhaseWithProxy
branch
from
94f8094 to
9d42375
Compare
|
@electron0zero: when are you planning to build a new release, where this fix could be included? |
|
@electron0zero : Can this be merged to main? |
prober/http_test.go
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
is this test name correct? I see the SkipResolvePhaseWithProxy: false is false? same for the With Proxy and without resolve test case?
There was a problem hiding this comment.
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": {},
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Test have been compacted.
prober/http_test.go
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
can we reword or remove this comment? I am not sure what this comment is trying to say?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Test have been refactored.
I would keep test test on ProbeHttp, as these also test if all the other Proxy configurations are working.
The comment was moved. I hope it makes more sense now.
left some review comments, can you address them? |
Signed-off-by: Tamas Berki <[email protected]>
berkitamas
force-pushed
the
fix-SkipResolvePhaseWithProxy
branch
from
19ade87 to
0a9ab0a
Compare
|
@electron0zero : Can you please have another look at the refactored code? |
Signed-off-by: Tamas Berki <[email protected]>
* [FEATURE] Support matching JSON body with CEL expressions #1255 * [BUGFIX] Fix condition when local dns lookup should happen #1272 * [BUGFIX] Stop scrape logger spam #1381 Signed-off-by: SuperQ <[email protected]>
* [FEATURE] Support matching JSON body with CEL expressions #1255 * [BUGFIX] Fix condition when local dns lookup should happen #1272 * [BUGFIX] Stop scrape logger spam #1381 Signed-off-by: SuperQ <[email protected]>
Bug was introduced with option proxy_from_enviroment with release V0.24.
fixes Issue #1154