Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

podman play should honour both ENTRYPOINT and CMD from image #8666

Closed
wants to merge 1 commit into from
Closed

podman play should honour both ENTRYPOINT and CMD from image #8666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions pkg/specgen/generate/kube/kube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,20 @@ func ToSpecGen(ctx context.Context, containerYAML v1.Container, iid string, newI

// TODO: We dont understand why specgen does not take of this, but
// integration tests clearly pointed out that it was required.
s.Command = []string{}
// s.Command = []string{}
imageData, err := newImage.Inspect(ctx)
if err != nil {
return nil, err
}
s.WorkDir = "/"

// TODO: determine if `newImage.Inspect(ctx)` can ever return without `err`
// TODO: and have `imageData` or `imageData.Config` equal `nil`
// TODO: if it can, determine how to handle that error generally and globally
if imageData != nil && imageData.Config != nil {
if imageData.Config.WorkingDir != "" {
s.WorkDir = imageData.Config.WorkingDir
}
s.Command = imageData.Config.Entrypoint
s.Labels = imageData.Config.Labels
if len(imageData.Config.StopSignal) > 0 {
stopSignal, err := util.ParseSignal(imageData.Config.StopSignal)
Expand All @@ -126,13 +129,29 @@ func ToSpecGen(ctx context.Context, containerYAML v1.Container, iid string, newI
s.StopSignal = &stopSignal
}
}
if len(containerYAML.Command) != 0 {
s.Command = containerYAML.Command
}

// doc https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes
if len(containerYAML.Args) != 0 {
s.Command = append(s.Command, containerYAML.Args...)

if len(containerYAML.Command) == 0 && len(containerYAML.Args) == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I am pretty sure that the initialization with []string{} from L109 is not necessary. It runs fine here without and s.ContainerBasicConfig.Command and .Entrypoint, which in theory should suffer from the same problem, are initialized just fine.

s.Entrypoint = imageData.Config.Entrypoint
s.Command = imageData.Config.Cmd
}

if len(containerYAML.Command) != 0 && len(containerYAML.Args) == 0 {
s.Entrypoint = containerYAML.Command
s.Command = nil
}

if len(containerYAML.Command) == 0 && len(containerYAML.Args) != 0 {
s.Entrypoint = imageData.Config.Entrypoint
s.Command = containerYAML.Args
}

if len(containerYAML.Command) != 0 && len(containerYAML.Args) != 0 {
s.Entrypoint = containerYAML.Command
s.Command = containerYAML.Args
}
Comment on lines +135 to +153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! can we boil this down to a simple "set entrypoint/command from imageData and override from containerYAML when set" ? I haven't seen this PR before I started fixing this and this would be my patch of this file: https://github.com/containers/podman/compare/master...bziemons:patch-kube-play-8428.patch

I also want to emphasize that the if imageData != nil && imageData.Config != nil { above is probably there for a reason and accessing imageData.Config without it is probably not a good idea.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with @bziemons, line: 132,142 need the the imageData != nil && imageData.Config != nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! can we boil this down to a simple "set entrypoint/command from imageData and override from containerYAML when set" ? I haven't seen this PR before I started fixing this and this would be my patch of this file: https://github.com/containers/podman/compare/master...bziemons:patch-kube-play-8428.patch

Yes, we can do that. It is functionally equivalent and less code, but it lacks the clarity in mirroring the k8s specs that I wished to express. I feel that your proposition warrants some line of comments to establish that link, but apart from that, I wouldn't complain.

I also want to emphasize that the if imageData != nil && imageData.Config != nil { above is probably there for a reason and accessing imageData.Config without it is probably not a good idea.

I fear I lost the context here. Are you referring to L115 before setting s.WorkDir = imageData.Config.WorkingDir? I did not notice that, but I supsect it is not necessary, because imageData is populated with imageData, err := newImage.Inspect(ctx) at L110. Any failure to get the imageData returns there.

In addition, I cannot imagine how to start any container without having both imageData and imageData.Config. Accordingly, the corresponding check, if it really is necessary, is misplaced when setting single values and should be performed at the "global" level somewhar at libpod/image/image.go, Inspect or when examining the return value of it's invocation at L110.

Copy link
Contributor Author

@cruwe cruwe Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also want to emphasize that the if imageData != nil && imageData.Config != nil { above is probably there for a reason and accessing imageData.Config without it is probably not a good idea.

I fear I lost the context here

Ok, now I get it. That part, if it is really necessary, about which I am yet not sure, should error much higher in the chain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you and the if should have gotten a comment from its writer about why it is needed. Even if it isn't needed it wouldn't make sense to make the code more confusing about it (I know that wasn't your intention ;) ). Thanks for accepting my feedback though


// FIXME,
// we are currently ignoring imageData.Config.ExposedPorts
if containerYAML.WorkingDir != "" {
Expand Down
235 changes: 176 additions & 59 deletions test/e2e/play_kube_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package integration

import (
"bytes"
"encoding/json"
"fmt"
"io/ioutil"
"os"
Expand Down Expand Up @@ -490,6 +491,12 @@ func getDeployment(options ...deploymentOption) *Deployment {

type deploymentOption func(*Deployment)

func withDeploymentName(name string) deploymentOption {
return func(deployment *Deployment) {
deployment.Name = name
}
}

func withDeploymentLabel(k, v string) deploymentOption {
return func(deployment *Deployment) {
deployment.Labels[k] = v
Expand Down Expand Up @@ -752,6 +759,18 @@ type EnvFrom struct {
From string
}

type ContainerConfig struct {
Cmd []string `json:"Cmd,omitempty"`
Entrypoint string `json:"Entrypoint"`
// we do not yer care for the rest
}

type ContainerState struct {
Args []string `json:"Args,omitempty"`
Config ContainerConfig `json:"Config"`
// we do not yer care for the rest
}

func milliCPUToQuota(milliCPU string) int {
milli, _ := strconv.Atoi(strings.Trim(milliCPU, "m"))
return milli * defaultCPUPeriod
Expand Down Expand Up @@ -803,76 +822,184 @@ var _ = Describe("Podman play kube", func() {

})

It("podman play kube test correct command", func() {
pod := getPod()
// NOTICE: what is called k8s spec in the following section is described at the k8s docs at
// https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes

It("podman play kube - k8s spec, 1st item, when you _do not_ supply command _or_ args", func() {
// given an image with `ENTRYPOINT` and `CMD` defined and a pod-spec _without any_,
// when the container is instantiated from the image, it should use `Entrypoint` and `Cmd`
// from the image and `Args` should equal `Cmd`

pod := getPod(withCtr(getCtr(withImage(redis), withCmd(nil), withArg(nil))))
err := generateKubeYaml("pod", pod, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod), "--format", "'{{ .Config.Cmd }}'"})
inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod)})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))
// Use the defined command to override the image's command
correctCmd := "[" + strings.Join(defaultCtrCmd, " ") + " " + strings.Join(defaultCtrArg, " ")
Expect(inspect.OutputToString()).To(ContainSubstring(correctCmd))

var containerState []ContainerState
err = json.Unmarshal([]byte(inspect.OutputToString()), &containerState)
Expect(err).To(BeNil())

// these we know from having had a look at the redis image
imgEntrypoint := `docker-entrypoint.sh`
imgCmd := []string{`redis-server`}
derivedArgs := imgCmd

containerEntrypoint := containerState[0].Config.Entrypoint
containerCmd := containerState[0].Config.Cmd
containerArgs := containerState[0].Args

Expect(containerEntrypoint).To(Equal(imgEntrypoint))
Expect(containerCmd).To(Equal(imgCmd))
Expect(containerArgs).To(Equal(derivedArgs))
})

It("podman play kube test correct command with only set command in yaml file", func() {
pod := getPod(withCtr(getCtr(withCmd([]string{"echo", "hello"}), withArg(nil))))
It("podman play kube - k8s spec, 2nd item, when you supply command _but no_ args", func() {
// given an image with `ENTRYPOINT` and `CMD` defined and a pod-spec with the `command`
// defined, when the container is instantiated from the image, it should replace any
// `Entrypoint` and `Cmd` and start with `command` _without_ further `Args`.

overrideCmd := []string{"echo", "hello"}
pod := getPod(withCtr(getCtr(withImage(redis), withCmd(overrideCmd), withArg(nil))))
err := generateKubeYaml("pod", pod, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod), "--format", "'{{ .Config.Cmd }}'"})
inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod)})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))
// Use the defined command to override the image's command, and don't set the args
// so the full command in result should not contains the image's command
Expect(inspect.OutputToString()).To(ContainSubstring(`[echo hello]`))

var containerState []ContainerState
err = json.Unmarshal([]byte(inspect.OutputToString()), &containerState)
Expect(err).To(BeNil())

// these we know from having had a look at the redis image
imgEntrypoint := `docker-entrypoint.sh`
imgCmd := []string{`redis-server`}

containerEntrypoint := containerState[0].Config.Entrypoint
containerCmd := containerState[0].Config.Cmd
containerArgs := containerState[0].Args

Expect(containerEntrypoint).NotTo(Equal(imgEntrypoint))
Expect(containerCmd).NotTo(Equal(imgCmd))

Expect(containerEntrypoint).To(Equal(strings.Join(overrideCmd, " ")))
Expect(len(containerCmd)).To(Equal(0))
Expect(containerArgs).To(Equal(overrideCmd[1:]))
})

It("podman play kube test correct command with only set args in yaml file", func() {
pod := getPod(withCtr(getCtr(withImage(redis), withCmd(nil), withArg([]string{"echo", "hello"}))))
It("podman play kube - k8s spec, 3rd item, when you supply _only_ args", func() {
// given an image with `ENTRYPOINT` and `CMD` defined and a pod-spec with the `command`
// _not_ defined, but `args` given, when the container is instantiated from the image,
// the `Entrypoint` should be taken from the image and `Cmd`/`Args` being replaced by `args`.

args := []string{"echo", "hello"}
pod := getPod(withCtr(getCtr(withImage(redis), withCmd(nil), withArg(args))))
err := generateKubeYaml("pod", pod, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod), "--format", "'{{ .Config.Cmd }}'"})
inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod)})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))
// this image's ENTRYPOINT is called `docker-entrypoint.sh`
// so result should be `docker-entrypoint.sh + withArg(...)`
Expect(inspect.OutputToString()).To(ContainSubstring(`[docker-entrypoint.sh echo hello]`))

