From 9eb8a7c0add822e8bc7dac64f7929be0124a2fe4 Mon Sep 17 00:00:00 2001 From: KyleBarton Date: Fri, 3 Jul 2026 11:37:06 -0700 Subject: [PATCH] Handle cases where Dockerfile aliases are chained (#57552) Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #56189 Release Notes: - Fixed bug to handle dev container Dockerfiles with chained aliases --------- Co-authored-by: Martin Ye --- .../src/devcontainer_manifest.rs | 267 ++++++++++++++---- 1 file changed, 217 insertions(+), 50 deletions(-) diff --git a/crates/dev_container/src/devcontainer_manifest.rs b/crates/dev_container/src/devcontainer_manifest.rs index fb2e4f6cd6b..1f2b2373819 100644 --- a/crates/dev_container/src/devcontainer_manifest.rs +++ b/crates/dev_container/src/devcontainer_manifest.rs @@ -2793,54 +2793,77 @@ chmod +x ./install.sh Ok(script) } +struct ParsedFromLine<'a> { + image: &'a str, + alias: Option<&'a str>, +} + +/// Parses a `FROM` instruction into its image and optional stage alias, +/// skipping flags like `--platform=...`. Returns `None` for non-`FROM` lines. +fn parse_from_line(line: &str) -> Option> { + let mut tokens = line.split_whitespace(); + if !tokens.next()?.eq_ignore_ascii_case("FROM") { + return None; + } + let image = tokens.find(|token| !token.starts_with("--"))?; + let alias = match (tokens.next(), tokens.next()) { + (Some(keyword), Some(alias)) if keyword.eq_ignore_ascii_case("as") => Some(alias), + _ => None, + }; + Some(ParsedFromLine { image, alias }) +} + fn dockerfile_inject_alias( dockerfile_content: &str, alias: &str, build_target: Option, ) -> String { - let from_lines: Vec<(usize, &str)> = dockerfile_content + let from_lines: Vec<(usize, ParsedFromLine)> = dockerfile_content .lines() .enumerate() - .filter(|(_, line)| line.starts_with("FROM")) + .filter_map(|(index, line)| parse_from_line(line).map(|parsed| (index, parsed))) .collect(); let target_entry = match &build_target { - Some(target) => from_lines.iter().rfind(|(_, line)| { - let parts: Vec<&str> = line.split_whitespace().collect(); - parts.len() >= 3 - && parts - .get(parts.len() - 2) - .map_or(false, |p| p.eq_ignore_ascii_case("as")) - && parts - .last() - .map_or(false, |p| p.eq_ignore_ascii_case(target)) + Some(target) => from_lines.iter().rfind(|(_, parsed)| { + parsed + .alias + .is_some_and(|alias| alias.eq_ignore_ascii_case(target)) }), None => from_lines.last(), }; - let Some(&(line_idx, from_line)) = target_entry else { + let Some((line_idx, parsed)) = target_entry else { + match &build_target { + Some(target) => log::warn!( + "Build target stage {target:?} not found in Dockerfile; leaving it unmodified" + ), + None => log::warn!("No FROM instruction found in Dockerfile; leaving it unmodified"), + } return dockerfile_content.to_string(); }; - let parts: Vec<&str> = from_line.split_whitespace().collect(); - let has_alias = parts.len() >= 3 - && parts - .get(parts.len() - 2) - .map_or(false, |p| p.eq_ignore_ascii_case("as")); - - if has_alias { - let Some(existing_alias) = parts.last() else { - return dockerfile_content.to_string(); - }; + if let Some(existing_alias) = parsed.alias { format!("{dockerfile_content}\nFROM {existing_alias} AS {alias}") } else { let lines: Vec<&str> = dockerfile_content.lines().collect(); + // Appending ` AS {alias}` to a line ending in a `\` continuation would + // corrupt the instruction, so leave the Dockerfile unmodified. + if lines + .get(*line_idx) + .is_some_and(|line| line.trim_end().ends_with('\\')) + { + log::warn!( + "FROM instruction spans multiple lines via `\\` continuation; cannot inject stage alias, leaving Dockerfile unmodified" + ); + return dockerfile_content.to_string(); + } let mut result = String::new(); for (i, line) in lines.iter().enumerate() { if i > 0 { result.push('\n'); } - if i == line_idx { + if i == *line_idx { result.push_str(&format!("{line} AS {alias}")); } else { result.push_str(line); @@ -2854,29 +2877,36 @@ fn dockerfile_inject_alias( } fn image_from_dockerfile(dockerfile_contents: String, target: &Option) -> Option { - dockerfile_contents + let stages: Vec = dockerfile_contents .lines() - .filter(|line| line.starts_with("FROM")) - .rfind(|from_line| match &target { - Some(target) => { - let parts = from_line.split(' ').collect::>(); - if parts.len() >= 3 - && parts.get(parts.len() - 2).unwrap_or(&"").to_lowercase() == "as" - { - parts.last().unwrap_or(&"").to_lowercase() == target.to_lowercase() - } else { - false - } - } - None => true, - }) - .and_then(|from_line| { - from_line - .split(' ') - .collect::>() - .get(1) - .map(|s| s.to_string()) - }) + .filter_map(parse_from_line) + .collect(); + + let start_index = match target { + Some(target) => stages.iter().rposition(|stage| { + stage + .alias + .is_some_and(|alias| alias.eq_ignore_ascii_case(target)) + })?, + None => stages.len().checked_sub(1)?, + }; + + // Follow alias chains (`FROM base AS development`) to a concrete image. + // Docker only resolves names to stages defined earlier in the file, so + // resolving strictly backwards is correct and cannot cycle. + let mut index = start_index; + loop { + let image = stages.get(index)?.image; + let previous_stage = stages.get(..index)?.iter().rposition(|stage| { + stage + .alias + .is_some_and(|alias| alias.eq_ignore_ascii_case(image)) + }); + match previous_stage { + Some(previous_index) => index = previous_index, + None => return Some(image.to_string()), + } + } } fn get_remote_user_from_config( @@ -2961,8 +2991,9 @@ mod test { devcontainer_json::MountDefinition, devcontainer_manifest::{ ConfigStatus, DevContainerManifest, DockerBuildResources, DockerComposeResources, - DockerInspect, extract_feature_id, find_primary_service, get_remote_user_from_config, - image_from_dockerfile, is_local_feature_ref, resolve_compose_dockerfile, + DockerInspect, dockerfile_inject_alias, extract_feature_id, find_primary_service, + get_remote_user_from_config, image_from_dockerfile, is_local_feature_ref, + resolve_compose_dockerfile, }, docker::{ DockerClient, DockerComposeConfig, DockerComposeService, DockerComposeServiceBuild, @@ -5877,6 +5908,93 @@ FROM ${IMAGE} AS production assert_eq!(base_image, "docker.io/stuff/mybuild:latest".to_string()); } + #[test] + fn test_image_from_dockerfile_resolves_one_hop_alias() { + let dockerfile = "FROM ubuntu:24.04 AS base\nFROM base AS development".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("development".to_string())), + Some("ubuntu:24.04".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_resolves_deep_alias_chain() { + let dockerfile = + "FROM ubuntu:24.04 AS base\nFROM base AS mid\nFROM mid AS development".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("development".to_string())), + Some("ubuntu:24.04".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_no_target_resolves_alias() { + let dockerfile = "FROM ubuntu:24.04 AS base\nFROM base".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &None), + Some("ubuntu:24.04".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_stage_alias_shadows_external_image() { + // The first `a` is an external image: stage `a` isn't defined yet. + // Target `a` builds from stage `b`, whose base is that external `a`. + let dockerfile = "FROM a AS b\nFROM b AS a".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("a".to_string())), + Some("a".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_only_resolves_earlier_stages() { + // The first `ubuntu` is the external image, not the later stage. + let dockerfile = "FROM ubuntu AS build\nFROM debian AS ubuntu".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("build".to_string())), + Some("ubuntu".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_skips_platform_flag() { + let dockerfile = + "FROM --platform=linux/amd64 ubuntu:24.04 AS base\nFROM base AS development" + .to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("development".to_string())), + Some("ubuntu:24.04".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_missing_target() { + let dockerfile = "FROM ubuntu:24.04 AS base".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("nonexistent".to_string())), + None + ); + } + + #[test] + fn test_image_from_dockerfile_case_insensitive_alias() { + let dockerfile = "FROM ubuntu:24.04 AS Base\nFROM Base AS Development".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("development".to_string())), + Some("ubuntu:24.04".to_string()) + ); + } + + #[test] + fn test_image_from_dockerfile_scratch_base() { + let dockerfile = "FROM scratch AS builder\nFROM builder AS final".to_string(); + assert_eq!( + image_from_dockerfile(dockerfile, &Some("final".to_string())), + Some("scratch".to_string()) + ); + } + #[gpui::test] async fn test_expands_args_in_dockerfile(cx: &mut TestAppContext) { cx.executor().allow_parking(); @@ -6090,13 +6208,62 @@ RUN echo $RUBY_VERSION2 } #[test] - fn test_aliases_dockerfile_with_pre_existing_aliases_for_build() {} + fn test_aliases_dockerfile_with_pre_existing_aliases_for_build() { + let dockerfile = "FROM ubuntu:24.04 AS base\nFROM base AS development"; + + assert_eq!( + dockerfile_inject_alias(dockerfile, "dev_container_auto_added_stage_label", None), + "FROM ubuntu:24.04 AS base\nFROM base AS development\nFROM development AS dev_container_auto_added_stage_label" + ); + } #[test] - fn test_aliases_dockerfile_with_no_aliases_for_build() {} + fn test_aliases_dockerfile_with_no_aliases_for_build() { + let dockerfile = "FROM --platform=linux/amd64 ubuntu:24.04\nRUN echo ok"; + + assert_eq!( + dockerfile_inject_alias(dockerfile, "dev_container_auto_added_stage_label", None), + "FROM --platform=linux/amd64 ubuntu:24.04 AS dev_container_auto_added_stage_label\nRUN echo ok" + ); + } #[test] - fn test_aliases_dockerfile_with_build_target_specified() {} + fn test_aliases_dockerfile_with_build_target_specified() { + let dockerfile = "FROM ubuntu:24.04 AS development\nFROM ubuntu:22.04 AS production"; + + assert_eq!( + dockerfile_inject_alias( + dockerfile, + "dev_container_auto_added_stage_label", + Some("development".to_string()) + ), + "FROM ubuntu:24.04 AS development\nFROM ubuntu:22.04 AS production\nFROM development AS dev_container_auto_added_stage_label" + ); + } + + #[test] + fn test_aliases_dockerfile_with_missing_build_target_is_unmodified() { + let dockerfile = "FROM ubuntu:24.04 AS development"; + + assert_eq!( + dockerfile_inject_alias( + dockerfile, + "dev_container_auto_added_stage_label", + Some("nonexistent".to_string()) + ), + dockerfile + ); + } + + #[test] + fn test_aliases_dockerfile_with_line_continuation_is_unmodified() { + let dockerfile = "FROM ubuntu:24.04 \\\n --platform=linux/amd64\nRUN echo ok"; + + assert_eq!( + dockerfile_inject_alias(dockerfile, "dev_container_auto_added_stage_label", None), + dockerfile + ); + } pub(crate) struct RecordedExecCommand { pub(crate) _container_id: String,