Correct generate kube on containers userns annotation
#25948
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
test/e2e/generate_kube_test.go
Outdated
| _ = podmanTest.PodmanExitCleanly("run", "--userns", "auto:size=10", "-dt", "--name", name1, ALPINE, "top") | ||
| _ = podmanTest.PodmanExitCleanly("run", "--userns", "auto:size=10", "-dt", "--name", name2, ALPINE, "top") |
There was a problem hiding this comment.
They have the exact same userns option so I would expect that to work and just use that one?! I agree that it should error if they are actually different.
There was a problem hiding this comment.
Fair enough, I'll check if the annotations are identical and allow if so
The `podman generate kube` command on containers follows a different codepath from pods. Pods store a lot of pod-level configuration - including user namespace information - in annotations, so it can be restored by `play kube`. Generating for a container does not do the same thing, because we don't have a pod. However, per-container generation was still generating a nearly identical user namespace annotation to a pod. Example: In Pod: io.podman.annotations.userns: auto:size=40 Not in Pod: io.podman.annotations.userns/awesomegreider: auto:size=2048 The second annotation seems like it should apply a user namespace config to the generated Kubernetes pod. Instead, it's just adding an annotation to the awesomegreider container, that says said container has a user namespace, when it does not in fact have a user namespace configured because it is now in a pod. After this PR, both containers in and out of pods generate identical annotations (the In Pod version, missing container name) and as such should generate pods with appropriately configured user namespaces. I also added some conflict detection to refuse to generate if you try to generate YAML containing two containers with conflicting user namespace configuration. Fixes containers#25896 Signed-off-by: Matt Heon <[email protected]>
| kubeAnnotations[k] = v | ||
| } else { | ||
| kubeAnnotations[fmt.Sprintf("%s/%s", k, removeUnderscores(ctr.Name()))] = v | ||
| } |
There was a problem hiding this comment.
I'm going to play the grumpy old man yelling "keep off my grass" for a moment. The code is fine, but I detest one letter variables, especially so if they are used multiple times across more than one or two lines. I realize k and v were predefined in the code, but in instances like this, please consider expanding them to perhaps key and value, just to make them much easier to pick out of the code, and IMO, makes it an easier read/review.
I now return back to my recliner to get back to napping.
|
LGTM |
The
podman generate kubecommand on containers follows a different codepath from pods. Pods store a lot of pod-level configuration - including user namespace information - in annotations, so it can be restored byplay kube. Generating for a container does not do the same thing, because we don't have a pod.However, per-container generation was still generating a nearly identical user namespace annotation to a pod. Example:
In Pod:
io.podman.annotations.userns: auto:size=40
Not in Pod:
io.podman.annotations.userns/awesomegreider: auto:size=2048
The second annotation seems like it should apply a user namespace config to the generated Kubernetes pod. Instead, it's just adding an annotation to the awesomegreider container, that says said container has a user namespace, when it does not in fact have a user namespace configured because it is now in a pod.
After this PR, both containers in and out of pods generate identical annotations (the In Pod version, missing container name) and as such should generate pods with appropriately configured user namespaces. I also added some conflict detection to refuse to generate if you try to generate YAML containing two containers with conflicting user namespace configuration.
Fixes #25896
Does this PR introduce a user-facing change?