var containerState []ContainerState
err = json.Unmarshal([]byte(inspect.OutputToString()), &containerState)
Expect(err).To(BeNil())

// these we know from having had a look at the redis image
imgEntrypoint := `docker-entrypoint.sh`
imgCmd := []string{`redis-server`}

containerEntrypoint := containerState[0].Config.Entrypoint
containerCmd := containerState[0].Config.Cmd
containerArgs := containerState[0].Args

Expect(containerEntrypoint).To(Equal(imgEntrypoint))
Expect(containerCmd).NotTo(Equal(imgCmd))
Expect(containerCmd).To(Equal(args))
Expect(containerArgs).To(Equal(args))

})

It("podman play kube test correct output", func() {
p := getPod(withCtr(getCtr(withCmd([]string{"echo", "hello"}), withArg([]string{"world"}))))
It("podman play kube - k8s spec, 4th item, when you supply command _and_ args", func() {
// given any image, irrespective of `ENTRYPOINT` and `CMD` being defined in the image or not,
// both `command` and `args` from the pod spec should _replace_ the images' `ENTRYPOINT` and `CMD`
// and the container's `Args` should equal `args`.
pod := getPod(withCtr(getCtr(withImage(redis), withCmd(defaultCtrCmd), withArg(defaultCtrArg))))
//
err := generateKubeYaml("pod", pod, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(pod)})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))

var containerState []ContainerState
err = json.Unmarshal([]byte(inspect.OutputToString()), &containerState)
Expect(err).To(BeNil())

// these we know from having had a look at the redis image
imgEntrypoint := `docker-entrypoint.sh`
imgCmd := []string{`redis-server`}

err := generateKubeYaml("pod", p, kubeYaml)
containerEntrypoint := containerState[0].Config.Entrypoint
containerCmd := containerState[0].Config.Cmd
containerArgs := containerState[0].Args

Expect(containerEntrypoint).NotTo(Equal(imgEntrypoint))
Expect(containerCmd).NotTo(Equal(imgCmd))

Expect(containerEntrypoint).To(Equal(strings.Join(defaultCtrCmd, " ")))
Expect(containerCmd).To(Equal(defaultCtrArg))
Expect(containerArgs).To(Equal(defaultCtrArg))
})

