Repository: knative/pkg
Pull request #1380 (2020-06-04 12:02:17)

Use annotation rather than label for configmap checksum
Slightly more correct to use an annotation rather than a label since this isn't something one would select on.

/assign @markusthoemmes 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1380#" title="Author self-approved">julz</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:

- **[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)**
- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/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/example.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268513478876860417/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[configmap/hash-gen/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268513478876860417/artifacts/line-cov.html#file1) | Do not exist | 68.0% | 
[webhook/configmaps/configmaps.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268513478876860417/artifacts/line-cov.html#file2) | 93.9% | 94.3% | 0.3

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/example.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268516246282833920/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[configmap/hash-gen/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268516246282833920/artifacts/line-cov.html#file1) | Do not exist | 68.0% | 
[webhook/configmaps/configmaps.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268516246282833920/artifacts/line-cov.html#file2) | 93.9% | 94.3% | 0.3

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/example.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268518888723714049/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[configmap/hash-gen/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268518888723714049/artifacts/line-cov.html#file1) | Do not exist | 68.0% | 
[webhook/configmaps/configmaps.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1380/pull-knative-pkg-go-coverage/1268518888723714049/artifacts/line-cov.html#file2) | 93.9% | 94.3% | 0.3

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1380#" title="Author self-approved">julz</a>*, *<a href="https://github.com/knative/pkg/pull/1380#pullrequestreview-424654080" title="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:

- **[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)**
- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/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/1380#" title="Author self-approved">julz</a>*, *<a href="https://github.com/knative/pkg/pull/1380#pullrequestreview-424654080" title="Approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1380#pullrequestreview-424953575" title="Approved">mattmoor</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:

- ~~[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)~~ [mattmoor]
- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/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":[]} -->
julz: tested out a bump locally and it looks like this should Just Work with a regular `./hack/update-deps.sh --upgrade && ./hack/update-codegen.sh` without any staging needed \o/. Unholding.

/unhold

Patch:
diff --git a/configmap/example.go b/configmap/example.go
index 2ffe448e5..78f8e4a57 100644
--- a/configmap/example.go
+++ b/configmap/example.go
@@ -25,11 +25,11 @@ const (
 	// ExampleKey signifies a given example configuration in a ConfigMap.
 	ExampleKey = "_example"
 
-	// ExampleChecksumLabel is the label that stores the computed checksum.
-	ExampleChecksumLabel = "knative.dev/example-checksum"
+	// ExampleChecksumAnnotation is the annotation that stores the computed checksum.
+	ExampleChecksumAnnotation = "knative.dev/example-checksum"
 )
 
-// Checksum generates a checksum for the example value to be compared against a respective label.
+// Checksum generates a checksum for the example value to be compared against a respective annotation.
 func Checksum(value string) string {
 	return fmt.Sprintf("%08x", crc32.ChecksumIEEE([]byte(value)))
 }
diff --git a/configmap/hash-gen/main.go b/configmap/hash-gen/main.go
index c6e5110e3..7bafc5a56 100644
--- a/configmap/hash-gen/main.go
+++ b/configmap/hash-gen/main.go
@@ -68,19 +68,24 @@ func process(data []byte) ([]byte, error) {
 		return nil, nil
 	}
 
-	labels := traverse(content, "metadata", "labels")
-	if labels == nil {
-		// TODO(markusthoemmes): Potentially handle missing metadata and labels?
-		return nil, errors.New("'metadata.labels' not found")
+	metadata := traverse(content, "metadata")
+	if metadata == nil {
+		return nil, errors.New("'metadata' not found")
+	}
+
+	annotations := traverse(metadata, "annotations")
+	if annotations == nil {
+		annotations = &yaml.Node{Kind: yaml.MappingNode}
+		metadata.Content = append(metadata.Content, strNode("annotations"), annotations)
 	}
 
 	checksum := configmap.Checksum(example.Value)
-	existingLabel := value(labels, configmap.ExampleChecksumLabel)
-	if existingLabel != nil {
-		existingLabel.Value = checksum
+	existingAnnotation := value(annotations, configmap.ExampleChecksumAnnotation)
+	if existingAnnotation != nil {
+		existingAnnotation.Value = checksum
 	} else {
-		labels.Content = append(labels.Content,
-			strNode(configmap.ExampleChecksumLabel),
+		annotations.Content = append(annotations.Content,
+			strNode(configmap.ExampleChecksumAnnotation),
 			strNode(checksum))
 	}
 
diff --git a/webhook/configmaps/configmaps.go b/webhook/configmaps/configmaps.go
index f6ffb81b7..acac11525 100644
--- a/webhook/configmaps/configmaps.go
+++ b/webhook/configmaps/configmaps.go
@@ -185,8 +185,8 @@ func (ac *reconciler) validate(ctx context.Context, req *admissionv1beta1.Admiss
 	}
 
 	exampleData, hasExampleData := newObj.Data[configmap.ExampleKey]
-	exampleChecksum, hasExampleChecksumLabel := newObj.Labels[configmap.ExampleChecksumLabel]
-	if hasExampleData && hasExampleChecksumLabel &&
+	exampleChecksum, hasExampleChecksumAnnotation := newObj.Annotations[configmap.ExampleChecksumAnnotation]
+	if hasExampleData && hasExampleChecksumAnnotation &&
 		exampleChecksum != configmap.Checksum(exampleData) {
 		return fmt.Errorf(
 			"%q modified, you likely wanted to create an unindented configuration",


Test patch:
diff --git a/configmap/hash-gen/testdata/add_want.yaml b/configmap/hash-gen/testdata/add_want.yaml
index 22de0a376..fb131aaf8 100644
--- a/configmap/hash-gen/testdata/add_want.yaml
+++ b/configmap/hash-gen/testdata/add_want.yaml
@@ -19,6 +19,7 @@ metadata:
   namespace: knative-serving
   labels:
     serving.knative.dev/release: devel
+  annotations:
     knative.dev/example-checksum: b05e1ab4
 data:
   leave-me: "alone"
diff --git a/configmap/hash-gen/testdata/update.yaml b/configmap/hash-gen/testdata/update.yaml
index 73125ed30..1e5874f6d 100644
--- a/configmap/hash-gen/testdata/update.yaml
+++ b/configmap/hash-gen/testdata/update.yaml
@@ -19,6 +19,7 @@ metadata:
   namespace: knative-serving
   labels:
     serving.knative.dev/release: devel
+  annotations:
     knative.dev/example-checksum: bla
 data:
   leave-me: "alone"
diff --git a/configmap/hash-gen/testdata/update_want.yaml b/configmap/hash-gen/testdata/update_want.yaml
index 22de0a376..fb131aaf8 100644
--- a/configmap/hash-gen/testdata/update_want.yaml
+++ b/configmap/hash-gen/testdata/update_want.yaml
@@ -19,6 +19,7 @@ metadata:
   namespace: knative-serving
   labels:
     serving.knative.dev/release: devel
+  annotations:
     knative.dev/example-checksum: b05e1ab4
 data:
   leave-me: "alone"
diff --git a/configmap/testing/configmap.go b/configmap/testing/configmap.go
index 5c4a2d8c1..34ed5cd30 100644
--- a/configmap/testing/configmap.go
+++ b/configmap/testing/configmap.go
@@ -83,12 +83,12 @@ func ConfigMapsFromTestFile(t *testing.T, name string, allowed ...string) (*core
 		}
 	}
 
-	// Check that the hashed exampleBody matches the assigned label, if present.
-	gotChecksum, hasExampleChecksumLabel := orig.Labels[configmap.ExampleChecksumLabel]
-	if hasExampleBody && hasExampleChecksumLabel {
+	// Check that the hashed exampleBody matches the assigned annotation, if present.
+	gotChecksum, hasExampleChecksumAnnotation := orig.Annotations[configmap.ExampleChecksumAnnotation]
+	if hasExampleBody && hasExampleChecksumAnnotation {
 		wantChecksum := configmap.Checksum(exampleBody)
 		if gotChecksum != wantChecksum {
-			t.Errorf("example checksum label = %s, want %s", gotChecksum, wantChecksum)
+			t.Errorf("example checksum annotation = %s, want %s", gotChecksum, wantChecksum)
 		}
 	}
 
diff --git a/webhook/configmaps/configmaps_test.go b/webhook/configmaps/configmaps_test.go
index eba75bdd6..6ba4c2f0b 100644
--- a/webhook/configmaps/configmaps_test.go
+++ b/webhook/configmaps/configmaps_test.go
@@ -222,8 +222,8 @@ func TestDenyInvalidUpdateConfigMapExample(t *testing.T) {
 
 	r := &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{
-				configmap.ExampleChecksumLabel: "foo",
+			Annotations: map[string]string{
+				configmap.ExampleChecksumAnnotation: "foo",
 			},
 		},
 		Data: map[string]string{