Repository: knative/pkg
Pull request #1335 (2020-05-15 08:46:11)

Use config map parsing lib and enhance it further.
- Use pkg/configmap parser for observability and leaderelection config. Logging config ain't worth it and I didn't yet dare to touch metric config.
- Adds a new helper for parsing StringSets. That one is useful in multiple places.
- Enhance the error reporting to always include the parsed key to be useful in error messages.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1335#" title="Author self-approved">markusthoemmes</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[configmap/parse.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1261216452451504129/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1261216452451504129/artifacts/line-cov.html#file1) | 68.8% | 56.5% | -12.2
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1261216452451504129/artifacts/line-cov.html#file2) | 81.0% | 58.3% | -22.6

knative-prow-robot: @markusthoemmes: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | e14ac0f5789a09eea6d46cdf8ad9dd94c0d83cb9 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-unit-tests/1261216452451504130) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1335). [Your PR dashboard](https://gubernator.knative.dev/pr/markusthoemmes).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
markusthoemmes: /test pull-knative-pkg-unit-tests

Git checkout.
yanweiguo: /lgtm
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[configmap/parse.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262268761285595137/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262268761285595137/artifacts/line-cov.html#file1) | 68.8% | 56.5% | -12.2
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262268761285595137/artifacts/line-cov.html#file2) | 81.0% | 58.3% | -22.6

knative-prow-robot: New changes are detected. LGTM label has been removed.
slinkydeveloper: /assign
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[configmap/parse.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262276688931196928/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262276688931196928/artifacts/line-cov.html#file1) | 68.8% | 56.5% | -12.2
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262276688931196928/artifacts/line-cov.html#file2) | 81.0% | 58.3% | -22.6
[tracing/config/tracing.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262276688931196928/artifacts/line-cov.html#file3) | 86.2% | 83.0% | -3.2

slinkydeveloper: /lgtm
knative-prow-robot: New changes are detected. LGTM label has been removed.
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[configmap/parse.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262431584271732739/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262431584271732739/artifacts/line-cov.html#file1) | 68.8% | 56.5% | -12.2
[metrics/config_observability.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262431584271732739/artifacts/line-cov.html#file2) | 81.0% | 58.3% | -22.6
[tracing/config/tracing.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1335/pull-knative-pkg-go-coverage/1262431584271732739/artifacts/line-cov.html#file3) | 86.2% | 83.0% | -3.2

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1335#" title="Author self-approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1335#pullrequestreview-413791742" title="LGTM">vagababov</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1335#" title="Author self-approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1335#pullrequestreview-413805684" title="Approved">mattmoor</a>*, *<a href="https://github.com/knative/pkg/pull/1335#pullrequestreview-413791742" title="LGTM">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)~~ [mattmoor]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/configmap/parse.go b/configmap/parse.go
index 097b38f90..6479598b7 100644
--- a/configmap/parse.go
+++ b/configmap/parse.go
@@ -17,9 +17,12 @@ limitations under the License.
 package configmap
 
 import (
+	"fmt"
 	"strconv"
 	"strings"
 	"time"
+
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // ParseFunc is a function taking ConfigMap data and applying a parse operation to it.
@@ -51,7 +54,7 @@ func AsInt32(key string, target *int32) ParseFunc {
 		if raw, ok := data[key]; ok {
 			val, err := strconv.ParseInt(raw, 10, 32)
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to parse %q: %w", key, err)
 			}
 			*target = int32(val)
 		}
@@ -65,7 +68,7 @@ func AsInt64(key string, target *int64) ParseFunc {
 		if raw, ok := data[key]; ok {
 			val, err := strconv.ParseInt(raw, 10, 64)
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to parse %q: %w", key, err)
 			}
 			*target = val
 		}
@@ -79,7 +82,7 @@ func AsFloat64(key string, target *float64) ParseFunc {
 		if raw, ok := data[key]; ok {
 			val, err := strconv.ParseFloat(raw, 64)
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to parse %q: %w", key, err)
 			}
 			*target = val
 		}
@@ -93,7 +96,7 @@ func AsDuration(key string, target *time.Duration) ParseFunc {
 		if raw, ok := data[key]; ok {
 			val, err := time.ParseDuration(raw)
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to parse %q: %w", key, err)
 			}
 			*target = val
 		}
@@ -101,6 +104,16 @@ func AsDuration(key string, target *time.Duration) ParseFunc {
 	}
 }
 
+// AsStringSet parses the value at key as a sets.String (split by ',') into the target, if it exists.
+func AsStringSet(key string, target *sets.String) ParseFunc {
+	return func(data map[string]string) error {
+		if raw, ok := data[key]; ok {
+			*target = sets.NewString(strings.Split(raw, ",")...)
+		}
+		return nil
+	}
+}
+
 // Parse parses the given map using the parser functions passed in.
 func Parse(data map[string]string, parsers ...ParseFunc) error {
 	for _, parse := range parsers {
diff --git a/leaderelection/config.go b/leaderelection/config.go
index 0ec0377ed..59ab45905 100644
--- a/leaderelection/config.go
+++ b/leaderelection/config.go
@@ -19,12 +19,13 @@ package leaderelection
 import (
 	"fmt"
 	"os"
-	"strings"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/uuid"
+
+	cm "knative.dev/pkg/configmap"
 )
 
 const ConfigMapNameEnv = "CONFIG_LEADERELECTION_NAME"
@@ -35,41 +36,23 @@ var validResourceLocks = sets.NewString("leases", "configmaps", "endpoints")
 func NewConfigFromMap(data map[string]string) (*Config, error) {
 	config := defaultConfig()
 
-	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 err := cm.Parse(data,
+		cm.AsString("resourceLock", &config.ResourceLock),
 
-	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
-		}
+		cm.AsDuration("leaseDuration", &config.LeaseDuration),
+		cm.AsDuration("renewDeadline", &config.RenewDeadline),
+		cm.AsDuration("retryPeriod", &config.RetryPeriod),
+
+		// enabledComponents are not validated here, because they are dependent on
+		// the component. Components should provide additional validation for this
+		// field.
+		cm.AsStringSet("enabledComponents", &config.EnabledComponents),
+	); err != nil {
+		return nil, err
 	}
 
-	// enabledComponents are not validated here, because they are dependent on
-	// the component. Components should provide additional validation for this
-	// field.
-	if enabledComponents, ok := data["enabledComponents"]; ok {
-		tokens := strings.Split(enabledComponents, ",")
-		config.EnabledComponents = sets.NewString(tokens...)
+	if !validResourceLocks.Has(config.ResourceLock) {
+		return nil, fmt.Errorf(`resourceLock: invalid value %q: valid values are "leases","configmaps","endpoints"`, config.ResourceLock)
 	}
 
 	return config, nil
diff --git a/metrics/config_observability.go b/metrics/config_observability.go
index e40cb2fb2..e4560ede0 100644
--- a/metrics/config_observability.go
+++ b/metrics/config_observability.go
@@ -18,10 +18,10 @@ package metrics
 
 import (
 	"os"
-	"strings"
 	texttemplate "text/template"
 
 	corev1 "k8s.io/api/core/v1"
+	cm "knative.dev/pkg/configmap"
 )
 
 const (
@@ -71,32 +71,23 @@ func defaultConfig() *ObservabilityConfig {
 // NewObservabilityConfigFromConfigMap creates a ObservabilityConfig from the supplied ConfigMap
 func NewObservabilityConfigFromConfigMap(configMap *corev1.ConfigMap) (*ObservabilityConfig, error) {
 	oc := defaultConfig()
-	if evlc, ok := configMap.Data["logging.enable-var-log-collection"]; ok {
-		oc.EnableVarLogCollection = strings.EqualFold(evlc, "true")
-	}
 
-	if rut, ok := configMap.Data["logging.revision-url-template"]; ok {
-		oc.LoggingURLTemplate = rut
+	if err := cm.Parse(configMap.Data,
+		cm.AsBool("logging.enable-var-log-collection", &oc.EnableVarLogCollection),
+		cm.AsString("logging.revision-url-template", &oc.LoggingURLTemplate),
+		cm.AsString("logging.request-log-template", &oc.RequestLogTemplate),
+		cm.AsBool("logging.enable-probe-request-log", &oc.EnableProbeRequestLog),
+		cm.AsString("metrics.request-metrics-backend-destination", &oc.RequestMetricsBackend),
+		cm.AsBool("profiling.enable", &oc.EnableProfiling),
+	); err != nil {
+		return nil, err
 	}
 
-	if rlt, ok := configMap.Data["logging.request-log-template"]; ok {
+	if oc.RequestLogTemplate != "" {
 		// Verify that we get valid templates.
-		if _, err := texttemplate.New("requestLog").Parse(rlt); err != nil {
+		if _, err := texttemplate.New("requestLog").Parse(oc.RequestLogTemplate); err != nil {
 			return nil, err
 		}
-		oc.RequestLogTemplate = rlt
-	}
-
-	if eprl, ok := configMap.Data["logging.enable-probe-request-log"]; ok {
-		oc.EnableProbeRequestLog = strings.EqualFold(eprl, "true")
-	}
-
-	if mb, ok := configMap.Data["metrics.request-metrics-backend-destination"]; ok {
-		oc.RequestMetricsBackend = mb
-	}
-
-	if prof, ok := configMap.Data["profiling.enable"]; ok {
-		oc.EnableProfiling = strings.EqualFold(prof, "true")
 	}
 
 	return oc, nil
diff --git a/tracing/config/tracing.go b/tracing/config/tracing.go
index 67aec651c..b3aceee16 100644
--- a/tracing/config/tracing.go
+++ b/tracing/config/tracing.go
@@ -25,6 +25,7 @@ import (
 
 	"cloud.google.com/go/compute/metadata"
 	corev1 "k8s.io/api/core/v1"
+	cm "knative.dev/pkg/configmap"
 )
 
 const (
@@ -98,15 +99,20 @@ func NewTracingConfigFromMap(cfgMap map[string]string) (*Config, error) {
 		}
 	}
 
-	if endpoint, ok := cfgMap[zipkinEndpointKey]; !ok && tc.Backend == Zipkin {
+	if err := cm.Parse(cfgMap,
+		cm.AsString(zipkinEndpointKey, &tc.ZipkinEndpoint),
+		cm.AsString(stackdriverProjectIDKey, &tc.StackdriverProjectID),
+		cm.AsBool(debugKey, &tc.Debug),
+		cm.AsFloat64(sampleRateKey, &tc.SampleRate),
+	); err != nil {
+		return nil, err
+	}
+
+	if tc.Backend == Zipkin && tc.ZipkinEndpoint == "" {
 		return nil, errors.New("zipkin tracing enabled without a zipkin endpoint specified")
-	} else {
-		tc.ZipkinEndpoint = endpoint
 	}
 
-	if projectID, ok := cfgMap[stackdriverProjectIDKey]; ok {
-		tc.StackdriverProjectID = projectID
-	} else if tc.Backend == Stackdriver {
+	if tc.Backend == Stackdriver && tc.StackdriverProjectID == "" {
 		projectID, err := metadata.ProjectID()
 		if err != nil {
 			return nil, fmt.Errorf("stackdriver tracing enabled without a project-id specified: %w", err)
@@ -114,23 +120,8 @@ func NewTracingConfigFromMap(cfgMap map[string]string) (*Config, error) {
 		tc.StackdriverProjectID = projectID
 	}
 
-	if debug, ok := cfgMap[debugKey]; ok {
-		debugBool, err := strconv.ParseBool(debug)
-		if err != nil {
-			return nil, fmt.Errorf("failed parsing tracing config %q", debugKey)
-		}
-		tc.Debug = debugBool
-	}
-
-	if sampleRate, ok := cfgMap[sampleRateKey]; ok {
-		sampleRateFloat, err := strconv.ParseFloat(sampleRate, 64)
-		if err != nil {
-			return nil, fmt.Errorf("failed to parse sampleRate in tracing config: %w", err)
-		}
-		if sampleRateFloat < 0 || sampleRateFloat > 1 {
-			return nil, fmt.Errorf("sampleRate = %v must be in [0, 1] range", sampleRateFloat)
-		}
-		tc.SampleRate = sampleRateFloat
+	if tc.SampleRate < 0 || tc.SampleRate > 1 {
+		return nil, fmt.Errorf("sample-rate = %v must be in [0, 1] range", tc.SampleRate)
 	}
 
 	return tc, nil


Test patch:
diff --git a/configmap/parse_test.go b/configmap/parse_test.go
index 65b9ab56d..14528d351 100644
--- a/configmap/parse_test.go
+++ b/configmap/parse_test.go
@@ -19,6 +19,9 @@ package configmap
 import (
 	"testing"
 	"time"
+
+	"github.com/google/go-cmp/cmp"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 type testConfig struct {
@@ -28,6 +31,7 @@ type testConfig struct {
 	i64 int64
 	f64 float64
 	dur time.Duration
+	set sets.String
 }
 
 func TestParse(t *testing.T) {
@@ -46,6 +50,7 @@ func TestParse(t *testing.T) {
 			"test-int64":    "2",
 			"test-float64":  "1.0",
 			"test-duration": "1m",
+			"test-set":      "a,b,c",
 		},
 		want: testConfig{
 			str: "foo.bar",
@@ -54,6 +59,7 @@ func TestParse(t *testing.T) {
 			i64: 2,
 			f64: 1.0,
 			dur: time.Minute,
+			set: sets.NewString("a", "b", "c"),
 		},
 	}, {
 		name: "respect defaults",
@@ -116,11 +122,12 @@ func TestParse(t *testing.T) {
 				AsInt64("test-int64", &test.conf.i64),
 				AsFloat64("test-float64", &test.conf.f64),
 				AsDuration("test-duration", &test.conf.dur),
+				AsStringSet("test-set", &test.conf.set),
 			); (err == nil) == test.expectErr {
 				t.Fatal("Failed to parse data:", err)
 			}
 
-			if test.conf != test.want {
+			if !cmp.Equal(test.conf, test.want, cmp.AllowUnexported(testConfig{})) {
 				t.Fatalf("parsed = %v, want %v", test.conf, test.want)
 			}
 		})
diff --git a/leaderelection/config_test.go b/leaderelection/config_test.go
index 8621be5c5..3d3767788 100644
--- a/leaderelection/config_test.go
+++ b/leaderelection/config_test.go
@@ -18,7 +18,6 @@ package leaderelection
 
 import (
 	"errors"
-	"reflect"
 	"testing"
 	"time"
 
@@ -84,19 +83,19 @@ func TestNewConfigMapFromData(t *testing.T) {
 		data: kmeta.UnionMaps(okData(), map[string]string{
 			"leaseDuration": "flops",
 		}),
-		err: errors.New(`leaseDuration: invalid duration: "flops"`),
+		err: errors.New(`failed to parse "leaseDuration": time: invalid duration flops`),
 	}, {
 		name: "invalid renewDeadline",
 		data: kmeta.UnionMaps(okData(), map[string]string{
 			"renewDeadline": "flops",
 		}),
-		err: errors.New(`renewDeadline: invalid duration: "flops"`),
+		err: errors.New(`failed to parse "renewDeadline": time: invalid duration flops`),
 	}, {
 		name: "invalid retryPeriod",
 		data: kmeta.UnionMaps(okData(), map[string]string{
 			"retryPeriod": "flops",
 		}),
-		err: errors.New(`retryPeriod: invalid duration: "flops"`),
+		err: errors.New(`failed to parse "retryPeriod": time: invalid duration flops`),
 	}}
 
 	for _, tc := range cases {
@@ -105,7 +104,8 @@ func TestNewConfigMapFromData(t *testing.T) {
 				&corev1.ConfigMap{
 					Data: tc.data,
 				})
-			if !reflect.DeepEqual(tc.err, actualErr) {
+
+			if tc.err != nil && tc.err.Error() != actualErr.Error() {
 				t.Fatalf("Error = %v, want: %v", actualErr, tc.err)
 			}
 
diff --git a/tracing/config/tracing_test.go b/tracing/config/tracing_test.go
index 309d5225c..7e1cf30c6 100644
--- a/tracing/config/tracing_test.go
+++ b/tracing/config/tracing_test.go
@@ -257,11 +257,6 @@ func TestNewConfigFailures(t *testing.T) {
 		input: map[string]string{
 			sampleRateKey: "1.01",
 		},
-	}, {
-		name: "debug key not a bool",
-		input: map[string]string{
-			debugKey: "not a bool",
-		},
 	}, {
 		name: "zipkin set without backend",
 		input: map[string]string{