It("podman play kube - test output being logged", func() {
pod := getPod(withCtr(getCtr(withCmd([]string{"echo"}), withArg([]string{"hello", "world"}))))
err := generateKubeYaml("pod", pod, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

logs := podmanTest.Podman([]string{"logs", getCtrNameInPod(p)})
logs := podmanTest.Podman([]string{"logs", getCtrNameInPod(pod)})
logs.WaitWithDefaultTimeout()
Expect(logs.ExitCode()).To(Equal(0))
Expect(logs.OutputToString()).To(ContainSubstring("hello world"))
})

inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(p), "--format", "'{{ .Config.Cmd }}'"})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))
Expect(inspect.OutputToString()).To(ContainSubstring(`[echo hello world]`))
It("podman play kube - test output being logged with strange, but legal `command`/`args` invocation", func() {
pod := getPod(withCtr(getCtr(withCmd([]string{"echo", "hello"}), withArg([]string{"world"}))))
err := generateKubeYaml("pod", pod, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

logs := podmanTest.Podman([]string{"logs", getCtrNameInPod(pod)})
logs.WaitWithDefaultTimeout()
Expect(logs.ExitCode()).To(Equal(0))
Expect(logs.OutputToString()).To(ContainSubstring("hello world"))
})

It("podman play kube test restartPolicy", func() {
Expand Down Expand Up @@ -1226,43 +1353,33 @@ spec:
Expect(ctr[0].Config.StopSignal).To(Equal(uint(51)))
})

// Deployment related tests
It("podman play kube deployment 1 replica test correct command", func() {
deployment := getDeployment()
err := generateKubeYaml("deployment", deployment, kubeYaml)
Expect(err).To(BeNil())
It("podman play kube deployment - test deployments' replica count", func() {
numReplicas := []int{1, 2, 5}
ctrOptions := getCtr(withCmd([]string{"echo"}), withArg([]string{"hello", "world"}))

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))
for _, replicaCount := range numReplicas {

podNames := getPodNamesInDeployment(deployment)
inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(&podNames[0]), "--format", "'{{ .Config.Cmd }}'"})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))
// yaml's command shuold override the image's Entrypoint
correctCmd := "[" + strings.Join(defaultCtrCmd, " ") + " " + strings.Join(defaultCtrArg, " ")
Expect(inspect.OutputToString()).To(ContainSubstring(correctCmd))
})

