Allow to customize DialContext in HTTPClientConfig #290
Conversation
Signed-off-by: Marco Pracucci <[email protected]>
| @@ -120,6 +122,9 @@ type HTTPClientConfig struct { | |||
| // The omitempty flag is not set, because it would be hidden from the | |||
| // marshalled configuration when set to false. | |||
| FollowRedirects bool `yaml:"follow_redirects"` | |||
| // DialContext allows downstream projects to customize the dialer used by the | |||
| // HTTP client to open the network connection to the server. | |||
| DialContext func(ctx context.Context, network, addr string) (net.Conn, error) `yaml:"-"` | |||
There was a problem hiding this comment.
I'm not sure if we want a config field here that doesn't parse from (or into) Yaml.
Perhaps it's better to add another parameter to NewRoundTripperFromConfig below?
I don't have a lot of context (no pun intended) on how this is used throughout the project.
There was a problem hiding this comment.
Let me give you a bit more context. We're trying to customise the dialer used by HTTP client used by Alertmanager notifiers (eg. webhook). If we move it away from the config we would have to propagate back the changes across notifiers using it so that we can customize their creation in Cortex. Having the DialContext in the config would make it transparent and reduce the change surface, but if you prefer that way I can work on it.
There was a problem hiding this comment.
Yeah, that's why we need the context how the HTTPClientConfig is used across the project. My gut feeling is that it is essentially used as a definition for how the config should look like in Yaml config files. Having a field in there that doesn't render as or parse from Yaml would go against that semantics. But my gut feeling might be wrong.
There was a problem hiding this comment.
Ok. Let's wait for @roidelapluie confirmation and then I can start working on refactoring the common HTTP client and alertmanager receiver integrations to be able to inject it everywhere.
There was a problem hiding this comment.
An alternative approach:
#291
There was a problem hiding this comment.
I am wondering what you need extra from the dialer, are we missing options in our http client? I also think we should indeed not adding this to this struct.
There was a problem hiding this comment.
We're trying to do this cortexproject/cortex#4085. Anyway, I proposed a different approach in #291.
In Cortex we have a use case where we would need to customize the dialer used by the HTTP client in the upstream Prometheus alertmanager. Unfortunately we can't do it without changing it upstream so, in this PR, I'm proposing to add a
DialContextoption toHTTPClientConfig. Is it something Prometheus is willing to accept?