-
Notifications
You must be signed in to change notification settings - Fork 168
WIP: Fallback to running system's auth.json #1934
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
base: main
Are you sure you want to change the base?
Conversation
This fixes a bug where running bootc upgrade/switch to an image that does not have an auth.json results in failing to pull new LBIs that are stored in an authenticated registry. Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Add reusable functions for running cstor-dist, a tool that serves images from containers-storage via an authenticated OCI registry endpoint. This enables testing authenticated logically bound images. The following functions are exported: - start_cstor_dist: Starts cstor-dist container with basic auth - get_cstor_auth: Returns registry address and base64-encoded credentials - setup_insecure_registry: Configures registries.conf.d for non-TLS access - setup_system_auth: Sets up /run/ostree/auth.json with credentials Also update test-logically-bound-switch.nu to: - Import get_cstor_auth from bootc_testlib.nu - Add --with-auth flag to build_image for baking auth.json into images - Fix verify_images to check if image name is IN the Names array (not exact match) to support images with multiple tags Assisted-by: Claude Code (claude-opus-4-5@20251101) Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Extend the logically bound images switch test to verify that authenticated bound images work correctly in two scenarios: 1. Auth credentials baked into the image itself (/etc/ostree/auth.json) 2. Auth credentials on the running system (/run/ostree/auth.json) The test uses cstor-dist to serve images from containers-storage via an authenticated OCI registry endpoint. On first boot, an image with auth baked in is switched to. On second boot, the test upgrades to a new image WITHOUT baked-in auth, forcing the use of the fallback system auth file. This validates the auth fallback fix. Test images: - cstor-alpine: authenticated LBI pulled on first switch - cstor-busybox: authenticated LBI added on upgrade (tests fallback) Assisted-by: Claude Code (claude-opus-4-5@20251101) Signed-off-by: ckyrouac <ckyrouac@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a fallback mechanism to use the host's auth.json when pulling images if the target image for an upgrade or switch doesn't contain one. This is a valuable fix for scenarios involving authenticated registries. The core logic change in Rust is sound, and I've offered a minor suggestion to improve its conciseness. The accompanying tests are thorough, effectively validating the new fallback behavior using a local authenticated registry. I've provided a couple of suggestions for the test scripts to enhance maintainability by reducing code duplication.
| let authfile = if let Some((path, file)) = ostree_ext::globals::get_global_authfile(sysroot)? { | ||
| tracing::debug!("Using authfile from sysroot: {path}"); | ||
| Some(file) | ||
| } else if let Some((path, file)) = get_host_authfile()? { | ||
| tracing::debug!("Using authfile from host root: {path}"); | ||
| Some(file) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if let ... else if let ... chain can be simplified by combining the fallback logic into a single else block, which makes the code slightly more concise.
| let authfile = if let Some((path, file)) = ostree_ext::globals::get_global_authfile(sysroot)? { | |
| tracing::debug!("Using authfile from sysroot: {path}"); | |
| Some(file) | |
| } else if let Some((path, file)) = get_host_authfile()? { | |
| tracing::debug!("Using authfile from host root: {path}"); | |
| Some(file) | |
| } else { | |
| None | |
| }; | |
| let authfile = if let Some((path, file)) = ostree_ext::globals::get_global_authfile(sysroot)? { | |
| tracing::debug!("Using authfile from sysroot: {path}"); | |
| Some(file) | |
| } else { | |
| get_host_authfile()?.map(|(path, file)| { | |
| tracing::debug!("Using authfile from host root: {path}"); | |
| file | |
| }) | |
| }; |
| let more_images = [ | ||
| { "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.4", "name": "ubi-minimal" }, | ||
| { "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.3", "name": "ubi-minimal-9-3" }, | ||
| { "bound": false, "image": "quay.io/centos-bootc/centos-bootc:stream9", "name": "centos-bootc" }, | ||
| { "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/alpine:latest", "name": "cstor-alpine" }, | ||
| { "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/busybox:latest", "name": "cstor-busybox" } | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability and reduce repetition, you can define more_images by appending to the existing $images list, which is defined just a few lines above. This approach is more concise and consistent with how this variable was constructed before this change.
let more_images = $images | append [
{ "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.3", "name": "ubi-minimal-9-3" },
{ "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/busybox:latest", "name": "cstor-busybox" }
]
| let images = [ | ||
| { "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.4", "name": "ubi-minimal" }, | ||
| { "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.3", "name": "ubi-minimal-9-3" }, | ||
| { "bound": false, "image": "quay.io/centos-bootc/centos-bootc:stream9", "name": "centos-bootc" } | ||
| { "bound": false, "image": "quay.io/centos-bootc/centos-bootc:stream9", "name": "centos-bootc" }, | ||
| { "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/alpine:latest", "name": "cstor-alpine" }, | ||
| { "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/busybox:latest", "name": "cstor-busybox" } | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list of images is identical to the more_images list defined in second_boot. To avoid this duplication and improve maintainability, consider extracting this list into a shared helper function or constant that can be used in both places.
For example, you could add a helper function:
def get_final_images [] {
[
{ "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.4", "name": "ubi-minimal" },
{ "bound": true, "image": "registry.access.redhat.com/ubi9/ubi-minimal:9.3", "name": "ubi-minimal-9-3" },
{ "bound": false, "image": "quay.io/centos-bootc/centos-bootc:stream9", "name": "centos-bootc" },
{ "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/alpine:latest", "name": "cstor-alpine" },
{ "bound": true, "image": $"($CSTOR_DIST_REGISTRY)/docker.io/library/busybox:latest", "name": "cstor-busybox" }
]
}Then you could use let more_images = get_final_images in second_boot and let images = get_final_images here.
This fixes a bug where running bootc upgrade/switch to an image that does not have an auth.json results in failing to pull new LBIs that are stored in an authenticated registry.
This also adds a test for this to the existing LBI switch TMT test. It does this by using cstor-dist as an authenticated registry, running on the TMT test VM. Marking as a WIP for now until I push the cstor-dist image somewhere permanent.