Add force preferred ip protocol #382

laghoule · 2018-11-14T19:33:54Z

In some case we may want to force a preferred protocol.
I'v added an option (false by default) for that purpose:
[ force_preferred_ip_protocol: <boolean | default = false> ]

SuperQ · 2018-11-17T03:18:50Z

What about calling this feature ip_protocol? IMO "forced" should be implied, as this is how most utilities do it. (IE curl -(4|6))

laghoule · 2018-11-19T01:22:12Z

preferred_ip_protocol is already existing in blackbox_exporter. The current default is ip6 and fallback to ip4 if there's no IPv6 available.

My patch make it possible to disable the fallback, and force the prefered ip protocol.

SuperQ · 2018-11-20T12:37:20Z

@laghoule Yes, I understand. What I'm suggesting is we simplify the naming so that we have two options. preferred_ip_protocol, and ip_protocol. In the new option "Forced" is implied, as this is the standard behavior expected by most tools when setting a protocol.

laghoule · 2018-11-20T14:15:32Z

@SuperQ sorry for the misunderstanding. I agree with you. I will make the modification.
Thanks for the feedback.

CONFIGURATION.md

SuperQ · 2018-11-21T05:48:00Z

I would like to maintain the original behavior, which is to "preferred" mode by default.

Also, with the switch to a bool, preferred doesn't make sense. what about fallback_ip_protocol with a default of true?

laghoule · 2018-11-22T17:33:04Z

Yeah make sense. I will do that.

Signed-off-by: Pascal Gauthier <[email protected]> Signed-off-by: Pascal Gauthier <[email protected]>

Signed-off-by: Pascal Gauthier <[email protected]>

This reverts commit 964bd87. Signed-off-by: Pascal Gauthier <[email protected]>

This reverts commit df88900. Signed-off-by: Pascal Gauthier <[email protected]>

Signed-off-by: Pascal Gauthier <[email protected]>

SuperQ · 2018-11-23T08:35:44Z

Oh, I think there was a misunderstanding. I want to keep ip_protocol, and drop preferred_ip_protocol entirely. This way the exporter will behave more like standard tools, but have fallback by default.

Signed-off-by: Pascal Gauthier <[email protected]>

laghoule · 2018-11-23T15:03:42Z

@SuperQ sorry, I was doing the change after work, my brain was not fully operational ;)
Should be good now.

SuperQ

LGTM

brian-brazil · 2018-11-23T18:15:45Z

prober/utils.go

