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

Fixing reproducibility error introduced by 24-CNB vs 24-Heroku #332

Closed
wants to merge 2 commits into from

Conversation

usiegj00
Copy link

The 24-Heroku stack adds a user like u12345 and a dyno user, both with HOME of /app. The 24-CNB stack breaks this convention by introducing a heroku user with a HOME of /home/heroku. This prevents packages such as fontconfig which expects .fonts to be available in HOME.

In non-CNB builds, HOME and /app are the same, so a /app/.fonts/this.ttf is available as expected at runtime.

In CNB builds, these are different, so the same font would be at /app/.fonts/this.ttf, while the runtime path searched would be /home/heroku/.fonts.

This fix has been thoroughly tested and helps Heroku customers achieve better reproducibility and alignment with open standards.

The 24-Heroku stack adds a user like `u12345` and a `dyno` user, both with HOME of /app. The 24-CNB stack breaks this convention by introducing a `heroku` user with a `HOME` of `/home/heroku`. This prevents packages such as [fontconfig which expects .fonts to be available in HOME](https://www.freedesktop.org/software/fontconfig/fontconfig-user.html). 

In non-CNB builds, `HOME` and `/app` are the same, so a `/app/.fonts/this.ttf` is available as expected at runtime.

In CNB builds, these are different, so the same font would be at `/app/.fonts/this.ttf`, while the runtime path searched would be `/home/heroku/.fonts`.

This fix has been thoroughly tested and helps Heroku customers achieve better reproducibility and alignment with open standards.
@usiegj00 usiegj00 requested a review from a team as a code owner November 12, 2024 01:21
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Hi!

Sorry for the delay in replying - we were focusing on work leading up to the recent Heroku Fir announcement:
https://blog.heroku.com/tags/nextgen

Thank you for taking a look at these images and opening the PR.

Unfortunately we won't be able to merge these changes since the differences you mention between the images are by design - the Heroku-24 behaviour is the preferred behaviour.

If anything we'd want to change the old images to match rather than vice versa - however, attempting to unify the behaviour across older and newer stacks (in either direction) would be a breaking change - and we cannot typically make breaking changes to stacks once released.

I would recommend reading the CNB spec for understanding the direction the standards are heading:
https://github.com/buildpacks/spec

There's also prior discussion on the HOME location trade-offs in:
buildpacks/spec#186
heroku/cnb-shim#23

In general, it's expected that there will be some differences between Cedar generation apps (using classic buildpacks) and Fir generation apps (using CNBs), since we are aligning on best practices and open standards. If there are specific cases where we can improve parity by backporting a change to classic in a non-breaking way, then please do let us know :-)


# Match HOME operation done in [heroku-22](https://github.com/usiegj00/base-images/blob/3bed84a6cf88bc6535b1ef18acbc454f88c854d8/heroku-22-cnb/Dockerfile#L17).
# This is related to [the user HOME issue exposed here](https://github.com/heroku/base-images/pull/332).
ENV HOME=/workspace
Copy link
Member

Choose a reason for hiding this comment

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

This would be a breaking change for existing Heroku-24 users.

It's also not safe to assume that the workspace dir is /workspace, since the CNB spec says this directory can vary and is user/platform provided. For example if I run pack build --workspace /my-app. See the CNB_APP_DIR references in:
https://github.com/buildpacks/spec/blob/main/platform.md

@@ -154,8 +154,8 @@ test "$(file --brief /etc/ssl/certs/java/cacerts)" = "Java KeyStore"
# that we have to remove before creating our own (`userdel` will remove the group too).
userdel ubuntu --remove

groupadd heroku --gid 1000
useradd heroku --uid 1000 --gid 1000 --shell /bin/bash --create-home
groupadd dyno --gid 1000
Copy link
Member

Choose a reason for hiding this comment

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

Our aim with CNBs is to make the run/build/builder images be as platform independent as possible, and to try and avoid Heroku-specific terminology where possible (eg preferring "base images" instead of "stack-images" etc) - and "dyno" is a Heroku-specific product name. Plus people may use the images in non-Heroku environments, in which case they won't be running on a dyno.

As such this username change was intentional.

groupadd heroku --gid 1000
useradd heroku --uid 1000 --gid 1000 --shell /bin/bash --create-home
groupadd dyno --gid 1000
useradd dyno --uid 1000 --gid 1000 --shell /bin/bash --home /app --create-home
Copy link
Member

@edmorley edmorley Dec 9, 2024

Choose a reason for hiding this comment

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

We're intentionally moving back to having a standard home directory location for the default user to (a) align with standards, (b) match the CNB spec and upstream CNB expectations.

In an ideal world we could make this change for Heroku-22 and older, though it would be a breaking change for those users, so we'll have to let those stacks age out.

@edmorley
Copy link
Member

Closing since this change isn't one we want to make (for the reasons above).

@edmorley edmorley closed this Dec 19, 2024
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.

2 participants