refactor: Split flag and config init into testable steps, and add tests

main.go

@@ -45,6 +45,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
+	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
 	"github.com/nutanix-cloud-native/cluster-api-provider-nutanix/controllers"
@@ -69,12 +70,26 @@ const (
 	defaultMaxConcurrentReconciles = 10
 )
 
+type options struct {
+	enableLeaderElection    bool
+	healthProbeAddr         string
+	maxConcurrentReconciles int
+
+	rateLimiterBaseDelay  time.Duration
+	rateLimiterMaxDelay   time.Duration
+	rateLimiterBucketSize int
+	rateLimiterQPS        int
+
+	managerOptions capiflags.ManagerOptions
+	zapOptions     zap.Options
+}
+
 type managerConfig struct {
 	enableLeaderElection               bool
-	probeAddr                          string
+	healthProbeAddr                    string
 	concurrentReconcilesNutanixCluster int
 	concurrentReconcilesNutanixMachine int
-	managerOptions                     capiflags.ManagerOptions
+	metricsServerOpts                  *server.Options
 
 	logger      logr.Logger
 	restConfig  *rest.Config
@@ -126,42 +141,79 @@ func validateRateLimiterConfig(baseDelay, maxDelay time.Duration, bucketSize, qp
 	return nil
 }
 
-func parseFlags(config *managerConfig) {
-	capiflags.AddManagerOptions(pflag.CommandLine, &config.managerOptions)
-	pflag.StringVar(&config.probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
-	pflag.BoolVar(&config.enableLeaderElection, "leader-elect", false,
+func initializeFlags() *options {
+	opts := &options{}
+
+	// Add the controller-runtime flags to the standard library FlagSet.
+	ctrl.RegisterFlags(flag.CommandLine)
+
+	// Add the Cluster API flags to the pflag FlagSet.
+	capiflags.AddManagerOptions(pflag.CommandLine, &opts.managerOptions)
+
+	// Add zap flags to the standard libary FlagSet.
+	opts.zapOptions.BindFlags(flag.CommandLine)
+
+	// Add our own flags to the pflag FlagSet.
+	pflag.StringVar(&opts.healthProbeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
+	pflag.BoolVar(&opts.enableLeaderElection, "leader-elect", false,
 		"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.")
-	var maxConcurrentReconciles int
-	pflag.IntVar(&maxConcurrentReconciles, "max-concurrent-reconciles", defaultMaxConcurrentReconciles,
-		"The maximum number of allowed, concurrent reconciles.")
 
-	var baseDelay, maxDelay time.Duration
-	var bucketSize, qps int
-	pflag.DurationVar(&baseDelay, "rate-limiter-base-delay", 500*time.Millisecond, "The base delay for the rate limiter.")
-	pflag.DurationVar(&maxDelay, "rate-limiter-max-delay", 15*time.Minute, "The maximum delay for the rate limiter.")
-	pflag.IntVar(&bucketSize, "rate-limiter-bucket-size", 100, "The bucket size for the rate limiter.")
-	pflag.IntVar(&qps, "rate-limiter-qps", 10, "The QPS for the rate limiter.")
+	pflag.IntVar(&opts.maxConcurrentReconciles, "max-concurrent-reconciles", defaultMaxConcurrentReconciles,
+		"The maximum number of allowed, concurrent reconciles.")
 
-	opts := zap.Options{
-		TimeEncoder: zapcore.RFC3339TimeEncoder,
-	}
-	opts.BindFlags(flag.CommandLine)
+	pflag.DurationVar(&opts.rateLimiterBaseDelay, "rate-limiter-base-delay", 500*time.Millisecond, "The base delay for the rate limiter.")
+	pflag.DurationVar(&opts.rateLimiterMaxDelay, "rate-limiter-max-delay", 15*time.Minute, "The maximum delay for the rate limiter.")
+	pflag.IntVar(&opts.rateLimiterBucketSize, "rate-limiter-bucket-size", 100, "The bucket size for the rate limiter.")
+	pflag.IntVar(&opts.rateLimiterQPS, "rate-limiter-qps", 10, "The QPS for the rate limiter.")
 
-	logger := zap.New(zap.UseFlagOptions(&opts))
-	ctrl.SetLogger(logger)
+	// At this point, we should be done adding flags to the standard library FlagSet, flag.CommandLine.
+	// So we can include the flags that third-party libraries, e.g. controller-runtime, and zap,
+	// have added to the standard library FlagSet, we merge it into the pflag FlagSet.
 	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
+
+	// Parse flags.
 	pflag.Parse()
 
-	config.concurrentReconcilesNutanixCluster = maxConcurrentReconciles
-	config.concurrentReconcilesNutanixMachine = maxConcurrentReconciles
+	return opts
+}
+
+func initializeConfig(opts *options) (*managerConfig, error) {
+	config := &managerConfig{}
 
-	rateLimiter, err := compositeRateLimiter(baseDelay, maxDelay, bucketSize, qps)
+	_, metricsServerOpts, err := capiflags.GetManagerOptions(opts.managerOptions)
 	if err != nil {
-		config.logger.Error(err, "unable to create composite rate limiter")
-		os.Exit(1)
+		return nil, fmt.Errorf("unable to get metrics server options: %w", err)
+	}
+	if metricsServerOpts == nil {
+		return nil, errors.New("parsed manager options are nil")
 	}
+	config.metricsServerOpts = metricsServerOpts
 
+	config.concurrentReconcilesNutanixCluster = opts.maxConcurrentReconciles
+	config.concurrentReconcilesNutanixMachine = opts.maxConcurrentReconciles
+
+	rateLimiter, err := compositeRateLimiter(opts.rateLimiterBaseDelay, opts.rateLimiterMaxDelay, opts.rateLimiterBucketSize, opts.rateLimiterQPS)
+	if err != nil {
+		return nil, fmt.Errorf("unable to create composite rate limiter: %w", err)
+	}
 	config.rateLimiter = rateLimiter
+
+	zapOptions := opts.zapOptions
+	zapOptions.TimeEncoder = zapcore.RFC3339TimeEncoder
+	config.logger = zap.New(zap.UseFlagOptions(&zapOptions))
+
+	// Configure controller-runtime logger before using calling any controller-runtime functions.
+	// Otherwise, the user will not see warnings and errors logged by these functions.
+	ctrl.SetLogger(config.logger)
+
+	// Before calling GetConfigOrDie, we have parsed flags, because the function reads value of
+	// the--kubeconfig flag.
+	config.restConfig, err = ctrl.GetConfig()
+	if err != nil {
+		return nil, fmt.Errorf("failed to load kubeconfig: %w", err)
+	}
+
+	return config, nil
 }
 
 func setupLogger() logr.Logger {
@@ -276,19 +328,10 @@ func runManager(ctx context.Context, mgr manager.Manager, config *managerConfig)
 }
 
 func initializeManager(config *managerConfig) (manager.Manager, error) {
-	_, metricsOpts, err := capiflags.GetManagerOptions(config.managerOptions)
-	if err != nil {
-		return nil, fmt.Errorf("unable to get manager options: %w", err)
-	}
-
-	if metricsOpts == nil {
-		return nil, errors.New("parsed manager options are nil")
-	}
-
 	mgr, err := ctrl.NewManager(config.restConfig, ctrl.Options{
 		Scheme:                 scheme,
-		Metrics:                *metricsOpts,
-		HealthProbeBindAddress: config.probeAddr,
+		Metrics:                *config.metricsServerOpts,
+		HealthProbeBindAddress: config.healthProbeAddr,
 		LeaderElection:         config.enableLeaderElection,
 		LeaderElectionID:       "f265110d.cluster.x-k8s.io",
 	})
@@ -306,16 +349,17 @@ func initializeManager(config *managerConfig) (manager.Manager, error) {
 func main() {
 	logger := setupLogger()
 
-	config := &managerConfig{}
-	parseFlags(config)
+	logger.Info("Initializing Nutanix Cluster API Infrastructure Provider", "Git Hash", gitCommitHash)
 
-	// Flags must be parsed before calling GetConfigOrDie, because
-	// it reads the value of the--kubeconfig flag.
-	config.restConfig = ctrl.GetConfigOrDie()
+	opts := initializeFlags()
+	// After this point, we must not add flags to either the pflag, or the standard library FlagSets.
 
-	config.logger = logger
+	config, err := initializeConfig(opts)
+	if err != nil {
+		logger.Error(err, "unable to configure manager")
+		os.Exit(1)
+	}
 
-	logger.Info("Initializing Nutanix Cluster API Infrastructure Provider", "Git Hash", gitCommitHash)
 	mgr, err := initializeManager(config)
 	if err != nil {
 		logger.Error(err, "unable to create manager")

main_test.go

@@ -2,18 +2,23 @@ package main
 
 import (
 	"context"
+	"flag"
 	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd/api"
+	"sigs.k8s.io/cluster-api/util/flags"
 	ctrlconfig "sigs.k8s.io/controller-runtime/pkg/config"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
@@ -23,16 +28,152 @@ import (
 	mockk8sclient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/k8sclient"
 )
 
-func TestParseFlags(t *testing.T) {
-	config := &managerConfig{}
-	os.Args = []string{"cmd", "--leader-elect=true", "--max-concurrent-reconciles=5", "--diagnostics-address=:8081", "--insecure-diagnostics=true"}
-	parseFlags(config)
+func TestInitializeFlags(t *testing.T) {
+	tt := []struct {
+		name   string
+		args   []string
+		want   *options
+		cmpOpt cmp.Option
+	}{
+		{
+			name: "our own flags",
+			args: []string{
+				"cmd",
+				"--leader-elect=true",
+				"--max-concurrent-reconciles=5",
+				"--health-probe-bind-address=:8081",
+				"--rate-limiter-base-delay=500ms",
+				"--rate-limiter-max-delay=10s",
+				"--rate-limiter-bucket-size=1000",
+				"--rate-limiter-qps=50",
+			},
+			want: &options{
+				enableLeaderElection:    true,
+				maxConcurrentReconciles: 5,
+				healthProbeAddr:         ":8081",
+				rateLimiterBaseDelay:    500 * time.Millisecond,
+				rateLimiterMaxDelay:     10 * time.Second,
+				rateLimiterBucketSize:   1000,
+				rateLimiterQPS:          50,
+			},
+			cmpOpt: cmpopts.IgnoreFields(options{},
+				"managerOptions",
+				"zapOptions",
+			),
+		},
+		{
+			name: "Cluster API flags",
+			args: []string{
+				"cmd",
+				"--metrics-bind-addr=1.2.3.4",
+				"--diagnostics-address=:9999",
+				"--insecure-diagnostics=true",
+			},
+			want: &options{
+				managerOptions: flags.ManagerOptions{
+					MetricsBindAddr:     "1.2.3.4",
+					DiagnosticsAddress:  ":9999",
+					InsecureDiagnostics: true,
+				},
+			},
+			cmpOpt: cmpopts.IgnoreFields(options{},
+				"enableLeaderElection",
+				"maxConcurrentReconciles",
+				"healthProbeAddr",
+				"rateLimiterBaseDelay",
+				"rateLimiterMaxDelay",
+				"rateLimiterBucketSize",
+				"rateLimiterQPS",
+
+				// Controller-runtime defaults these values,
+				// so we ignore them.
+				"managerOptions.TLSMinVersion",
+				"managerOptions.TLSCipherSuites",
+			),
+		},
+		// Unfortunately, we cannot test parsing of the single controller-runtime flag,
+		// --kubeconfig, because its values is not exported. However, we do effectively test parsing
+		// by testing manager initialization; that creates loads a kubeconfig specified by the flag.
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			os.Args = tc.args
+
+			// Make sure we are only reading options from flags, not the environment.
+			os.Clearenv()
 
-	assert.Equal(t, true, config.enableLeaderElection)
-	assert.Equal(t, 5, config.concurrentReconcilesNutanixCluster)
-	assert.Equal(t, 5, config.concurrentReconcilesNutanixMachine)
-	assert.Equal(t, ":8081", config.managerOptions.DiagnosticsAddress)
-	assert.Equal(t, true, config.managerOptions.InsecureDiagnostics)
+			// Clear flags initialized by any other test.
+			flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+			pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
+
+			got := initializeFlags()
+			if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(options{}), tc.cmpOpt); diff != "" {
+				t.Errorf("MakeGatewayInfo() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
+
+func TestInitializeConfig(t *testing.T) {
+	tt := []struct {
+		name    string
+		args    []string
+		wantErr bool
+	}{
+		{
+			name: "pass with misc. options",
+			args: []string{
+				"cmd",
+				"--leader-elect=true",
+				"--max-concurrent-reconciles=5",
+				"--health-probe-bind-address=:8081",
+				"--insecure-diagnostics=true",
+				"--rate-limiter-base-delay=500ms",
+				"--rate-limiter-max-delay=10s",
+				"--rate-limiter-bucket-size=1000",
+				"--rate-limiter-qps=50",
+				"--diagnostics-address=:9999",
+				"--insecure-diagnostics=false",
+			},
+			wantErr: false,
+		},
+		{
+			name: "pass with kubeconfig",
+			args: []string{
+				"--kubeconfig=testdata/kubeconfig",
+			},
+		},
+		{
+			name: "fail with missing kubeconfig",
+			args: []string{
+				"cmd",
+				"--kubeconfig=notfound",
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			os.Args = tc.args
+
+			// Make sure we are only reading options from flags, not the environment.
+			os.Clearenv()
+
+			// Clear flags initialized by any other test.
+			flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+			pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
+
+			opts := initializeFlags()
+
+			_, err := initializeConfig(opts)
+
+			if tc.wantErr != (err != nil) {
+				t.Errorf("unexpected error: %s", err)
+			}
+		})
+	}
 }
 
 func TestSetupLogger(t *testing.T) {
@@ -60,7 +201,7 @@ func TestInitializeManager(t *testing.T) {
 
 	config := &managerConfig{
 		enableLeaderElection:               false,
-		probeAddr:                          ":8081",
+		healthProbeAddr:                    ":8081",
 		concurrentReconcilesNutanixCluster: 1,
 		concurrentReconcilesNutanixMachine: 1,
 		logger:                             setupLogger(),

testdata/kubeconfig

@@ -0,0 +1,12 @@
+apiVersion: v1
+clusters:
+- cluster:
+    server: https://127.0.0.1:6443
+  name: test
+contexts:
+- context:
+    cluster: test
+  name: test
+current-context: test
+kind: Config
+preferences: {}