Skip to content

Commit 0f22b59

Browse files
dlipovetskydlipovetsky
authored
refactor: Split flag and config init into testable steps (#497)
Splits the flag and config initialization into testable steps, and adds tests of each.
1 parent bc14457 commit 0f22b59

File tree

3 files changed

+260
-54
lines changed

3 files changed

+260
-54
lines changed

‎main.go

Lines changed: 91 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ import (
4545
"sigs.k8s.io/controller-runtime/pkg/healthz"
4646
"sigs.k8s.io/controller-runtime/pkg/log/zap"
4747
"sigs.k8s.io/controller-runtime/pkg/manager"
48+
"sigs.k8s.io/controller-runtime/pkg/metrics/server"
4849

4950
infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
5051
"github.com/nutanix-cloud-native/cluster-api-provider-nutanix/controllers"
@@ -69,12 +70,26 @@ const (
6970
defaultMaxConcurrentReconciles = 10
7071
)
7172

73+
type options struct {
74+
enableLeaderElection bool
75+
healthProbeAddr string
76+
maxConcurrentReconciles int
77+
78+
rateLimiterBaseDelay time.Duration
79+
rateLimiterMaxDelay time.Duration
80+
rateLimiterBucketSize int
81+
rateLimiterQPS int
82+
83+
managerOptions capiflags.ManagerOptions
84+
zapOptions zap.Options
85+
}
86+
7287
type managerConfig struct {
7388
enableLeaderElection bool
74-
probeAddr string
89+
healthProbeAddr string
7590
concurrentReconcilesNutanixCluster int
7691
concurrentReconcilesNutanixMachine int
77-
managerOptions capiflags.ManagerOptions
92+
metricsServerOpts server.Options
7893

7994
logger logr.Logger
8095
restConfig *rest.Config
@@ -126,42 +141,82 @@ func validateRateLimiterConfig(baseDelay, maxDelay time.Duration, bucketSize, qp
126141
return nil
127142
}
128143

129-
func parseFlags(config *managerConfig) {
130-
capiflags.AddManagerOptions(pflag.CommandLine, &config.managerOptions)
131-
pflag.StringVar(&config.probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
132-
pflag.BoolVar(&config.enableLeaderElection, "leader-elect", false,
144+
func initializeFlags() *options {
145+
opts := &options{}
146+
147+
// Add the controller-runtime flags to the standard library FlagSet.
148+
ctrl.RegisterFlags(flag.CommandLine)
149+
150+
// Add the Cluster API flags to the pflag FlagSet.
151+
capiflags.AddManagerOptions(pflag.CommandLine, &opts.managerOptions)
152+
153+
// Add zap flags to the standard libary FlagSet.
154+
opts.zapOptions.BindFlags(flag.CommandLine)
155+
156+
// Add our own flags to the pflag FlagSet.
157+
pflag.StringVar(&opts.healthProbeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
158+
pflag.BoolVar(&opts.enableLeaderElection, "leader-elect", false,
133159
"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.")
134-
var maxConcurrentReconciles int
135-
pflag.IntVar(&maxConcurrentReconciles, "max-concurrent-reconciles", defaultMaxConcurrentReconciles,
160+
161+
pflag.IntVar(&opts.maxConcurrentReconciles, "max-concurrent-reconciles", defaultMaxConcurrentReconciles,
136162
"The maximum number of allowed, concurrent reconciles.")
137163

138-
var baseDelay, maxDelay time.Duration
139-
var bucketSize, qps int
140-
pflag.DurationVar(&baseDelay, "rate-limiter-base-delay", 500*time.Millisecond, "The base delay for the rate limiter.")
141-
pflag.DurationVar(&maxDelay, "rate-limiter-max-delay", 15*time.Minute, "The maximum delay for the rate limiter.")
142-
pflag.IntVar(&bucketSize, "rate-limiter-bucket-size", 100, "The bucket size for the rate limiter.")
143-
pflag.IntVar(&qps, "rate-limiter-qps", 10, "The QPS for the rate limiter.")
164+
pflag.DurationVar(&opts.rateLimiterBaseDelay, "rate-limiter-base-delay", 500*time.Millisecond, "The base delay for the rate limiter.")
165+
pflag.DurationVar(&opts.rateLimiterMaxDelay, "rate-limiter-max-delay", 15*time.Minute, "The maximum delay for the rate limiter.")
166+
pflag.IntVar(&opts.rateLimiterBucketSize, "rate-limiter-bucket-size", 100, "The bucket size for the rate limiter.")
167+
pflag.IntVar(&opts.rateLimiterQPS, "rate-limiter-qps", 10, "The QPS for the rate limiter.")
144168

145-
opts := zap.Options{
146-
TimeEncoder: zapcore.RFC3339TimeEncoder,
147-
}
148-
opts.BindFlags(flag.CommandLine)
149-
150-
logger := zap.New(zap.UseFlagOptions(&opts))
151-
ctrl.SetLogger(logger)
169+
// At this point, we should be done adding flags to the standard library FlagSet, flag.CommandLine.
170+
// So we can include the flags that third-party libraries, e.g. controller-runtime, and zap,
171+
// have added to the standard library FlagSet, we merge it into the pflag FlagSet.
152172
pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
173+
174+
// Parse flags.
153175
pflag.Parse()
154176

155-
config.concurrentReconcilesNutanixCluster = maxConcurrentReconciles
156-
config.concurrentReconcilesNutanixMachine = maxConcurrentReconciles
177+
return opts
178+
}
179+
180+
func initializeConfig(opts *options) (*managerConfig, error) {
181+
config := &managerConfig{
182+
enableLeaderElection: opts.enableLeaderElection,
183+
healthProbeAddr: opts.healthProbeAddr,
184+
}
157185

158-
rateLimiter, err := compositeRateLimiter(baseDelay, maxDelay, bucketSize, qps)
186+
_, metricsServerOpts, err := capiflags.GetManagerOptions(opts.managerOptions)
159187
if err != nil {
160-
config.logger.Error(err, "unable to create composite rate limiter")
161-
os.Exit(1)
188+
return nil, fmt.Errorf("unable to get metrics server options: %w", err)
189+
}
190+
if metricsServerOpts == nil {
191+
return nil, errors.New("parsed manager options are nil")
162192
}
193+
config.metricsServerOpts = *metricsServerOpts
194+
195+
config.concurrentReconcilesNutanixCluster = opts.maxConcurrentReconciles
196+
config.concurrentReconcilesNutanixMachine = opts.maxConcurrentReconciles
163197

198+
rateLimiter, err := compositeRateLimiter(opts.rateLimiterBaseDelay, opts.rateLimiterMaxDelay, opts.rateLimiterBucketSize, opts.rateLimiterQPS)
199+
if err != nil {
200+
return nil, fmt.Errorf("unable to create composite rate limiter: %w", err)
201+
}
164202
config.rateLimiter = rateLimiter
203+
204+
zapOptions := opts.zapOptions
205+
zapOptions.TimeEncoder = zapcore.RFC3339TimeEncoder
206+
config.logger = zap.New(zap.UseFlagOptions(&zapOptions))
207+
208+
// Configure controller-runtime logger before using calling any controller-runtime functions.
209+
// Otherwise, the user will not see warnings and errors logged by these functions.
210+
ctrl.SetLogger(config.logger)
211+
212+
// Before calling GetConfigOrDie, we have parsed flags, because the function reads value of
213+
// the--kubeconfig flag.
214+
config.restConfig, err = ctrl.GetConfig()
215+
if err != nil {
216+
return nil, fmt.Errorf("failed to load kubeconfig: %w", err)
217+
}
218+
219+
return config, nil
165220
}
166221

167222
func setupLogger() logr.Logger {
@@ -276,19 +331,10 @@ func runManager(ctx context.Context, mgr manager.Manager, config *managerConfig)
276331
}
277332

278333
func initializeManager(config *managerConfig) (manager.Manager, error) {
279-
_, metricsOpts, err := capiflags.GetManagerOptions(config.managerOptions)
280-
if err != nil {
281-
return nil, fmt.Errorf("unable to get manager options: %w", err)
282-
}
283-
284-
if metricsOpts == nil {
285-
return nil, errors.New("parsed manager options are nil")
286-
}
287-
288334
mgr, err := ctrl.NewManager(config.restConfig, ctrl.Options{
289335
Scheme: scheme,
290-
Metrics: *metricsOpts,
291-
HealthProbeBindAddress: config.probeAddr,
336+
Metrics: config.metricsServerOpts,
337+
HealthProbeBindAddress: config.healthProbeAddr,
292338
LeaderElection: config.enableLeaderElection,
293339
LeaderElectionID: "f265110d.cluster.x-k8s.io",
294340
})
@@ -306,16 +352,17 @@ func initializeManager(config *managerConfig) (manager.Manager, error) {
306352
func main() {
307353
logger := setupLogger()
308354

309-
config := &managerConfig{}
310-
parseFlags(config)
355+
logger.Info("Initializing Nutanix Cluster API Infrastructure Provider", "Git Hash", gitCommitHash)
311356

312-
// Flags must be parsed before calling GetConfigOrDie, because
313-
// it reads the value of the--kubeconfig flag.
314-
config.restConfig = ctrl.GetConfigOrDie()
357+
opts := initializeFlags()
358+
// After this point, we must not add flags to either the pflag, or the standard library FlagSets.
315359

316-
config.logger = logger
360+
config, err := initializeConfig(opts)
361+
if err != nil {
362+
logger.Error(err, "unable to configure manager")
363+
os.Exit(1)
364+
}
317365

318-
logger.Info("Initializing Nutanix Cluster API Infrastructure Provider", "Git Hash", gitCommitHash)
319366
mgr, err := initializeManager(config)
320367
if err != nil {
321368
logger.Error(err, "unable to create manager")

‎main_test.go

Lines changed: 157 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,23 @@ package main
22

33
import (
44
"context"
5+
"flag"
56
"fmt"
67
"os"
78
"path/filepath"
89
"testing"
910
"time"
1011

12+
"github.com/google/go-cmp/cmp"
13+
"github.com/google/go-cmp/cmp/cmpopts"
14+
"github.com/spf13/pflag"
1115
"github.com/stretchr/testify/assert"
1216
"github.com/stretchr/testify/require"
1317
"go.uber.org/mock/gomock"
1418
"k8s.io/apimachinery/pkg/api/meta"
1519
"k8s.io/client-go/rest"
1620
"k8s.io/client-go/tools/clientcmd/api"
21+
"sigs.k8s.io/cluster-api/util/flags"
1722
ctrlconfig "sigs.k8s.io/controller-runtime/pkg/config"
1823
"sigs.k8s.io/controller-runtime/pkg/envtest"
1924

@@ -23,16 +28,158 @@ import (
2328
mockk8sclient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/k8sclient"
2429
)
2530

26-
func TestParseFlags(t *testing.T) {
27-
config := &managerConfig{}
28-
os.Args = []string{"cmd", "--leader-elect=true", "--max-concurrent-reconciles=5", "--diagnostics-address=:8081", "--insecure-diagnostics=true"}
29-
parseFlags(config)
31+
func TestInitializeFlags(t *testing.T) {
32+
tt := []struct {
33+
name string
34+
args []string
35+
want *options
36+
cmpOpt cmp.Option
37+
}{
38+
{
39+
name: "our own flags",
40+
args: []string{
41+
"cmd",
42+
"--leader-elect=true",
43+
"--max-concurrent-reconciles=5",
44+
"--health-probe-bind-address=:8081",
45+
"--rate-limiter-base-delay=500ms",
46+
"--rate-limiter-max-delay=10s",
47+
"--rate-limiter-bucket-size=1000",
48+
"--rate-limiter-qps=50",
49+
},
50+
want: &options{
51+
enableLeaderElection: true,
52+
maxConcurrentReconciles: 5,
53+
healthProbeAddr: ":8081",
54+
rateLimiterBaseDelay: 500 * time.Millisecond,
55+
rateLimiterMaxDelay: 10 * time.Second,
56+
rateLimiterBucketSize: 1000,
57+
rateLimiterQPS: 50,
58+
},
59+
cmpOpt: cmpopts.IgnoreFields(options{},
60+
"managerOptions",
61+
"zapOptions",
62+
),
63+
},
64+
{
65+
name: "Cluster API flags",
66+
args: []string{
67+
"cmd",
68+
"--metrics-bind-addr=1.2.3.4",
69+
"--diagnostics-address=:9999",
70+
"--insecure-diagnostics=true",
71+
},
72+
want: &options{
73+
managerOptions: flags.ManagerOptions{
74+
MetricsBindAddr: "1.2.3.4",
75+
DiagnosticsAddress: ":9999",
76+
InsecureDiagnostics: true,
77+
},
78+
},
79+
cmpOpt: cmpopts.IgnoreFields(options{},
80+
"enableLeaderElection",
81+
"maxConcurrentReconciles",
82+
"healthProbeAddr",
83+
"rateLimiterBaseDelay",
84+
"rateLimiterMaxDelay",
85+
"rateLimiterBucketSize",
86+
"rateLimiterQPS",
87+
88+
// Controller-runtime defaults these values,
89+
// so we ignore them.
90+
"managerOptions.TLSMinVersion",
91+
"managerOptions.TLSCipherSuites",
92+
),
93+
},
94+
// Unfortunately, we cannot test parsing of the single controller-runtime flag,
95+
// --kubeconfig, because its values is not exported. However, we do effectively test parsing
96+
// by testing manager initialization; that creates loads a kubeconfig specified by the flag.
97+
}
98+
99+
for _, tc := range tt {
100+
t.Run(tc.name, func(t *testing.T) {
101+
os.Args = tc.args
102+
103+
// Clear flags initialized by any other test.
104+
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
105+
pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
106+
107+
got := initializeFlags()
108+
if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(options{}), tc.cmpOpt); diff != "" {
109+
t.Errorf("MakeGatewayInfo() mismatch (-want +got):\n%s", diff)
110+
}
111+
})
112+
}
113+
}
114+
115+
func TestInitializeConfig(t *testing.T) {
116+
tt := []struct {
117+
name string
118+
args []string
119+
want managerConfig
120+
wantErr bool
121+
}{
122+
{
123+
name: "pass with misc. options",
124+
args: []string{
125+
"cmd",
126+
// Our options.
127+
"--leader-elect=true",
128+
"--max-concurrent-reconciles=5",
129+
"--health-probe-bind-address=:8081",
130+
"--rate-limiter-base-delay=500ms",
131+
"--rate-limiter-max-delay=10s",
132+
"--rate-limiter-bucket-size=1000",
133+
"--rate-limiter-qps=50",
134+
// Cluster API options.
135+
"--insecure-diagnostics=true",
136+
"--diagnostics-address=:9999",
137+
"--insecure-diagnostics=false",
138+
// Controller-runtime options.
139+
"--kubeconfig=testdata/kubeconfig",
140+
},
141+
want: managerConfig{},
142+
wantErr: false,
143+
},
144+
{
145+
name: "fail with missing kubeconfig",
146+
args: []string{
147+
"cmd",
148+
"--kubeconfig=notfound",
149+
},
150+
wantErr: true,
151+
},
152+
}
153+
154+
for _, tc := range tt {
155+
t.Run(tc.name, func(t *testing.T) {
156+
os.Args = tc.args
30157

31-
assert.Equal(t, true, config.enableLeaderElection)
32-
assert.Equal(t, 5, config.concurrentReconcilesNutanixCluster)
33-
assert.Equal(t, 5, config.concurrentReconcilesNutanixMachine)
34-
assert.Equal(t, ":8081", config.managerOptions.DiagnosticsAddress)
35-
assert.Equal(t, true, config.managerOptions.InsecureDiagnostics)
158+
// Clear flags initialized by any other test.
159+
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
160+
pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
161+
162+
opts := initializeFlags()
163+
164+
got, err := initializeConfig(opts)
165+
if tc.wantErr {
166+
if err == nil {
167+
t.Errorf("unexpected error: %s", err)
168+
}
169+
return
170+
}
171+
172+
assert.Equal(t, got.enableLeaderElection, opts.enableLeaderElection)
173+
assert.Equal(t, got.healthProbeAddr, opts.healthProbeAddr)
174+
assert.Equal(t, got.concurrentReconcilesNutanixCluster, opts.maxConcurrentReconciles)
175+
assert.Equal(t, got.concurrentReconcilesNutanixMachine, opts.maxConcurrentReconciles)
176+
assert.Equal(t, got.metricsServerOpts.BindAddress, opts.managerOptions.DiagnosticsAddress)
177+
178+
assert.NotNil(t, got.rateLimiter)
179+
assert.True(t, got.logger.Enabled())
180+
assert.Equal(t, got.restConfig.Host, "https://example.com:6443")
181+
})
182+
}
36183
}
37184

38185
func TestSetupLogger(t *testing.T) {
@@ -60,7 +207,7 @@ func TestInitializeManager(t *testing.T) {
60207

61208
config := &managerConfig{
62209
enableLeaderElection: false,
63-
probeAddr: ":8081",
210+
healthProbeAddr: ":8081",
64211
concurrentReconcilesNutanixCluster: 1,
65212
concurrentReconcilesNutanixMachine: 1,
66213
logger: setupLogger(),

0 commit comments

Comments
 (0)