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.

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

docker: Base the docker image on RedHat UBI #54812

Merged

Conversation

jlinder
Copy link
Collaborator

@jlinder jlinder commented Sep 25, 2020

Before: The docker image was based on Debian 9.12 slim.

Why: This change will help on-prem customers from a security and
compliance perspective. It also aligns with our publishing images into
the RedHat Marketplace.

Now: Published docker images are based on the RedHat UBI 8 base image.

Fixes: #49643

Release note (backward-incompatible change): CockroachDB Docker images
are now based on the RedHat ubi8/ubi base image instead of Debian 9.12
slim. This will help on-prem customers from a security and compliance
perspective.

Before: The docker image was based on Debian 9.12 slim.

Why: This change will help on-prem customers from a security and
compliance perspective. It also aligns with our publishing images into
the RedHat Marketplace.

Now: Published docker images are based on the RedHat UBI 8 base image.

Fixes: cockroachdb#49643

Release note (backward-incompatible change): CockroachDB Docker images
are now based on the RedHat ubi8/ubi base image instead of Debian 9.12
slim. This will help on-prem customers from a security and compliance
perspective.
@jlinder
Copy link
Collaborator Author

jlinder commented Sep 25, 2020

@bdarnell For the "Why", I think I captured the Why ok. If I missed something or didn't capture it quite right, I'm happy to modify the commit message.

And on the release note, I chose "backward-incompatible change" as the category because if a user is building upon our images then those additions are likely to no longer work. "general" seemed appropriate if this doesn't make the bar for backward-incompatible.

@joshimhoff @DuskEagle Wanted to get your eyes on this in case it changes anything you've got set up to work with debian images. I can make a build of this for testing if that's helpful.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DuskEagle and @joshimhoff)

Copy link
Member

@DuskEagle DuskEagle left a comment

Choose a reason for hiding this comment

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

registry.access.redhat.com/ubi8/ubi

:lgtm: . Nothing in CC is dependent on the Docker container being based on Debian.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DuskEagle and @joshimhoff)

@jlinder
Copy link
Collaborator Author

jlinder commented Sep 29, 2020

TFTR!

bors r=bdarnell,DuskEagle

@joshimhoff
Copy link
Collaborator

@jlinder
Copy link
Collaborator Author

jlinder commented Sep 29, 2020

bors r-

@craig
Copy link
Contributor

craig bot commented Sep 29, 2020

Canceled.

@jlinder
Copy link
Collaborator Author

jlinder commented Sep 30, 2020

Since this will be on master only for the moment, I'm re-merging. Ideally, we'd get this into 20.2.

@bdarnell My biggest concern with backporting to 20.2 is that I haven't done much testing of crdb on RedHat (just verified it runs) so I don't have a sense of how stable CRDB is on RedHat. Is this a concern for you? Do you have any other concerns with getting it in so late in the release cycle?

@joshimhoff @DuskEagle What would it take to update the above-mentioned code to work with both Debian and RedHat containers? (because I'd suggest we only backport this to 20.2, if we do.)

bors r=bdarnell,DuskEagle

@joshimhoff
Copy link
Collaborator

What would it take to update the above-mentioned code to work with both Debian and RedHat containers? (because I'd suggest we only backport this to 20.2, if we do.)

Ya we'll take care of it. Sorry to block. Merge away.

@craig
Copy link
Contributor

craig bot commented Sep 30, 2020

Build succeeded:

@craig craig bot merged commit 1ac75dd into cockroachdb:master Sep 30, 2020
@jlinder jlinder deleted the jhl/docker-image-based-on-redhat-ubi branch September 30, 2020 16:25
@@ -1,28 +1,26 @@
FROM debian:9.12-slim
FROM registry.access.redhat.com/ubi8/ubi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ubi-minimal instead of the regular ubi? It looks like all the packages we list below are present there (https://catalog.redhat.com/software/containers/ubi8/ubi-minimal/5c359a62bed8bd75a2c3fba8?container-tabs=packages).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had tried ubi-minimal. Cockroach couldn't find the installed tzinfo. I verified the package was installed but didn't dig deeper at the time. I just dug a bit deeper and found that the minimal image gets it's smaller size by excluding documentation and language files for many RPMs. For tzinfo, all the files in /usr/share/zoneinfo are excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. Sounds like a bug on redhat's side, but when this was reported in rhel-atomic they responded by removing the rest of the tzdata package instead of fixing it. It looks like it's intended to be there since it still shows up in the package list, but the actual contents are missing.

Is there any way to use ubi-minimal and then reinstall the tzdata package? It looks like yum is not present on ubi-minimal and I don't know enough of the redhat ecosystem to know if there's an alternative that is present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tool to use in ubi-minimal is microdnf. This is the primary page I've been referencing for information on what's in each of the UBI image versions and how they differ from each other. The "Based on RHEL packaging" bullet of section 4.1.2 on that page is where I found the note about what is excluded from the ubi-minimal image. The command rpm -Va shows the excluded files for the installed RPMs.

I tried uninstalling the tzdata package and found it is depended upon by a large number of packages. There doesn't appear to be a way to force uninstallation and I'm not finding a way to install a different version of the file.

It does seem odd that they'd intentionally include a package and remove all the useful information from it. I can put a bug report in for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I figured out a hack to make the image use the ubi8/ubi-minimal image. See #55467.

Copy link
Contributor

Choose a reason for hiding this comment

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

This issue says that there's now a microdnf reinstall command: rpm-software-management/microdnf#34

And if that's too recent, it offers a workaround: rpm --erase --nodeps tzdata && microdnf install tzdata

Copy link
Contributor

@bdarnell bdarnell Oct 13, 2020

Choose a reason for hiding this comment

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

~~Ah, I see you're already using microdnf reinstall in #55467. ~~ No, that's dnf reinstall, not microdnf reinstall

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh. Nice find. I'm switching to use the rpm erase hack.

@DuskEagle
Copy link
Member

@DuskEagle, what about this code? https://github.com/cockroachlabs/managed-service/blob/master/pkg/kube/kubecommon/assert.go

Oh, good point. We can fix that pretty quickly though, so this change is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a deployment Docker image based on RedHat UBI
5 participants