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

⚠️ make *http.Client configurable and use/ share the same http.Client in the default configuration #2122

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions pkg/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cache
import (
"context"
"fmt"
"net/http"
"reflect"
"time"

Expand Down Expand Up @@ -108,6 +109,9 @@ type SelectorsByObject map[client.Object]ObjectSelector

// Options are the optional arguments for creating a new InformersMap object.
type Options struct {
// HTTPClient is the http client to use for the REST client
HTTPClient *http.Client

// Scheme is the scheme to use for mapping objects to GroupVersionKinds
Scheme *runtime.Scheme

Expand Down Expand Up @@ -184,6 +188,7 @@ func New(config *rest.Config, opts Options) (Cache, error) {
return &informerCache{
scheme: opts.Scheme,
Informers: internal.NewInformers(config, &internal.InformersOpts{
HTTPClient: opts.HTTPClient,
Scheme: opts.Scheme,
Mapper: opts.Mapper,
ResyncPeriod: *opts.Resync,
Expand Down Expand Up @@ -414,6 +419,18 @@ func combineTransform(inherited, current toolscache.TransformFunc) toolscache.Tr
}

func defaultOpts(config *rest.Config, opts Options) (Options, error) {
logger := log.WithName("setup")

// Use the rest HTTP client for the provided config if unset
if opts.HTTPClient == nil {
var err error
opts.HTTPClient, err = rest.HTTPClientFor(config)
if err != nil {
logger.Error(err, "Failed to get HTTP client")
inteon marked this conversation as resolved.
Show resolved Hide resolved
return opts, fmt.Errorf("could not create HTTP client from config")
inteon marked this conversation as resolved.
Show resolved Hide resolved
}
inteon marked this conversation as resolved.
Show resolved Hide resolved
}

// Use the default Kubernetes Scheme if unset
if opts.Scheme == nil {
opts.Scheme = scheme.Scheme
Expand All @@ -422,9 +439,9 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
// Construct a new Mapper if unset
if opts.Mapper == nil {
var err error
opts.Mapper, err = apiutil.NewDiscoveryRESTMapper(config)
opts.Mapper, err = apiutil.NewDiscoveryRESTMapper(config, opts.HTTPClient)
if err != nil {
log.WithName("setup").Error(err, "Failed to get API Group-Resources")
logger.Error(err, "Failed to get API Group-Resources")
return opts, fmt.Errorf("could not create RESTMapper from config")
}
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/cache/informer_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ var _ = Describe("informerCache", func() {
It("should not require LeaderElection", func() {
cfg := &rest.Config{}

mapper, err := apiutil.NewDynamicRESTMapper(cfg, apiutil.WithLazyDiscovery)
httpClient, err := rest.HTTPClientFor(cfg)
Expect(err).ToNot(HaveOccurred())
mapper, err := apiutil.NewDynamicRESTMapper(cfg, httpClient, apiutil.WithLazyDiscovery)
Expect(err).ToNot(HaveOccurred())

c, err := cache.New(cfg, cache.Options{Mapper: mapper})
Expand Down
18 changes: 12 additions & 6 deletions pkg/cache/internal/informers.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"math/rand"
"net/http"
"sync"
"time"

Expand All @@ -44,6 +45,7 @@ func init() {

// InformersOpts configures an InformerMap.
type InformersOpts struct {
HTTPClient *http.Client
Scheme *runtime.Scheme
Mapper meta.RESTMapper
ResyncPeriod time.Duration
Expand All @@ -62,9 +64,10 @@ type InformersOptsByGVK struct {
// NewInformers creates a new InformersMap that can create informers under the hood.
func NewInformers(config *rest.Config, options *InformersOpts) *Informers {
return &Informers{
config: config,
scheme: options.Scheme,
mapper: options.Mapper,
config: config,
httpClient: options.HTTPClient,
scheme: options.Scheme,
mapper: options.Mapper,
tracker: tracker{
Structured: make(map[schema.GroupVersionKind]*Cache),
Unstructured: make(map[schema.GroupVersionKind]*Cache),
Expand Down Expand Up @@ -99,6 +102,9 @@ type tracker struct {
// Informers create and caches Informers for (runtime.Object, schema.GroupVersionKind) pairs.
// It uses a standard parameter codec constructed based on the given generated Scheme.
type Informers struct {
// httpClient is used to create a new REST client
httpClient *http.Client

// scheme maps runtime.Objects to GroupVersionKinds
scheme *runtime.Scheme

Expand Down Expand Up @@ -364,7 +370,7 @@ func (ip *Informers) makeListWatcher(gvk schema.GroupVersionKind, obj runtime.Ob
// we should remove it and use the one that the dynamic client sets for us.
cfg := rest.CopyConfig(ip.config)
cfg.NegotiatedSerializer = nil
dynamicClient, err := dynamic.NewForConfig(cfg)
dynamicClient, err := dynamic.NewForConfigAndClient(cfg, ip.httpClient)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -394,7 +400,7 @@ func (ip *Informers) makeListWatcher(gvk schema.GroupVersionKind, obj runtime.Ob
cfg.NegotiatedSerializer = nil

// Grab the metadata metadataClient.
metadataClient, err := metadata.NewForConfig(cfg)
metadataClient, err := metadata.NewForConfigAndClient(cfg, ip.httpClient)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -435,7 +441,7 @@ func (ip *Informers) makeListWatcher(gvk schema.GroupVersionKind, obj runtime.Ob
// Structured.
//
default:
client, err := apiutil.RESTClientForGVK(gvk, false, ip.config, ip.codecs)
client, err := apiutil.RESTClientForGVK(gvk, false, ip.config, ip.codecs, ip.httpClient)
if err != nil {
return nil, err
}
Expand Down
16 changes: 12 additions & 4 deletions pkg/client/apiutil/apimachinery.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package apiutil
import (
"errors"
"fmt"
"net/http"
"reflect"
"sync"

Expand Down Expand Up @@ -61,9 +62,13 @@ func AddToProtobufScheme(addToScheme func(*runtime.Scheme) error) error {

// NewDiscoveryRESTMapper constructs a new RESTMapper based on discovery
// information fetched by a new client with the given config.
func NewDiscoveryRESTMapper(c *rest.Config) (meta.RESTMapper, error) {
func NewDiscoveryRESTMapper(c *rest.Config, httpClient *http.Client) (meta.RESTMapper, error) {
inteon marked this conversation as resolved.
Show resolved Hide resolved
if httpClient == nil {
inteon marked this conversation as resolved.
Show resolved Hide resolved
panic("httpClient must not be nil")
}

// Get a mapper
dc, err := discovery.NewDiscoveryClientForConfig(c)
dc, err := discovery.NewDiscoveryClientForConfigAndClient(c, httpClient)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -150,8 +155,11 @@ func GVKForObject(obj runtime.Object, scheme *runtime.Scheme) (schema.GroupVersi
// RESTClientForGVK constructs a new rest.Interface capable of accessing the resource associated
// with the given GroupVersionKind. The REST client will be configured to use the negotiated serializer from
// baseConfig, if set, otherwise a default serializer will be set.
func RESTClientForGVK(gvk schema.GroupVersionKind, isUnstructured bool, baseConfig *rest.Config, codecs serializer.CodecFactory) (rest.Interface, error) {
return rest.RESTClientFor(createRestConfig(gvk, isUnstructured, baseConfig, codecs))
func RESTClientForGVK(gvk schema.GroupVersionKind, isUnstructured bool, baseConfig *rest.Config, codecs serializer.CodecFactory, httpClient *http.Client) (rest.Interface, error) {
inteon marked this conversation as resolved.
Show resolved Hide resolved
if httpClient == nil {
panic("httpClient must not be nil")
}
return rest.RESTClientForConfigAndClient(createRestConfig(gvk, isUnstructured, baseConfig, codecs), httpClient)
}

// createRestConfig copies the base config and updates needed fields for a new rest config.
Expand Down
9 changes: 7 additions & 2 deletions pkg/client/apiutil/dynamicrestmapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package apiutil

import (
"net/http"
"sync"
"sync/atomic"

Expand Down Expand Up @@ -75,8 +76,12 @@ func WithCustomMapper(newMapper func() (meta.RESTMapper, error)) DynamicRESTMapp
// NewDynamicRESTMapper returns a dynamic RESTMapper for cfg. The dynamic
// RESTMapper dynamically discovers resource types at runtime. opts
// configure the RESTMapper.
func NewDynamicRESTMapper(cfg *rest.Config, opts ...DynamicRESTMapperOption) (meta.RESTMapper, error) {
client, err := discovery.NewDiscoveryClientForConfig(cfg)
func NewDynamicRESTMapper(cfg *rest.Config, httpClient *http.Client, opts ...DynamicRESTMapperOption) (meta.RESTMapper, error) {
inteon marked this conversation as resolved.
Show resolved Hide resolved
if httpClient == nil {
panic("httpClient must not be nil")
}

client, err := discovery.NewDiscoveryClientForConfigAndClient(cfg, httpClient)
if err != nil {
return nil, err
}
Expand Down
9 changes: 7 additions & 2 deletions pkg/client/apiutil/dynamicrestmapper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"golang.org/x/time/rate"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/rest"
)

var (
Expand All @@ -50,8 +51,10 @@ var _ = Describe("Dynamic REST Mapper", func() {
baseMapper.Add(targetGVK, meta.RESTScopeNamespace)
}

httpClient, err := rest.HTTPClientFor(cfg)
Expect(err).ToNot(HaveOccurred())
lim = rate.NewLimiter(rate.Limit(5), 5)
mapper, err = NewDynamicRESTMapper(cfg, WithLimiter(lim), WithCustomMapper(func() (meta.RESTMapper, error) {
mapper, err = NewDynamicRESTMapper(cfg, httpClient, WithLimiter(lim), WithCustomMapper(func() (meta.RESTMapper, error) {
baseMapper := meta.NewDefaultRESTMapper(nil)
addToMapper(baseMapper)

Expand Down Expand Up @@ -150,7 +153,9 @@ var _ = Describe("Dynamic REST Mapper", func() {
var err error
var failedOnce bool
mockErr := fmt.Errorf("mock failed once")
mapper, err = NewDynamicRESTMapper(cfg, WithLazyDiscovery, WithCustomMapper(func() (meta.RESTMapper, error) {
httpClient, err := rest.HTTPClientFor(cfg)
Expect(err).ToNot(HaveOccurred())
mapper, err = NewDynamicRESTMapper(cfg, httpClient, WithLazyDiscovery, WithCustomMapper(func() (meta.RESTMapper, error) {
// Make newMapper fail once
if !failedOnce {
failedOnce = true
Expand Down
26 changes: 20 additions & 6 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"fmt"
"net/http"
"strings"

"k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -38,6 +39,9 @@ import (

// Options are creation options for a Client.
type Options struct {
// HTTPClient is the HTTP client to use for requests.
HTTPClient *http.Client

// Scheme, if provided, will be used to map go structs to GroupVersionKinds
Scheme *runtime.Scheme

Expand Down Expand Up @@ -116,6 +120,15 @@ func newClient(config *rest.Config, options Options) (*client, error) {
)
}

// Use the rest HTTP client for the provided config if unset
if options.HTTPClient == nil {
var err error
options.HTTPClient, err = rest.HTTPClientFor(config)
if err != nil {
return nil, err
}
inteon marked this conversation as resolved.
Show resolved Hide resolved
}

// Init a scheme if none provided
if options.Scheme == nil {
options.Scheme = scheme.Scheme
Expand All @@ -124,23 +137,24 @@ func newClient(config *rest.Config, options Options) (*client, error) {
// Init a Mapper if none provided
if options.Mapper == nil {
var err error
options.Mapper, err = apiutil.NewDynamicRESTMapper(config)
options.Mapper, err = apiutil.NewDynamicRESTMapper(config, options.HTTPClient)
if err != nil {
return nil, err
}
}

resources := &clientRestResources{
config: config,
scheme: options.Scheme,
mapper: options.Mapper,
codecs: serializer.NewCodecFactory(options.Scheme),
httpClient: options.HTTPClient,
config: config,
scheme: options.Scheme,
mapper: options.Mapper,
codecs: serializer.NewCodecFactory(options.Scheme),

structuredResourceByType: make(map[schema.GroupVersionKind]*resourceMeta),
unstructuredResourceByType: make(map[schema.GroupVersionKind]*resourceMeta),
}

rawMetaClient, err := metadata.NewForConfig(config)
rawMetaClient, err := metadata.NewForConfigAndClient(config, options.HTTPClient)
if err != nil {
return nil, fmt.Errorf("unable to construct metadata-only client for use as part of client: %w", err)
}
Expand Down
6 changes: 5 additions & 1 deletion pkg/client/client_rest_resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package client

import (
"net/http"
"strings"
"sync"

Expand All @@ -32,6 +33,9 @@ import (

// clientRestResources creates and stores rest clients and metadata for Kubernetes types.
type clientRestResources struct {
// httpClient is the http client to use for requests
httpClient *http.Client

// config is the rest.Config to talk to an apiserver
config *rest.Config

Expand Down Expand Up @@ -59,7 +63,7 @@ func (c *clientRestResources) newResource(gvk schema.GroupVersionKind, isList, i
gvk.Kind = gvk.Kind[:len(gvk.Kind)-4]
}

client, err := apiutil.RESTClientForGVK(gvk, isUnstructured, c.config, c.codecs)
client, err := apiutil.RESTClientForGVK(gvk, isUnstructured, c.config, c.codecs, c.httpClient)
if err != nil {
return nil, err
}
Expand Down
Loading