Add binary configmap #57938

dims · 2018-01-06T23:10:00Z

Reviving code from #33549 submitted by @zreigz

What this PR does / why we need it:
Add support for binary files in ConfigMap

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 #32432

Special notes for your reviewer:

Sample API data:

apiVersion: v1
kind: ConfigMap
data:
  foo: text value
binaryData:
  bar: jmjKxUGNs/WqDQ==

Release note:

ConfigMap objects now support binary data via a new `binaryData` field. When using `kubectl create configmap --from-file`, files containing non-UTF8 data will be placed in this new field in order to preserve the non-UTF8 data. Use of this feature requires 1.10+ apiserver and kubelets.

dims · 2018-01-06T23:10:53Z

New variation based on discussion in #57899

dims · 2018-01-06T23:12:01Z

/assign @liggitt
/assign @thockin

liggitt · 2018-01-07T02:18:33Z

pkg/apis/core/validation/validation.go

@@ -4353,6 +4353,12 @@ func ValidateConfigMap(cfg *core.ConfigMap) field.ErrorList {
 		}
 		totalSize += len(value)
 	}
+	for key, value := range cfg.BinaryData {
+		for _, msg := range validation.IsConfigMapKey(key) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("binarydata").Key(key), key, msg))


liggitt · 2018-01-07T02:18:58Z

pkg/apis/core/validation/validation.go

@@ -4353,6 +4353,12 @@ func ValidateConfigMap(cfg *core.ConfigMap) field.ErrorList {
 		}
 		totalSize += len(value)
 	}
