Add missing binaryData field to the ConfigMap Hash #61146

dims · 2018-03-14T01:37:33Z

What this PR does / why we need it:

In 7e158fb, we added a BinaryData
to ConfigMap, but totally forgot to add it to the hash method.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

The value of of BinaryData is now used in calculating the hash for the configmap

dims · 2018-03-14T01:38:06Z

/assign @mtaufen
/assign @liggitt

dims · 2018-03-14T01:39:53Z

/kind bug
/sig api-machinery
/priority important-soon

dims · 2018-03-14T01:40:10Z

/milestone v1.10

liggitt · 2018-03-14T01:40:50Z

pkg/kubectl/util/hash/hash.go

-	data, err := json.Marshal(map[string]interface{}{"kind": "ConfigMap", "name": cm.Name, "data": cm.Data})
+	m := map[string]interface{}{"kind": "ConfigMap", "name": cm.Name, "data": cm.Data}
+	if len(cm.BinaryData) > 0 {
+		m["binaryData"] = cm.BinaryData


does the stdlib marshal []byte in unstructured maps correctly?

yes, see TestEncodeConfigMap method "three keys" variation. uses base64 encode

https://golang.org/pkg/encoding/json/#Marshal

Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string

mtaufen · 2018-03-14T01:56:19Z

pkg/kubectl/util/hash/hash_test.go

+		// three keys with binary data (tests sorting order)
+		{"three keys with binary data", &v1.ConfigMap{BinaryData: map[string][]byte{"two": []byte("2"), "one": []byte(""), "three": []byte("3")}}, "t458mc6db2", ""},
+		// two keys, one with string and another with binary data
+		{"two keys with one each", &v1.ConfigMap{BinaryData: map[string][]byte{"one": []byte("")}, Data: map[string]string{"two": ""}}, "b8b92gc26c", ""},


personal nit: keep field order the same as the struct definition; swap BinaryData and Data so Data comes first

mtaufen · 2018-03-14T01:56:40Z

pkg/kubectl/util/hash/hash_test.go

+		// three keys with binary data (tests sorting order)
+		{"three keys", &v1.ConfigMap{BinaryData: map[string][]byte{"two": []byte("2"), "one": []byte(""), "three": []byte("3")}}, `{"binaryData":{"one":"","three":"Mw==","two":"Mg=="},"data":null,"kind":"ConfigMap","name":""}`, ""},
+		// two keys, one string and one binary values
+		{"two keys", &v1.ConfigMap{BinaryData: map[string][]byte{"one": []byte("")}, Data: map[string]string{"two": ""}}, `{"binaryData":{"one":""},"data":{"two":""},"kind":"ConfigMap","name":""}`, ""},


mtaufen · 2018-03-14T01:59:10Z

pkg/kubectl/util/hash/hash.go

-	data, err := json.Marshal(map[string]interface{}{"kind": "ConfigMap", "name": cm.Name, "data": cm.Data})
+	m := map[string]interface{}{"kind": "ConfigMap", "name": cm.Name, "data": cm.Data}
+	if len(cm.BinaryData) > 0 {
+		m["binaryData"] = cm.BinaryData


https://golang.org/pkg/encoding/json/#Marshal

Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string

mtaufen · 2018-03-14T02:01:26Z

/status approved-for-milestone

k8s-github-robot · 2018-03-14T02:01:41Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dims @liggitt @mtaufen

Pull Request Labels

sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

In 7e158fb, we added a BinaryData to ConfigMap, but totally forgot to add it to the hash method.

mtaufen · 2018-03-14T02:08:27Z

pkg/kubectl/util/hash/hash_test.go

 		// one key
 		{"one key", &v1.ConfigMap{Data: map[string]string{"one": ""}}, "9g67k2htb6", ""},
 		// three keys (tests sorting order)
 		{"three keys", &v1.ConfigMap{Data: map[string]string{"two": "2", "one": "", "three": "3"}}, "f5h7t85m9b", ""},
+		// empty binary data map
+		{"empty binary data", &v1.ConfigMap{BinaryData: map[string][]byte{}}, "dk855m5d49", ""},


Why does this have a different hash from "empty data"? Oh... I bet json.Marshal encodes null for Data in this case, and an empty object in the other one?

Can you print the serialization to double check?
Maybe we should also consider nilling-out empty maps before hashing to make this consistent, since we don't semantically treat nil and empty as different in the API. Making this change might break backwards compatibility of the hash... or maybe not since Data was omitempty anyway?

@mtaufen compare line 95 and 101, i have the encoded strings there. the difference between empty and null Data is existing behavior. Are we allowed to change that?

Right now, --append-hash is only available as an option to the kubectl create configmap subcommand, and it looks like we always allocate the map there: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/configmap.go#L131.

We might get away with encoding nil maps as {} (treating them the same as empty maps) in our encoding function. We should look at secrets too, since the same problem probably exists in that encoding function.

@liggitt wdyt?

@mtaufen ack, i can make the change quickly if it sounds ok to @liggitt

@liggitt WDYT? (please see @mtaufen 's comment above)

I wouldn't change existing treatment of data, or mess with nilling out map values. The length check on binaryData seems fine to me.

dims · 2018-03-14T02:35:24Z

/milestone clear

per discussion on #sig-release with @jdumars and @liggitt this can wait until 1.10.1 (merge when master opens up and then cherry pick after 1.10 ships)

dims · 2018-03-14T13:56:47Z

/test pull-kubernetes-e2e-gce

mtaufen · 2018-03-14T14:25:40Z

sgtm wrt milestone

jennybuckley · 2018-03-15T20:20:19Z

/cc @cheftako

dims · 2018-03-27T00:05:11Z

@mtaufen now that master is open, let's please get this in.

mtaufen · 2018-03-27T17:46:22Z

I want to settle the nil/empty thing first, but otherwise this lgtm.

liggitt · 2018-03-27T23:53:41Z

/lgtm

mtaufen · 2018-03-28T00:49:56Z

/lgtm

k8s-ci-robot · 2018-03-28T00:50:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, mtaufen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubectl/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-03-28T08:34:52Z

Automatic merge from submit-queue (batch tested with PRs 60465, 61773, 61371, 61146). If you want to cherry-pick this change to another branch, please follow the instructions here.

xcompass · 2018-06-06T22:00:59Z

Shouldn't this be in 1.10 branch? As the binary data field in configmap is introduced in 1.10 and it seems it doesn't work without change.

mtaufen · 2018-06-06T22:30:46Z

@dims was this cherry-picked?

dims · 2018-06-06T23:34:10Z

@mtaufen Done! filed #64859

dims · 2018-06-06T23:34:57Z

@xcompass @mtaufen the code in 1.10 will work even without this change, was it failing for you?

xcompass · 2018-06-06T23:41:59Z

when I create the configmap with binary data field, the configmap created is empty. I thought it is related to the missing hash.

▶ ls -lha
total 200
drwxr-xr-x   3 compass  staff    96B Apr 30 15:33 .
drwxr-xr-x  11 compass  staff   352B May 29 00:35 ..
-rw-r--r--   1 compass  staff   100K Apr 30 15:28 saml.jks

▶ kubectl create configmap --from-file=saml.jks test-config
configmap "test-config" created

▶ kubectl get cm test-config
NAME          DATA      AGE
test-config   0         20s

▶ kubectl describe cm test-config
Name:         test-config
Namespace:    default
Labels:       <none>
Annotations:  <none>

Data
====
Events:  <none>

Client Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.3", GitCommit:"2bba0127d85d5a46ab4b778548be28623b32d0b0", GitTreeState:"clean", BuildDate:"2018-05-21T09:17:39Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.3+coreos.0", GitCommit:"f1b890dbbf11abe58280b3ffe17d67749f5ae70e", GitTreeState:"clean", BuildDate:"2018-05-21T17:27:17Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}

xcompass · 2018-06-07T06:12:22Z

Just found out that the data is actually there, I have to use get cm -o yaml to see it. It is kind of misleading when nothing shows up using get cm or describe cm

…pstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #61146: Add missing binaryData field to the ConfigMap Hash Cherry pick of #61146 on release-1.10. #61146: Add missing binaryData field to the ConfigMap Hash ```release-note The value of of BinaryData is now used in calculating the hash for the configmap ```

k8s-ci-robot requested review from adohe-zz and shiywang March 14, 2018 01:37

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 14, 2018

k8s-ci-robot assigned liggitt and mtaufen Mar 14, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 14, 2018

k8s-ci-robot added this to the v1.10 milestone Mar 14, 2018

k8s-github-robot added the milestone/needs-approval label Mar 14, 2018

liggitt reviewed Mar 14, 2018

View reviewed changes

mtaufen reviewed Mar 14, 2018

View reviewed changes

k8s-ci-robot added the status/approved-for-milestone label Mar 14, 2018

k8s-github-robot removed the milestone/needs-approval label Mar 14, 2018

Add missing binaryData field to the ConfigMap Hash

4cbdbae

In 7e158fb, we added a BinaryData to ConfigMap, but totally forgot to add it to the hash method.

dims force-pushed the add-missing-binary-field-to-configmap-hash branch from 9c6c7d8 to 4cbdbae Compare March 14, 2018 02:08

mtaufen reviewed Mar 14, 2018

View reviewed changes

k8s-ci-robot removed this from the v1.10 milestone Mar 14, 2018

k8s-ci-robot requested a review from cheftako March 15, 2018 20:20

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 27, 2018

k8s-github-robot merged commit e00a602 into kubernetes:master Mar 28, 2018

dims mentioned this pull request Jun 6, 2018

Automated cherry pick of #61146: Add missing binaryData field to the ConfigMap Hash #64859

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 7, 2018

Add missing binaryData field to the ConfigMap Hash #61146

Add missing binaryData field to the ConfigMap Hash #61146

Conversation

dims commented Mar 14, 2018 • edited Loading

dims commented Mar 14, 2018

dims commented Mar 14, 2018

dims commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtaufen commented Mar 14, 2018

k8s-github-robot commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Mar 14, 2018

dims commented Mar 14, 2018

mtaufen commented Mar 14, 2018

jennybuckley commented Mar 15, 2018

dims commented Mar 27, 2018

mtaufen commented Mar 27, 2018

liggitt commented Mar 27, 2018

mtaufen commented Mar 28, 2018

k8s-ci-robot commented Mar 28, 2018

k8s-github-robot commented Mar 28, 2018

xcompass commented Jun 6, 2018

mtaufen commented Jun 6, 2018

dims commented Jun 6, 2018

dims commented Jun 6, 2018

xcompass commented Jun 6, 2018 • edited Loading

xcompass commented Jun 7, 2018 • edited Loading

dims commented Mar 14, 2018 •

edited

Loading

xcompass commented Jun 6, 2018 •

edited

Loading

xcompass commented Jun 7, 2018 •

edited

Loading