Improve error handling when builder image isn't a builder

Fixes gh-22179
spring-projects · Jul 1, 2020 · 9317135 · 9317135
1 parent 0e1ded6
commit 9317135
Show file tree

Hide file tree

Showing 8 changed files with 80 additions and 23 deletions.
diff --git a/...-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildOwner.java b/...-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildOwner.java
@@ -26,6 +26,7 @@
  * The {@link Owner} that should perform the build.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 class BuildOwner implements Owner {
 
@@ -49,13 +50,14 @@ class BuildOwner implements Owner {
 
 	private long getValue(Map<String, String> env, String name) {
 		String value = env.get(name);
-		Assert.state(StringUtils.hasText(value), () -> "Missing '" + name + "' value from the builder environment");
+		Assert.state(StringUtils.hasText(value),
+				() -> "Missing '" + name + "' value from the builder environment '" + env + "'");
 		try {
 			return Long.parseLong(value);
 		}
 		catch (NumberFormatException ex) {
-			throw new IllegalStateException("Malformed '" + name + "' value '" + value + "' in the builder environment",
-					ex);
+			throw new IllegalStateException(
+					"Malformed '" + name + "' value '" + value + "' in the builder environment '" + env + "'", ex);
 		}
 	}
 

diff --git a/...form/src/main/java/org/springframework/boot/buildpack/platform/build/BuilderMetadata.java b/...form/src/main/java/org/springframework/boot/buildpack/platform/build/BuilderMetadata.java
@@ -18,7 +18,6 @@
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
-import java.util.Map;
 import java.util.function.Consumer;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
@@ -30,11 +29,13 @@
 import org.springframework.boot.buildpack.platform.json.MappedObject;
 import org.springframework.boot.buildpack.platform.json.SharedObjectMapper;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 
 /**
  * Builder metadata information.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 class BuilderMetadata extends MappedObject {
 
@@ -121,9 +122,9 @@ static BuilderMetadata fromImage(Image image) throws IOException {
 	 */
 	static BuilderMetadata fromImageConfig(ImageConfig imageConfig) throws IOException {
 		Assert.notNull(imageConfig, "ImageConfig must not be null");
-		Map<String, String> labels = imageConfig.getLabels();
-		String json = (labels != null) ? labels.get(LABEL_NAME) : null;
-		Assert.notNull(json, () -> "No '" + LABEL_NAME + "' label found in image config");
+		String json = imageConfig.getLabels().get(LABEL_NAME);
+		Assert.notNull(json, () -> "No '" + LABEL_NAME + "' label found in image config labels '"
+				+ StringUtils.collectionToCommaDelimitedString(imageConfig.getLabels().keySet()) + "'");
 		return fromJson(json);
 	}
 

diff --git a/...ack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/StackId.java b/...ack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/StackId.java
@@ -16,8 +16,6 @@
 
 package org.springframework.boot.buildpack.platform.build;
 
-import java.util.Map;
-
 import org.springframework.boot.buildpack.platform.docker.type.Image;
 import org.springframework.boot.buildpack.platform.docker.type.ImageConfig;
 import org.springframework.util.Assert;
@@ -27,6 +25,7 @@
  * A Stack ID.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 class StackId {
 
@@ -75,8 +74,7 @@ static StackId fromImage(Image image) {
 	 * @return the extracted stack ID
 	 */
 	private static StackId fromImageConfig(ImageConfig imageConfig) {
-		Map<String, String> labels = imageConfig.getLabels();
-		String value = (labels != null) ? labels.get(LABEL_NAME) : null;
+		String value = imageConfig.getLabels().get(LABEL_NAME);
 		Assert.state(StringUtils.hasText(value), () -> "Missing '" + LABEL_NAME + "' stack label");
 		return new StackId(value);
 	}

diff --git a/...rm/src/main/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfig.java b/...rm/src/main/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfig.java
@@ -31,6 +31,7 @@
  * Image configuration information.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  * @since 2.3.0
  */
 public class ImageConfig extends MappedObject {
@@ -39,16 +40,27 @@ public class ImageConfig extends MappedObject {
 
 	private final Map<String, String> configEnv;
 
-	@SuppressWarnings("unchecked")
 	ImageConfig(JsonNode node) {
 		super(node, MethodHandles.lookup());
-		this.labels = valueAt("/Labels", Map.class);
+		this.labels = extractLabels();
 		this.configEnv = parseConfigEnv();
 	}
 
+	@SuppressWarnings("unchecked")
+	private Map<String, String> extractLabels() {
+		Map<String, String> labels = valueAt("/Labels", Map.class);
+		if (labels == null) {
+			return Collections.emptyMap();
+		}
+		return labels;
+	}
+
 	private Map<String, String> parseConfigEnv() {
-		Map<String, String> env = new LinkedHashMap<>();
 		String[] entries = valueAt("/Env", String[].class);
+		if (entries == null) {
+			return Collections.emptyMap();
+		}
+		Map<String, String> env = new LinkedHashMap<>();
 		for (String entry : entries) {
 			int i = entry.indexOf('=');
 			String name = (i != -1) ? entry.substring(0, i) : entry;
@@ -63,16 +75,18 @@ JsonNode getNodeCopy() {
 	}
 
 	/**
-	 * Return the image labels.
-	 * @return the image labels
+	 * Return the image labels. If the image has no labels, an empty {@code Map} is
+	 * returned.
+	 * @return the image labels, never {@code null}
 	 */
 	public Map<String, String> getLabels() {
 		return this.labels;
 	}
 
 	/**
-	 * Return the image environment variables.
-	 * @return the env
+	 * Return the image environment variables. If the image has no environment variables,
+	 * an empty {@code Map} is returned.
+	 * @return the env, never {@code null}
 	 */
 	public Map<String, String> getEnv() {
 		return this.configEnv;

diff --git a/...form/src/test/java/org/springframework/boot/buildpack/platform/build/BuildOwnerTests.java b/...form/src/test/java/org/springframework/boot/buildpack/platform/build/BuildOwnerTests.java
@@ -29,6 +29,7 @@
  * Tests for {@link BuildOwner}.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 class BuildOwnerTests {
 
@@ -54,15 +55,15 @@ void fromEnvWhenUserPropertyIsMissingThrowsException() {
 		Map<String, String> env = new LinkedHashMap<>();
 		env.put("CNB_GROUP_ID", "456");
 		assertThatIllegalStateException().isThrownBy(() -> BuildOwner.fromEnv(env))
-				.withMessage("Missing 'CNB_USER_ID' value from the builder environment");
+				.withMessage("Missing 'CNB_USER_ID' value from the builder environment '" + env + "'");
 	}
 
 	@Test
 	void fromEnvWhenGroupPropertyIsMissingThrowsException() {
 		Map<String, String> env = new LinkedHashMap<>();
 		env.put("CNB_USER_ID", "123");
 		assertThatIllegalStateException().isThrownBy(() -> BuildOwner.fromEnv(env))
-				.withMessage("Missing 'CNB_GROUP_ID' value from the builder environment");
+				.withMessage("Missing 'CNB_GROUP_ID' value from the builder environment '" + env + "'");
 	}
 
 	@Test
@@ -71,7 +72,7 @@ void fromEnvWhenUserPropertyIsMalformedThrowsException() {
 		env.put("CNB_USER_ID", "nope");
 		env.put("CNB_GROUP_ID", "456");
 		assertThatIllegalStateException().isThrownBy(() -> BuildOwner.fromEnv(env))
-				.withMessage("Malformed 'CNB_USER_ID' value 'nope' in the builder environment");
+				.withMessage("Malformed 'CNB_USER_ID' value 'nope' in the builder environment '" + env + "'");
 	}
 
 	@Test
@@ -80,7 +81,7 @@ void fromEnvWhenGroupPropertyIsMalformedThrowsException() {
 		env.put("CNB_USER_ID", "123");
 		env.put("CNB_GROUP_ID", "nope");
 		assertThatIllegalStateException().isThrownBy(() -> BuildOwner.fromEnv(env))
-				.withMessage("Malformed 'CNB_GROUP_ID' value 'nope' in the builder environment");
+				.withMessage("Malformed 'CNB_GROUP_ID' value 'nope' in the builder environment '" + env + "'");
 	}
 
 }
diff --git a/...src/test/java/org/springframework/boot/buildpack/platform/build/BuilderMetadataTests.java b/...src/test/java/org/springframework/boot/buildpack/platform/build/BuilderMetadataTests.java
@@ -17,6 +17,7 @@
 package org.springframework.boot.buildpack.platform.build;
 
 import java.io.IOException;
+import java.util.Collections;
 
 import org.junit.jupiter.api.Test;
 
@@ -34,6 +35,7 @@
  *
  * @author Phillip Webb
  * @author Scott Frederick
+ * @author Andy Wilkinson
  */
 class BuilderMetadataTests extends AbstractJsonTests {
 
@@ -69,8 +71,9 @@ void fromImageConfigWhenLabelIsMissingThrowsException() {
 		Image image = mock(Image.class);
 		ImageConfig imageConfig = mock(ImageConfig.class);
 		given(image.getConfig()).willReturn(imageConfig);
+		given(imageConfig.getLabels()).willReturn(Collections.singletonMap("alpha", "a"));
 		assertThatIllegalArgumentException().isThrownBy(() -> BuilderMetadata.fromImage(image))
-				.withMessage("No 'io.buildpacks.builder.metadata' label found in image config");
+				.withMessage("No 'io.buildpacks.builder.metadata' label found in image config labels 'alpha'");
 	}
 
 	@Test

diff --git a/...c/test/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfigTests.java b/...c/test/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfigTests.java
@@ -30,6 +30,7 @@
  * Tests for {@link ImageConfig}.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 class ImageConfigTests extends AbstractJsonTests {
 
@@ -42,6 +43,20 @@ void getEnvContainsParsedValues() throws Exception {
 				entry("CNB_STACK_ID", "org.cloudfoundry.stacks.cflinuxfs3"));
 	}
 
+	@Test
+	void whenConfigHasNoEnvThenImageConfigEnvIsEmpty() throws Exception {
+		ImageConfig imageConfig = getMinimalImageConfig();
+		Map<String, String> env = imageConfig.getEnv();
+		assertThat(env).isEmpty();
+	}
+
+	@Test
+	void whenConfigHasNoLabelsThenImageConfigLabelsIsEmpty() throws Exception {
+		ImageConfig imageConfig = getMinimalImageConfig();
+		Map<String, String> env = imageConfig.getLabels();
+		assertThat(env).isEmpty();
+	}
+
 	@Test
 	void getLabelsReturnsLabels() throws Exception {
 		ImageConfig imageConfig = getImageConfig();
@@ -63,4 +78,8 @@ private ImageConfig getImageConfig() throws IOException {
 		return new ImageConfig(getObjectMapper().readTree(getContent("image-config.json")));
 	}
 
+	private ImageConfig getMinimalImageConfig() throws IOException {
+		return new ImageConfig(getObjectMapper().readTree(getContent("minimal-image-config.json")));
+	}
+
 }
diff --git a/...sources/org/springframework/boot/buildpack/platform/docker/type/minimal-image-config.json b/...sources/org/springframework/boot/buildpack/platform/docker/type/minimal-image-config.json
@@ -0,0 +1,19 @@
+{
+	"Hostname": "",
+	"Domainname": "",
+	"User": "",
+	"AttachStdin": false,
+	"AttachStdout": false,
+	"AttachStderr": false,
+	"Tty": false,
+	"OpenStdin": false,
+	"StdinOnce": false,
+	"Env": null,
+	"Cmd": null,
+	"Image": "",
+	"Volumes": null,
+	"WorkingDir": "",
+	"Entrypoint": null,
+	"OnBuild": null,
+	"Labels": null
+}