Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Labels in Container Config #750

Merged
merged 6 commits into from
Aug 1, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.cloud.tools.jib.image.InvalidImageReferenceException;
import com.google.cloud.tools.jib.registry.LocalRegistry;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.Resources;
import java.io.IOException;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -147,15 +148,23 @@ public void testSteps_forBuildToDockerDaemon()
new Caches.Initializer(cacheDirectory, false, cacheDirectory, false))
.run();

String dockerContainerConfig = new Command("docker", "inspect", "testdocker").run();
Assert.assertThat(
new Command("docker", "inspect", "testdocker").run(),
dockerContainerConfig,
CoreMatchers.containsString(
" \"ExposedPorts\": {\n"
+ " \"1000/tcp\": {},\n"
+ " \"2000/tcp\": {},\n"
+ " \"2001/tcp\": {},\n"
+ " \"2002/tcp\": {},\n"
+ " \"3000/udp\": {}"));
Assert.assertThat(
dockerContainerConfig,
CoreMatchers.containsString(
" \"Labels\": {\n"
+ " \"key1\": \"value1\",\n"
+ " \"key2\": \"value2\"\n"
+ " }"));
Assert.assertEquals(
"Hello, world. An argument.\n", new Command("docker", "run", "testdocker").run());
}
Expand Down Expand Up @@ -199,6 +208,7 @@ private BuildConfiguration getBuildConfiguration(
.setProgramArguments(Collections.singletonList("An argument."))
.setExposedPorts(
ExposedPortsParser.parse(Arrays.asList("1000", "2000-2002/tcp", "3000/udp")))
.setLabels(ImmutableMap.of("key1", "value1", "key2", "value2"))
.build();
return BuildConfiguration.builder(logger)
.setBaseImageConfiguration(baseImageConfiguration)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ private Image<CachedLayer> afterCachedLayersSteps()
imageBuilder.setEntrypoint(containerConfiguration.getEntrypoint());
imageBuilder.setJavaArguments(containerConfiguration.getProgramArguments());
imageBuilder.setExposedPorts(containerConfiguration.getExposedPorts());
imageBuilder.setLabels(containerConfiguration.getLabels());
}

// Gets the container configuration content descriptor.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public static class Builder {
@Nullable private ImmutableList<String> programArguments;
@Nullable private ImmutableMap<String, String> environmentMap;
@Nullable private ImmutableList<Port> exposedPorts;
@Nullable private ImmutableMap<String, String> labels;

/**
* Sets the image creation time.
Expand Down Expand Up @@ -98,6 +99,23 @@ public Builder setExposedPorts(@Nullable List<Port> exposedPorts) {
return this;
}

/**
* Sets the container's labels.
*
* @param labels the map of labels
* @return this
*/
public Builder setLabels(@Nullable Map<String, String> labels) {
if (labels == null) {
this.labels = null;
} else {
Preconditions.checkArgument(!Iterables.any(labels.keySet(), Objects::isNull));
Preconditions.checkArgument(!Iterables.any(labels.values(), Objects::isNull));
this.labels = ImmutableMap.copyOf(labels);
}
return this;
}

/**
* Sets the container entrypoint.
*
Expand All @@ -121,7 +139,7 @@ public Builder setEntrypoint(@Nullable List<String> entrypoint) {
*/
public ContainerConfiguration build() {
return new ContainerConfiguration(
creationTime, entrypoint, programArguments, environmentMap, exposedPorts);
creationTime, entrypoint, programArguments, environmentMap, exposedPorts, labels);
}

