-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Inherit ENTRYPOINT and CMD from base image for WAR projects #1071
Inherit ENTRYPOINT and CMD from base image for WAR projects #1071
Conversation
af9b956
to
5e9602e
Compare
4f801d0
to
e15f370
Compare
e15f370
to
e0ccae4
Compare
+ buildConfiguration.getContainerConfiguration().getEntrypoint())); | ||
String eventMessage; | ||
if (buildConfiguration.getContainerConfiguration().isEntrypointInferredFromBaseImage()) { | ||
eventMessage = "Container entrypoint inferred from base image"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, for UX, I would prefer this to include the entrypoint that was inherited from the base image, but this would require a bit more code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coollog, what do you think about moving this code into BuildImageSteps.afterCacheEntrySteps()
method ?
First of all, thanks, @CyrilleH. Ah, now I see this PR is about inheriting ENTRYPOINT and CMD from the base image. And doing so only when it's a WAR project. It makes sense we always use the ENTRYPOINT and CMD of the base image of Servlet engines, of course. However, this is also related to #924 for non-WAR cases too, so I think we should understand the use cases of #924 and give some more thoughts about the best way to inherit ENTRYPOINT and CMD for both WAR and non-WAR. |
e0ccae4
to
452080c
Compare
@@ -209,6 +247,14 @@ public Instant getCreationTime() { | |||
return labels; | |||
} | |||
|
|||
public boolean isEntrypointInferredFromBaseImage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording nit: Since this is the configuration, the flag is more of like "should entrypoint be inferred from base image?" rather than "is entrypoint inferred from base image?". Perhaps this could be named shouldInheritEntrypoint
(base image is implied)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's better.
@@ -209,6 +247,14 @@ public Instant getCreationTime() { | |||
return labels; | |||
} | |||
|
|||
public boolean isEntrypointInferredFromBaseImage() { | |||
return isEntrypointInferredFromBaseImage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also thinking perhaps we should have the field entrypoint
as null
to indicate that it should be inherited. The API for ContainerConfiguration
would be the same, but the builder's setter could be called setInheritEntrypoint
and it would just set the entrypoint to null. This would make it so that we never have a state where getEntrypoint
returns something while shouldInheritEntrypoint
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GoogleContainerTools/java-tools ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also leaning toward to not having this extra flag but interpreting the null
value of entrypoint
as inheriting it from the base image. And I think we can apply this policy consistently across other fields.
Does it need an additional setInheritEntrypoint
? The builder's setEntrypoint
already accepts null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we go for the null-means-inherit, I think we also need to consider the validity of possible Entrypoint
and Cmd
combinations. For example, with Dockerfile
, if you define a new Entypoint
but no Cmd
, Cmd
is not inherited but set to null
explicitly in the image metadata. On the other hand, if you define a new Cmd
but no Entrypoint
, you inherit Entrypoint
. So, if Entrypoint
is non-null, we shouldn't inherit Cmd
even if Cmd
is null.
93e0bcd
to
b4388dd
Compare
boolean shouldInheritProgramArguments = | ||
containerConfiguration.getEntrypoint() == null | ||
&& (containerConfiguration.getProgramArguments() == null | ||
|| containerConfiguration.getProgramArguments().isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it should not inherit the base image program arguments if the program arguments configuration was set to an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it does not work since by default it's set to an empty list.
For example in Gradle, ContainerParameters
:
private boolean useCurrentTimestamp = false;
private List<String> jvmFlags = Collections.emptyList();
private Map<String, String> environment = Collections.emptyMap();
private List<String> entrypoint = Collections.emptyList();
@Nullable private String mainClass;
private List<String> args = Collections.emptyList();
If you want, I can change to null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should decide whether to inherit CMD or not in the same way as ENTRYPOINT.
@@ -173,12 +174,43 @@ public DescriptorDigest getDiffId() throws LayerPropertyNotFoundException { | |||
.build()); | |||
} | |||
if (containerConfiguration != null) { | |||
boolean shouldInheritEntrypoint = containerConfiguration.getEntrypoint() == null; | |||
boolean shouldInheritProgramArguments = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about breaking this out for readability.
imageBuilder.setEntryPoint(calculateEntrypoint(containerConfiguration, baseImage))
imageBuilder.setJavaArguments(calculateProgramArguments(containerConfiguration, baseImage))
and implement the calculateX
methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need these boolean for these logs :
if (shouldInheritEntrypoint && baseImage.getEntrypoint() != null) {
buildConfiguration
.getEventDispatcher()
.dispatch(
LogEvent.lifecycle(
"Container entrypoint set to "
+ baseImage.getEntrypoint()
+ " (inherited from base image)"));
}
if (shouldInheritProgramArguments && baseImage.getJavaArguments() != null) {
buildConfiguration
.getEventDispatcher()
.dispatch(
LogEvent.lifecycle(
"Container program arguments set to "
+ baseImage.getJavaArguments()
+ " (inherited from base image)"));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In second thought, I've introduced two method computeEntrypoint and computeProgramArguments.
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/BuildImageStep.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/BuildImageStep.java
Outdated
Show resolved
Hide resolved
@@ -132,7 +131,7 @@ private static String mapToDockerfileString(Map<String, String> map, String comm | |||
private final ImmutableList<CopyDirective> copyDirectives; | |||
|
|||
@Nullable private String baseImage; | |||
private List<String> entrypoint = Collections.emptyList(); | |||
@Nullable private List<String> entrypoint = Collections.emptyList(); | |||
private List<String> javaArguments = Collections.emptyList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think javaArguments
should be @Nullable
too?
dockerfile.append("\nENTRYPOINT ").append(objectMapper.writeValueAsString(entrypoint)); | ||
} | ||
if (entrypoint != null || !javaArguments.isEmpty()) { | ||
dockerfile.append("\nCMD ").append(objectMapper.writeValueAsString(javaArguments)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it's possible to have CMD while not having (i.e., while inheriting) ENTRYPOINT (and vice versa) in Dockerfile
. Also, I think we should support printing "CMD []" (i.e., the empty list for javaArguments
. Maybe this should be just if (javaArguments != null) {
after making javaArguments
@Nullable
. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the logic into plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PluginConfigurationProcessor.computeProgramArguments
Path dockerfile = projectRoot.getRoot().toPath().resolve("target/Dockerfile"); | ||
List<String> lines = Files.readAllLines(dockerfile); | ||
return lines.stream().filter(line -> line.startsWith("CMD")).findFirst().get(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about removing code duplication?
private String getEntrypoint() {
getDockerfileLine("ENTRYPOINT");
}
private String getBaseImage() {
getDockerfileLine("FROM");
}
private String getCmd() {
getDockerfileLine("CMD");
}
private String getDockerfileLine(String command) {
Path dockerfile = projectRoot.getRoot().toPath().resolve("target/Dockerfile");
List<String> lines = Files.readAllLines(dockerfile);
return lines.stream().filter(line -> line.startsWith(command)).findFirst().get();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this for DockerContextTaskTest
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes obviously.
@Nullable | ||
private ImmutableList<String> computeEntrypoint( | ||
Image<Layer> baseImage, ContainerConfiguration containerConfiguration) { | ||
boolean shouldInheritEntrypoint = containerConfiguration.getEntrypoint() == null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not consistent with program arguments in that it doesn't inherit if it is an empty list. Reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tomcat have no entrypoing but cmd. I changed how to deal with it in Gradle and Maven plugins. Tell me if it suits you now.
boolean shouldInheritProgramArguments = | ||
containerConfiguration.getEntrypoint() == null | ||
&& (containerConfiguration.getProgramArguments() == null | ||
|| containerConfiguration.getProgramArguments().isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strings.isNullOrEmpty
is handy here. But then, this is not consistent with ENTRYPOINT, as I said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type is ImmutableList<String>
not String
.
} | ||
|
||
String mainClass = projectProperties.getMainClass(jibPluginConfiguration); | ||
return JavaEntrypointConstructor.makeDefaultEntrypoint( | ||
getAppRootChecked(jibPluginConfiguration), jibPluginConfiguration.getJvmFlags(), mainClass); | ||
} | ||
|
||
/** | ||
* Compute the container program arguments. If entrypoint is inherited (null), program arguments | ||
* must be inherited (null) if empty (used for Tomcat base images). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This special handling for Tomcat and the inconsistency between the entrypoint being null and the program arguments being empty bother me. Ideally, I think the entrypoint and the program arguments could be nullable and start with null at the beginning instead of the current defaulting to empty, indicating Jib will by default inherit entrypoint and cmd. Then for non-WAR projects, we are already setting the entrypoint and the cmd explicitly, so we still won't inherit them for the non-WAR projects as usual. That is, the non-WAR is a special case that overrides the default. On the other hand, for WAR, we would not touch the null entrypoint and cmd, and inheriting them would be just the default behavior. Also, I imagine fixing #924 fits naturally in this model, since we can later add an option to not override them in the non-WAR case (through some configuration or so). @GoogleContainerTools/java-tools Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - we should have the default be null
and set to empty if needed (so non-WAR would set entrypoint as generated and program arguments to empty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a problem. If the user set the ENTRYPOINT to [catalina.sh, run]
and the CMD is null (so it will be inherited) then, we will have ENTRYPOINT and CMD equal to [catalina.sh, run]
.
Can I set CMD to an empty list if the ENTRYPOINT is not null ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's right to not inherit CMD when ENTRYPOINT is redefined: #1071 (comment)
I'm just wondering if CMD []
in Dockerfile
will set the value of the CMD metadata to null or an empty string. I'll have to double-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I set CMD to an empty list if the ENTRYPOINT is not null ?
Hmm... this actually is tricky.
The value of an empty list is supposed to mean CMD []
, which will specifically define CMD (and override the base value) to be an empty string. And we are trying to set the behavior here that the null value means inheriting CMD from the base image. That said, there is no way to explicitly set CMD to null in the container metadata. Sure, setting to an empty list might be a feasible choice, but this diverges from how Docker build actually works, although I'm not saying we always have to stick to how Docker build works.
e485def
to
22c77de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
"Container entrypoint set to " | ||
+ buildConfiguration.getContainerConfiguration().getEntrypoint())); | ||
|
||
if (buildConfiguration.getContainerConfiguration().getEntrypoint() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should definitely still output the entrypoint when it is inherited, but it might require some more reworking of the code, so let's put in a TODO for that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
For example when program arguments are inherited we have :
Container program arguments set to [catalina.sh, run] (inherited from base image)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clarify TODO:
// TODO refactor code to also log ENTRYPOINT and CMD when inheriting them in this code, instead of logging them elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@Nullable | ||
private ImmutableList<String> computeProgramArguments( | ||
Image<Layer> baseImage, ContainerConfiguration containerConfiguration) { | ||
boolean shouldInheritProgramArguments = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to lay out the logic here as:
if (baseImage.getProgramArguments() == null || containerConfiguration.getEntrypoint() != null || containerConfiguration.getProgramArguments != null) {
return containerConfiguration.getProgramArguments();
}
buildConfiguration.getEventDispatcher().dispatch(...);
return baseImage.getProgramArguments();
(and similarly for computeEntrypoint
)
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/BuildImageStep.java
Show resolved
Hide resolved
|
||
@Test | ||
public void test_inheritedEntrypoint() throws ExecutionException, InterruptedException { | ||
Mockito.when(mockContainerConfiguration.getEntrypoint()).thenReturn(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add one more test where entrypoint
is not null but program arguments
are null and check that program arguments
are not inherited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. See test_notInheritedProgramArguments
.
@@ -39,7 +39,7 @@ | |||
private Map<String, String> environment = Collections.emptyMap(); | |||
private List<String> entrypoint = Collections.emptyList(); | |||
@Nullable private String mainClass; | |||
private List<String> args = Collections.emptyList(); | |||
@Nullable private List<String> args = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can drop the = null;
getEntrypoint(); | ||
Assert.fail(); | ||
} catch (NoSuchElementException e) { | ||
Assert.assertEquals("No value present", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the generic message for NoSuchElementException
, we can just replace this with // pass
.
@@ -128,7 +128,7 @@ public void set(String image) { | |||
|
|||
@Nullable @Parameter private String mainClass; | |||
|
|||
@Parameter private List<String> args = Collections.emptyList(); | |||
@Nullable @Parameter private List<String> args = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ = null
//
* @return the program arguments | ||
*/ | ||
@Nullable | ||
static List<String> computeProgramArguments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto this
getEntrypoint(); | ||
Assert.fail(); | ||
} catch (NoSuchElementException e) { | ||
Assert.assertEquals("No value present", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Path dockerfile = projectRoot.getRoot().toPath().resolve("target/Dockerfile"); | ||
List<String> lines = Files.readAllLines(dockerfile); | ||
return lines.stream().filter(line -> line.startsWith("CMD")).findFirst().get(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this for DockerContextTaskTest
as well.
5df37fd
to
696ef27
Compare
"Container entrypoint set to " | ||
+ buildConfiguration.getContainerConfiguration().getEntrypoint())); | ||
|
||
if (buildConfiguration.getContainerConfiguration().getEntrypoint() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clarify TODO:
// TODO refactor code to also log ENTRYPOINT and CMD when inheriting them in this code, instead of logging them elsewhere.
dockerfile.append("\nCMD ").append(objectMapper.writeValueAsString(programArguments)); | ||
} else if (entrypoint != null) { | ||
// Do not inherit CMD from base image if ENTRYPOINT is not inherited | ||
dockerfile.append("\nCMD []"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this. if entrypoint
is not null, Dockefile
will have the line ENTRYPOINT
but no CMD
, and Docker build won't inherit CMD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've removed this.
Just making a note: integration tests will fail, but I think it will be difficult for an external contributor to fix them. It's probably more productive for us to fix them later. |
Removes "CMD []" when entrypoint is present in Dockerfile Changes a TODO description
696ef27
to
3256715
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good? @GoogleContainerTools/java-tools
FTR: we will stop adding |
Thanks, @CyrilleH. The PR is merged. |
Thanks again for your contribution! |
Towards #431, here's my proposal to support inferring "entrypoint" from base images, for WAR project.
These configurations are no longer needed for Tomcat base images :
This PR is based from #1068.Please review only the last commit.