Add --append-hash flag to kubectl create configmap/secret #49961

mtaufen · 2017-08-01T16:25:54Z

What this PR does / why we need it:
Specifying this new flag will automatically hash the configmap/secret
contents with sha256 and append the first 40 hex-encoded bits of the
hash to the name of the configmap/secret. This is especially useful for
workflows that generate configmaps/secrets from files (e.g.
--from-file).

See this Google doc for more background:
https://docs.google.com/document/d/1x1fJ3pGRx20ujR-Y89HUAw8glUL8-ygaztLkkmQeCdU/edit

Release note:

Adds --append-hash flag to kubectl create configmap/secret, which will append a short hash of the configmap/secret contents to the name during creation.

mtaufen · 2017-08-01T16:26:36Z

/cc @bgrant0607 @pwittrock @erictune

dims · 2017-08-01T16:52:45Z

/unassign @dims

liggitt · 2017-08-01T16:58:48Z

this seems like a rarely called-for option that I find it odd to bake into these commands... if you want a name based on a hash of the contents, you can create that yourself external to the command.

cc @kubernetes/sig-cli-pr-reviews

mtaufen · 2017-08-01T17:29:27Z

I don't think I'd call it "rarely called for." The recommended workflow for rolling out new ConfigMaps and Secrets is to create new ones and update references. The new ones need new names. Most workflows that generate and roll out new ConfigMaps and Secrets via `--from-file` thus need to generate new names, in which case they can all just use `--hash` instead of coming up with fragmented, bespoke solutions.

…

On Tue, Aug 1, 2017 at 9:59 AM, Jordan Liggitt ***@***.***> wrote: this seems like a rarely called-for option that I find it odd to bake into these commands... if you want a name based on a hash of the contents, you can create that yourself external to the command. cc @kubernetes/sig-cli-pr-reviews <https://github.com/orgs/kubernetes/teams/sig-cli-pr-reviews> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49961 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3Jwdm8ItKXROuuqa5lms7_DwPTNQ0nks5sT1l2gaJpZM4Op-cM> .

-- Michael Taufen MTV SWE

bgrant0607 · 2017-08-01T17:59:34Z

@liggitt This is one part of a solution to #22368. My expected usage model is for users to do kubectl create configmap from their source data whenever their source data changes, and then update env and volume references from Deployment, DaemonSet, etc. to refer to the new resource. That enables a predictable rolling update.

kow3ns

The approach your taking will concatenate the upper 40 bits of the SHA2-256 hash of the contents with a user supplied name to provide a unique name for a ConfigMap. This differs from the approach taken by the controllers when generating unique names in that it doesn't handle collisions. Given the low probability of collision based on the expected number "versions" of the ConfigMap, the collision resistance of SHA2-256, and the use case, this is probably okay, but, if a collision does occur how does a user resolve it? Would they just delete the existing ConfigMap and create a new one?

kow3ns · 2017-08-01T18:55:08Z

pkg/kubectl/util/hash/hash.go

+)
+
+// HashConfigMap returns a hash of the EncodeConfigMap encoding of `cm`
+func HashConfigMap(cm *api.ConfigMap) string {


hash.Hash stutters - linter will not like it

Yeah just noticed that. Thanks.

kow3ns · 2017-08-01T18:55:35Z

pkg/kubectl/util/hash/hash.go

+}
+
+// HashSecret returns a hash of the EncodeSecret encoding of `sec`
+func HashSecret(sec *api.Secret) string {


Same as above - linter will be pissed

mtaufen · 2017-08-01T19:49:18Z

I don't think we should resolve collisions. The chance of a collision is very low (~1 in 200 million, with 100 concurrent ConfigMaps with the same name prefix). I think the reduced complexity is worth those odds. If someone does hit a collision, it's pretty easy to manually change the source data by adding a bogus ConfigMap key. And you should never have to do this twice in your lifetime (assuming <100 ConfigMaps that share a prefix, and infrequent changes, which is reasonable).

kow3ns · 2017-08-02T14:31:46Z

/retest

bgrant0607 · 2017-08-03T16:17:49Z

+1 to @mtaufen's proposal of how to deal with collisions. I think we can do without additional complexity for now.

janetkuo · 2017-08-06T07:15:06Z

pkg/kubectl/util/hash/hash.go

+// with `,`. If m is non-empty, there is a trailing comma in the pre-hash serialization. If m is empty,
+// there is no trailing comma. The entire encoding starts with "{" and ends with "}".
+func encodeMapStringString(m map[string]string) string {
+	kv := make([][]string, len(m))


[]string should be enough? Store keys of m in it and sort those keys. You can get value from m[key] when encoding to a string

Done, thanks Janet!

liggitt · 2017-08-26T15:29:39Z

pkg/kubectl/util/hash/hash.go

+
+// encodeConfigMap encodes a ConfigMap by encoding the Data with EncodeMapStringString and wrapping with {} braces
+func encodeConfigMap(cm *api.ConfigMap) string {
+	return fmt.Sprintf("{data:%s,}", encodeMapStringString(cm.Data))


including the pre-hash name and the kind (Secret, ConfigMap, etc) would prevent comparison between two hash suffixes based solely on the name of the object (e.g. you could deduce secrets foo-014fbff9a07c and bar-014fbff9a07c likely contain identical data based only on their names)

liggitt · 2017-08-26T19:32:00Z

pkg/kubectl/util/hash/hash.go

+
+// encodeSecret encodes a Secret by encoding the Data with EncodeMapStringByte and
+// appending ",type:"+sec.Type+"," to the encoded string, and wraps with {} braces
+func encodeSecret(sec *api.Secret) string {


same here, json.Marshal(map[string]interface{}{"kind":"Secret","name":sec.Name,"data":sec.Data}) seems more straightforward

liggitt · 2017-08-26T19:33:31Z

pkg/kubectl/util/hash/hash.go

+// We picked some arbitrary consonants to map to from the same character set as GenerateName.
+// See: https://github.com/kubernetes/apimachinery/blob/dc1f89aff9a7509782bde3b68824c8043a3e58cc/pkg/util/rand/rand.go#L75
+func encodeHash(hex string) string {
+	enc := []rune(hex[:10])


length check on hex

liggitt · 2017-08-28T20:15:25Z

pkg/apis/extensions/v1beta1/zz_generated.conversion.go

@@ -333,7 +332,8 @@ func Convert_extensions_DaemonSetList_To_v1beta1_DaemonSetList(in *extensions.Da

 func autoConvert_v1beta1_DaemonSetSpec_To_extensions_DaemonSetSpec(in *v1beta1.DaemonSetSpec, out *extensions.DaemonSetSpec, s conversion.Scope) error {
 	out.Selector = (*v1.LabelSelector)(unsafe.Pointer(in.Selector))
-	if err := api_v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+	// TODO: Inefficient conversion - can we improve it?


you got a bad generation here... these diffs shouldn't exist

Huh. I'll look into it.

liggitt · 2017-08-28T20:43:35Z

pkg/kubectl/util/hash/hash.go

+// Data, Kind, and Name are taken into account.
+func encodeConfigMap(cm *api.ConfigMap) (string, error) {
+	// json.Marshal sorts the keys in a stable order in the encoding
+	data, err := json.Marshal(cm.Data)


any reason not to delegate the full serialization to json.Marshal rather than mixing with sprinf?

Including all of ObjectMeta makes the hash dependent on too many unnecessary fields, IMO. And it would automatically bind us to any future ObjectMeta (or Secret/ConfigMap) fields, which we may not want to include in the hash.

Cleared up on Slack; comment recommended json.Marshal(map[string]interface{}) over Sprintf. Agree this is better.

liggitt · 2017-08-28T20:49:37Z

other than fixing the generated change and the preference for delegating the serialization fully to json.Marshal, no other comments from me

Specifying this new flag will automatically hash the configmap/secret contents with sha256 and append the first 40 hex-encoded bits of the hash to the name of the configmap/secret. This is especially useful for workflows that generate configmaps/secrets from files (e.g. --from-file). Note that vowels and vowel-like characters in the hash are remapped to consonants to make it more difficult to accidentally form bad words. See this Google doc for more background: https://docs.google.com/document/d/1x1fJ3pGRx20ujR-Y89HUAw8glUL8-ygaztLkkmQeCdU/edit

mtaufen · 2017-08-28T22:39:49Z

/retest

liggitt · 2017-08-29T13:42:08Z

LGTM

janetkuo · 2017-08-29T20:07:27Z

/lgtm

janetkuo · 2017-08-29T20:08:47Z

/approve no-issue

k8s-github-robot · 2017-08-29T20:09:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, janetkuo, mtaufen

Associated issue requirement bypassed by: janetkuo

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

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

~~pkg/kubectl/OWNERS~~ [janetkuo]

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

fejta-bot · 2017-08-29T23:49:48Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-30T01:37:24Z

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

k8s-github-robot · 2017-08-30T04:43:28Z

Automatic merge from submit-queue (batch tested with PRs 49961, 50005, 50738, 51045, 49927)

anguslees · 2017-10-01T05:58:30Z

Repeating my comment on the "background doc" here for visibility:

[When used on Secrets] This exposes the contents of the secret in the hash, and could be used in a dictionary attack to discover the plaintext secret.

My recommendation is to add a sizeable salt when hashing Secrets, and include the salt in the output hash value (unfortunately making it longer).
Another alternative that would be acceptable for user-namespace Secrets (but not "well known" kube-system Secrets?) is to include the object-name prefix and namespace in the hash. (Makes the pre-computed dictionary table non-portable)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2017

mtaufen assigned janetkuo and kow3ns Aug 1, 2017

k8s-github-robot assigned smarterclayton and dims Aug 1, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 1, 2017

k8s-ci-robot requested review from bgrant0607, erictune and pwittrock August 1, 2017 16:26

mtaufen force-pushed the kubectl-hash branch from 160b6dd to 4cc328f Compare August 1, 2017 16:45

mtaufen added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Aug 1, 2017

mtaufen added this to the v1.8 milestone Aug 1, 2017

k8s-ci-robot unassigned dims Aug 1, 2017

mtaufen force-pushed the kubectl-hash branch from 4cc328f to 9d0aaa1 Compare August 1, 2017 17:51

kow3ns reviewed Aug 1, 2017

View reviewed changes

mtaufen force-pushed the kubectl-hash branch from 9d0aaa1 to f167e6c Compare August 1, 2017 19:26

kow3ns approved these changes Aug 2, 2017

View reviewed changes

janetkuo reviewed Aug 6, 2017

View reviewed changes

mtaufen force-pushed the kubectl-hash branch from f167e6c to f17a4d6 Compare August 7, 2017 00:03

liggitt reviewed Aug 26, 2017

View reviewed changes

mtaufen force-pushed the kubectl-hash branch 3 times, most recently from 0c0b9b8 to dbbb846 Compare August 28, 2017 19:35

liggitt reviewed Aug 28, 2017

View reviewed changes

mtaufen force-pushed the kubectl-hash branch from dbbb846 to 3802196 Compare August 28, 2017 20:56

mtaufen force-pushed the kubectl-hash branch from 3802196 to 503a6a8 Compare August 28, 2017 21:18

apelisse mentioned this pull request Aug 29, 2017

kubectl cycle pods kubernetes/kubectl#56

Closed

janetkuo approved these changes Aug 29, 2017

View reviewed changes

k8s-ci-robot assigned janetkuo Aug 29, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2017

k8s-github-robot merged commit 2cf5118 into kubernetes:master Aug 30, 2017

bgrant0607 mentioned this pull request Sep 22, 2017

add manifest type #52570

Closed

janetkuo mentioned this pull request Oct 10, 2017

Add a proposal for ConfigMap/Secret garbage collection kubernetes/community#1163

Closed

kav mentioned this pull request May 24, 2021

Support or document existing mechanism to --append-hash to configmaps and secrets helm/helm#9735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --append-hash flag to kubectl create configmap/secret #49961

Add --append-hash flag to kubectl create configmap/secret #49961

mtaufen commented Aug 1, 2017 •

edited by bgrant0607

Loading

mtaufen commented Aug 1, 2017

dims commented Aug 1, 2017

liggitt commented Aug 1, 2017

mtaufen commented Aug 1, 2017 via email •

edited

Loading

bgrant0607 commented Aug 1, 2017

kow3ns left a comment

kow3ns Aug 1, 2017

mtaufen Aug 1, 2017

kow3ns Aug 1, 2017

mtaufen Aug 1, 2017

mtaufen commented Aug 1, 2017

kow3ns commented Aug 2, 2017

bgrant0607 commented Aug 3, 2017

janetkuo Aug 6, 2017

mtaufen Aug 7, 2017

liggitt Aug 26, 2017 •

edited

Loading

liggitt Aug 26, 2017

liggitt Aug 26, 2017

liggitt Aug 28, 2017

mtaufen Aug 28, 2017

mtaufen Aug 28, 2017

liggitt Aug 28, 2017

mtaufen Aug 28, 2017

mtaufen Aug 28, 2017

liggitt commented Aug 28, 2017

mtaufen commented Aug 28, 2017

liggitt commented Aug 29, 2017

janetkuo commented Aug 29, 2017

janetkuo commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

fejta-bot commented Aug 29, 2017

k8s-github-robot commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

anguslees commented Oct 1, 2017 •

edited

Loading

Add --append-hash flag to kubectl create configmap/secret #49961

Add --append-hash flag to kubectl create configmap/secret #49961

Conversation

mtaufen commented Aug 1, 2017 • edited by bgrant0607 Loading

mtaufen commented Aug 1, 2017

dims commented Aug 1, 2017

liggitt commented Aug 1, 2017

mtaufen commented Aug 1, 2017 via email • edited Loading

bgrant0607 commented Aug 1, 2017

kow3ns left a 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 Aug 1, 2017

kow3ns commented Aug 2, 2017

bgrant0607 commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Aug 26, 2017 • edited Loading

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

Choose a reason for hiding this comment

liggitt commented Aug 28, 2017

mtaufen commented Aug 28, 2017

liggitt commented Aug 29, 2017

janetkuo commented Aug 29, 2017

janetkuo commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

fejta-bot commented Aug 29, 2017

k8s-github-robot commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

anguslees commented Oct 1, 2017 • edited Loading

mtaufen commented Aug 1, 2017 •

edited by bgrant0607

Loading

mtaufen commented Aug 1, 2017 via email •

edited

Loading

liggitt Aug 26, 2017 •

edited

Loading

anguslees commented Oct 1, 2017 •

edited

Loading