Run generate.CompleteSpec() for initContainers as well

[dcermak]

initContainers in kubernetes deployments had no call to CompleteSpec in the generation, which means that the default environment is not configured for these. This causes issues with missing default environment variables like $HOME or $PATH.

This fixes #18384

Does this PR introduce a user-facing change?

- ensure that initContainers in k8s deployments have a correctly setup default environment

[Luap99]

Can you add a test for it? Also see #17107 for an example, I guess you can just add it there.

[dcermak]

I have added a regression test. I am not 100% happy with it though, as it just tests that the (by default unset) environment variable container equals podman in the initContainers' CMD. If you have ideas how to improve it, I'd be more than happy to get your input.

[Luap99]

I have added a regression test. I am not 100% happy with it though, as it just tests that the (by default unset) environment variable container equals podman in the initContainers' CMD. If you have ideas how to improve it, I'd be more than happy to get your input.

Couldn't you just add your test directly to the one in https://github.com/containers/podman/pull/17107/files, just add a init container to the yaml there and the check the env var for the init container.

[dcermak]

Couldn't you just add your test directly to the one in https://github.com/containers/podman/pull/17107/files, just add a init container to the yaml there and the check the env var for the init container.

That would be an option as well. I picked the e2e tests hoping that I would be able to access podman's internal data structures, but that turned out to be a misconception on my end. I can move this to the same place as #17107 if you consider that a better place.

[Luap99]

if you consider that a better place.

I think that makes sense. General speaking they test the same thing so having them to together is better IMO.
However because you already wrote the e2e test I am totally fine with that, I don't want to put extra work on you. The important part is to have a test not where it is.

[dcermak]

if you consider that a better place.

I think that makes sense. General speaking they test the same thing so having them to together is better IMO. However because you already wrote the e2e test I am totally fine with that, I don't want to put extra work on you. The important part is to have a test not where it is.

I have added it into the bats tests as a fixup commit. Please pick your favorite, I'll delete the other :-)

[Luap99]

@edsantiago @vrothberg PTAL at the system test.
code LGTM

[TomSweeneyRedHat]

LGTM
would like a head nod from @edsantiago

[edsantiago]

Nice work! One comment inline re: my neuroses about testing... and, as my atonement for ranting, here's a possible patch that is short, minimally invasive, fails on podman@main, passes with your PR.

diff --git a/test/e2e/play_kube_test.go b/test/e2e/play_kube_test.go
index ca0624b13..e43c93045 100644
--- a/test/e2e/play_kube_test.go
+++ b/test/e2e/play_kube_test.go
@@ -2210,7 +2210,7 @@ var _ = Describe("Podman play kube", func() {
 	// With annotation set to always
 	It("podman play kube test with init containers and annotation set", func() {
 		// With the init container type annotation set to always
-		pod := getPod(withAnnotation("io.podman.annotations.init.container.type", "always"), withPodInitCtr(getCtr(withImage(ALPINE), withCmd([]string{"echo", "hello"}), withInitCtr(), withName("init-test"))), withCtr(getCtr(withImage(ALPINE), withCmd([]string{"top"}))))
+		pod := getPod(withAnnotation("io.podman.annotations.init.container.type", "always"), withPodInitCtr(getCtr(withImage(ALPINE), withCmd([]string{"printenv", "container"}), withInitCtr(), withName("init-test"))), withCtr(getCtr(withImage(ALPINE), withCmd([]string{"top"}))))
 		err := generateKubeYaml("pod", pod, kubeYaml)
 		Expect(err).ToNot(HaveOccurred())
 
@@ -2233,6 +2233,12 @@ var _ = Describe("Podman play kube", func() {
 		inspect.WaitWithDefaultTimeout()
 		Expect(inspect).Should(Exit(0))
 		Expect(inspect.OutputToString()).To(ContainSubstring("running"))
+
+		// Init containers need environment too! #18385
+		logs := podmanTest.Podman([]string{"logs", "testPod-init-test"})
+		logs.WaitWithDefaultTimeout()
+		Expect(logs).Should(Exit(0))
+		Expect(logs.OutputToString()).To(Equal("podman"))
 	})
 
 	// If you have an init container in the pod yaml, podman should create and run the init container with play kube

This is a suggestion only, there are many ways to do something similar.

Now, two favors please:

1. squash commits, for a nice clean history, and
2. rebase, because there have been many drastic changes to our e2e tests since your branch point.

Thank you!

[edsantiago]

As the poor schmoe who has to look at test failures, I reeeeeeeeally hate this kind of test. Here's why:

Sample failure:
     test failed! Exit status was nonzero!

...and, for comparison, here is what makes me wiggle my toes with joy:

    test failed: '$container' variable in container is undefined

or

    test failed: `$container` variable in container is 'something-surprising' (expected it to be 'podman')

That latter one is (probably) impossible IRL, so, yes, the test as it is will almost certainly show a failure to set envariable... but if you make a general practice of show-the-value type of testing, every so often you will get a surprise failure and the test logs will show exactly what happened and you will have a fix ready in minutes instead of hours.

And, I write this in the spirit of appreciation (you fixed a bug! you took the time to write a test! Then you wrote MORE tests!). So, thank you!

[dcermak]

@edsantiago Thanks for the extensive review! I have applied your testing patch as it tests the important part and is far smaller than my own tests (I removed these as well, as there's imho no point in testing the same thing three ways, but I can certainly add them back if you prefer that)

[edsantiago]

LGTM, thanks again!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcermak, edsantiago

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:

• OWNERS [edsantiago]

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

[Luap99]

/lgtm