Fix the defaulting for the leader election config map

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
 }

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))
+			}
+		})
 	}
 }