-		level.Warn(logger).Log("msg", "Resolution with preferred IP protocol failed, attempting fallback protocol", "fallback_protocol", fallbackProtocol, "err", err)
-		ip, err = net.ResolveIPAddr(fallbackProtocol, target)
+		if fallbackIPProtocol == false {
+			level.Error(logger).Log("msg", "Resolution with IP protocol failed (fallback IP protocol is FALSE): err", err)


fallback_ip_protocol would be easier for users to find, and the FALSE could be false.

CONFIGURATION.md

brian-brazil · 2018-11-23T21:02:07Z

The default behavior is the same though.

…

On Fri 23 Nov 2018, 19:44 Ben Kochie ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In CONFIGURATION.md <#382 (comment)> : > @@ -84,8 +84,9 @@ The other placeholders are specified separately. # HTTP proxy server to use to connect to the targets. [ proxy_url: <string> ] - # The preferred IP protocol of the HTTP probe (ip4, ip6). - [ preferred_ip_protocol: <string> | default = "ip6" ] + # The IP protocol of the HTTP probe (ip4, ip6). + [ ip_protocol: <string> | default = "ip6" ] I wanted to make it obvious that this is a behavior change. But I like the idea of a deprecation warning. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#382 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGyTdkcUO8guGnYX1ROn_ylVq-ODmgciks5uyEIAgaJpZM4YebpL> .

SuperQ · 2018-11-24T08:30:35Z

@brian-brazil Yea, works for me. I think adding preferred_ip_protocol as a config alias with a deprecation warning is a good option.

SuperQ · 2018-11-28T14:38:39Z

@laghoule, do you want to implement the preferred_ip_protocol as an alias for ip_protocol, with a warning that it's deprecated? I think that's the only thing we need for merging.

laghoule · 2018-11-28T14:40:29Z

@SuperQ, yes I will do it. I hope to have some free time this week to work on this.

Signed-off-by: Pascal Gauthier <[email protected]>

brian-brazil · 2018-12-04T16:39:12Z

CONFIGURATION.md

-  # The preferred IP protocol of the HTTP probe (ip4, ip6).
-  [ preferred_ip_protocol: <string> | default = "ip6" ]
+  # The IP protocol of the HTTP probe (ip4, ip6).
+  [ preferred_ip_protocol: <string> | default = "ip6" *DEPRECATED* ]


I don't see why we're deprecating this just to change its name, this would cause unnecessary work for all our existing users.

I never liked the name or behavior of this code. With the new code, it's nolonger exactly "preferred", it's just specifying a specific protocol, and having a fallback or not.

I do not want to go in circles on this review.

It's still preferred, and we've already broken this config option before. If we're going to break it again I want to to be due a complete change in how it works, not a minor name change.

It is not, the new behavior does not represent preferred anymore. With preferred, fallback was always an option. Now fallback is optional, and this necessitates a name change.

I don't think that adding a new option necessitates breaking existing users.

brian-brazil · 2018-12-04T16:39:38Z

CONFIGURATION.md

+  # The IP protocol of the HTTP probe (ip4, ip6).
+  [ preferred_ip_protocol: <string> | default = "ip6" *DEPRECATED* ]
+  [ ip_protocol: <string> | default = "ip6" ]
+  [ fallback_ip_protocol: <boolean | default = true> ]


ip_protocol_fallback would make it clearer that this is a bool, rather than a string.

Sorry, posted comment to the wrong spot.

For this one, I agree, ip_protocol_fallback does parse a little better in my head.

What do you think @laghoule?

I agree, it's better visually and conceptually.

Signed-off-by: Pascal Gauthier <[email protected]>

laghoule · 2018-12-07T14:24:17Z

@brian-brazil @SuperQ Let me know what you want to do about the breaking changes / deprecation issues.

brian-brazil · 2018-12-07T14:29:07Z

As I said I don't see the point of causing disruption for our existing users for no benefit to them.

laghoule · 2018-12-07T14:31:33Z

Ok, I will adjust the behavior.

laghoule · 2018-12-07T14:35:57Z

Is it OK to keep IPProtocol var, but use the preferred_ip_protocol yaml config?

IPProtocol             string                  `yaml:"preferred_ip_protocol,omitempty"`

brian-brazil · 2018-12-07T14:41:35Z

That's fine.

Signed-off-by: Pascal Gauthier <[email protected]>

brian-brazil · 2018-12-07T15:20:42Z

Thanks!

brian-brazil · 2019-02-25T14:16:20Z

I was just testing this for release, and the default isn't true as the docs say.

laghoule · 2019-02-25T18:00:15Z

I will take a look at it as soon as possible.

Daniel15 · 2019-03-12T06:02:28Z

Is there somewhere I can download an alpha build containing this change, or would I need to build blackbox_exporter myself?

laghoule · 2019-03-12T11:27:24Z

I will have the merge request this week. I will post it here. Pascal Gauthier

…

On Tue., Mar. 12, 2019, 02:02 Daniel Lo Nigro, ***@***.***> wrote: Is there somewhere I can download an alpha build containing this change, or would I need to build blackbox_exporter myself? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#382 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhiZ0DKMiW9zgUyX3gQGWJMAgf0B1XSks5vV0L8gaJpZM4YebpL> .

brian-brazil · 2019-03-12T11:34:58Z

Great!

laghoule · 2019-03-12T19:28:26Z

This MR should fix the issue: #436
Give it a try / review / comments.

SuperQ reviewed Nov 21, 2018

View reviewed changes

CONFIGURATION.md Outdated Show resolved Hide resolved

laghoule and others added 9 commits November 23, 2018 02:08

Add support for forcing PreferredIPProtocol

1a64378

Signed-off-by: Pascal Gauthier <[email protected]> Signed-off-by: Pascal Gauthier <[email protected]>

Add force_preferred_ip_protocol for http_2xx_example

5ae9cb7

Signed-off-by: Pascal Gauthier <[email protected]> Signed-off-by: Pascal Gauthier <[email protected]>

Change to ip_protocol and preferred_ip_protocol

ebd02f1

Signed-off-by: Pascal Gauthier <[email protected]>

Fix style

c5c37d0

Signed-off-by: Pascal Gauthier <[email protected]>

Revert "Fix style"

500213e

This reverts commit 964bd87. Signed-off-by: Pascal Gauthier <[email protected]>

Revert "Change to ip_protocol and preferred_ip_protocol"

f93e357

This reverts commit df88900. Signed-off-by: Pascal Gauthier <[email protected]>

fallback_ip_protocol

6bb57d8

Signed-off-by: Pascal Gauthier <[email protected]>

Fix style

a7bd9fd

Signed-off-by: Pascal Gauthier <[email protected]>

Fix tests

fb13f15

Signed-off-by: Pascal Gauthier <[email protected]>

preferredIPProtocol -> IPProtocol

9d40c97

Signed-off-by: Pascal Gauthier <[email protected]>

SuperQ approved these changes Nov 23, 2018

View reviewed changes

brian-brazil reviewed Nov 23, 2018

View reviewed changes

laghoule added 2 commits December 4, 2018 10:47

Add a warning if user use preferred_ip_protocol instead of ip_protocol

dea9406

Signed-off-by: Pascal Gauthier <[email protected]>

Fix gofmt error

fa7504e

Signed-off-by: Pascal Gauthier <[email protected]>

brian-brazil reviewed Dec 4, 2018

View reviewed changes

FallbackIPProtocol to IPProtocolFallback

14d89bc

Signed-off-by: Pascal Gauthier <[email protected]>

Don't break user configuration

87bc5f7

Signed-off-by: Pascal Gauthier <[email protected]>

brian-brazil merged commit a75399e into prometheus:master Dec 7, 2018

jacksontj mentioned this pull request Feb 21, 2019

Redirect to different HTTPS host broken #237

Closed

brian-brazil mentioned this pull request Mar 11, 2019

Add regexp matching of HTTP response headers to the http probe #419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add force preferred ip protocol #382

Add force preferred ip protocol #382

laghoule commented Nov 14, 2018

SuperQ commented Nov 17, 2018

laghoule commented Nov 19, 2018

SuperQ commented Nov 20, 2018

laghoule commented Nov 20, 2018

SuperQ commented Nov 21, 2018

laghoule commented Nov 22, 2018

SuperQ commented Nov 23, 2018

laghoule commented Nov 23, 2018

SuperQ left a comment

brian-brazil Nov 23, 2018

brian-brazil commented Nov 23, 2018 via email

SuperQ commented Nov 24, 2018

SuperQ commented Nov 28, 2018

laghoule commented Nov 28, 2018

brian-brazil Dec 4, 2018

SuperQ Dec 4, 2018

brian-brazil Dec 4, 2018

SuperQ Dec 4, 2018

brian-brazil Dec 4, 2018

brian-brazil Dec 4, 2018

SuperQ Dec 4, 2018 •

edited

Loading

laghoule Dec 4, 2018

laghoule commented Dec 7, 2018

brian-brazil commented Dec 7, 2018

laghoule commented Dec 7, 2018

laghoule commented Dec 7, 2018

brian-brazil commented Dec 7, 2018

brian-brazil commented Dec 7, 2018

brian-brazil commented Feb 25, 2019

laghoule commented Feb 25, 2019

Daniel15 commented Mar 12, 2019

laghoule commented Mar 12, 2019 via email

brian-brazil commented Mar 12, 2019

laghoule commented Mar 12, 2019

Add force preferred ip protocol #382

Add force preferred ip protocol #382

Conversation

laghoule commented Nov 14, 2018

SuperQ commented Nov 17, 2018

laghoule commented Nov 19, 2018

SuperQ commented Nov 20, 2018

laghoule commented Nov 20, 2018

SuperQ commented Nov 21, 2018

laghoule commented Nov 22, 2018

SuperQ commented Nov 23, 2018

laghoule commented Nov 23, 2018

SuperQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil commented Nov 23, 2018 via email

SuperQ commented Nov 24, 2018

SuperQ commented Nov 28, 2018

laghoule commented Nov 28, 2018

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

SuperQ Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laghoule commented Dec 7, 2018

brian-brazil commented Dec 7, 2018

laghoule commented Dec 7, 2018

laghoule commented Dec 7, 2018

brian-brazil commented Dec 7, 2018

brian-brazil commented Dec 7, 2018

brian-brazil commented Feb 25, 2019

laghoule commented Feb 25, 2019

Daniel15 commented Mar 12, 2019

laghoule commented Mar 12, 2019 via email

brian-brazil commented Mar 12, 2019

laghoule commented Mar 12, 2019

SuperQ Dec 4, 2018 •

edited

Loading