From 049d3193e80d2ca1e7796b67f284035fafc9826f Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Tue, 5 Jan 2021 15:00:18 +0100 Subject: [PATCH 1/9] main.go: sort imports So first group is standard library as it was previously, then we have external imports and module-local imports as third group. Signed-off-by: Mateusz Gozdek --- main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index 764c705b..2c7dd066 100644 --- a/main.go +++ b/main.go @@ -21,17 +21,17 @@ import ( "os" "time" - "github.com/tinkerbell/cluster-api-provider-tinkerbell/controllers" "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/record" + clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" infrastructurev1alpha3 "github.com/tinkerbell/cluster-api-provider-tinkerbell/api/v1alpha3" - clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" + "github.com/tinkerbell/cluster-api-provider-tinkerbell/controllers" // +kubebuilder:scaffold:imports ) From 5c4746cfedc5b0c5ddd08a33ee7ab0887a973f83 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Tue, 5 Jan 2021 15:17:27 +0100 Subject: [PATCH 2/9] main.go: make flags formatting consistent If description exeeds line length limits of lll linter, then move it to another line. Signed-off-by: Mateusz Gozdek --- main.go | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/main.go b/main.go index 2c7dd066..17ca597f 100644 --- a/main.go +++ b/main.go @@ -65,39 +65,25 @@ func main() { "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") - flag.StringVar( - &leaderElectionNamespace, - "leader-election-namespace", - "", + flag.StringVar(&leaderElectionNamespace, "leader-election-namespace", "", "Namespace that the controller performs leader election in. "+ "If unspecified, the controller will discover which namespace it is running in.", ) - flag.StringVar(&healthAddr, - "health-addr", - ":9440", - "The address the health endpoint binds to.", - ) + flag.StringVar(&healthAddr, "health-addr", ":9440", "The address the health endpoint binds to.") flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.") - flag.DurationVar(&syncPeriod, - "sync-period", - 10*time.Minute, + flag.DurationVar(&syncPeriod, "sync-period", 10*time.Minute, "The minimum interval at which watched resources are reconciled (e.g. 15m)", ) - flag.StringVar( - &watchNamespace, - "namespace", - "", + flag.StringVar(&watchNamespace, "namespace", "", "Namespace that the controller watches to reconcile cluster-api objects. "+ "If unspecified, the controller watches for cluster-api objects across all namespaces.", ) - flag.IntVar(&webhookPort, - "webhook-port", - 0, + flag.IntVar(&webhookPort, "webhook-port", 0, "Webhook Server port, disabled by default. When enabled, the manager will only "+ "work as webhook server, no reconcilers are installed.", ) From ff294051290c4f5d3ca514a5070ea93666bda2a9 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Tue, 5 Jan 2021 15:18:51 +0100 Subject: [PATCH 3/9] main.go: avoid using else to fail early and keep happy path on the left Signed-off-by: Mateusz Gozdek --- main.go | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/main.go b/main.go index 17ca597f..def844fe 100644 --- a/main.go +++ b/main.go @@ -121,31 +121,31 @@ func main() { // TODO: Get a Tinkerbell client. - if webhookPort == 0 { - if err = (&controllers.TinkerbellClusterReconciler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("TinerellCluster"), - Recorder: mgr.GetEventRecorderFor("tinerellcluster-controller"), - Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "TinkerbellCluster") - os.Exit(1) - } - - if err = (&controllers.TinkerbellMachineReconciler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("TinkerbellMachine"), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("tinkerbellmachine-controller"), - }).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "TinkerbellMachine") - os.Exit(1) - } - } else { + if webhookPort != 0 { // TODO: add the webhook configuration setupLog.Error(errors.New("webhook not implemented"), "webhook", "not available") os.Exit(1) } + + if err = (&controllers.TinkerbellClusterReconciler{ + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("TinerellCluster"), + Recorder: mgr.GetEventRecorderFor("tinerellcluster-controller"), + Scheme: mgr.GetScheme(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "TinkerbellCluster") + os.Exit(1) + } + + if err = (&controllers.TinkerbellMachineReconciler{ + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("TinkerbellMachine"), + Scheme: mgr.GetScheme(), + Recorder: mgr.GetEventRecorderFor("tinkerbellmachine-controller"), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "TinkerbellMachine") + os.Exit(1) + } // +kubebuilder:scaffold:builder if err := mgr.AddReadyzCheck("ping", healthz.Ping); err != nil { From bf72b91346c8e201be1ee71b00b731240e007d88 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Wed, 6 Jan 2021 10:01:07 +0100 Subject: [PATCH 4/9] main.go: re-use ctrl.Options fields where possible So only pointer fields must be declared independently. Signed-off-by: Mateusz Gozdek --- main.go | 60 +++++++++++++++++++++++---------------------------------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/main.go b/main.go index def844fe..617b51ba 100644 --- a/main.go +++ b/main.go @@ -51,39 +51,44 @@ func init() { //nolint:funlen,gomnd func main() { - var ( - enableLeaderElection bool - leaderElectionNamespace string - healthAddr string - metricsAddr string - webhookPort int - syncPeriod time.Duration - watchNamespace string - ) + // Machine and cluster operations can create enough events to trigger the event recorder spam filter + // Setting the burst size higher ensures all events will be recorded and submitted to the API + broadcaster := record.NewBroadcasterWithCorrelatorOptions(record.CorrelatorOptions{ + BurstSize: 100, + }) + + var syncPeriod time.Duration + + options := ctrl.Options{ + Scheme: scheme, + LeaderElectionID: "controller-leader-election-capt", + EventBroadcaster: broadcaster, + SyncPeriod: &syncPeriod, + } - flag.BoolVar(&enableLeaderElection, "enable-leader-election", false, + flag.BoolVar(&options.LeaderElection, "enable-leader-election", false, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") - flag.StringVar(&leaderElectionNamespace, "leader-election-namespace", "", + flag.StringVar(&options.LeaderElectionNamespace, "leader-election-namespace", "", "Namespace that the controller performs leader election in. "+ "If unspecified, the controller will discover which namespace it is running in.", ) - flag.StringVar(&healthAddr, "health-addr", ":9440", "The address the health endpoint binds to.") + flag.StringVar(&options.HealthProbeBindAddress, "health-addr", ":9440", "The address the health endpoint binds to.") - flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.") + flag.StringVar(&options.MetricsBindAddress, "metrics-addr", ":8080", "The address the metric endpoint binds to.") flag.DurationVar(&syncPeriod, "sync-period", 10*time.Minute, "The minimum interval at which watched resources are reconciled (e.g. 15m)", ) - flag.StringVar(&watchNamespace, "namespace", "", + flag.StringVar(&options.Namespace, "namespace", "", "Namespace that the controller watches to reconcile cluster-api objects. "+ "If unspecified, the controller watches for cluster-api objects across all namespaces.", ) - flag.IntVar(&webhookPort, "webhook-port", 0, + flag.IntVar(&options.Port, "webhook-port", 0, "Webhook Server port, disabled by default. When enabled, the manager will only "+ "work as webhook server, no reconcilers are installed.", ) @@ -92,28 +97,11 @@ func main() { ctrl.SetLogger(zap.New(zap.UseDevMode(true))) - if watchNamespace != "" { - setupLog.Info("Watching cluster-api objects only in namespace for reconciliation", "namespace", watchNamespace) + if options.Namespace != "" { + setupLog.Info("Watching cluster-api objects only in namespace for reconciliation", "namespace", options.Namespace) } - // Machine and cluster operations can create enough events to trigger the event recorder spam filter - // Setting the burst size higher ensures all events will be recorded and submitted to the API - broadcaster := record.NewBroadcasterWithCorrelatorOptions(record.CorrelatorOptions{ - BurstSize: 100, - }) - - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ - Scheme: scheme, - MetricsBindAddress: metricsAddr, - Port: webhookPort, - EventBroadcaster: broadcaster, - LeaderElection: enableLeaderElection, - LeaderElectionID: "controller-leader-election-capt", - LeaderElectionNamespace: leaderElectionNamespace, - Namespace: watchNamespace, - SyncPeriod: &syncPeriod, - HealthProbeBindAddress: healthAddr, - }) + mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), options) if err != nil { setupLog.Error(err, "unable to start manager") os.Exit(1) @@ -121,7 +109,7 @@ func main() { // TODO: Get a Tinkerbell client. - if webhookPort != 0 { + if options.Port != 0 { // TODO: add the webhook configuration setupLog.Error(errors.New("webhook not implemented"), "webhook", "not available") os.Exit(1) From 361c5a3146caa045889202ffa5c4557dfdeb61f6 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Wed, 6 Jan 2021 10:02:08 +0100 Subject: [PATCH 5/9] main.go: fix panic when webhook port is configured Logger complains about odd number of arguments given to Error function. Signed-off-by: Mateusz Gozdek --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 617b51ba..5ef0c995 100644 --- a/main.go +++ b/main.go @@ -111,7 +111,7 @@ func main() { if options.Port != 0 { // TODO: add the webhook configuration - setupLog.Error(errors.New("webhook not implemented"), "webhook", "not available") + setupLog.Error(errors.New("webhook not implemented"), "webhook not available") os.Exit(1) } From f58e5f1fd429de20a2a6c42d600af22d35b1d30e Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Wed, 6 Jan 2021 10:05:36 +0100 Subject: [PATCH 6/9] main.go: move flag parsing to separate function To make main() shorter. Signed-off-by: Mateusz Gozdek --- main.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index 5ef0c995..83b6d323 100644 --- a/main.go +++ b/main.go @@ -49,8 +49,8 @@ func init() { // +kubebuilder:scaffold:scheme } -//nolint:funlen,gomnd -func main() { +// optionsFromFlags parse CLI flags and converts them to controller runtime options. +func optionsFromFlags() ctrl.Options { // Machine and cluster operations can create enough events to trigger the event recorder spam filter // Setting the burst size higher ensures all events will be recorded and submitted to the API broadcaster := record.NewBroadcasterWithCorrelatorOptions(record.CorrelatorOptions{ @@ -95,8 +95,15 @@ func main() { flag.Parse() + return options +} + +//nolint:funlen,gomnd +func main() { ctrl.SetLogger(zap.New(zap.UseDevMode(true))) + options := optionsFromFlags() + if options.Namespace != "" { setupLog.Info("Watching cluster-api objects only in namespace for reconciliation", "namespace", options.Namespace) } From ef16da86e637f2667c6baaab72dd630797465076 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Wed, 6 Jan 2021 10:07:19 +0100 Subject: [PATCH 7/9] main.go: fix linter annotations Signed-off-by: Mateusz Gozdek --- main.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 83b6d323..8a352fac 100644 --- a/main.go +++ b/main.go @@ -54,7 +54,7 @@ func optionsFromFlags() ctrl.Options { // Machine and cluster operations can create enough events to trigger the event recorder spam filter // Setting the burst size higher ensures all events will be recorded and submitted to the API broadcaster := record.NewBroadcasterWithCorrelatorOptions(record.CorrelatorOptions{ - BurstSize: 100, + BurstSize: 100, //nolint:gomnd }) var syncPeriod time.Duration @@ -79,7 +79,7 @@ func optionsFromFlags() ctrl.Options { flag.StringVar(&options.MetricsBindAddress, "metrics-addr", ":8080", "The address the metric endpoint binds to.") - flag.DurationVar(&syncPeriod, "sync-period", 10*time.Minute, + flag.DurationVar(&syncPeriod, "sync-period", 10*time.Minute, //nolint:gomnd "The minimum interval at which watched resources are reconciled (e.g. 15m)", ) @@ -98,7 +98,6 @@ func optionsFromFlags() ctrl.Options { return options } -//nolint:funlen,gomnd func main() { ctrl.SetLogger(zap.New(zap.UseDevMode(true))) From c34a044bf08ad39330b399c30aff2b7ab618f0f3 Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Wed, 6 Jan 2021 10:09:30 +0100 Subject: [PATCH 8/9] main.go: switch logger from zap to klog As klog is a standard logger for Kubernetes controllers which should be used. Signed-off-by: Mateusz Gozdek --- go.mod | 1 + main.go | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 94b8deaa..5eedbfc7 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( k8s.io/api v0.17.16 k8s.io/apimachinery v0.17.16 k8s.io/client-go v0.17.16 + k8s.io/klog v1.0.0 k8s.io/utils v0.0.0-20201110183641-67b214c5f920 // indirect sigs.k8s.io/cluster-api v0.3.12 sigs.k8s.io/controller-runtime v0.5.14 diff --git a/main.go b/main.go index 8a352fac..639b5e79 100644 --- a/main.go +++ b/main.go @@ -25,10 +25,11 @@ import ( clientgoscheme "k8s.io/client-go/kubernetes/scheme" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/record" + "k8s.io/klog" + "k8s.io/klog/klogr" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" - "sigs.k8s.io/controller-runtime/pkg/log/zap" infrastructurev1alpha3 "github.com/tinkerbell/cluster-api-provider-tinkerbell/api/v1alpha3" "github.com/tinkerbell/cluster-api-provider-tinkerbell/controllers" @@ -51,6 +52,8 @@ func init() { // optionsFromFlags parse CLI flags and converts them to controller runtime options. func optionsFromFlags() ctrl.Options { + klog.InitFlags(nil) + // Machine and cluster operations can create enough events to trigger the event recorder spam filter // Setting the burst size higher ensures all events will be recorded and submitted to the API broadcaster := record.NewBroadcasterWithCorrelatorOptions(record.CorrelatorOptions{ @@ -99,7 +102,7 @@ func optionsFromFlags() ctrl.Options { } func main() { - ctrl.SetLogger(zap.New(zap.UseDevMode(true))) + ctrl.SetLogger(klogr.New()) options := optionsFromFlags() From b0efcf01b76e099f4ebdd869f4ed439aa1bc2c4a Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek Date: Wed, 6 Jan 2021 10:12:52 +0100 Subject: [PATCH 9/9] main.go: move validating options code to separate function To again make main() shorter and to give this piece of code a name. Signed-off-by: Mateusz Gozdek --- main.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/main.go b/main.go index 639b5e79..a763962f 100644 --- a/main.go +++ b/main.go @@ -101,13 +101,27 @@ func optionsFromFlags() ctrl.Options { return options } +func validateOptions(options ctrl.Options) error { + if options.Namespace != "" { + setupLog.Info("Watching cluster-api objects only in namespace for reconciliation", "namespace", options.Namespace) + } + + if options.Port != 0 { + // TODO: add the webhook configuration + return errors.New("webhook not implemented") + } + + return nil +} + func main() { ctrl.SetLogger(klogr.New()) options := optionsFromFlags() - if options.Namespace != "" { - setupLog.Info("Watching cluster-api objects only in namespace for reconciliation", "namespace", options.Namespace) + if err := validateOptions(options); err != nil { + setupLog.Error(err, "validating controllers configuration") + os.Exit(1) } mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), options) @@ -118,12 +132,6 @@ func main() { // TODO: Get a Tinkerbell client. - if options.Port != 0 { - // TODO: add the webhook configuration - setupLog.Error(errors.New("webhook not implemented"), "webhook not available") - os.Exit(1) - } - if err = (&controllers.TinkerbellClusterReconciler{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("TinerellCluster"),