Merge pull request #2199 from vincepri/manager-opts-refactor

⚠ Expose Manager.Cache/Client options, deprecate older opts

Author: k8s-ci-robot

.golangci.yml

@@ -92,7 +92,7 @@ issues:
       text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
     - linters:
         - staticcheck
-      text: "SA1019: .* The component config package has been deprecated and will be removed in a future release."
+      text: "SA1019: .*The component config package has been deprecated and will be removed in a future release."
     # With Go 1.16, the new embed directive can be used with an un-named import,
     # revive (previously, golint) only allows these to be imported in a main.go, which wouldn't work for us.
     # This directive allows the embed package to be imported with an underscore everywhere.

pkg/cache/cache.go

@@ -41,7 +41,7 @@ import (
 
 var (
 	log               = logf.RuntimeLog.WithName("object-cache")
-	defaultResyncTime = 10 * time.Hour
+	defaultSyncPeriod = 10 * time.Hour
 )
 
 // Cache knows how to load Kubernetes objects, fetch informers to request
@@ -114,11 +114,32 @@ type Options struct {
 	// Mapper is the RESTMapper to use for mapping GroupVersionKinds to Resources
 	Mapper meta.RESTMapper
 
-	// ResyncEvery is the base frequency the informers are resynced.
-	// Defaults to defaultResyncTime.
-	// A 10 percent jitter will be added to the ResyncEvery period between informers
-	// So that all informers will not send list requests simultaneously.
-	ResyncEvery *time.Duration
+	// SyncPeriod determines the minimum frequency at which watched resources are
+	// reconciled. A lower period will correct entropy more quickly, but reduce
+	// responsiveness to change if there are many watched resources. Change this
+	// value only if you know what you are doing. Defaults to 10 hours if unset.
+	// there will a 10 percent jitter between the SyncPeriod of all controllers
+	// so that all controllers will not send list requests simultaneously.
+	//
+	// This applies to all controllers.
+	//
+	// A period sync happens for two reasons:
+	// 1. To insure against a bug in the controller that causes an object to not
+	// be requeued, when it otherwise should be requeued.
+	// 2. To insure against an unknown bug in controller-runtime, or its dependencies,
+	// that causes an object to not be requeued, when it otherwise should be
+	// requeued, or to be removed from the queue, when it otherwise should not
+	// be removed.
+	//
+	// If you want
+	// 1. to insure against missed watch events, or
+	// 2. to poll services that cannot be watched,
+	// then we recommend that, instead of changing the default period, the
+	// controller requeue, with a constant duration `t`, whenever the controller
+	// is "done" with an object, and would otherwise not requeue it, i.e., we
+	// recommend the `Reconcile` function return `reconcile.Result{RequeueAfter: t}`,
+	// instead of `reconcile.Result{}`.
+	SyncPeriod *time.Duration
 
 	// Namespaces restricts the cache's ListWatch to the desired namespaces
 	// Default watches all namespaces
@@ -203,7 +224,7 @@ func New(config *rest.Config, opts Options) (Cache, error) {
 			HTTPClient:   opts.HTTPClient,
 			Scheme:       opts.Scheme,
 			Mapper:       opts.Mapper,
-			ResyncPeriod: *opts.ResyncEvery,
+			ResyncPeriod: *opts.SyncPeriod,
 			Namespace:    opts.Namespaces[0],
 			ByGVK:        byGVK,
 		}),
@@ -243,7 +264,7 @@ func (options Options) inheritFrom(inherited Options) (*Options, error) {
 	)
 	combined.Scheme = combineScheme(inherited.Scheme, options.Scheme)
 	combined.Mapper = selectMapper(inherited.Mapper, options.Mapper)
-	combined.ResyncEvery = selectResync(inherited.ResyncEvery, options.ResyncEvery)
+	combined.SyncPeriod = selectResync(inherited.SyncPeriod, options.SyncPeriod)
 	combined.Namespaces = selectNamespaces(inherited.Namespaces, options.Namespaces)
 	combined.DefaultLabelSelector = combineSelector(
 		internal.Selector{Label: inherited.DefaultLabelSelector},
@@ -416,8 +437,8 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
 	}
 
 	// Default the resync period to 10 hours if unset
-	if opts.ResyncEvery == nil {
-		opts.ResyncEvery = &defaultResyncTime
+	if opts.SyncPeriod == nil {
+		opts.SyncPeriod = &defaultSyncPeriod
 	}
 	return opts, nil
 }

pkg/cache/cache_unit_test.go

@@ -92,20 +92,20 @@ var _ = Describe("cache.inheritFrom", func() {
 	})
 	Context("Resync", func() {
 		It("is nil when specified and inherited are unset", func() {
-			Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(BeNil())
+			Expect(checkError(specified.inheritFrom(inherited)).SyncPeriod).To(BeNil())
 		})
 		It("is unchanged when only specified is set", func() {
-			specified.ResyncEvery = pointer.Duration(time.Second)
-			Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(Equal(specified.ResyncEvery))
+			specified.SyncPeriod = pointer.Duration(time.Second)
+			Expect(checkError(specified.inheritFrom(inherited)).SyncPeriod).To(Equal(specified.SyncPeriod))
 		})
 		It("is inherited when only inherited is set", func() {
-			inherited.ResyncEvery = pointer.Duration(time.Second)
-			Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(Equal(inherited.ResyncEvery))
+			inherited.SyncPeriod = pointer.Duration(time.Second)
+			Expect(checkError(specified.inheritFrom(inherited)).SyncPeriod).To(Equal(inherited.SyncPeriod))
 		})
 		It("is unchanged when both inherited and specified are set", func() {
-			specified.ResyncEvery = pointer.Duration(time.Second)
-			inherited.ResyncEvery = pointer.Duration(time.Minute)
-			Expect(checkError(specified.inheritFrom(inherited)).ResyncEvery).To(Equal(specified.ResyncEvery))
+			specified.SyncPeriod = pointer.Duration(time.Second)
+			inherited.SyncPeriod = pointer.Duration(time.Minute)
+			Expect(checkError(specified.inheritFrom(inherited)).SyncPeriod).To(Equal(specified.SyncPeriod))
 		})
 	})
 	Context("Namespace", func() {

pkg/cluster/cluster.go

@@ -83,8 +83,6 @@ type Options struct {
 	Scheme *runtime.Scheme
 
 	// MapperProvider provides the rest mapper used to map go types to Kubernetes APIs
-	//
-	// Deprecated: Set Cache.Mapper and Client.Mapper directly instead.
 	MapperProvider func(c *rest.Config, httpClient *http.Client) (meta.RESTMapper, error)
 
 	// Logger is the logger that should be used by this Cluster.
@@ -210,8 +208,8 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) {
 		if cacheOpts.HTTPClient == nil {
 			cacheOpts.HTTPClient = options.HTTPClient
 		}
-		if cacheOpts.ResyncEvery == nil {
-			cacheOpts.ResyncEvery = options.SyncPeriod
+		if cacheOpts.SyncPeriod == nil {
+			cacheOpts.SyncPeriod = options.SyncPeriod
 		}
 		if len(cacheOpts.Namespaces) == 0 && options.Namespace != "" {
 			cacheOpts.Namespaces = []string{options.Namespace}

pkg/manager/internal.go

@@ -18,7 +18,6 @@ package manager
 
 import (
 	"context"
-	"crypto/tls"
 	"errors"
 	"fmt"
 	"net"
@@ -127,17 +126,6 @@ type controllerManager struct {
 	// election was configured.
 	elected chan struct{}
 
-	// port is the port that the webhook server serves at.
-	port int
-	// host is the hostname that the webhook server binds to.
-	host string
-	// CertDir is the directory that contains the server key and certificate.
-	// if not set, webhook server would look up the server key and certificate in
-	// {TempDir}/k8s-webhook-server/serving-certs
-	certDir string
-	// tlsOpts is used to allow configuring the TLS config used for the webhook server.
-	tlsOpts []func(*tls.Config)
-
 	webhookServer *webhook.Server
 	// webhookServerOnce will be called in GetWebhookServer() to optionally initialize
 	// webhookServer if unset, and Add() it to controllerManager.
@@ -288,12 +276,7 @@ func (cm *controllerManager) GetAPIReader() client.Reader {
 func (cm *controllerManager) GetWebhookServer() *webhook.Server {
 	cm.webhookServerOnce.Do(func() {
 		if cm.webhookServer == nil {
-			cm.webhookServer = &webhook.Server{
-				Port:    cm.port,
-				Host:    cm.host,
-				CertDir: cm.certDir,
-				TLSOpts: cm.tlsOpts,
-			}
+			panic("webhook should not be nil")
 		}
 		if err := cm.Add(cm.webhookServer); err != nil {
 			panic(fmt.Sprintf("unable to add webhook server to the controller manager: %s", err))

pkg/manager/manager.go

@@ -100,11 +100,45 @@ type Options struct {
 	// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources.
 	// Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always better
 	// to pass your own scheme in. See the documentation in pkg/scheme for more information.
+	//
+	// If set, the Scheme will be used to create the default Client and Cache.
 	Scheme *runtime.Scheme
 
-	// MapperProvider provides the rest mapper used to map go types to Kubernetes APIs
+	// MapperProvider provides the rest mapper used to map go types to Kubernetes APIs.
+	//
+	// If set, the RESTMapper returned by this function is used to create the RESTMapper
+	// used by the Client and Cache.
 	MapperProvider func(c *rest.Config, httpClient *http.Client) (meta.RESTMapper, error)
 
+	// Cache is the cache.Options that will be used to create the default Cache.
+	// By default, the cache will watch and list requested objects in all namespaces.
+	Cache cache.Options
+
+	// NewCache is the function that will create the cache to be used
+	// by the manager. If not set this will use the default new cache function.
+	//
+	// When using a custom NewCache, the Cache options will be passed to the
+	// NewCache function.
+	//
+	// NOTE: LOW LEVEL PRIMITIVE!
+	// Only use a custom NewCache if you know what you are doing.
+	NewCache cache.NewCacheFunc
+
+	// Client is the client.Options that will be used to create the default Client.
+	// By default, the client will use the cache for reads and direct calls for writes.
+	Client client.Options
+
+	// NewClient is the func that creates the client to be used by the manager.
+	// If not set this will create a Client backed by a Cache for read operations
+	// and a direct Client for write operations.
+	//
+	// When using a custom NewClient, the Client options will be passed to the
+	// NewClient function.
+	//
+	// NOTE: LOW LEVEL PRIMITIVE!
+	// Only use a custom NewClient if you know what you are doing.
+	NewClient client.NewClientFunc
+
 	// SyncPeriod determines the minimum frequency at which watched resources are
 	// reconciled. A lower period will correct entropy more quickly, but reduce
 	// responsiveness to change if there are many watched resources. Change this
@@ -130,6 +164,8 @@ type Options struct {
 	// is "done" with an object, and would otherwise not requeue it, i.e., we
 	// recommend the `Reconcile` function return `reconcile.Result{RequeueAfter: t}`,
 	// instead of `reconcile.Result{}`.
+	//
+	// Deprecated: Use Cache.SyncPeriod instead.
 	SyncPeriod *time.Duration
 
 	// Logger is the logger that should be used by this manager.
@@ -215,6 +251,8 @@ type Options struct {
 	// Note: If a namespace is specified, controllers can still Watch for a
 	// cluster-scoped resource (e.g Node). For namespaced resources, the cache
 	// will only hold objects from the desired namespace.
+	//
+	// Deprecated: Use Cache.Namespaces instead.
 	Namespace string
 
 	// MetricsBindAddress is the TCP address that the controller should bind to
@@ -235,48 +273,49 @@ type Options struct {
 
 	// Port is the port that the webhook server serves at.
 	// It is used to set webhook.Server.Port if WebhookServer is not set.
+	//
+	// Deprecated: Use WebhookServer.Port instead.
 	Port int
 	// Host is the hostname that the webhook server binds to.
 	// It is used to set webhook.Server.Host if WebhookServer is not set.
+	//
+	// Deprecated: Use WebhookServer.Host instead.
 	Host string
 
 	// CertDir is the directory that contains the server key and certificate.
 	// If not set, webhook server would look up the server key and certificate in
 	// {TempDir}/k8s-webhook-server/serving-certs. The server key and certificate
 	// must be named tls.key and tls.crt, respectively.
 	// It is used to set webhook.Server.CertDir if WebhookServer is not set.
+	//
+	// Deprecated: Use WebhookServer.CertDir instead.
 	CertDir string
 
 	// TLSOpts is used to allow configuring the TLS config used for the webhook server.
+	//
+	// Deprecated: Use WebhookServer.TLSConfig instead.
 	TLSOpts []func(*tls.Config)
 
 	// WebhookServer is an externally configured webhook.Server. By default,
 	// a Manager will create a default server using Port, Host, and CertDir;
 	// if this is set, the Manager will use this server instead.
 	WebhookServer *webhook.Server
 
-	// Functions to allow for a user to customize values that will be injected.
-
-	// NewCache is the function that will create the cache to be used
-	// by the manager. If not set this will use the default new cache function.
-	NewCache cache.NewCacheFunc
-
-	// NewClient is the func that creates the client to be used by the manager.
-	// If not set this will create a Client backed by a Cache for read operations
-	// and a direct Client for write operations.
-	NewClient client.NewClientFunc
-
 	// BaseContext is the function that provides Context values to Runnables
 	// managed by the Manager. If a BaseContext function isn't provided, Runnables
 	// will receive a new Background Context instead.
 	BaseContext BaseContextFunc
 
 	// ClientDisableCacheFor tells the client that, if any cache is used, to bypass it
 	// for the given objects.
+	//
+	// Deprecated: Use Client.Cache.DisableCacheFor instead.
 	ClientDisableCacheFor []client.Object
 
 	// DryRunClient specifies whether the client should be configured to enforce
 	// dryRun mode.
+	//
+	// Deprecated: Use Client.DryRun instead.
 	DryRunClient bool
 
 	// EventBroadcaster records Events emitted by the manager and sends them to the Kubernetes API
@@ -348,12 +387,14 @@ func New(config *rest.Config, options Options) (Manager, error) {
 
 	cluster, err := cluster.New(config, func(clusterOptions *cluster.Options) {
 		clusterOptions.Scheme = options.Scheme
-		clusterOptions.MapperProvider = options.MapperProvider //nolint:staticcheck
+		clusterOptions.MapperProvider = options.MapperProvider
 		clusterOptions.Logger = options.Logger
 		clusterOptions.SyncPeriod = options.SyncPeriod
-		clusterOptions.Namespace = options.Namespace //nolint:staticcheck
 		clusterOptions.NewCache = options.NewCache
 		clusterOptions.NewClient = options.NewClient
+		clusterOptions.Cache = options.Cache
+		clusterOptions.Client = options.Client
+		clusterOptions.Namespace = options.Namespace                         //nolint:staticcheck
 		clusterOptions.ClientDisableCacheFor = options.ClientDisableCacheFor //nolint:staticcheck
 		clusterOptions.DryRunClient = options.DryRunClient                   //nolint:staticcheck
 		clusterOptions.EventBroadcaster = options.EventBroadcaster           //nolint:staticcheck
@@ -432,10 +473,6 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		controllerConfig:              options.Controller,
 		logger:                        options.Logger,
 		elected:                       make(chan struct{}),
-		port:                          options.Port,
-		host:                          options.Host,
-		certDir:                       options.CertDir,
-		tlsOpts:                       options.TLSOpts,
 		webhookServer:                 options.WebhookServer,
 		leaderElectionID:              options.LeaderElectionID,
 		leaseDuration:                 *options.LeaseDuration,
@@ -496,14 +533,19 @@ func (o Options) AndFrom(loader config.ControllerManagerConfiguration) (Options,
 	if o.Port == 0 && newObj.Webhook.Port != nil {
 		o.Port = *newObj.Webhook.Port
 	}
-
 	if o.Host == "" && newObj.Webhook.Host != "" {
 		o.Host = newObj.Webhook.Host
 	}
-
 	if o.CertDir == "" && newObj.Webhook.CertDir != "" {
 		o.CertDir = newObj.Webhook.CertDir
 	}
+	if o.WebhookServer == nil {
+		o.WebhookServer = &webhook.Server{
+			Port:    o.Port,
+			Host:    o.Host,
+			CertDir: o.CertDir,
+		}
+	}
 
 	if newObj.Controller != nil {
 		if o.Controller.CacheSyncTimeout == 0 && newObj.Controller.CacheSyncTimeout != nil {
@@ -657,5 +699,14 @@ func setOptionsDefaults(options Options) Options {
 		options.BaseContext = defaultBaseContext
 	}
 
+	if options.WebhookServer == nil {
+		options.WebhookServer = &webhook.Server{
+			Host:    options.Host,
+			Port:    options.Port,
+			CertDir: options.CertDir,
+			TLSOpts: options.TLSOpts,
+		}
+	}
+
 	return options
 }