Target distro name/version not derived from `/etc/os-release` when Docker labels aren't set

edmorley commented 5 months ago

Summary

The Platform API 0.12 spec says:

If target distribution data is missing, it will be inferred from /etc/os-release for Linux images

(https://github.com/buildpacks/spec/blob/platform/0.12/platform.md#analyzedtoml-toml)

As such lifecycle has an implementation for this: https://github.com/buildpacks/lifecycle/blob/feb0d757c9bc16b85f19cb07c80dc28f0bdd8504/platform/target_data.go#L66-L81

However, it seems this implementation never actually gets used in practice since it's behind a if tm.OS == "" conditional, and OS is always available, so the /etc/os-release fallback isn't used even if other properties about the target (such as the distro name and version) are not available: https://github.com/buildpacks/lifecycle/blob/feb0d757c9bc16b85f19cb07c80dc28f0bdd8504/platform/target_data.go#L15-L19 (Also, why does that code comment say build image - surely it should be analysing the run image?)

This means that for any run images where the io.buildpacks.base.distro.* Docker labels haven't been set (which is any run image that hasn't yet made the transition from stacks to targets), the CNB_TARGET_DISTRO_NAME and CNB_TARGET_DISTRO_VERSION env vars are correctly set in the buildpack environment.

This can be seen by using one of our older run images from before we added the relevant Docker labels - by passing --run-image heroku/heroku:22-cnb.v122 to pack build.

Reproduction

Steps

pack buildpack new testcase-targets --api 0.10 --targets "linux/amd64"
sed -i 's/exit 0/printenv | grep CNB_TARGET | sort/' testcase-targets/bin/build
pack build --builder heroku/builder:22 --run-image heroku/heroku:22-cnb.v122 --buildpack testcase-targets/ --path testcase-targets/ testcase-targets --verbose

Current behavior

The CNB_TARGET_DISTRO_NAME and CNB_TARGET_DISTRO_VERSION env vars are not set correctly in the buildpack env:

$ pack build --builder heroku/builder:22 --run-image heroku/heroku:22-cnb.v122 --buildpack testcase-targets/ --path testcase-targets/ testcase-targets --verbose
...
===> ANALYZING
...
Run image info in analyzed metadata is: 
{"Reference":"6d5f868e91eba6a6ec22a9a9b9b31a2fd486a0f0f57082fd0864e0bf0b3e4395","Image":"heroku/heroku:22-cnb.v122","Extend":false,"target":{"os":"linux","arch":"amd64"}}
...
===> BUILDING
...
CNB_TARGET_ARCH=amd64
CNB_TARGET_ARCH_VARIANT=
CNB_TARGET_DISTRO_NAME=
CNB_TARGET_DISTRO_VERSION=
CNB_TARGET_OS=linux
...

Expected behavior

For the CNB_TARGET_DISTRO_* env vars to be set based on the contents of /etc/os-release, ie:

CNB_TARGET_DISTRO_NAME=ubuntu
CNB_TARGET_DISTRO_VERSION=22.04

...given that /etc/os-release exists and has the necessary ID and VERSION_ID values:

$ docker run --rm heroku/heroku:22-cnb.v122 cat /etc/os-release | rg ID= | sort
ID=ubuntu
VERSION_ID="22.04"

Context

lifecycle version

0.19.3

platform version(s)

$ pack report
Pack:
  Version:  0.33.2+git-f2cffc4.build-5562
  OS/Arch:  darwin/arm64

The build is using Platform API 0.12 and Buildpack API 0.10.

natalieparellano commented 5 months ago

Thanks for finding all of these bugs @edmorley - we can fix this in the next patch

edmorley commented 5 months ago

I wonder whether lifecycle should emit a warning (or at least info message) if it's having to use the /etc/os-release fallback to find out the distro name/version? It seems like it would be best to let base image authors know there is an optimisation they haven't made to their images (ie: adding the labels to allow skipping the /etc/os-release invocation).

edmorley commented 5 months ago

IMO ideally at some point in a future spec release (once more people have made the the stacks -> targets migration) setting all the base image io.buildpacks.base.* labels would be changed from a SHOULD to a MUST, allowing the fallback to be removed entirely. Having a warning in the meantime would help with that goal.

buildpacks / lifecycle