remotewrite: Use TLS ServerName as Host in all requests #5802
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change modifies HTTP requests made via remote-write to set the `Host` field to the TLS config's `ServerName`, when a TLS config is present. Otherwise, the empty default is maintained, and the Go stdlib will fall back to using the host component of the URL. This is useful to work around a somewhat-unfortunate setup we are currently dealing with, involving: * a client machine that has limited ability to resolve DNS queries (so we must point to our reverse-proxy instance via IP) * a reverse proxy that proxies based on the request's target host We can almost get this to work by passing both `-remoteWrite.url` and `-remoteWrite.tlsServerName`; however, the host will still be the host specified in the former and not the latter, which our reverse proxy mappings will not be able to handle. --- This PR as-is is likely not production-worthy: * this may not be the best place/way to get the TLS config and set the Host field * more similar instances may need to be modified across the codebase for consistency With this PR, I'm looking to get feedback on: * which behavior is the most correct/useful (Host field comes from URL vs Host field comes from TLS ServerName) * if this change is desired, then particulars on how would be appreciated!
minor-fixes
force-pushed
the
host_fix
branch
from
d454c49
to
e7adee9
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5802 +/- ##
==========================================
- Coverage 60.37% 56.48% -3.89%
==========================================
Files 411 521 +110
Lines 76609 71325 -5284
==========================================
- Hits 46253 40291 -5962
- Misses 27794 28188 +394
- Partials 2562 2846 +284 ☔ View full report in Codecov by Sentry. |
valyala
added a commit
that referenced
this pull request
Mar 6, 2024
If tlsServerName isn't empty, then it is likely the https request is sent to IP instead of hostname. In this case the request will fail, since Go automatically sets the Host header to the IP instead of the desired hostname at tlsServerName. So set the Host header to tlsServerName if itsn't empty. Updates #5802
valyala
added a commit
that referenced
this pull request
Mar 6, 2024
If tlsServerName isn't empty, then it is likely the https request is sent to IP instead of hostname. In this case the request will fail, since Go automatically sets the Host header to the IP instead of the desired hostname at tlsServerName. So set the Host header to tlsServerName if itsn't empty. Updates #5802
|
@minor-fixes , thanks for the initial pull request! I implemented it properly at the commit cb25911 . Could you build |
hardproblems
pushed a commit
to hardproblems/VictoriaMetrics
that referenced
this pull request
Mar 13, 2024
If tlsServerName isn't empty, then it is likely the https request is sent to IP instead of hostname. In this case the request will fail, since Go automatically sets the Host header to the IP instead of the desired hostname at tlsServerName. So set the Host header to tlsServerName if itsn't empty. Updates VictoriaMetrics#5802
valyala
added a commit
that referenced
this pull request
Mar 17, 2024
valyala
added a commit
that referenced
this pull request
Mar 17, 2024
|
Closing this pull request as obsolete after cb25911 |
possibull
pushed a commit
to possibull/VictoriaMetrics
that referenced
this pull request
Mar 27, 2024
If tlsServerName isn't empty, then it is likely the https request is sent to IP instead of hostname. In this case the request will fail, since Go automatically sets the Host header to the IP instead of the desired hostname at tlsServerName. So set the Host header to tlsServerName if itsn't empty. Updates VictoriaMetrics#5802
possibull
pushed a commit
to possibull/VictoriaMetrics
that referenced
this pull request
Mar 27, 2024
possibull
pushed a commit
to possibull/VictoriaMetrics
that referenced
this pull request
Mar 27, 2024
If tlsServerName isn't empty, then it is likely the https request is sent to IP instead of hostname. In this case the request will fail, since Go automatically sets the Host header to the IP instead of the desired hostname at tlsServerName. So set the Host header to tlsServerName if itsn't empty. Updates VictoriaMetrics#5802
possibull
pushed a commit
to possibull/VictoriaMetrics
that referenced
this pull request
Mar 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change modifies HTTP requests made via remote-write to set the
Hostfield to the TLS config'sServerName, when a TLS config is present. Otherwise, the empty default is maintained, and the Go stdlib will fall back to using the host component of the URL.This is useful to work around a somewhat-unfortunate setup we are currently dealing with, involving:
We can almost get this to work by passing both
-remoteWrite.urland-remoteWrite.tlsServerName; however, the host will still be the host specified in the former and not the latter, which our reverse proxy mappings will not be able to handle.With this tweak, we are able to successfully ingest metrics.
This PR as-is is likely not production-worthy:
With this PR, I'm looking to get feedback on: