fix kubectl create deployment image name

[zhouya0]

What type of PR is this?

/kind bug

What this PR does / why we need it:
Kubectl create deployment command will use image's name as container's name. When image's name have "_" or "."(which is valide in docker), it will be failed because the name doesn't meet the DNS1123 rules. So it PR will delete these two symbol.
Which issue(s) this PR fixes:

Fixes #kubernetes/kubectl#789

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix kubectl create deployment image name

[zhouya0]

/test pull-kubernetes-e2e-gce

[thockin]

Hi! This is a start, but I don't think it is sufficient. kubectl create deployment supports multiple images, and I could legally say foo_bar and foobar and foo.bar in the same pod, which would collide. I am not saying it's a great idea, but it is valid and needs to be handled.

It gets more complicated - you can't simply escape characters. E.g. if I turned _ into -underscore- a) it could still collide and b) it could overflow.

Also I am not 100% confident that those are the only 2 such characters that we need to handle. Maybe, but I'd like to see a comment citing a spec or something authoritative.

To do this "properly" you need to look at the complete set of container images and their sanitized-generated names. If none of them collide, no problem. If any of them collide either a) throw an error and fail (see below) or b) further randomize them by adding a unique suffix (see below).

Option a is terrible because the user has no recourse. If that is ACTUALLY what they want to do, they simply can't. To fix this, we'd need a way for them to manually remap container names, which actually sounds OK to me. For example --image=bar-qux=foo/bar_qux:123. If we detect an = sign, one half becomes the container name ("bar-qux") and the other half becomes the image. In fact, if we do this, we don't need to sanitize at all, maybe?

Option b) is terrible because you need to stay less than max-length, so you end up potentially eating the last few characters to replace them with suffix, which demands further conflict checking.

[thockin]

Assigning to myself just so I see notifications

[zhouya0]

Thanks for your reply. But by far i haven't thought out solutions, but the image naming rules are referred by https://github.com/moby/moby/blob/be97c66708c24727836a22247319ff2943d91a03/daemon/names/names.go

[thockin]

Revisiting.

What about normalizing the names and then uniquifiying? That should be such a corner-case it won't matter.

E.g.

names = []
for each value in --image {
    name = sanitize(value)
    if name in names {
        name = uniquify(name, names)
    }
    // process container with name
}

Where sanitize() converts anything not supported into '-' and uniquify() truncates the input and adds a counter to the end. For example inputs of [ "foo_bar", "foo-bar" ] produce [ "foo-bar", "foo-bar-2" ]. Very long names will bump against the limit so "a-very-long-name" might become "a-very-long-na-2". Adding a 5-character hash of the image string might be simpler, still has to truncate.

Last corner case is what to do when the sanitized name is exactly colliding with a natural name, as above: inputs of [ "foo_bar", "foo-bar" ] produce [ "foo-bar", "foo-bar-2" ], but it might be better to produce [ "foo-bar-2", "foo-bar" ]. Probably not worth the hassle.

[zhouya0]

The root cause of this is the different naming rules with docker and k8s. And actually this barely happens(naming images names with "_" or "." ), I just think add two more functions like sanitize and uniquify in this special case is not a good idea.

[thockin]

Well, look. We have a problem. `kubectl run` works fine, but is deprecated in favor of `kubectl create` We have container images that are perfectly valid names which are being rejected by `kubectl create`. Ergo, we can never remove `run` until we fix `create`. The job of Kubernetes is to adapt between humans and container runtimes. `pod.spec.containers[].name` is *NOT* intended to be the image name. It's a human-friendly name. Kubectl is the one deciding to set that value, so it carries the responsibility for doing it correctly. The proposed fix is just an idea, but frankly, I have a hard time imagining a simpler solution that maintains the "correctness" property. The input string must be reduced in its character set, aka escaping. If you want a simpler escape, convert all "_" and "." to "--", and convert all original "--" to "----" but you STILL have a length problem, so you need to truncate, and then you still have a uniqueness problem. We have user-facing documentation which uses `run` and CAN NOT change to `create` until this is fixed.

…

On Tue, Feb 18, 2020 at 2:30 AM zhouya0 ***@***.***> wrote: The root cause of this is the different naming rules with docker and k8s. And actually this barely happens(naming images names with "_" or "." ), I just think add two more functions like sanitize and uniquify in this special case is not a good idea. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#86636?email_source=notifications&email_token=ABKWAVEHUI52DXV5I3FPGQTRDO2DRA5CNFSM4J7IZJD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMBOABY#issuecomment-587390983>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVD2XB6YDI3OPCLHZYTRDO2DRANCNFSM4J7IZJDQ> .

[soltysh]

@zhouya0 how fast can you address Tim's comment, which is valid and I'd love to see happen asap, since this is something that will need to be fixed in other generators as well.

[zhouya0]

@zhouya0 how fast can you address Tim's comment, which is valid and I'd love to see happen asap, since this is something that will need to be fixed in other generators as well.

Ok, I'll try to do it.

[zhouya0]

@thockin I opened a new PR #88403.
The solution exactly follows your suggestion. But I didn't check the string limit. I have some questions:

1. What is the max length of container's name?
2. If the max length is set, where to put? Because is't duplicate of this part of code :
   

   kubernetes/staging/src/k8s.io/apiserver/pkg/storage/names/generate.go

   Lines 42 to 47 in 4e26c35

   	const (
   	// TODO: make this flexible for non-core resources with alternate naming rules.
   	maxNameLength = 63
   	randomLength = 5
   	maxGeneratedNameLength = maxNameLength - randomLength
   	)

[soltysh]

@zhouya0 pls don't open new PRs, but apply the necessary fixes here. Also that other PR only addressed the sanitization part, but did not cover uniqueness Tim mentioned before. Please follow the advices given from reviewers.

[zhouya0]

/test pull-kubernetes-e2e-kind

[zhouya0]

/assign @soltysh
Thanks for your @soltysh advice. I move my code to this PR. But the thing with uniqueness is done by this code isn't it?

name = fmt.Sprintf("%s-%s", name, utilrand.String(5))

[soltysh]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, zhouya0

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:

• staging/src/k8s.io/kubectl/OWNERS [soltysh]

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

[zhouya0]

/kind bug