Avoid to use the http.DefaultClient #4667
Conversation
Signed-off-by: Andres Gomez Ferrer <[email protected]>
…rr/flyte into not-use-default-http-client
dosubot
bot
added
size:S
| @@ -40,9 +38,14 @@ type Options struct { | |||
| // NewKubeClient creates a new KubeClient that caches reads and falls back to | |||
| // make API calls on failure. Write calls are not cached. | |||
| func NewKubeClient(config *rest.Config, options Options) (core.KubeClient, error) { | |||
| httpClient, err := rest.HTTPClientFor(config) | |||
There was a problem hiding this comment.
A question. We used to have three different http clients for different purposes, and now we will only have one. Do you know the implication, or do you prefer we still have three? @eapolinario
There was a problem hiding this comment.
To be clear, I don't see why we would need three different struct instances given the config is the same.
There was a problem hiding this comment.
// The Client's Transport typically has internal state (cached TCP
// connections), so Clients should be reused instead of created as
// needed. Clients are safe for concurrent use by multiple goroutines.
There was a problem hiding this comment.
I think this was the PR kubernetes-sigs/controller-runtime#2122, which carried exactly the same rationale.
Signed-off-by: Andres Gomez Ferrer <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4667 +/- ##
==========================================
- Coverage 58.12% 58.11% -0.01%
==========================================
Files 626 626
Lines 53815 53815
==========================================
- Hits 31278 31274 -4
- Misses 20035 20040 +5
+ Partials 2502 2501 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Tracking issue
Closes #4666
Why are the changes needed?
We need to avoid using the
http.DefaultClientbecause it doesn't have the proper transport configured with the TLS configurations.What changes were proposed in this pull request?
Instead of using the
http.DefaultClientto configure the read-only requests (cache) and the dynamic rest mapper. We are going to create a new client using the config to ensure that the TLS configuration is loaded.Check all the applicable boxes