diff --git a/leaderelection/config.go b/leaderelection/config.go index 1e54705786..4bba700a52 100644 --- a/leaderelection/config.go +++ b/leaderelection/config.go @@ -37,32 +37,35 @@ var ( // NewConfigFromMap returns a Config for the given map, or an error. func NewConfigFromMap(data map[string]string) (*Config, error) { - config := &Config{ - EnabledComponents: sets.NewString(), - } + config := defaultConfig() - if resourceLock := data["resourceLock"]; !validResourceLocks.Has(resourceLock) { - return nil, fmt.Errorf(`resourceLock: invalid value %q: valid values are "leases","configmaps","endpoints"`, resourceLock) - } else { + if resourceLock, ok := data["resourceLock"]; ok { + if !validResourceLocks.Has(resourceLock) { + return nil, fmt.Errorf(`resourceLock: invalid value %q: valid values are "leases","configmaps","endpoints"`, resourceLock) + } config.ResourceLock = resourceLock } - if leaseDuration, err := time.ParseDuration(data["leaseDuration"]); err != nil { - return nil, fmt.Errorf("leaseDuration: invalid duration: %q", data["leaseDuration"]) - } else { - config.LeaseDuration = leaseDuration - } - - if renewDeadline, err := time.ParseDuration(data["renewDeadline"]); err != nil { - return nil, fmt.Errorf("renewDeadline: invalid duration: %q", data["renewDeadline"]) - } else { - config.RenewDeadline = renewDeadline - } - - if retryPeriod, err := time.ParseDuration(data["retryPeriod"]); err != nil { - return nil, fmt.Errorf("retryPeriod: invalid duration: %q", data["retryPeriod"]) - } else { - config.RetryPeriod = retryPeriod + for _, d := range []struct { + key string + val *time.Duration + }{{ + "leaseDuration", + &config.LeaseDuration, + }, { + "renewDeadline", + &config.RenewDeadline, + }, { + "retryPeriod", + &config.RetryPeriod, + }} { + if v, ok := data[d.key]; ok { + dur, err := time.ParseDuration(v) + if err != nil { + return nil, fmt.Errorf("%s: invalid duration: %q", d.key, v) + } + *d.val = dur + } } // enabledComponents are not validated here, because they are dependent on @@ -82,7 +85,6 @@ func NewConfigFromConfigMap(configMap *corev1.ConfigMap) (*Config, error) { config := defaultConfig() return config, nil } - return NewConfigFromMap(configMap.Data) } @@ -154,5 +156,5 @@ func UniqueID() (string, error) { return "", err } - return (id + "_" + string(uuid.NewUUID())), nil + return id + "_" + string(uuid.NewUUID()), nil } diff --git a/leaderelection/config_test.go b/leaderelection/config_test.go index 72624463c7..09c1871de6 100644 --- a/leaderelection/config_test.go +++ b/leaderelection/config_test.go @@ -22,7 +22,10 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" + "knative.dev/pkg/kmeta" ) func okConfig() *Config { @@ -54,111 +57,62 @@ func TestNewConfigMapFromData(t *testing.T) { data map[string]string expected *Config err error - }{ - { - name: "disabled but OK config", - data: func() map[string]string { - data := okData() - delete(data, "enabledComponents") - return data - }(), - expected: okConfig(), - }, - { - name: "OK config - controller enabled", - data: okData(), - expected: func() *Config { - config := okConfig() - config.EnabledComponents.Insert("controller") - return config - }(), - }, - { - name: "missing resourceLock", - data: func() map[string]string { - data := okData() - delete(data, "resourceLock") - return data - }(), - err: errors.New(`resourceLock: invalid value "": valid values are "leases","configmaps","endpoints"`), - }, - { - name: "invalid resourceLock", - data: func() map[string]string { - data := okData() - data["resourceLock"] = "flarps" - return data - }(), - err: errors.New(`resourceLock: invalid value "flarps": valid values are "leases","configmaps","endpoints"`), - }, - { - name: "missing leaseDuration", - data: func() map[string]string { - data := okData() - delete(data, "leaseDuration") - return data - }(), - err: errors.New(`leaseDuration: invalid duration: ""`), - }, - { - name: "invalid leaseDuration", - data: func() map[string]string { - data := okData() - data["leaseDuration"] = "flops" - return data - }(), - err: errors.New(`leaseDuration: invalid duration: "flops"`), - }, - { - name: "missing renewDeadline", - data: func() map[string]string { - data := okData() - delete(data, "renewDeadline") - return data - }(), - err: errors.New(`renewDeadline: invalid duration: ""`), - }, - { - name: "invalid renewDeadline", - data: func() map[string]string { - data := okData() - data["renewDeadline"] = "flops" - return data - }(), - err: errors.New(`renewDeadline: invalid duration: "flops"`), - }, - { - name: "missing retryPeriod", - data: func() map[string]string { - data := okData() - delete(data, "retryPeriod") - return data - }(), - err: errors.New(`retryPeriod: invalid duration: ""`), - }, - { - name: "invalid retryPeriod", - data: func() map[string]string { - data := okData() - data["retryPeriod"] = "flops" - return data - }(), - err: errors.New(`retryPeriod: invalid duration: "flops"`), - }, - } + }{{ + name: "disabled but OK config", + data: func() map[string]string { + data := okData() + delete(data, "enabledComponents") + return data + }(), + expected: okConfig(), + }, { + name: "OK config - controller enabled", + data: okData(), + expected: func() *Config { + config := okConfig() + config.EnabledComponents.Insert("controller") + return config + }(), + }, { + name: "invalid resourceLock", + data: kmeta.UnionMaps(okData(), map[string]string{ + "resourceLock": "flarps", + }), + err: errors.New(`resourceLock: invalid value "flarps": valid values are "leases","configmaps","endpoints"`), + }, { + name: "invalid leaseDuration", + data: kmeta.UnionMaps(okData(), map[string]string{ + "leaseDuration": "flops", + }), + err: errors.New(`leaseDuration: invalid duration: "flops"`), + }, { + name: "invalid renewDeadline", + data: kmeta.UnionMaps(okData(), map[string]string{ + "renewDeadline": "flops", + }), + err: errors.New(`renewDeadline: invalid duration: "flops"`), + }, { + name: "invalid retryPeriod", + data: kmeta.UnionMaps(okData(), map[string]string{ + "retryPeriod": "flops", + }), + err: errors.New(`retryPeriod: invalid duration: "flops"`), + }} - for i := range cases { - tc := cases[i] - actualConfig, actualErr := NewConfigFromMap(tc.data) - if !reflect.DeepEqual(tc.err, actualErr) { - t.Errorf("%v: expected error %v, got %v", tc.name, tc.err, actualErr) - continue - } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actualConfig, actualErr := NewConfigFromConfigMap( + &corev1.ConfigMap{ + Data: tc.data, + }) + if !reflect.DeepEqual(tc.err, actualErr) { + t.Fatalf("Error = %v, want: %v", actualErr, tc.err) + } - if !reflect.DeepEqual(tc.expected, actualConfig) { - t.Errorf("%v: expected config:\n%+v\ngot:\n%+v", tc.name, tc.expected, actualConfig) - continue - } + if got, want := actualConfig, tc.expected; !cmp.Equal(got, want) { + t.Errorf("Config = %#v, want: %#v, diff(-want,+got):\n%s", got, want, cmp.Diff(want, got)) + } + }) } } @@ -167,44 +121,42 @@ func TestGetComponentConfig(t *testing.T) { name string config Config expected ComponentConfig - }{ - { - name: "component enabled", - config: Config{ - ResourceLock: "leases", - LeaseDuration: 15 * time.Second, - RenewDeadline: 10 * time.Second, - RetryPeriod: 2 * time.Second, - EnabledComponents: sets.NewString("component"), - }, - expected: ComponentConfig{ - LeaderElect: true, - ResourceLock: "leases", - LeaseDuration: 15 * time.Second, - RenewDeadline: 10 * time.Second, - RetryPeriod: 2 * time.Second, - }, + }{{ + name: "component enabled", + config: Config{ + ResourceLock: "leases", + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, + EnabledComponents: sets.NewString("component"), }, - { - name: "component disabled", - config: Config{ - ResourceLock: "leases", - LeaseDuration: 15 * time.Second, - RenewDeadline: 10 * time.Second, - RetryPeriod: 2 * time.Second, - EnabledComponents: sets.NewString("not-the-component"), - }, - expected: ComponentConfig{ - LeaderElect: false, - }, + expected: ComponentConfig{ + LeaderElect: true, + ResourceLock: "leases", + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, }, - } + }, { + name: "component disabled", + config: Config{ + ResourceLock: "leases", + LeaseDuration: 15 * time.Second, + RenewDeadline: 10 * time.Second, + RetryPeriod: 2 * time.Second, + EnabledComponents: sets.NewString("not-the-component"), + }, + expected: ComponentConfig{ + LeaderElect: false, + }, + }} - for i := range cases { - tc := cases[i] - actual := tc.config.GetComponentConfig("component") - if !reflect.DeepEqual(tc.expected, actual) { - t.Errorf("%v: expected:\n%+v\ngot:\n%+v", tc.name, tc.expected, actual) - } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actual := tc.config.GetComponentConfig("component") + if got, want := actual, tc.expected; !cmp.Equal(got, want) { + t.Errorf("Incorrect config: diff(-want,+got):\n%s", cmp.Diff(want, got)) + } + }) } }