Skip to content

Conversation

@jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Mar 15, 2025

Hi,

I'm looking into setting up the discovery integration for STACKIT in prometheus/prometheus.

While the integration generally works fine, I encountered some issues injecting the http.RoundTripper from the Prometheus ecosystem into the SDK CLI.

Ref: https://github.com/jkroepke/prometheus/blob/8bec5e7cce575b18af42fe8cd67ee0fd5f47ac73/discovery/stackit/server.go#L91

It seems that each authentication method creates a new http.Client instead of simply extending the chain of RoundTripper functions.

This PR modifies the behavior so that the auth function accepts an existing http.RoundTripper and extends it, as is usual in the Go ecosystem.

Theoretically, this avoids the need for proprietary middleware approaches, as existing middleware can be injected through the Transport of a custom http.Client. That's how it usually works in the Go ecosystem.

@jkroepke jkroepke changed the title Use Transport of custom http client Accept custom transport of custom http client Mar 15, 2025
@bahkauv70
Copy link
Contributor

hello @jkroepke , thanks for your contribution! I think this would be a good idea, but the current PR breaks backwards compatibility. In the present form, all projects using the SDK would have to be adapted to the changed api. In order to go forward with this, the compatibility to existing projects would have to be maintained

@jkroepke jkroepke force-pushed the roundTripper branch 3 times, most recently from 76c5906 to 6f7f005 Compare March 17, 2025 17:00
@jkroepke
Copy link
Contributor Author

Hi @bahkauv70

the current approach should preserve the backwards compatibility.

@marceljk
Copy link
Contributor

Hi @jkroepke,

Thanks for your contribution! The PR looks good. Only a nitpick, can you add some changelogs to these two files:

https://github.com/stackitcloud/stackit-sdk-go/blob/main/core/CHANGELOG.md
https://github.com/stackitcloud/stackit-sdk-go/blob/main/CHANGELOG.md

Signed-off-by: Jan-Otto Kröpke <[email protected]>
@jkroepke
Copy link
Contributor Author

Hi,

I added some test case to increase the test coverage. At least with that tests, I feel a bit more confident that everything still works.

Signed-off-by: Jan-Otto Kröpke <[email protected]>
@marceljk
Copy link
Contributor

Hi,

there are some linter issues which you can check here in the pipeline or you can run make lint to see them local

jkroepke and others added 3 commits March 24, 2025 19:01
Copy link
Contributor

@marceljk marceljk left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution 😃

@marceljk marceljk merged commit 1e1db68 into stackitcloud:main Mar 25, 2025
8 checks passed
@jkroepke jkroepke deleted the roundTripper branch April 5, 2025 09:23
@henninge
Copy link

Hi!
Thanks for adding this. I stumbled onto this when trying to use the HTTPClient from from golang.org/x/oauth2/clientcredentials.

First I tried this which did not work (no credentials found):

client, err := dns.NewAPIClient(config.WithHTTPClient(httpClient))

My initial workaround was:

client, err := dns.NewAPIClient(config.WithHTTPClient(httpClient), config.WithCustomAuth(httpClient.Transport))

With this PR, though, using the existing transport from the httpClient is put in the NoAuth branch of execution, so I have to do this, to use is.

client, err := dns.NewAPIClient(config.WithHTTPClient(httpClient), config.WithoutAuthentication())

Is this the intended way to use this? The WithoutAuthentication looks rather counter-intuitive.

The docstring for WithHTTPClient states "Warning: providing this option overrides authentication, if you just want to customize the http client parameters except Transport ..." which makes it sound that the transport provided with the HTTPClient takes precedence over any other configuration. This is also what I would expect.

If this is still a bug and not just a misunderstanding on my part, I am happy to provide a PR.

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