Remove runAs from test deployment files

[jwendell]

They're not strictly required and do not play well with OpenShift.

[istio-testing]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[howardjohn]

A bit worried about this. I think this means it will run as root since that is the image default? And running as root makes the test moot, IIUC, since we have different iptables rules for root vs non-root?

Plausible I am wrong

[howardjohn]

A bit worried about this. I think this means it will run as root since that is the image default? And running as root makes the test moot, IIUC, since we have different iptables rules for root vs non-root?

Plausible I am wrong

[bleggett]

A bit worried about this. I think this means it will run as root since that is the image default?

AFAIK it means processes inside the container will run as root inside the container, since there's no longer anything telling them to run as anything else. OpenShift won't actually run the processes as root on the node, though.

And running as root makes the test moot, IIUC, since we have different iptables rules for root vs non-root?

Plausible I am wrong

I'm not sure what we're testing here? The only things that actually do anything with UID 1338 are

• pkg/test/echo/docker/Dockerfile.app_sidecar_base_centos
• pkg/test/echo/docker/Dockerfile.app_sidecar_base
  but we never actually seem to use that user for anything, presumably the runAs stuff was a (rather fragile) way of forcing that user to be used.

Either way adding USER application to both of those dockerfiles would be equivalent to specifying the runAs stuff in the manifest - it will just make all subsequent commands run as the user switched to by USER. That shouldn't affect the tests and probably won't make OShift unhappy, since it shouldn't care if nonroot users are created inside the container or not.

That being said, exactly zero tests failed here which implies it's actually a no-op, so maybe it doesn't matter :D

[howardjohn]

Yeah if we add `USER` to those SGTM

…

On Wed, Nov 15, 2023 at 12:54 PM Ben Leggett ***@***.***> wrote: A bit worried about this. I think this means it will run as root since that is the image default? AFAIK it means processes inside the container will run as root inside the container. OpenShift won't actually run the processes as root on the node, though. And running as root makes the test moot, IIUC, since we have different iptables rules for root vs non-root? Plausible I am wrong I'm not sure what we're testing here? The only things that actually do anything with UID 1338 are - pkg/test/echo/docker/Dockerfile.app_sidecar_base_centos - `pkg/test/echo/docker/Dockerfile.app_sidecar_base but we never actually *use* that user for anything. Either way adding USER application to both of those dockerfiles would be equivalent to specifying the runAs stuff in the manifest, and shouldn't affect the tests and *probably* won't make OShift unhappy. — Reply to this email directly, view it on GitHub <#47850 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXNX5D6D57OFDZJ6BZTYEUT2HAVCNFSM6AAAAAA7LXUE4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJTGI2DQMBQGI> . You are receiving this because your review was requested.Message ID: ***@***.***>

[bleggett]

Adding USER to both dockerfiles that actually create a user with UID 1338 would be equivalent.

But, I actually don't see any tests that rely on the root-vs-nonroot behavior so I don't think it really matters for the tests.

[jwendell]

We'd need to add USER to pkg/test/echo/docker/Dockerfile.app which is the main Dockerfile for the echo app. Let me try.

[linsun]

If adduser works for openshift and all other k8s, can we actually document this somewhere too, with some explanation why openshift requires this?

[bleggett]

If actually running the test app as the application user doesn't work/causes the tests to fail, then that means @howardjohn was correct and the runAs was a no-op - we don't actually use that created user for any tests.

In that case let's just remove that custom user we create (but never actually use) entirely from all the Dockerfiles, e.g.

• pkg/test/echo/docker/Dockerfile.app_sidecar_base_centos
• pkg/test/echo/docker/Dockerfile.app_sidecar_base

as well, those are also likely useless/vestigial.

[jwendell]

@howardjohn @bleggett Adding USER to the Dockerfile works as before. This is good, right?

@linsun Where specifically do you expect such documentation? These are test files, only used in integration tests.

[bleggett]

@howardjohn @bleggett Adding USER to the Dockerfile works as before. This is good, right?

Yep! My bad, I saw the previous test failure and didn't realize it was because some of the changes were missing in that particular push, ignore that comment.

[bleggett]

LGTM

[jwendell]

@howardjohn @bleggett Adding USER to the Dockerfile works as before. This is good, right?

Yep! My bad, I saw the previous test failure and didn't realize it was because some of the changes were missing in that particular push, ignore that comment.

Yeah, those failures were related to USER application. It works with USER 1338 though.

[jwendell]

/retest

[linsun]

@howardjohn @bleggett Adding USER to the Dockerfile works as before. This is good, right?

@linsun Where specifically do you expect such documentation? These are test files, only used in integration tests.

Sorry for not replying this earlier, i think you documented well in #47898 - thank you!!

[istio-testing]

In response to a cherrypick label: new pull request created: #47966