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.

Sign up for GitHub

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

Jump to bottom

⚠ Remove deprecated manager, webhook and cluster options #2422

Merged
merged 1 commit into from Jul 26, 2023
Merged

⚠ Remove deprecated manager, webhook and cluster options #2422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 4 additions & 38 deletions pkg/cluster/cluster.go
View file
Open in desktop
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We forgot to deprecate this one before


// 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
Expand Down Expand Up @@ -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
//
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
13 changes: 7 additions & 6 deletions pkg/envtest/webhook_test.go
View file
Open in desktop
Expand Up @@ -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"
Expand All @@ -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()
Expand Down
119 changes: 15 additions & 104 deletions pkg/manager/manager.go
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@ package manager

import (
"context"
"crypto/tls"
"fmt"
"net"
"net/http"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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
//
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 != "" {
Expand All @@ -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,
})
}

Expand Down Expand Up @@ -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
Expand Down
35 changes: 12 additions & 23 deletions pkg/manager/manager_test.go
View file
Open in desktop
Expand Up @@ -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() {
Expand Down Expand Up @@ -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",
Expand All @@ -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"))
Expand All @@ -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())

Expand Down