It("podman play kube deployment more than 1 replica test correct command", func() {
var i, numReplicas int32
numReplicas = 5
deployment := getDeployment(withReplicas(numReplicas))
err := generateKubeYaml("deployment", deployment, kubeYaml)
Expect(err).To(BeNil())
pod := getPod(withCtr(ctrOptions))
deployment := getDeployment(withReplicas(int32(replicaCount)), withPod(pod), withDeploymentName(fmt.Sprintf("with-%v-repl", replicaCount)))
err := generateKubeYaml("deployment", deployment, kubeYaml)
Expect(err).To(BeNil())

kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))
kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube.ExitCode()).To(Equal(0))

podNames := getPodNamesInDeployment(deployment)
correctCmd := "[" + strings.Join(defaultCtrCmd, " ") + " " + strings.Join(defaultCtrArg, " ")
for i = 0; i < numReplicas; i++ {
inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(&podNames[i]), "--format", "'{{ .Config.Cmd }}'"})
inspect.WaitWithDefaultTimeout()
Expect(inspect.ExitCode()).To(Equal(0))
Expect(inspect.OutputToString()).To(ContainSubstring(correctCmd))
podNames := getPodNamesInDeployment(deployment)
Expect(len(podNames)).To(Equal(replicaCount))

for idx, podName := range podNames {
println(getCtrNameInPod(&podName))
inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(&podNames[idx])})
println(inspect)
logs := podmanTest.Podman([]string{"logs", getCtrNameInPod(&podNames[idx])})
logs.WaitWithDefaultTimeout()
Expect(logs.ExitCode()).To(Equal(0))
Expect(logs.OutputToString()).To(ContainSubstring("hello world"))
}
}
})

Expand Down