+	for key, value := range cfg.BinaryData {


also need to ensure there is no key duplication between data and binarydata maps

Please see note from @smarterclayton here, there was an ask to remove that validation i believe:
9ba0da8#r100961600

hmm... allowing the same key in both data and binaryData seems buggy and undefined...

also, from #33549 (comment):

Needs a much more descriptive comment, including a description of the validation rules (must not overlap with data)

@smarterclayton can you please chime in?

thockin · 2018-01-07T03:01:19Z

Did you think or write about the options for design? Would we want to make a parallel change to Secret so they both feel the same?

…

On Jan 6, 2018 3:11 PM, "Davanum Srinivas" ***@***.***> wrote: New variation based on discussion in #57899 <#57899> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#57938 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVPKOS-AkTNi5rNxTo7yQhJLq_6aaks5tH_2cgaJpZM4RVeO7> .

dims · 2018-01-07T14:47:05Z

/test pull-kubernetes-verify
/test pull-kubernetes-bazel-test

dims · 2018-01-08T18:47:41Z

What we have today is as follows

staging/src/k8s.io/api/core/v1/types.go:
1. type ConfigMap struct
  - has field Data - which is a map of string to string
2. type Secret struct
  - has field Data - which is a map of string to byte array
  - has write-only convenience field StringData - which is a map of string to string
  - has optional field Type - only possible value is "Opaque"
pkg/apis/core/types.go:
1. type ConfigMap struct
  - has field Data - which is a map of string to string
2. type Secret struct
  - has field Data - which is a map of string to byte array
  - has optional field Type

Proposal is to:

proposed field BinaryData to type ConfigMap (both spots) - which is a map of string to byte array, this will enable to functionality needed to save binary files using ConfigMap.

Problems still that need to be thought about:

Data field is different between Secret and ConfigMap, one takes map with byte array values and another takes string values. This is an existing problem, not introduced by this PR
with this PR we will have BinaryData in ConfigMap and StringData in ConfigMap (and Data in both!) which seems bad.
1. Do we make sure both have BinaryData and StringData?
2. In ConfigMap, we will have two buckets stored in etcd (Data and BinaryData), while in Secret, we have only one. Does it matter to the end user as long as we have validation rules that the buckets cannot have duplicate items?
Do we need a Type in ConfigMap to be consistent with Secret's Type?

Suggestions welcome!

( cc @thockin @liggitt @smarterclayton )

liggitt · 2018-01-08T21:20:09Z

Do we make sure both have BinaryData and StringData?

I don't think so. The existing data field in Secret objects can express both binary and text content. I don't think the improved usability of representing text as text outweighs the loss of compatibility with old clients that wouldn't know about any new field.

In ConfigMap, we will have two buckets stored in etcd (Data and BinaryData), while in Secret, we have only one. Does it matter to the end user as long as we have validation rules that the buckets cannot have duplicate items?

the objects are fundamentally different today, and ConfigMap is less expressive than Secret. duplicating the same data in multiple fields won't work (see https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#backward-compatibility-gotchas - "A single feature/property cannot be represented using multiple spec fields in the same API version simultaneously")

I probably wouldn't try to harmonize the two types here. This change makes ConfigMap as expressive as Secret.

dims · 2018-01-08T21:47:06Z

+1 @liggitt. i agree.

dims · 2018-01-09T11:24:37Z

/assign @smarterclayton

smarterclayton · 2018-01-09T20:59:20Z

api/swagger-spec/v1.json

+     },
+     "binaryData": {
+      "type": "object",
+      "description": "Data contains the binary data. Each key must consist of alphanumeric characters, '-', '_' or '.'."


Better description here, and improve the existing data description. binaryData contains configuration data that is not required to be UTF-8 and .... Describe how the two fields validate together. Describe why I would use one or the other.

smarterclayton · 2018-01-09T21:01:08Z

I agree with Jordan's position - V2 can pick up the pieces, as long as validation is crisp, this is the best way I can see to add the feature without breaking old clients. The minor ugliness seems outweighed by the value in the config map.

I'd like to see the api description significantly improved so users understand when to use each, and understand that keys are exclusive. Improve both. Otherwise makes sense to do this change.

dims · 2018-01-09T22:56:46Z

@smarterclayton @liggitt thanks for the guidance, looks like we are on the right track. will add validation, update description, add some tests over the next few days.

dims · 2018-01-09T22:59:53Z

/hold

liggitt · 2018-01-10T16:52:39Z

pkg/apis/core/types.go

@@ -4383,8 +4383,22 @@ type ConfigMap struct {

 	// Data contains the configuration data.
 	// Each key must consist of alphanumeric characters, '-', '_' or '.'.
+	// The value must be a string (UTF-8). If the value has byte
+	// sequences not in the UTF-8 range, please use the BinaryData field.
+	// The keys stored in Data should not overlap with the keys in


must not overlap

liggitt · 2018-01-10T16:53:11Z

pkg/apis/core/types.go

@@ -4383,8 +4383,22 @@ type ConfigMap struct {

 	// Data contains the configuration data.
 	// Each key must consist of alphanumeric characters, '-', '_' or '.'.
+	// The value must be a string (UTF-8). If the value has byte
+	// sequences not in the UTF-8 range, please use the BinaryData field.


values with non-UTF-8 byte sequences must use the binaryData field

dims · 2018-01-23T11:15:48Z

/retest

dims · 2018-01-23T12:46:23Z

thanks @thockin i've added a comment and rebased the PR to latest master as well.

liggitt · 2018-01-23T12:54:53Z

/lgtm

dims · 2018-01-23T15:59:06Z

/test pull-kubernetes-e2e-kops-aws

dims · 2018-01-23T17:00:49Z

/test pull-kubernetes-e2e-kops-aws

dims · 2018-01-25T01:38:02Z

@pmorie @saad-ali @thockin @matchstick - Can one of you please approve for "pkg/volume/configmap"? thanks!

saad-ali · 2018-01-26T08:10:29Z

/lgtm
/approve

k8s-ci-robot · 2018-01-26T08:10:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dims, liggitt, saad-ali

Associated issue: #32432

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

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~hack/OWNERS~~ [cblecker,liggitt]
~~pkg/apis/core/OWNERS~~ [liggitt]
~~pkg/kubectl/OWNERS~~ [liggitt]
~~pkg/volume/configmap/OWNERS~~ [saad-ali]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt,saad-ali]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-01-26T08:46:27Z

/test all [submit-queue is verifying that this PR is safe to merge]

dims · 2018-01-26T10:47:48Z

/test pull-kubernetes-unit

k8s-github-robot · 2018-01-26T11:50:06Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-01-26T12:34:33Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

mtaufen · 2018-03-14T00:35:11Z

pkg/kubectl/util/hash/hash_test.go

@@ -148,7 +148,7 @@ not their metadata (e.g. the Data of a ConfigMap, but nothing in ObjectMeta).
 		obj      interface{}
 		expect   int
 	}{
-		{"ConfigMap", v1.ConfigMap{}, 3},
+		{"ConfigMap", v1.ConfigMap{}, 4},


Shouldn't you have also updated the hash function to include the new binary data field in the hash? @liggitt @dims

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 6, 2018

k8s-github-robot assigned roberthbailey and ncdc Jan 6, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 6, 2018

k8s-ci-robot assigned liggitt and thockin Jan 6, 2018

liggitt reviewed Jan 7, 2018

View reviewed changes

dims force-pushed the add-binary-configmap branch 2 times, most recently from 8aaaee1 to 3b810dd Compare January 7, 2018 14:29

roberthbailey removed their assignment Jan 8, 2018

k8s-ci-robot assigned smarterclayton Jan 9, 2018

smarterclayton reviewed Jan 9, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2018

dims force-pushed the add-binary-configmap branch from 551f394 to 690244d Compare January 10, 2018 16:25

liggitt reviewed Jan 10, 2018

View reviewed changes

dims force-pushed the add-binary-configmap branch from facdcae to df1351f Compare January 23, 2018 12:28

Lukasz Zajaczkowski and others added 4 commits January 23, 2018 07:28

Add support for binary file in configmap

7e158fb

generated code and docs

426eb04

run a full round trip scenario

43aa077

Add a e2e test for binary data in configmap

df1351f

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018

k8s-ci-robot assigned saad-ali Jan 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2018

k8s-github-robot merged commit 27d01b5 into kubernetes:master Jan 26, 2018

tengqm mentioned this pull request Jan 27, 2018

Document binaryData for ConfigMap kubernetes/website#7108

Closed

2 tasks

mtaufen reviewed Mar 14, 2018

View reviewed changes

sferich888 mentioned this pull request Oct 18, 2018

Missing Binary data in Configmap feature in documentation openshift/openshift-docs#12099

Merged

desimone mentioned this pull request Mar 11, 2019

authorize: support kubernetes pomerium/pomerium#62

Merged

pdecat mentioned this pull request Apr 16, 2019

Create config map from folder hashicorp/terraform-provider-kubernetes#399

Closed

philwebb mentioned this pull request Apr 30, 2020

Add InputStreamSourceToByteArrayConverter spring-projects/spring-boot#21285

Closed

Shegox mentioned this pull request Mar 15, 2021

clarify: add that binaryData is base64-encoded kubernetes/website#27066

Merged

adietish mentioned this pull request Feb 16, 2024

As a user I would like to easily create/edit Secrets in the editor redhat-developer/intellij-kubernetes#663

Closed

Add binary configmap #57938

Add binary configmap #57938

Conversation

dims commented Jan 6, 2018 • edited by liggitt

dims commented Jan 6, 2018

dims commented Jan 6, 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

liggitt Jan 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Jan 7, 2018 via email

dims commented Jan 7, 2018

dims commented Jan 8, 2018 • edited

liggitt commented Jan 8, 2018 • edited

dims commented Jan 8, 2018

dims commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 9, 2018

dims commented Jan 9, 2018

dims commented Jan 9, 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

dims commented Jan 23, 2018

dims commented Jan 23, 2018

liggitt commented Jan 23, 2018

dims commented Jan 23, 2018 • edited

dims commented Jan 23, 2018

dims commented Jan 25, 2018

saad-ali commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

dims commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

Choose a reason for hiding this comment

dims commented Jan 6, 2018 •

edited by liggitt

liggitt Jan 7, 2018 •

edited

dims commented Jan 8, 2018 •

edited

liggitt commented Jan 8, 2018 •

edited

dims commented Jan 23, 2018 •

edited