sclorg / s2i-nodejs-container

NodeJS images based on Red Hat Software Collections and intended for OpenShift and general usage, that provide a platform for building and running NodeJS applications. Users can choose between Red Hat Enterprise Linux, Fedora, and CentOS based images.
http://softwarecollections.org
Apache License 2.0
164 stars 286 forks source link

Use of LD_PRELOAD causes conflict with Dynatrace #385

Closed mhdawson closed 11 months ago

mhdawson commented 1 year ago

Container platform

No response

Version

all

OS version of the container image

RHEL 8

Bugzilla, Jira

No response

Description

Dyantrace tries to inject an agent through LD_PRELOAD but fails if there is already an LD_PRELOAD specified.

libnss_wrapper.so is added to LD_PRELOAD in the containers in https://github.com/sclorg/s2i-nodejs-container/blob/926eb1c9244ad47b2c57357c76894116f0f8cd8d/18/root/opt/app-root/etc/generate_container_user#L16 as an example.

From https://manpages.debian.org/testing/libnss-wrapper/nss_wrapper.1.en.html it sounds like libnss_wrapper.so is often used for testing.

The first question is if this library needed at runtime in the containers or as part of the S2i build or only during internal testing.

While it is acknowlege that Dynatrace should potentially be more tolerant, we have a customer who is having trouble enabling Dynatrace and it would be good to agree on the right thing to change or document in order to help customers in this situation.

Reproducer

No response

mhdawson commented 1 year ago

@kasicka can you tag the people from the RHEL team that might have the context on how libnss_wrapper is being used?

pkubatrh commented 1 year ago

I was not around for its introduction, so take my answer with a grain of salt (@hhorak might remember more) but I believe nss_wrapper was used for the Openshift use case - Openshift runs the image as a user with a random ID, but some of the images (postgresql for example) expects the image to be run under a specific username. nss_wrapper was used to bridge the gap. iirc when we were looking at the minimal images for node, the question about nss_wrapper being used came up as well, and we decided to remove it and see if it breaks anything. In my opinion it is not needed as node (afaik) does not expect a user with a specific username, so I would be up to removing it entirely if that is really the case.

mhdawson commented 1 year ago

We had originally thought that the minimal images did not include it but the customer later reported still having issues. I'll try to validate with them that they are actually using the minimal image as @pkubatrh sounds like the nss_wrapper should have been removed from those.

pkubatrh commented 1 year ago

Dug into this for more details. The nss_wrapper packages are certainly not installed in the minimal images and the images do not even have the generate_container_user file the full sized images do. I also tracked down the original request that added this for node: https://github.com/sclorg/s2i-base-container/issues/91 Seems like there was at least one dependency that was looking at passwd in the past and did not find the random UID Openshift user there. Actually not sure if this is still the case, or if there was some enablement for Openshift use cases in the container runtimes (or base images) to make this more seamless.

pkubatrh commented 1 year ago

That said, even the original issue that introduced the change said "The bad news, is that I'm having trouble reproducing https://github.com/openshift/s2i-base/issues/91 :(((" (https://github.com/sclorg/s2i-nodejs-container/pull/95#issuecomment-214944810) so my opinion about complete removal in case it is needed still stands unchanged.

zmiklank commented 1 year ago

The nss_wrapper packages are certainly not installed in the minimal images and the images do not even have the generate_container_user file the full sized images do.

Indeed. We removed the generate_container_user last year (https://github.com/sclorg/s2i-nodejs-container/commit/48c83985b9934a25830693d3275be1ae7eafaef1) as it was not needed, nor usable without nss_wrapper which is not installed by minimal Dockerfiles.

rauferna commented 1 year ago

Hi, I have followed customer's steps and, indeed, we can see the nss_wrapper lib loaded.

$ oc import-image ubi9/nodejs-16-minimal:1-101 --from=registry.access.redhat.com/ubi9/nodejs-16-minimal:1-101 --confirm

$ oc new-app image-registry.openshift-image-registry.svc:5000/nss/nodejs-16-minimal:1-101~https://github.com/sclorg/nodejs-ex.git

$ oc rsh nodejs-ex-74d7cb8758-p8fss 
sh-5.1$ cat /proc/21/environ | tr '\0' '\n' | grep -i LD_PRELOAD
LD_PRELOAD=libnss_wrapper.so

Why can we see the library loaded, if it is not included? Is it added by some other process?

pkubatrh commented 1 year ago

Ah, this is about the downstream images. In that case yes, LD_PRELOAD is still present in those, as we did not release a new version that is synced with upstream sources yet.

rauferna commented 1 year ago

Thanks @pkubatrh. Is there any planned date for the new version release, which does not include it?

mhdawson commented 1 year ago

A work around until there is a release that reflects the code where setting LD_PRELOAD is removed might be to:

1) use a technique similar to that in https://developers.redhat.com/articles/2022/06/29/how-add-libraries-nodejs-container-s2i to build new base image in openshift 2) Replace what is in the dockerfile in the example (what follows "dockerfile: |") with

   FROM [registry.access.redhat.com/ubi9/nodejs-16-minimal:1-101](http://registry.access.redhat.com/ubi9/nodejs-16-minimal:1-101)
   RUN rm /opt/app-root/etc/generate_container_user

3) Use the image from the imagestream as outlined in https://developers.redhat.com/articles/2022/06/29/how-add-libraries-nodejs-container-s2i in the oc new-app command.

mhdawson commented 11 months ago

@pkubatrh lets sync on the plan for this. I will put it on the agenda for the next interlock

pkubatrh commented 11 months ago

I am just wondering if there is anything else we need to plan for. The change mentioned above (generate_container_user file removal) is present in released images now and I expect that is all that was left to be done. That said I only checked ubi9/nodejs-16-minimal manually so please let us know in case you find it missing in some other version of the node images

rauferna commented 11 months ago

Hi, My customer confirmed that ubi9/nodejs-16-minimal image works fine for them. We can close this.

mhdawson commented 11 months ago

@pkubatrh thanks for the confirmation, I was not aware the removal was complete and just wanted to follow up on that. Given that is has been removed and @rauferna confirmed things are working for the customer I think we can close this out.