fix(composefs): merge default proxy opts for registry auth discovery#2086
Conversation
The composefs backend creates an ImageProxyConfig via new_proxy_config() in three places but never calls merge_default_container_proxy_opts(), so the global authfile (e.g. /etc/ostree/auth.json) is never discovered. This causes bootc upgrade, switch, and install to fail with "access denied on private registries when using the composefs backend. The ostree backend handles this correctly via ImageImporter::new() which calls merge_default_container_proxy_opts() internally. Signed-off-by: Peter Siegel <psiegel2000@icloud.com>
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where the composefs backend failed to discover registry authentication files by adding the necessary call to merge_default_container_proxy_opts(). The changes are applied consistently across three locations. My review includes suggestions to refactor the newly added, duplicated code into a shared helper function to improve maintainability.
| let mut config = crate::deploy::new_proxy_config(); | ||
| ostree_ext::container::merge_default_container_proxy_opts(&mut config)?; |
There was a problem hiding this comment.
This logic for creating and configuring the ImageProxyConfig is duplicated in pull_composefs_repo within this file, and also in get_container_manifest_and_config in crates/lib/src/bootc_composefs/status.rs.
To improve maintainability and reduce code duplication, you could extract this logic into a new helper function. Since new_proxy_config is in crates/lib/src/deploy.rs, that would be a good place for a new function like this:
/// Create an ImageProxyConfig with bootc's user agent and default proxy options.
pub(crate) fn new_proxy_config_with_defaults() -> anyhow::Result<ostree_ext::containers_image_proxy::ImageProxyConfig> {
let mut config = new_proxy_config();
ostree_ext::container::merge_default_container_proxy_opts(&mut config)?;
Ok(config)
}Then you could replace the duplicated blocks with a single call:
let config = crate::deploy::new_proxy_config_with_defaults()?;
There was a problem hiding this comment.
I'll probably skip this and the other code suggestions unless someone suggests otherwise. It might not be worth abstracting away right now.
The composefs backend creates an
ImageProxyConfigvianew_proxy_config()in three places but never callsmerge_default_container_proxy_opts(), so the global authfile (e.g./etc/ostree/auth.json,/usr/lib/ostree/auth.json) is never discovered. This causes bootc upgrade, switch to fail with "access denied" on private registries when using the composefs backend.The ostree backend handles this correctly via
ImageImporter::new()which callsmerge_default_container_proxy_opts()internally.Maybe fixes #2070 if the composefs backend was used there.