crate / docker-crate

Source repository for the official CrateDB Docker image
https://registry.hub.docker.com/_/crate/
Apache License 2.0
49 stars 22 forks source link

Remove explicit upgrade of `openssl` package #218

Closed hammerhead closed 1 year ago

hammerhead commented 1 year ago

Summary of the changes / Why this is an improvement

The openssl packages provides the following files:

sh-5.1# repoquery --list openssl
Last metadata expiration check: 0:00:06 ago on Tue Mar 14 13:24:59 2023.
/usr/bin/make-dummy-cert
/usr/bin/openssl
/usr/bin/renew-dummy-cert
/usr/lib/.build-id
/usr/lib/.build-id/84
/usr/lib/.build-id/84/1bf9c21ad5c7f2ec794159ff8ef2c11f78d498
/usr/share/doc/openssl
/usr/share/doc/openssl/Makefile.certificate
... [man pages/other READMEs skipped]

None of these files appear to be used when building the image.

Open question: Is any cloud-related component depending on it?

Checklist

hammerhead commented 1 year ago

Not sure about the integration test failure, passes locally for me. The same error occurred on master as well a few times lately as per https://github.com/crate/docker-crate/actions/workflows/test.yml

amotl commented 1 year ago

Hi Niklas.

The crash CLI is bundled with the OCI image, and it may need the OpenSSL package. If it would only be about the root certificate bundle, we could look into using the certifi package [^1], but I guess openssl will be a real runtime dependency for the Python SSL bindings?

With kind regards, Andreas.

[^1]: - https://github.com/earthobservations/wetterdienst/issues/827#issuecomment-1454804618

amotl commented 1 year ago

Observation

Unrelated to this patch, I've spotted this on the CI run:

AssertionError: b'ls: cannot access /crate-*: No such file or directory\n' != b"ls: cannot access '/crate-*': No such file or directory\n"

-- https://github.com/crate/docker-crate/actions/runs/4416371380/jobs/7740683329#step:5:57

Background

Within GH-206, we needed 0ed7191 to compensate for a different output first, but then have been able to remove it with 7135142 again, after AlmaLinux apparently changed something. It looks like they changed it back to the previous behavior?

hammerhead commented 1 year ago

I just realised that openssl is actually installed by default already, as part of the base image. yum install -y openssl only updates it.

Nevertheless, the openssl package seems very limited, besides tons of man pages, it really only provides the executables make-dummy-cert, renew-dummy-cert, and renew-dummy-cert. I couldn't spot any actual certificates.

As we don't do a general system-wide package upgrade, was there a specific reason to update openssl in particular?

hammerhead commented 1 year ago

Looking at the old CentOS-based Dockerfile, openssl was installed there as well. Are we just seeing a copy&paste leftover from CentOS here in the way that it was required to install openssl explicitly on CentOS, but now on AlmaLinux it's already provided by default?

Here you can see it's already installed by default: https://hub.docker.com/layers/library/almalinux/9.1/images/sha256-0a7e7312fd88caf158361b97dc3c62e5cba46dff34b8ccf6baa9a3a66957baa8?context=explore:

Screenshot 2023-03-14 at 16 17 52
amotl commented 1 year ago

Thanks for your investigations 💯. I've wrapped the outcome about omitting openssl into another patch which further shrinks the image size based on your suggestions, and reported about it at https://github.com/crate/docker-crate/issues/219#issuecomment-1468418331.

On the failing CI task, you may want to rebase, now that GH-220 has been merged.

hammerhead commented 1 year ago

Will open a new PR.