From 9317135690cf82120bb2767b6a8632d5e6651314 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 1 Jul 2020 11:42:36 +0100 Subject: [PATCH] Improve error handling when builder image isn't a builder Fixes gh-22179 --- .../buildpack/platform/build/BuildOwner.java | 8 ++++-- .../platform/build/BuilderMetadata.java | 9 +++--- .../buildpack/platform/build/StackId.java | 6 ++-- .../platform/docker/type/ImageConfig.java | 28 ++++++++++++++----- .../platform/build/BuildOwnerTests.java | 9 +++--- .../platform/build/BuilderMetadataTests.java | 5 +++- .../docker/type/ImageConfigTests.java | 19 +++++++++++++ .../docker/type/minimal-image-config.json | 19 +++++++++++++ 8 files changed, 80 insertions(+), 23 deletions(-) create mode 100644 spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/resources/org/springframework/boot/buildpack/platform/docker/type/minimal-image-config.json diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildOwner.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildOwner.java index e1f8283a2c..a027d7237c 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildOwner.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildOwner.java @@ -26,6 +26,7 @@ import org.springframework.util.StringUtils; * 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 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/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuilderMetadata.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuilderMetadata.java index 4e5ee2b59f..06f8f4a575 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuilderMetadata.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuilderMetadata.java @@ -18,7 +18,6 @@ package org.springframework.boot.buildpack.platform.build; 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.docker.type.ImageConfig; 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 @@ class BuilderMetadata extends MappedObject { */ static BuilderMetadata fromImageConfig(ImageConfig imageConfig) throws IOException { Assert.notNull(imageConfig, "ImageConfig must not be null"); - Map 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/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/StackId.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/StackId.java index ad5c754122..b3e344ee53 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/StackId.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-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 @@ import org.springframework.util.StringUtils; * A Stack ID. * * @author Phillip Webb + * @author Andy Wilkinson */ class StackId { @@ -75,8 +74,7 @@ class StackId { * @return the extracted stack ID */ private static StackId fromImageConfig(ImageConfig imageConfig) { - Map 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/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfig.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfig.java index 7ac21f115e..6b5f510ed8 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfig.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfig.java @@ -31,6 +31,7 @@ import org.springframework.boot.buildpack.platform.json.MappedObject; * 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 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 extractLabels() { + Map labels = valueAt("/Labels", Map.class); + if (labels == null) { + return Collections.emptyMap(); + } + return labels; + } + private Map parseConfigEnv() { - Map env = new LinkedHashMap<>(); String[] entries = valueAt("/Env", String[].class); + if (entries == null) { + return Collections.emptyMap(); + } + Map env = new LinkedHashMap<>(); for (String entry : entries) { int i = entry.indexOf('='); String name = (i != -1) ? entry.substring(0, i) : entry; @@ -63,16 +75,18 @@ public class ImageConfig extends MappedObject { } /** - * 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 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 getEnv() { return this.configEnv; diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuildOwnerTests.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuildOwnerTests.java index cd8fb02d0e..44697e9c4f 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuildOwnerTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuildOwnerTests.java @@ -29,6 +29,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; * Tests for {@link BuildOwner}. * * @author Phillip Webb + * @author Andy Wilkinson */ class BuildOwnerTests { @@ -54,7 +55,7 @@ class BuildOwnerTests { Map 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 @@ -62,7 +63,7 @@ class BuildOwnerTests { Map 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 @@ class BuildOwnerTests { 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 @@ class BuildOwnerTests { 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/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderMetadataTests.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderMetadataTests.java index 0d85f481cc..d860acf90e 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderMetadataTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/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 @@ import static org.mockito.Mockito.mock; * * @author Phillip Webb * @author Scott Frederick + * @author Andy Wilkinson */ class BuilderMetadataTests extends AbstractJsonTests { @@ -69,8 +71,9 @@ class BuilderMetadataTests extends AbstractJsonTests { 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/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfigTests.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfigTests.java index 2f52d1aeba..38eb2e6787 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfigTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/ImageConfigTests.java @@ -30,6 +30,7 @@ import static org.assertj.core.api.Assertions.entry; * Tests for {@link ImageConfig}. * * @author Phillip Webb + * @author Andy Wilkinson */ class ImageConfigTests extends AbstractJsonTests { @@ -42,6 +43,20 @@ class ImageConfigTests extends AbstractJsonTests { entry("CNB_STACK_ID", "org.cloudfoundry.stacks.cflinuxfs3")); } + @Test + void whenConfigHasNoEnvThenImageConfigEnvIsEmpty() throws Exception { + ImageConfig imageConfig = getMinimalImageConfig(); + Map env = imageConfig.getEnv(); + assertThat(env).isEmpty(); + } + + @Test + void whenConfigHasNoLabelsThenImageConfigLabelsIsEmpty() throws Exception { + ImageConfig imageConfig = getMinimalImageConfig(); + Map env = imageConfig.getLabels(); + assertThat(env).isEmpty(); + } + @Test void getLabelsReturnsLabels() throws Exception { ImageConfig imageConfig = getImageConfig(); @@ -63,4 +78,8 @@ class ImageConfigTests extends AbstractJsonTests { 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/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/resources/org/springframework/boot/buildpack/platform/docker/type/minimal-image-config.json b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/resources/org/springframework/boot/buildpack/platform/docker/type/minimal-image-config.json new file mode 100644 index 0000000000..4949addaaf --- /dev/null +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/resources/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 +} \ No newline at end of file