Merge pull request #2422 from sbueringer/pr-deprecate-opts

⚠ Remove deprecated manager, webhook and cluster options

Author: k8s-ci-robot

pkg/cluster/cluster.go

@@ -28,12 +28,11 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
-	"k8s.io/utils/pointer"
-	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
-	logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
 
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
+	logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
 	intrec "sigs.k8s.io/controller-runtime/pkg/internal/recorder"
 )
 
@@ -95,17 +94,9 @@ type Options struct {
 	// 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.
-	SyncPeriod *time.Duration
-
-	// Namespace if specified restricts the manager's cache to watch objects in
-	// the desired namespace Defaults to all namespaces
 	//
-	// 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
+	// Deprecated: Use Cache.SyncPeriod instead.
+	SyncPeriod *time.Duration
 
 	// HTTPClient is the http client that will be used to create the default
 	// Cache and Client. If not set the rest.HTTPClientFor function will be used
@@ -141,18 +132,6 @@ type Options struct {
 	// Only use a custom NewClient if you know what you are doing.
 	NewClient client.NewClientFunc
 
-	// ClientDisableCacheFor tells the client that, if any cache is used, to bypass it
-	// for the given objects.
-	//
-	// Deprecated: Use Client.Cache.DisableFor 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
 	// Use this to customize the event correlator and spam filter
 	//
@@ -211,9 +190,6 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) {
 		if cacheOpts.SyncPeriod == nil {
 			cacheOpts.SyncPeriod = options.SyncPeriod
 		}
-		if len(cacheOpts.Namespaces) == 0 && options.Namespace != "" {
-			cacheOpts.Namespaces = []string{options.Namespace}
-		}
 	}
 	cache, err := options.NewCache(config, cacheOpts)
 	if err != nil {
@@ -240,16 +216,6 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) {
 		if clientOpts.Cache.Reader == nil {
 			clientOpts.Cache.Reader = cache
 		}
-
-		// For backward compatibility, the ClientDisableCacheFor option should
-		// be appended to the DisableFor option in the client.
-		clientOpts.Cache.DisableFor = append(clientOpts.Cache.DisableFor, options.ClientDisableCacheFor...)
-
-		if clientOpts.DryRun == nil && options.DryRunClient {
-			// For backward compatibility, the DryRunClient (if set) option should override
-			// the DryRun option in the client (if unset).
-			clientOpts.DryRun = pointer.Bool(true)
-		}
 	}
 	clientWriter, err := options.NewClient(config, clientOpts)
 	if err != nil {

pkg/envtest/webhook_test.go

@@ -29,6 +29,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -40,12 +41,12 @@ var _ = Describe("Test", func() {
 	Describe("Webhook", func() {
 		It("should reject create request for webhook that rejects all requests", func() {
 			m, err := manager.New(env.Config, manager.Options{
-				Port:    env.WebhookInstallOptions.LocalServingPort,
-				Host:    env.WebhookInstallOptions.LocalServingHost,
-				CertDir: env.WebhookInstallOptions.LocalServingCertDir,
-				TLSOpts: []func(*tls.Config){
-					func(config *tls.Config) {},
-				},
+				WebhookServer: webhook.NewServer(webhook.Options{
+					Port:    env.WebhookInstallOptions.LocalServingPort,
+					Host:    env.WebhookInstallOptions.LocalServingHost,
+					CertDir: env.WebhookInstallOptions.LocalServingCertDir,
+					TLSOpts: []func(*tls.Config){func(config *tls.Config) {}},
+				}),
 			}) // we need manager here just to leverage manager.SetFields
 			Expect(err).NotTo(HaveOccurred())
 			server := m.GetWebhookServer()

pkg/manager/manager.go

@@ -18,7 +18,6 @@ package manager
 
 import (
 	"context"
-	"crypto/tls"
 	"fmt"
 	"net"
 	"net/http"
@@ -140,35 +139,6 @@ type Options struct {
 	// 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
-	// 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{}`.
-	//
-	// Deprecated: Use Cache.SyncPeriod instead.
-	SyncPeriod *time.Duration
-
 	// Logger is the logger that should be used by this manager.
 	// If none is set, it defaults to log.Log global logger.
 	Logger logr.Logger
@@ -239,23 +209,15 @@ type Options struct {
 	// wait to force acquire leadership. This is measured against time of
 	// last observed ack. Default is 15 seconds.
 	LeaseDuration *time.Duration
+
 	// RenewDeadline is the duration that the acting controlplane will retry
 	// refreshing leadership before giving up. Default is 10 seconds.
 	RenewDeadline *time.Duration
+
 	// RetryPeriod is the duration the LeaderElector clients should wait
 	// between tries of actions. Default is 2 seconds.
 	RetryPeriod *time.Duration
 
-	// Namespace, if specified, restricts the manager's cache to watch objects in
-	// the desired namespace. Defaults to all namespaces.
-	//
-	// 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
 	// for serving prometheus metrics.
 	// It can be set to "0" to disable the metrics serving.
@@ -279,31 +241,6 @@ type Options struct {
 	// before exposing it to public.
 	PprofBindAddress string
 
-	// 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 instead. A WebhookServer can be created via webhook.NewServer.
-	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 instead. A WebhookServer can be created via webhook.NewServer.
-	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 instead. A WebhookServer can be created via webhook.NewServer.
-	CertDir string
-
-	// TLSOpts is used to allow configuring the TLS config used for the webhook server.
-	//
-	// Deprecated: Use WebhookServer instead. A WebhookServer can be created via webhook.NewServer.
-	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.
@@ -314,18 +251,6 @@ type Options struct {
 	// 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
 	// Use this to customize the event correlator and spam filter
 	//
@@ -401,15 +326,11 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		clusterOptions.Scheme = options.Scheme
 		clusterOptions.MapperProvider = options.MapperProvider
 		clusterOptions.Logger = options.Logger
-		clusterOptions.SyncPeriod = options.SyncPeriod
 		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
+		clusterOptions.EventBroadcaster = options.EventBroadcaster //nolint:staticcheck
 	})
 	if err != nil {
 		return nil, err
@@ -526,12 +447,12 @@ func (o Options) AndFrom(loader config.ControllerManagerConfiguration) (Options,
 
 	o = o.setLeaderElectionConfig(newObj)
 
-	if o.SyncPeriod == nil && newObj.SyncPeriod != nil {
-		o.SyncPeriod = &newObj.SyncPeriod.Duration
+	if o.Cache.SyncPeriod == nil && newObj.SyncPeriod != nil {
+		o.Cache.SyncPeriod = &newObj.SyncPeriod.Duration
 	}
 
-	if o.Namespace == "" && newObj.CacheNamespace != "" {
-		o.Namespace = newObj.CacheNamespace
+	if len(o.Cache.Namespaces) == 0 && newObj.CacheNamespace != "" {
+		o.Cache.Namespaces = []string{newObj.CacheNamespace}
 	}
 
 	if o.MetricsBindAddress == "" && newObj.Metrics.BindAddress != "" {
@@ -550,20 +471,15 @@ 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
-	}
-	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 {
+		port := 0
+		if newObj.Webhook.Port != nil {
+			port = *newObj.Webhook.Port
+		}
 		o.WebhookServer = webhook.NewServer(webhook.Options{
-			Port:    o.Port,
-			Host:    o.Host,
-			CertDir: o.CertDir,
+			Port:    port,
+			Host:    newObj.Webhook.Host,
+			CertDir: newObj.Webhook.CertDir,
 		})
 	}
 
@@ -737,12 +653,7 @@ func setOptionsDefaults(options Options) Options {
 	}
 
 	if options.WebhookServer == nil {
-		options.WebhookServer = webhook.NewServer(webhook.Options{
-			Host:    options.Host,
-			Port:    options.Port,
-			CertDir: options.CertDir,
-			TLSOpts: options.TLSOpts,
-		})
+		options.WebhookServer = webhook.NewServer(webhook.Options{})
 	}
 
 	return options

pkg/manager/manager_test.go

@@ -156,22 +156,22 @@ var _ = Describe("manger.Manager", func() {
 			m, err := Options{}.AndFrom(&fakeDeferredLoader{ccfg})
 			Expect(err).ToNot(HaveOccurred())
 
-			Expect(*m.SyncPeriod).To(Equal(duration.Duration))
+			Expect(*m.Cache.SyncPeriod).To(Equal(duration.Duration))
 			Expect(m.LeaderElection).To(Equal(leaderElect))
 			Expect(m.LeaderElectionResourceLock).To(Equal("leases"))
 			Expect(m.LeaderElectionNamespace).To(Equal("default"))
 			Expect(m.LeaderElectionID).To(Equal("ctrl-lease"))
 			Expect(m.LeaseDuration.String()).To(Equal(duration.Duration.String()))
 			Expect(m.RenewDeadline.String()).To(Equal(duration.Duration.String()))
 			Expect(m.RetryPeriod.String()).To(Equal(duration.Duration.String()))
-			Expect(m.Namespace).To(Equal("default"))
+			Expect(m.Cache.Namespaces).To(Equal([]string{"default"}))
 			Expect(m.MetricsBindAddress).To(Equal(":6000"))
 			Expect(m.HealthProbeBindAddress).To(Equal("6060"))
 			Expect(m.ReadinessEndpointName).To(Equal("/readyz"))
 			Expect(m.LivenessEndpointName).To(Equal("/livez"))
-			Expect(m.Port).To(Equal(port))
-			Expect(m.Host).To(Equal("localhost"))
-			Expect(m.CertDir).To(Equal("/certs"))
+			Expect(m.WebhookServer.(*webhook.DefaultServer).Options.Port).To(Equal(port))
+			Expect(m.WebhookServer.(*webhook.DefaultServer).Options.Host).To(Equal("localhost"))
+			Expect(m.WebhookServer.(*webhook.DefaultServer).Options.CertDir).To(Equal("/certs"))
 		})
 
 		It("should be able to keep Options when cfg.ControllerManagerConfiguration set", func() {
@@ -213,15 +213,17 @@ var _ = Describe("manger.Manager", func() {
 				func(config *tls.Config) {},
 			}
 			m, err := Options{
-				SyncPeriod:                 &optDuration,
+				Cache: cache.Options{
+					SyncPeriod: &optDuration,
+					Namespaces: []string{"ctrl"},
+				},
 				LeaderElection:             true,
 				LeaderElectionResourceLock: "configmaps",
 				LeaderElectionNamespace:    "ctrl",
 				LeaderElectionID:           "ctrl-configmap",
 				LeaseDuration:              &optDuration,
 				RenewDeadline:              &optDuration,
 				RetryPeriod:                &optDuration,
-				Namespace:                  "ctrl",
 				MetricsBindAddress:         ":7000",
 				HealthProbeBindAddress:     "5000",
 				ReadinessEndpointName:      "/readiness",
@@ -235,15 +237,15 @@ var _ = Describe("manger.Manager", func() {
 			}.AndFrom(&fakeDeferredLoader{ccfg})
 			Expect(err).ToNot(HaveOccurred())
 
-			Expect(m.SyncPeriod.String()).To(Equal(optDuration.String()))
+			Expect(m.Cache.SyncPeriod.String()).To(Equal(optDuration.String()))
 			Expect(m.LeaderElection).To(BeTrue())
 			Expect(m.LeaderElectionResourceLock).To(Equal("configmaps"))
 			Expect(m.LeaderElectionNamespace).To(Equal("ctrl"))
 			Expect(m.LeaderElectionID).To(Equal("ctrl-configmap"))
 			Expect(m.LeaseDuration.String()).To(Equal(optDuration.String()))
 			Expect(m.RenewDeadline.String()).To(Equal(optDuration.String()))
 			Expect(m.RetryPeriod.String()).To(Equal(optDuration.String()))
-			Expect(m.Namespace).To(Equal("ctrl"))
+			Expect(m.Cache.Namespaces).To(Equal([]string{"ctrl"}))
 			Expect(m.MetricsBindAddress).To(Equal(":7000"))
 			Expect(m.HealthProbeBindAddress).To(Equal("5000"))
 			Expect(m.ReadinessEndpointName).To(Equal("/readiness"))
@@ -267,23 +269,10 @@ var _ = Describe("manger.Manager", func() {
 			Expect(svr.(*webhook.DefaultServer).Options.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())
-			Expect(m).NotTo(BeNil())
-
-			By("checking options are passed to the webhook server")
-			svr := m.GetWebhookServer()
-			Expect(svr).NotTo(BeNil())
-			Expect(svr.(*webhook.DefaultServer).Options.Port).To(Equal(9440))
-			Expect(svr.(*webhook.DefaultServer).Options.Host).To(Equal("foo.com"))
-		})
-
 		It("should not initialize a webhook server if Options.WebhookServer is set", func() {
 			By("creating a manager with options")
 			srv := webhook.NewServer(webhook.Options{Port: 9440})
-			m, err := New(cfg, Options{Port: 9441, WebhookServer: srv})
+			m, err := New(cfg, Options{WebhookServer: srv})
 			Expect(err).NotTo(HaveOccurred())
 			Expect(m).NotTo(BeNil())