Skip to content
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.

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

Expose HTTPHost property in HTTPClientConfig #645

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Jun 3, 2024

While I was integrate that feature in grafana/alloy#698, it figure out that the Host property was removed in #597. Turns out that I forget something.

Since host is problematic with DockerSD, i choice the term http_host to follow the pattern from http_headers

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Turns out that I forget something.

Please explain this further.

@@ -319,6 +319,9 @@ type HTTPClientConfig struct {
// The omitempty flag is not set, because it would be hidden from the
// marshalled configuration when set to false.
EnableHTTP2 bool `yaml:"enable_http2" json:"enable_http2"`
// HTTPHost optionally overrides the Host header to send.
// If empty, the host from the URL is used.
Copy link
Member

Choose a reason for hiding this comment

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

How does this reconcile with the code that says:

rt = NewHostRoundTripper(opts.host, rt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If empty, the host from the URL is used.

This is go default behavior. It's similar the the TLSServerName property.

If the property is not set by caller, then the value will be used from URL.

Ref: https://github.com/golang/go/blob/959b3fd4265d7e4efb18af454cd18799ed70b8fe/src/net/http/request.go#L240-L243

Copy link
Member

Choose a reason for hiding this comment

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

Would it not be more correct to mention opts.host ?

@bboreham
Copy link
Member

In #549 you added Host to HTTPClientConfig and host to httpClientOptions.

Since only the latter was used, I removed the former. Now you adding it back, and hopefully using it.
Do we need both? Why?

@jkroepke
Copy link
Contributor Author

Hey @bboreham,

I only need the property in HTTPClientConfig within the context of Grafana Alloy.

I have to apologize. When I started with Golang a year ago, I implemented the wrong side due to my lack of knowledge and missing tooling. I just replicated the userAgent logic, which misled me.

Looking at it now, I realize that the host property in httpClientOptions is incorrect.

If you agree, I can revert the host in httpClientOptions.

@bboreham
Copy link
Member

Thanks for the explanation. It's fine, everybody makes mistakes. Just easier for me to follow if I can see the path.

However we may prefer to deprecate the un-needed one rather than removing it immediately.
That way we can see if anyone else came to depend on it.

@KarstenSiemer
Copy link

Hi! Anything missing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants