Skip to content

Commit

Permalink
Merge pull request #34962 from iocanel/single-segment-images-34748
Browse files Browse the repository at this point in the history
Request single segment images when using local lookup ImageStreams
  • Loading branch information
geoand authored Jul 25, 2023
2 parents 1c7557a + 06ee624 commit 0683853
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,22 @@ public class ContainerImageConfig {
/**
* The group the container image will be part of
*/
@ConfigItem(defaultValue = "${user.name}")
@ConfigItem
@ConvertWith(TrimmedStringConverter.class)
public Optional<String> group;
Optional<String> group; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead

/**
* The name of the container image. If not set defaults to the application name
*/
@ConfigItem(defaultValue = "${quarkus.application.name:unset}")
@ConvertWith(TrimmedStringConverter.class)
public String name;
String name; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead

/**
* The tag of the container image. If not set defaults to the application version
*/
@ConfigItem(defaultValue = "${quarkus.application.version:latest}")
public Optional<String> tag;
Optional<String> tag; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead

/**
* Additional tags of the container image.
Expand Down Expand Up @@ -110,26 +110,4 @@ public boolean isPushExplicitlyEnabled() {
public boolean isPushExplicitlyDisabled() {
return push.isPresent() && !push.get();
}

/**
* Since user.name which is default value can be uppercase and uppercase values are not allowed
* in the repository part of image references, we need to make the username lowercase.
* If spaces exist in the user name, we replace them with the dash character.
*
* We purposely don't change the value of an explicitly set group.
*/
public Optional<String> getEffectiveGroup() {
if (group.isPresent()) {
String originalGroup = group.get();
if (originalGroup.equals(System.getProperty("user.name"))) {
if (originalGroup.isEmpty()) {
return Optional.empty();
}
return Optional.of(originalGroup.toLowerCase().replace(' ', '-'));
} else if (originalGroup.isEmpty()) {
return Optional.empty();
}
}
return group;
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
package io.quarkus.container.image.deployment;

import static io.quarkus.container.image.deployment.util.EnablementUtil.*;
import static io.quarkus.container.image.deployment.util.EnablementUtil.buildOrPushContainerImageNeeded;
import static io.quarkus.container.spi.ImageReference.DEFAULT_TAG;
import static io.quarkus.deployment.builditem.ApplicationInfoBuildItem.UNSET_VALUE;

import java.util.Collections;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.stream.StreamSupport;

import org.eclipse.microprofile.config.ConfigProvider;
import org.jboss.logging.Logger;

import io.quarkus.container.spi.ContainerImageBuildRequestBuildItem;
Expand All @@ -15,17 +18,18 @@
import io.quarkus.container.spi.ContainerImagePushRequestBuildItem;
import io.quarkus.container.spi.FallbackContainerImageRegistryBuildItem;
import io.quarkus.container.spi.ImageReference;
import io.quarkus.container.spi.SingleSegmentContainerImageRequestBuildItem;
import io.quarkus.deployment.Capabilities;
import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.builditem.ApplicationInfoBuildItem;
import io.quarkus.deployment.builditem.SuppressNonRuntimeConfigChangedWarningBuildItem;
import io.quarkus.deployment.pkg.builditem.ArtifactResultBuildItem;
import io.quarkus.deployment.pkg.steps.NativeSourcesBuild;
import io.quarkus.runtime.util.StringUtil;

public class ContainerImageProcessor {

private static final String UNKNOWN_USER = "?";
private static final Logger log = Logger.getLogger(ContainerImageProcessor.class);

@BuildStep(onlyIf = NativeSourcesBuild.class)
Expand All @@ -48,14 +52,16 @@ public void ignoreCredentialsChange(BuildProducer<SuppressNonRuntimeConfigChange
@BuildStep
public void publishImageInfo(ApplicationInfoBuildItem app,
ContainerImageConfig containerImageConfig,
Optional<SingleSegmentContainerImageRequestBuildItem> singleSegmentImageRequest,
Optional<FallbackContainerImageRegistryBuildItem> containerImageRegistry,
Optional<ContainerImageCustomNameBuildItem> containerImageCustomName,
Capabilities capabilities,
BuildProducer<ContainerImageInfoBuildItem> containerImage) {

ensureSingleContainerImageExtension(capabilities);

// additionalTags are used even containerImageConfig.image is set because that string cannot contain multiple tags
// additionalTags are used even containerImageConfig.image is set because that
// string cannot contain multiple tags
if (containerImageConfig.additionalTags.isPresent()) {
for (String additionalTag : containerImageConfig.additionalTags.get()) {
if (!ImageReference.isValidTag(additionalTag)) {
Expand All @@ -65,35 +71,37 @@ public void publishImageInfo(ApplicationInfoBuildItem app,
}
}

String effectiveGroup = containerImageConfig.getEffectiveGroup().orElse("");
//This can be the case when running inside kubernetes/minikube in dev-mode. Instead of failing, we should just catch and log.
if (UNKNOWN_USER.equals(effectiveGroup)) {
log.warn(
"Can't get the current user name, which is used as default for container image group. Can't publish container image info.");
return;
}
Optional<String> effectiveGroup = getEffectiveGroup(containerImageConfig.group, singleSegmentImageRequest.isPresent());

// if the user supplied the entire image string, use it
if (containerImageConfig.image.isPresent()) {
ImageReference imageReference = ImageReference.parse(containerImageConfig.image.get());
String repository = imageReference.getRepository();
containerImage.produce(new ContainerImageInfoBuildItem(Optional.of(imageReference.getRegistry()), repository,
if (singleSegmentImageRequest.isPresent() && imageReference.getRepository().contains("/")
&& StringUtil.isNullOrEmpty(imageReference.getRegistry())) {
log.warn("A single segment image is preferred, but a local multi segment has been provided: "
+ containerImageConfig.image.get());
}
containerImage.produce(new ContainerImageInfoBuildItem(Optional.of(imageReference.getRegistry()),
repository,
imageReference.getTag(), containerImageConfig.additionalTags.orElse(Collections.emptyList())));
return;
}

String registry = containerImageConfig.registry
.orElseGet(() -> containerImageRegistry.map(FallbackContainerImageRegistryBuildItem::getRegistry).orElse(null));
.orElseGet(() -> containerImageRegistry.map(FallbackContainerImageRegistryBuildItem::getRegistry)
.orElse(null));
if ((registry != null) && !ImageReference.isValidRegistry(registry)) {
throw new IllegalArgumentException("The supplied container-image registry '" + registry + "' is invalid");
}

String effectiveName = containerImageCustomName.map(ContainerImageCustomNameBuildItem::getName)
.orElse(containerImageConfig.name);
String repository = (containerImageConfig.getEffectiveGroup().map(s -> s + "/").orElse("")) + effectiveName;
String group = effectiveGroup.orElse("");
String repository = group.isBlank() ? effectiveName : group + "/" + effectiveName;
if (!ImageReference.isValidRepository(repository)) {
throw new IllegalArgumentException("The supplied combination of container-image group '"
+ effectiveGroup + "' and name '" + effectiveName + "' is invalid");
+ group + "' and name '" + effectiveName + "' is invalid");
}

String effectiveTag = containerImageConfig.tag.orElse(app.getVersion());
Expand All @@ -106,12 +114,43 @@ public void publishImageInfo(ApplicationInfoBuildItem app,
}

containerImage.produce(new ContainerImageInfoBuildItem(Optional.ofNullable(registry),
containerImageConfig.getEffectiveGroup(),
effectiveGroup,
effectiveName, effectiveTag,
containerImageConfig.additionalTags.orElse(Collections.emptyList())));
}

private void ensureSingleContainerImageExtension(Capabilities capabilities) {
ContainerImageCapabilitiesUtil.getActiveContainerImageCapability(capabilities);
}

/**
* Since user.name which is default value can be uppercase and uppercase values
* are not allowed
* in the repository part of image references, we need to make the username
* lowercase.
* If spaces exist in the user name, we replace them with the dash character.
*
* We purposely don't change the value of an explicitly set group.
*/
static Optional<String> getEffectiveGroup(Optional<String> group, boolean isSingleSegmentRequested) {
return group.or(() -> isSingleSegmentRequested || isGroupSpecified()
? Optional.empty()
: Optional.ofNullable(System.getProperty("user.name")).map(s -> s.replace(' ', '-')))
.map(String::toLowerCase)
.filter(Predicate.not(StringUtil::isNullOrEmpty));
}

static Optional<String> getEffectiveGroup() {
return getEffectiveGroup(Optional.empty(), false);
}

/**
* Users are allowed to specify an empty group, however this is mapped to Optional.emtpy().
* We need to know if the user has actually specified a group or not.
* The only way is to check the property names provided.
**/
static boolean isGroupSpecified() {
return StreamSupport.stream(ConfigProvider.getConfig().getPropertyNames().spliterator(), false)
.anyMatch(n -> n.equals("quarkus.container-image.group") || n.equals("QUARKUS_CONTAINER_IMAGE_GROUP"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,38 +9,37 @@
class ContainerImageConfigEffectiveGroupTest {

public static final String USER_NAME_SYSTEM_PROPERTY = "user.name";

private ContainerImageConfig sut = new ContainerImageConfig();
private final Optional<String> EMPTY = Optional.empty();

@Test
void testFromLowercaseUsername() {
testWithNewUsername("test", () -> {
sut.group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY));
assertEquals(sut.getEffectiveGroup(), Optional.of("test"));
Optional<String> group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY));
assertEquals(ContainerImageProcessor.getEffectiveGroup(EMPTY, false), Optional.of("test"));
});
}

@Test
void testFromUppercaseUsername() {
testWithNewUsername("TEST", () -> {
sut.group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY));
assertEquals(sut.getEffectiveGroup(), Optional.of("test"));
Optional<String> group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY));
assertEquals(ContainerImageProcessor.getEffectiveGroup(EMPTY, false), Optional.of("test"));
});
}

@Test
void testFromSpaceInUsername() {
testWithNewUsername("user name", () -> {
sut.group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY));
assertEquals(sut.getEffectiveGroup(), Optional.of("user-name"));
Optional<String> group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY));
assertEquals(ContainerImageProcessor.getEffectiveGroup(EMPTY, false), Optional.of("user-name"));
});
}

@Test
void testFromOther() {
testWithNewUsername("WhateveR", () -> {
sut.group = Optional.of("OtheR");
assertEquals(sut.getEffectiveGroup(), Optional.of("OtheR"));
Optional<String> group = Optional.of("OtheR");
assertEquals(ContainerImageProcessor.getEffectiveGroup(group, false), Optional.of("other"));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ private void whenPublishImageInfo() {
Capabilities capabilities = new Capabilities(Collections.emptySet());
BuildProducer<ContainerImageInfoBuildItem> containerImage = actualImageConfig -> actualContainerImageInfo = actualImageConfig;
ContainerImageProcessor processor = new ContainerImageProcessor();
processor.publishImageInfo(app, containerImageConfig, Optional.empty(), Optional.empty(), capabilities, containerImage);
processor.publishImageInfo(app, containerImageConfig, Optional.empty(), Optional.empty(), Optional.empty(),
capabilities, containerImage);
}

private void thenImageIs(String expectedImage) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package io.quarkus.container.spi;

import io.quarkus.builder.item.SimpleBuildItem;

/**
* There are cases where a single segment image (an image without group) is preferred.
* This build item is used to express this preferrence.
**/
public final class SingleSegmentContainerImageRequestBuildItem extends SimpleBuildItem {
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import io.quarkus.container.spi.ContainerImageInfoBuildItem;
import io.quarkus.container.spi.ContainerImageLabelBuildItem;
import io.quarkus.container.spi.FallbackContainerImageRegistryBuildItem;
import io.quarkus.container.spi.SingleSegmentContainerImageRequestBuildItem;
import io.quarkus.deployment.Capabilities;
import io.quarkus.deployment.Capability;
import io.quarkus.deployment.annotations.BuildProducer;
Expand Down Expand Up @@ -109,13 +110,15 @@ public void populateCustomImageName(OpenshiftConfig openshiftConfig,
@BuildStep
public void populateInternalRegistry(OpenshiftConfig openshiftConfig, ContainerImageConfig containerImageConfig,
Capabilities capabilities,
BuildProducer<FallbackContainerImageRegistryBuildItem> containerImageRegistry) {
BuildProducer<FallbackContainerImageRegistryBuildItem> containerImageRegistry,
BuildProducer<SingleSegmentContainerImageRequestBuildItem> singleSegmentContainerImageRequest) {

if (!containerImageConfig.registry.isPresent() && !containerImageConfig.image.isPresent()) {
DeploymentResourceKind deploymentResourceKind = openshiftConfig.getDeploymentResourceKind(capabilities);
if (deploymentResourceKind != DeploymentResourceKind.DeploymentConfig) {
if (openshiftConfig.isOpenshiftBuildEnabled(containerImageConfig, capabilities)) {
//Don't need fallback namespace, we use local lookup instead.
singleSegmentContainerImageRequest.produce(new SingleSegmentContainerImageRequestBuildItem());
} else {
containerImageRegistry.produce(new FallbackContainerImageRegistryBuildItem(DOCKERIO_REGISTRY));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@

package io.quarkus.it.kubernetes;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.entry;

import java.io.IOException;
import java.nio.file.Path;
import java.util.List;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.openshift.api.model.ImageStream;
import io.quarkus.builder.Version;
import io.quarkus.maven.dependency.Dependency;
import io.quarkus.test.ProdBuildResults;
import io.quarkus.test.ProdModeTestResults;
import io.quarkus.test.QuarkusProdModeTest;

//
// The purpose of this test is to assert that
// When:
// - We run an in-cluster container builds targeting Openshift (and `Deployment` is used).
// - No group / image is explicitly specified
//
// Then:
// - A BuildConfg is generated.
// - Two ImageStream are generated (one named after the app).
// - A Deployment resource was created
// - local lookup is enabled
// - single segement image is requested
//
public class OpenshiftWithDeploymentResourceAndLocalLookupTest {

private static final String NAME = "openshift-with-deployment-resource-and-local-lookup";

@RegisterExtension
static final QuarkusProdModeTest config = new QuarkusProdModeTest()
.withApplicationRoot((jar) -> jar.addClasses(GreetingResource.class))
.setApplicationName(NAME)
.setApplicationVersion("0.1-SNAPSHOT")
.overrideConfigKey("quarkus.openshift.deployment-kind", "Deployment")
.setLogFileName("k8s.log")
.setForcedDependencies(List.of(Dependency.of("io.quarkus", "quarkus-openshift", Version.getVersion())));

@ProdBuildResults
private ProdModeTestResults prodModeTestResults;

@Test
public void assertGeneratedResources() throws IOException {
final Path kubernetesDir = prodModeTestResults.getBuildDir().resolve("kubernetes");
assertThat(kubernetesDir)
.isDirectoryContaining(p -> p.getFileName().endsWith("openshift.json"))
.isDirectoryContaining(p -> p.getFileName().endsWith("openshift.yml"));
List<HasMetadata> kubernetesList = DeserializationUtil
.deserializeAsList(kubernetesDir.resolve("openshift.yml"));

assertThat(kubernetesList).filteredOn(h -> "BuildConfig".equals(h.getKind())).hasSize(1);
assertThat(kubernetesList).filteredOn(h -> "ImageStream".equals(h.getKind())).hasSize(2);
assertThat(kubernetesList).filteredOn(h -> "ImageStream".equals(h.getKind())
&& h.getMetadata().getName().equals(NAME)).hasSize(1);

assertThat(kubernetesList).filteredOn(i -> i instanceof Deployment).singleElement().satisfies(i -> {
assertThat(i).isInstanceOfSatisfying(Deployment.class, d -> {
assertThat(d.getMetadata()).satisfies(m -> {
assertThat(m.getName()).isEqualTo(NAME);
});

assertThat(d.getSpec()).satisfies(deploymentSpec -> {
assertThat(deploymentSpec.getTemplate()).satisfies(t -> {
assertThat(t.getMetadata()).satisfies(metadata -> assertThat(metadata.getLabels()).containsAnyOf(
entry("app.kubernetes.io/name", NAME),
entry("app.kubernetes.io/version", "0.1-SNAPSHOT")));

assertThat(t.getSpec()).satisfies(podSpec -> {
assertThat(podSpec.getContainers()).singleElement().satisfies(container -> {
assertThat(container.getImage())
.isEqualTo("openshift-with-deployment-resource-and-local-lookup:0.1-SNAPSHOT");
});
});
});
});
});
});

assertThat(kubernetesList).filteredOn(r -> r instanceof ImageStream && r.getMetadata().getName().equals(NAME))
.singleElement().satisfies(r -> {
assertThat(r).isInstanceOfSatisfying(ImageStream.class, i -> {
assertThat(i.getSpec()).satisfies(spec -> {
assertThat(spec.getLookupPolicy().getLocal()).isEqualTo(true);
});
});
});

}
}

0 comments on commit 0683853

Please sign in to comment.