Add support for defining an entrypoint #873

briandealwis · 2018-08-22T20:40:57Z

Fixes #579
Fixes #850

Adds support for providing an entrypoint using <container><entrypoint> (Maven) / container.entrypoint (Gradle). entrypoint causes jvmFlags and mainClass to be ignored, and a warning is issued if they are found.

[WARNING] <mainClass> and <jvmFlags> are ignored when entrypoint is specified

Maven supports both using a list or just a string:

 <container>
  <entrypoint>/busybox/sh</entrypoint>
</container>

And it turns out that Maven doesn't actually check the names of child elements of a list. So you can use something like the following if you like:

 <container>
  <entrypoint>
    <executable>/busybox/sh</executable>
    <argument>-x</argument>
    <scriptFile>/app/start.sh</scriptFile>
  </entrypoint>
</container>

(PR: Not Ready while I get the ITs working.)

loosebazooka · 2018-08-22T22:36:30Z

Is there a way we could provide a shortcut to the jib specific java classpath through some kind of template or keyword?

<entrypoint>
  <java>/weird/path/to/jre/on/my/corp/image/java</java>
  <cp>-cp</cp>
  <classpath>__jib_classpath__</classpath>
  ...
</entrypoint>

briandealwis · 2018-08-24T02:46:51Z

I wonder if we can expose the class path as a property — so that it can be referenced but also changed.

coollog · 2018-08-24T14:39:07Z

@briandealwis I believe that can be done by calling MavenProject#getProperties and then setProperty. The main thing though is that the pom.xml would not be able to resolve this property before some Jib goal actually sets it. I believe it may need to be done in the initialize phase ("initialize build state, e.g. set properties or create directories."). Haven't tried this though.

briandealwis · 2018-08-24T21:21:24Z

I did some investigation and setting the project properties doesn't seem to much matter. I'm instead going to allow the user to set the classpath — it's not like we have a complicated class path — and use Maven Surefire-style "late-binding" property references (e.g., @{property}). For example:

<container>
  <entrypoint>
    <arg>/my/custom/java</arg>
    <arg>-cp</arg>
    <arg>@{jib.classpath}:/some/other/item</arg>
  </entrypoint>
</container

Gradle doesn't need that providing we expose the classpath as a variable, right?

   container {
      entrypoint = ['/path/to/java', '-cp', classpath + ":/some/other/thing"]
   }

Or would it make sense to allow setting the classpath too? For example, if a user wanted to add libraries that had been pulled in via src/main/jib/<extraDirectory>?

<container>
  <classpath>
    <classes>/opt/path</classes>
    <jar>/opt/foo/bar.jar</jar>
  </classpath>
</container>

container {
  classpath = ['/classpath']
}

What do you think?

coollog · 2018-08-24T21:54:04Z

@briandealwis

The late-binding construct is pretty neat!

For Gradle, yea, a variable is sufficient - it could be part of the jib extension (jib.classpath).

I think we wouldn't need to give a special construct for just setting the classpath. The current workaround is to just place other jars in src/main/jib/app/libs/.

briandealwis · 2018-08-28T21:17:20Z

I've opened #894 for exposing the classpath and other possibly other variables.

chanseokoh · 2018-08-29T14:03:23Z

...aven-plugin/src/main/java/com/google/cloud/tools/jib/maven/PluginConfigurationProcessor.java

+          JavaEntrypointConstructor.makeDefaultEntrypoint(
+              jibPluginConfiguration.getJvmFlags(), mainClass);
+    } else if (jibPluginConfiguration.getMainClass() != null
+        || jibPluginConfiguration.getJvmFlags() != null) {


Just double-checking: for the Gradle plugin code, you do .getJvmFlags().isEmpty()

} else if (jibExtension.getMainClass() != null || !jibExtension.getJvmFlags().isEmpty()) {

while here, you do .getJvmFlags() != null. Are they correct?

chanseokoh · 2018-08-29T14:05:55Z

jib-maven-plugin/README.md

+    <!-- Maven doesn't check the names of child tags --> 
+    <entrypoint>
+      <executable>/busybox/sh</executable>
+      <script>/bin/start.sh</script>


I couldn't understand what it means by "Maven doesn't check the names of child tags" until I saw examples using <arg> below. This also gives me the impression that <executable> and <script> are fixed names that I must use. TBH, I think this is confusing. It also makes me to believe that the order of <executable> and <script> shouldn't matter. I'd just go with using a single tag name.

coollog · 2018-08-29T14:43:42Z

Is this PR ready for review? (The Not Ready is present)

briandealwis · 2018-08-29T14:47:11Z

Oops, let me fix up @chanseokoh's findings and I'll remove the label.

briandealwis · 2018-08-29T15:16:42Z

PTAL

coollog · 2018-08-29T15:47:01Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

@@ -149,6 +150,18 @@ public JavaDockerContextGenerator setBaseImage(String baseImage) {
    return this;
  }

+  /**
+   * Sets the entrypoint to be used as the {@code ENTRYPOINT}. If not empty, then overrides the
+   * {@link #setJvmFlags(List) jvmFlags} and {@link #setMainClass(String) mainclass}.


I think if we have this method, we should just remove the setJvmFlags and setMainClass methods and delegate the call of JavaEntrypointConstructor#makeEntrypoint to the user of JavaDockerContextGenerator.

Makes sense.

There's not much specific to Java in JavaDockerContextGenerator now!

coollog · 2018-08-29T15:50:01Z

jib-gradle-plugin/README.md

@@ -197,6 +197,7 @@ Property | Type | Default | Description
 `ports` | `List<String>` | *None* | Ports that the container exposes at runtime (similar to Docker's [EXPOSE](https://docs.docker.com/engine/reference/builder/#expose) instruction).
 `format` | `String` | `Docker` | Use `OCI` to build an [OCI container image](https://www.opencontainers.org/).
 `useCurrentTimestamp` | `boolean` | `false` | By default, Jib wipes all timestamps to guarantee reproducibility. If this parameter is set to `true`, Jib will set the image's creation timestamp to the time of the build, which sacrifices reproducibility for easily being able to tell when your image was created.
+`entrypoint` | list | *None* | The executable to be run (similar to Docker's [ENTRYPOINT](https://docs.docker.com/engine/reference/builder/#entrypoint) instruction). If set then `jvmFlags` and `mainClass` are ignored.


We should leave changes to documentation to another PR (like #871) after the feature is released as a new version of Jib, so users do not see an unreleased feature documented.

In fact, if this is going in 0.9.10, feel free to push to that branch so we can merge all the documentation changes in at once after releasing.

will back out the docs.

coollog · 2018-08-29T18:35:07Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/DockerContextMojo.java

@@ -70,8 +70,7 @@ public void execute() throws MojoExecutionException {

      new JavaDockerContextGenerator(mavenProjectProperties.getJavaLayerConfigurations())
          .setBaseImage(getBaseImage())
-          .setJvmFlags(getJvmFlags())
-          .setMainClass(mainClass)
+          .setEntrypoint(getEntrypoint())


I think this is missing the logic to handle JVM flags and main class? We might want to add some tests to make sure JVM flags and mainClass still work.

TadCordle · 2018-08-29T18:43:38Z

jib-gradle-plugin/src/integration-test/resources/projects/default-target/build.gradle

@@ -16,6 +16,7 @@ dependencies {

 jib {
    container {
+        entrypoint = ['foo', 'bar', 'baz']


These cause the integration tests to fail, since the integration tests actually try to run the built image, and 'foo' isn't an executable. For example, for testBuildTar_simple:

java.lang.RuntimeException: Command 'docker run simpleimage:gradle3455722031277561' failed: docker: Error response from daemon: OCI runtime create failed: container_linux.go:348: starting container process caused "exec: \"foo\": executable file not found in $PATH": unknown. time="2018-08-29T14:41:10-04:00" level=error msg="error waiting for container: context canceled"

To go off of what @coollog said, maybe it'd be a good idea to put this in a separate test that doesn't actually run the image and just inspects it, but continues to run the images generated using mainClass and jvmFlags.

briandealwis · 2018-08-29T19:53:46Z

Sorry :-( Had someone drop by and pushed — ITs don't run well locally.

coollog · 2018-08-29T21:29:07Z

jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/EntrypointIntegrationTest.java

+    verifier.setAutoclean(false);
+    verifier.executeGoal("package");
+
+    verifier.executeGoal("jib:" + BuildTarMojo.GOAL_NAME);


It looks like this should be part of BuildTarMojoIntegrationTest?

This isn't a test of building a tar — I chose the tar as it has the lowest overhead of the three build options.

Okay, let's add a javadoc describing what this class is testing/

But we are using jib:buildTar to build a tar and then verifying that it has the correct entrypoint here, aren't we? I think I agree that it does make more sense to put this in BuildTarMojoIntegrationTest. As far as I understand, the test file should correspond directly to a source file (and we don't have an Entrypoint class in jib-maven-plugin).

coollog · 2018-08-29T21:31:07Z

...in/src/integration-test/java/com/google/cloud/tools/jib/gradle/JibPluginIntegrationTest.java

+  }
+
+  @Test
+  public void testBuildTar_entrypoint() throws IOException {


If we do choose only one of the tasks to test, I would lean towards jib (build to registry) since it is the main functionality of Jib.

Communicating back to the registry to pull out the entrypoint only adds more overhead. I'd rather build a test mojo / task.

Maybe this method should just be called testCustomEntrypoint then?

I like what you did here, I wonder if it's viable to replace some of our other integration tests with this style to speed things up.

This reverts commit 5de242d.

TadCordle · 2018-08-30T18:45:20Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

@@ -150,24 +149,13 @@ public JavaDockerContextGenerator setBaseImage(String baseImage) {
  }

  /**
-   * Sets the JVM flags used in the {@code ENTRYPOINT}.
+   * Sets the entrypoint to be used as the {@code ENTRYPOINT}.


Maybe just Sets the {@code ENTRYPOINT}.?

TadCordle · 2018-08-30T18:50:14Z

...in/src/integration-test/java/com/google/cloud/tools/jib/gradle/JibPluginIntegrationTest.java

+    Assert.assertNotNull(jibTask);
+    Assert.assertEquals(TaskOutcome.SUCCESS, jibTask.getOutcome());
+    Assert.assertThat(
+        buildResult.getOutput(), CoreMatchers.containsString("Created Docker context at "));


Maybe we should add a bit more verification here, if you think it's necessary (e.g. build with docker like the other tests and inspect, although that could be too much overhead. Or maybe just check for the dockerfile and make sure it contains the correct commands)?

TadCordle · 2018-08-30T18:59:31Z

...in/src/integration-test/java/com/google/cloud/tools/jib/gradle/JibPluginIntegrationTest.java

+  }
+
+  @Test
+  public void testBuildTar_entrypoint() throws IOException {


I like what you did here, I wonder if it's viable to replace some of our other integration tests with this style to speed things up.

TadCordle · 2018-08-30T19:06:46Z

jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/EntrypointIntegrationTest.java

+    verifier.setAutoclean(false);
+    verifier.executeGoal("package");
+
+    verifier.executeGoal("jib:" + BuildTarMojo.GOAL_NAME);


But we are using jib:buildTar to build a tar and then verifying that it has the correct entrypoint here, aren't we? I think I agree that it does make more sense to put this in BuildTarMojoIntegrationTest. As far as I understand, the test file should correspond directly to a source file (and we don't have an Entrypoint class in jib-maven-plugin).

briandealwis · 2018-08-30T19:08:20Z

I've reverted the integration tests and instead added tests around PluginConfigurationProcessor instead. Much simpler.

coollog · 2018-08-30T19:09:58Z