private Builder() {}
Expand All @@ -141,18 +159,21 @@ public static Builder builder() {
@Nullable private final ImmutableList<String> programArguments;
@Nullable private final ImmutableMap<String, String> environmentMap;
@Nullable private final ImmutableList<Port> exposedPorts;
@Nullable private final ImmutableMap<String, String> labels;

private ContainerConfiguration(
Instant creationTime,
@Nullable ImmutableList<String> entrypoint,
@Nullable ImmutableList<String> programArguments,
@Nullable ImmutableMap<String, String> environmentMap,
@Nullable ImmutableList<Port> exposedPorts) {
@Nullable ImmutableList<Port> exposedPorts,
@Nullable ImmutableMap<String, String> labels) {
this.creationTime = creationTime;
this.entrypoint = entrypoint;
this.programArguments = programArguments;
this.environmentMap = environmentMap;
this.exposedPorts = exposedPorts;
this.labels = labels;
}

public Instant getCreationTime() {
Expand All @@ -178,4 +199,9 @@ public ImmutableMap<String, String> getEnvironmentMap() {
public ImmutableList<Port> getExposedPorts() {
return exposedPorts;
}

@Nullable
public ImmutableMap<String, String> getLabels() {
return labels;
}
}
31 changes: 27 additions & 4 deletions jib-core/src/main/java/com/google/cloud/tools/jib/image/Image.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public static class Builder<T extends Layer> {
@Nullable private ImmutableList<String> entrypoint;
@Nullable private ImmutableList<String> javaArguments;
@Nullable private ImmutableList<Port> exposedPorts;
@Nullable private ImmutableMap<String, String> labels;

/**
* Sets the image creation time.
Expand Down Expand Up @@ -110,15 +111,26 @@ public Builder<T> setJavaArguments(@Nullable List<String> javaArguments) {
* @param exposedPorts the list of exposed ports to add
* @return this
*/
public Builder<T> setExposedPorts(@Nullable ImmutableList<Port> exposedPorts) {
public Builder<T> setExposedPorts(@Nullable List<Port> exposedPorts) {
if (exposedPorts == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could do the same thing here as with labels and use a ternary operator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was going to, but figured it should be in a followup. This ImmutableList -> List change probably shouldn't be part of this PR either. I might go through and do some cleanup after this.

this.exposedPorts = null;
} else {
this.exposedPorts = exposedPorts;
this.exposedPorts = ImmutableList.copyOf(exposedPorts);
}
return this;
}

/**
* Sets the items in the "Labels" field in the container configuration.
*
* @param labels that map of labels to add
* @return this
*/
public Builder<T> setLabels(@Nullable Map<String, String> labels) {
this.labels = (labels == null) ? null : ImmutableMap.copyOf(labels);
return this;
}

/**
* Adds a layer to the image.
*
Expand All @@ -138,7 +150,8 @@ public Image<T> build() {
environmentBuilder.build(),
entrypoint,
javaArguments,
exposedPorts);
exposedPorts,
labels);
}
}

Expand All @@ -164,19 +177,24 @@ public static <T extends Layer> Builder<T> builder() {
/** Ports that the container listens on. */
@Nullable private final ImmutableList<Port> exposedPorts;

/** Labels on the container configuration */
@Nullable private final ImmutableMap<String, String> labels;

private Image(
@Nullable Instant created,
ImageLayers<T> layers,
@Nullable ImmutableMap<String, String> environment,
@Nullable ImmutableList<String> entrypoint,
@Nullable ImmutableList<String> javaArguments,
@Nullable ImmutableList<Port> exposedPorts) {
@Nullable ImmutableList<Port> exposedPorts,
@Nullable ImmutableMap<String, String> labels) {
this.created = created;
this.layers = layers;
this.environment = environment;
this.entrypoint = entrypoint;
this.javaArguments = javaArguments;
this.exposedPorts = exposedPorts;
this.labels = labels;
}

@Nullable
Expand Down Expand Up @@ -204,6 +222,11 @@ public ImmutableList<Port> getExposedPorts() {
return exposedPorts;
}

@Nullable
public ImmutableMap<String, String> getLabels() {
return labels;
}

public ImmutableList<T> getLayers() {
return layers.getLayers();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
* "Entrypoint": ["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"],
* "Cmd": ["arg1", "arg2"]
* "ExposedPorts": { "6000/tcp":{}, "8000/tcp":{}, "9000/tcp":{} }
* "Labels": { "com.example.label": "value" }
* },
* "rootfs": {
* "diff_ids": [
Expand Down Expand Up @@ -87,6 +88,9 @@ private static class ConfigurationObjectTemplate implements JsonTemplate {

/** Network ports the container exposes. */
@Nullable private Map<String, Map<?, ?>> ExposedPorts;

/** Labels. */
@Nullable private Map<String, String> Labels;
}

/**
Expand Down Expand Up @@ -125,6 +129,10 @@ public void setContainerExposedPorts(@Nullable Map<String, Map<?, ?>> exposedPor
config.ExposedPorts = exposedPorts;
}

public void setContainerLabels(@Nullable Map<String, String> labels) {
config.Labels = labels;
}

public void addLayerDiffId(DescriptorDigest diffId) {
rootfs.diff_ids.add(diffId);
}
Expand Down Expand Up @@ -158,6 +166,11 @@ List<String> getContainerCmd() {
return config.ExposedPorts;
}

@Nullable
Map<String, String> getContainerLabels() {
return config.Labels;
}

@VisibleForTesting
DescriptorDigest getLayerDiffId(int index) {
return rootfs.diff_ids.get(index);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ public Blob getContainerConfigurationBlob() {
// Sets the exposed ports.
template.setContainerExposedPorts(portListToMap(image.getExposedPorts()));

// Sets the labels.
template.setContainerLabels(image.getLabels());

// Serializes into JSON.
return JsonTemplateMapper.toBlob(template);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public void testBuilder() {
Map<String, String> expectedEnvironment = ImmutableMap.of("key", "value");
ImmutableList<Port> expectedExposedPorts =
ImmutableList.of(new Port(1000, Protocol.TCP), new Port(2000, Protocol.TCP));
Map<String, String> expectedLabels = ImmutableMap.of("key1", "value1", "key2", "value2");
Class<? extends BuildableManifestTemplate> expectedTargetFormat = OCIManifestTemplate.class;
CacheConfiguration expectedApplicationLayersCacheConfiguration =
CacheConfiguration.forPath(Paths.get("application/layers"));
Expand Down Expand Up @@ -90,6 +91,7 @@ public void testBuilder() {
.setProgramArguments(expectedJavaArguments)
.setEnvironment(expectedEnvironment)
.setExposedPorts(expectedExposedPorts)
.setLabels(expectedLabels)
.build();
BuildConfiguration.Builder buildConfigurationBuilder =
BuildConfiguration.builder(Mockito.mock(BuildLogger.class))
Expand Down Expand Up @@ -134,6 +136,7 @@ public void testBuilder() {
expectedEnvironment, buildConfiguration.getContainerConfiguration().getEnvironmentMap());
Assert.assertEquals(
expectedExposedPorts, buildConfiguration.getContainerConfiguration().getExposedPorts());
Assert.assertEquals(expectedLabels, buildConfiguration.getContainerConfiguration().getLabels());
Assert.assertEquals(expectedTargetFormat, buildConfiguration.getTargetFormat());
Assert.assertEquals(
expectedApplicationLayersCacheConfiguration,
Expand Down Expand Up @@ -250,6 +253,17 @@ public void testBuilder_nullValues() {
Assert.assertNull(ex.getMessage());
}

// Labels element should not be null.
try {
Map<String, String> badLabels = new HashMap<>();
badLabels.put("label-key", null);
BuildConfiguration.builder(Mockito.mock(BuildLogger.class))
.setContainerConfiguration(ContainerConfiguration.builder().setLabels(badLabels).build());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try block should only be focused on the method being tested. I actually think BuildConfiguration.builder() doesn't play a role. Maybe

Map<String, String> badLabels = ...
badLabels.put(...)
Builder builder = ContainerConfiguration.builder();
try {
  builder.setLabels(badLabels);
  Assert.fail(...);
} catch (...) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrmm... looks like we might need to cleanup this whole test class :\

Assert.fail("The IllegalArgumentException should be thrown.");
} catch (IllegalArgumentException ex) {
Assert.assertNull(ex.getMessage());
}

// Environment keys element should not be null.
Map<String, String> nullKeyMap = new HashMap<>();
nullKeyMap.put(null, "value");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public void testToJson() throws IOException, URISyntaxException, DigestException
ImmutableMap.of(),
"3000/udp",
ImmutableMap.of()));
containerConfigJson.setContainerLabels(ImmutableMap.of("key1", "value1", "key2", "value2"));

containerConfigJson.addLayerDiffId(
DescriptorDigest.fromDigest(
Expand Down Expand Up @@ -85,6 +86,9 @@ public void testFromJson() throws IOException, URISyntaxException, DigestExcepti
Arrays.asList("some", "entrypoint", "command"),
containerConfigJson.getContainerEntrypoint());
Assert.assertEquals(Arrays.asList("arg1", "arg2"), containerConfigJson.getContainerCmd());
Assert.assertEquals(
ImmutableMap.of("key1", "value1", "key2", "value2"),
containerConfigJson.getContainerLabels());
Assert.assertEquals(
DescriptorDigest.fromDigest(
"sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public void setUp() throws DigestException, LayerPropertyNotFoundException {
new Port(1000, Protocol.TCP),
new Port(2000, Protocol.TCP),
new Port(3000, Protocol.UDP)));
testImageBuilder.setLabels(ImmutableMap.of("key1", "value1", "key2", "value2"));

DescriptorDigest fakeDigest =
DescriptorDigest.fromDigest(
Expand Down
2 changes: 1 addition & 1 deletion jib-core/src/test/resources/json/containerconfig.json
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"created":"1970-01-01T00:00:20Z","architecture":"amd64","os":"linux","config":{"Env":["VAR1=VAL1","VAR2=VAL2"],"Entrypoint":["some","entrypoint","command"],"Cmd":["arg1","arg2"],"ExposedPorts":{"1000/tcp":{},"2000/tcp":{},"3000/udp":{}}},"rootfs":{"type":"layers","diff_ids":["sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad"]}}
{"created":"1970-01-01T00:00:20Z","architecture":"amd64","os":"linux","config":{"Env":["VAR1=VAL1","VAR2=VAL2"],"Entrypoint":["some","entrypoint","command"],"Cmd":["arg1","arg2"],"ExposedPorts":{"1000/tcp":{},"2000/tcp":{},"3000/udp":{}},"Labels":{"key1":"value1","key2":"value2"}},"rootfs":{"type":"layers","diff_ids":["sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad"]}}
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json","config":{"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:1bc0730d8135d29caa0e0c5b502164195afcab6494e1b024e419dc9d591d778d","size":353},"layers":[{"mediaType":"application/vnd.oci.image.layer.v1.tar+gzip","digest":"sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad","size":1000}]}
{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json","config":{"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:b988b86bf80435bdfe7b2f361f5e5838bdf2abd5dcb5ff298122504f8fb80e95","size":396},"layers":[{"mediaType":"application/vnd.oci.image.layer.v1.tar+gzip","digest":"sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad","size":1000}]}
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.v2+json","config":{"mediaType":"application/vnd.docker.container.image.v1+json","digest":"sha256:1bc0730d8135d29caa0e0c5b502164195afcab6494e1b024e419dc9d591d778d","size":353},"layers":[{"mediaType":"application/vnd.docker.image.rootfs.diff.tar.gzip","digest":"sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad","size":1000}]}
{"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.v2+json","config":{"mediaType":"application/vnd.docker.container.image.v1+json","digest":"sha256:b988b86bf80435bdfe7b2f361f5e5838bdf2abd5dcb5ff298122504f8fb80e95","size":396},"layers":[{"mediaType":"application/vnd.docker.image.rootfs.diff.tar.gzip","digest":"sha256:8c662931926fa990b41da3c9f42663a537ccd498130030f9149173a0493832ad","size":1000}]}