From eb78ee6a0d15b46edd92814b3af34feb6dd04761 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Wed, 15 Feb 2023 08:55:10 -0800 Subject: [PATCH] Expose Manager.Cache/Client options, deprecate older opts Signed-off-by: Vince Prignano --- .golangci.yml | 2 +- pkg/cache/cache.go | 41 +++++++++---- pkg/cache/cache_unit_test.go | 16 ++--- pkg/cluster/cluster.go | 6 +- pkg/manager/internal.go | 19 +----- pkg/manager/manager.go | 109 ++++++++++++++++++++++++++--------- pkg/manager/manager_test.go | 35 ++++++++--- 7 files changed, 152 insertions(+), 76 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 8c98246a31..c10200c4cc 100644 --- a/.golangci.yml +++ b/.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. diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index f74e8e1077..9c5e475114 100644 --- a/pkg/cache/cache.go +++ b/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 } diff --git a/pkg/cache/cache_unit_test.go b/pkg/cache/cache_unit_test.go index 1006c72812..8e2e56e718 100644 --- a/pkg/cache/cache_unit_test.go +++ b/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() { diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 6bb23a1b75..bd1720eb79 100644 --- a/pkg/cluster/cluster.go +++ b/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} diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 86dddf088a..54e1fed5df 100644 --- a/pkg/manager/internal.go +++ b/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)) diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 93f9f60d8e..03a70e5ef9 100644 --- a/pkg/manager/manager.go +++ b/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,9 +273,13 @@ 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. @@ -245,9 +287,13 @@ type Options struct { // {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, @@ -255,17 +301,6 @@ type Options struct { // 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. @@ -273,10 +308,14 @@ type Options struct { // 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, @@ -488,16 +525,27 @@ func (o Options) AndFrom(loader config.ControllerManagerConfiguration) (Options, o.LivenessEndpointName = newObj.Health.LivenessEndpointName } - if o.Port == 0 && newObj.Webhook.Port != nil { - o.Port = *newObj.Webhook.Port + webhookServer := &webhook.Server{} + if newObj.Webhook.Port != nil { + if o.Port == 0 { + o.Port = *newObj.Webhook.Port + } + webhookServer.Port = *newObj.Webhook.Port } - - if o.Host == "" && newObj.Webhook.Host != "" { - o.Host = newObj.Webhook.Host + if newObj.Webhook.Host != "" { + if o.Host == "" { + o.Host = newObj.Webhook.Host + } + webhookServer.Host = newObj.Webhook.Host } - - if o.CertDir == "" && newObj.Webhook.CertDir != "" { - o.CertDir = newObj.Webhook.CertDir + if newObj.Webhook.CertDir != "" { + if o.CertDir == "" { + o.CertDir = newObj.Webhook.CertDir + } + webhookServer.CertDir = newObj.Webhook.CertDir + } + if o.WebhookServer == nil { + o.WebhookServer = webhookServer } if newObj.Controller != nil { @@ -647,5 +695,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 } diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go index 6581bfc5ec..1368ea83f0 100644 --- a/pkg/manager/manager_test.go +++ b/pkg/manager/manager_test.go @@ -226,10 +226,12 @@ var _ = Describe("manger.Manager", func() { HealthProbeBindAddress: "5000", ReadinessEndpointName: "/readiness", LivenessEndpointName: "/liveness", - Port: 8080, - Host: "example.com", - CertDir: "/pki", - TLSOpts: optionsTlSOptsFuncs, + WebhookServer: &webhook.Server{ + Port: 8080, + Host: "example.com", + CertDir: "/pki", + TLSOpts: optionsTlSOptsFuncs, + }, }.AndFrom(&fakeDeferredLoader{ccfg}) Expect(err).To(BeNil()) @@ -246,13 +248,26 @@ var _ = Describe("manger.Manager", func() { Expect(m.HealthProbeBindAddress).To(Equal("5000")) Expect(m.ReadinessEndpointName).To(Equal("/readiness")) Expect(m.LivenessEndpointName).To(Equal("/liveness")) - Expect(m.Port).To(Equal(8080)) - Expect(m.Host).To(Equal("example.com")) - Expect(m.CertDir).To(Equal("/pki")) - Expect(m.TLSOpts).To(Equal(optionsTlSOptsFuncs)) + Expect(m.WebhookServer.Port).To(Equal(8080)) + Expect(m.WebhookServer.Host).To(Equal("example.com")) + Expect(m.WebhookServer.CertDir).To(Equal("/pki")) + Expect(m.WebhookServer.TLSOpts).To(Equal(optionsTlSOptsFuncs)) }) It("should lazily initialize a webhook server if needed", func() { + By("creating a manager with options") + m, err := New(cfg, Options{WebhookServer: &webhook.Server{Port: 9440, Host: "foo.com"}}) + Expect(err).NotTo(HaveOccurred()) + Expect(m).NotTo(BeNil()) + + By("checking options are passed to the webhook server") + svr := m.GetWebhookServer() + Expect(svr).NotTo(BeNil()) + Expect(svr.Port).To(Equal(9440)) + Expect(svr.Host).To(Equal("foo.com")) + }) + + It("should lazily initialize a webhook server if needed (deprecated)", func() { By("creating a manager with options") m, err := New(cfg, Options{Port: 9440, Host: "foo.com"}) Expect(err).NotTo(HaveOccurred()) @@ -267,13 +282,15 @@ var _ = Describe("manger.Manager", func() { It("should not initialize a webhook server if Options.WebhookServer is set", func() { By("creating a manager with options") - m, err := New(cfg, Options{Port: 9441, WebhookServer: &webhook.Server{Port: 9440}}) + srv := &webhook.Server{Port: 9440} + m, err := New(cfg, Options{Port: 9441, WebhookServer: srv}) Expect(err).NotTo(HaveOccurred()) Expect(m).NotTo(BeNil()) By("checking the server contains the Port set on the webhook server and not passed to Options") svr := m.GetWebhookServer() Expect(svr).NotTo(BeNil()) + Expect(svr).To(Equal(srv)) Expect(svr.Port).To(Equal(9440)) })