Add core changes for configuring exposed ports #432

TadCordle · 2018-06-20T20:56:16Z

Part of #383

Still missing Docker context export stuff, but I'll get to that after this

…_ports_config

coollog · 2018-06-21T00:41:56Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -204,6 +277,7 @@ public static boolean isValidJavaClass(String className) {
  private final ImmutableList<String> javaArguments;
  private final ImmutableList<String> jvmFlags;
  private final ImmutableMap<String, String> environmentMap;
+  private final ImmutableSortedMap<String, EmptyStruct> exposedPorts;


This could just be a list of strings right? (and in the other places)

coollog · 2018-06-21T00:44:45Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

@@ -88,6 +91,9 @@

    /** Arguments to pass into main. */
    @Nullable private List<String> Cmd;
+
+    /** Network ports the container listens on. */


Maybe just "...container exposes"? The container doesn't necessarily need to listen on those ports.

coollog · 2018-06-21T00:45:37Z

jib-core/src/main/java/com/google/cloud/tools/jib/json/EmptyStruct.java

+package com.google.cloud.tools.jib.json;
+
+/** Empty class used for empty "{}" blocks in json. */
+public class EmptyStruct implements JsonTemplate {


A json "{}" I believe can just be represented as Object, or an empty Map.

coollog · 2018-06-21T00:46:46Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

@@ -118,6 +124,10 @@ public void setContainerCmd(List<String> cmd) {
    config.Cmd = cmd;
  }

+  public void setContainerExposedPorts(SortedMap<String, EmptyStruct> exposedPorts) {
+    config.ExposedPorts = exposedPorts;


This could be where the list is converted into a SortedMap.

coollog · 2018-06-21T00:47:18Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

@@ -141,6 +151,11 @@ public void addLayerDiffId(DescriptorDigest diffId) {
    return config.Cmd;
  }

+  @Nullable
+  SortedMap<String, EmptyStruct> getContainerExposedPorts() {
+    return config.ExposedPorts;


And converted back to a list.

coollog · 2018-06-21T00:48:49Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

+
+    for (String port : ports) {
+      // Remove all spaces
+      port = port.trim().replace(" ", "");


I believe Maven parameters automatically trim whitespace.

What about gradle?

In gradle, you would set something to a specific string, so it should be up to the user to make sure they type like port = '2345' rather than port = ' 2345', and to error on the latter so they fix their stray space.

coollog · 2018-06-21T00:57:38Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

+      if (port.matches("\\d+")) {
+        // Port is a single number
+        int portNum = Integer.parseInt(port);
+        if (portNum < 1 || portNum > 65535) {


I'm not sure if we want to error on possibly invalid ports, since it is not technically an invalid container configuration. A warning may be enough.

loosebazooka · 2018-06-21T21:25:42Z

It'd be nice if we had a description of the issue in either the bug or pull request. Kind of what the external surface will look like after this change.

TadCordle · 2018-06-21T21:39:25Z

@loosebazooka I left a comment on the issue for what it might look like: #383

…_ports_config

coollog · 2018-06-22T14:09:28Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

-import java.util.Map;
-import java.util.SortedMap;
-import java.util.TreeMap;
+import java.util.*;


Should use explicit imports and no wildcards

coollog · 2018-06-22T14:10:19Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

-      throws NumberFormatException {
-    SortedMap<String, EmptyStruct> result = new TreeMap<>();
+  static ImmutableList<String> expandPortRanges(List<String> ports, BuildLogger logger) {
+    List<String> result = new ArrayList<>();


ImmutableList.Builder can avoid doing the copyOf

coollog · 2018-06-22T14:12:32Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java


      if (port.matches("\\d+")) {
        // Port is a single number
        int portNum = Integer.parseInt(port);
        if (portNum < 1 || portNum > 65535) {
-          throw new NumberFormatException("Invalid port number " + port);
+          logger.warn("Port number '" + port + "' is out of range (1-65535)");


This is fine for now, but we should add a TODO to give more details on where this port number is from and what the configuration parameter is, and have the configurable message be in HelpfulSuggestions.

coollog · 2018-06-22T14:13:27Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java


    for (String port : ports) {
      // Remove all spaces
-      port = port.trim().replace(" ", "");
+      port = port.replace(" ", "");


I don't think we should be allowing the user to type like 1_2____4_3 and have it be a valid port.

coollog · 2018-06-22T14:16:47Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

-  static ImmutableSortedMap<String, EmptyStruct> portListToPortMap(List<String> ports)
-      throws NumberFormatException {
-    SortedMap<String, EmptyStruct> result = new TreeMap<>();
+  static ImmutableList<String> expandPortRanges(List<String> ports, BuildLogger logger) {


It looks like this is only used in the Builder class, so it should prob live in there. Making it non-static would remove the need to have BuildLogger as the second param.

coollog · 2018-06-22T14:23:37Z

jib-core/src/main/java/com/google/cloud/tools/jib/image/Image.java

@@ -100,8 +98,8 @@
     *     appear in the configuration json (e.g. "portNum/tcp")
     * @return this
     */
-    public Builder<T> setExposedPorts(Map<String, EmptyStruct> exposedPorts) {
-      this.exposedPorts = ImmutableSortedMap.copyOf(exposedPorts);
+    public Builder<T> setExposedPorts(List<String> exposedPorts) {


I think we can actually change these to ImmutableList to avoid doing a copyOf on another ImmutableList, or just leave a TODO for now.

coollog · 2018-06-22T14:24:23Z

jib-core/src/main/java/com/google/cloud/tools/jib/image/Image.java

@@ -123,7 +121,7 @@
          environmentBuilder.build(),
          ImmutableList.copyOf(entrypoint),
          ImmutableList.copyOf(javaArguments),
-          ImmutableSortedMap.copyOf(exposedPorts));
+          ImmutableList.copyOf(exposedPorts));


These copyOfs are not necessary anymore.

coollog · 2018-06-22T14:26:16Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

-  public void setContainerExposedPorts(SortedMap<String, EmptyStruct> exposedPorts) {
-    config.ExposedPorts = exposedPorts;
+  public void setContainerExposedPorts(List<String> exposedPorts) {
+    SortedMap<String, Map<?, ?>> result = new TreeMap<>();


ImmutableSortedMap.Builder

coollog · 2018-06-22T14:30:23Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

+    List<String> ports = new ArrayList<>();
+    for (Map.Entry<String, Map<?, ?>> entry : config.ExposedPorts.entrySet()) {
+      // Remove the "/tcp"
+      ports.add(Splitter.on('/').splitToList(entry.getKey()).get(0));


This might break if the entry was not in the form \d+/tcp.

coollog · 2018-06-22T14:31:01Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

+  public void setContainerExposedPorts(List<String> exposedPorts) {
+    SortedMap<String, Map<?, ?>> result = new TreeMap<>();
+    for (String port : exposedPorts) {
+      result.put(port + "/tcp", Collections.emptyMap());


I believe we can just do port without /tcp since by default it is tcp.

https://github.com/opencontainers/image-spec/blob/master/config.md#properties

TadCordle · 2018-06-22T16:29:36Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

-import java.util.ArrayList;
-import java.util.List;
+import com.google.common.collect.ImmutableSortedMap;
+import java.util.*;


I don't know why IntelliJ keeps doing this

I think there's a configuration you have to disable.

https://stackoverflow.com/questions/3348816/intellij-never-use-wildcard-imports

coollog · 2018-06-22T18:19:44Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

+        String protocol = splitProtocol.size() > 1 ? "/" + splitProtocol.get(1) : "";
+
+        // Parse range (or treat as range of min-min if only single port configuration)
+        List<String> splitRange = Splitter.on("-").splitToList(splitProtocol.get(0));


Since you are using pattern match above already, it might make sense to just use the regex matcher to get the parts, so the regex would be like:

(\d+)(-\d+|)(/tcp|/udp|)

So that it would match 4 groups, where 1 is min, 2 is max or nothing, and 3 is protocol or nothing.

coollog · 2018-06-22T18:21:52Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

+
+        // Add all numbers in range to list
+        for (int portNum = min; portNum <= max; portNum++) {
+          result.add(portNum + protocol);


Since we are passing this through the code, we might want to consider putting this in a canonical form rather than the docker image format for of port/protocol. Like a class that has a port and a protocol. Can also just be in a TODO for now.

coollog · 2018-06-22T18:56:22Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -201,6 +203,7 @@ public BuildConfiguration build() {
    @VisibleForTesting
    ImmutableList<String> expandPortRanges(List<String> ports) throws NumberFormatException {
      ImmutableList.Builder<String> result = new ImmutableList.Builder<>();
+      Pattern portPattern = Pattern.compile("(\\d+)(-\\d+|)(\\/tcp|\\/udp|)");


This can be a constant so that it doesnt have to be compiled each time.

coollog · 2018-06-22T18:58:57Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -201,6 +203,7 @@ public BuildConfiguration build() {
    @VisibleForTesting
    ImmutableList<String> expandPortRanges(List<String> ports) throws NumberFormatException {
      ImmutableList.Builder<String> result = new ImmutableList.Builder<>();
+      Pattern portPattern = Pattern.compile("(\\d+)(-\\d+|)(\\/tcp|\\/udp|)");


I just tested it more and realized you can just do

(\d+)(?:-(\d+))?(/tcp|/udp)?

And it'll match those groups, with them being null if it didn't match.

coollog · 2018-06-22T18:59:20Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

        int max = min;
-        if (splitRange.size() > 1) {
-          max = Integer.parseInt(splitRange.get(1));
+        if (!matcher.group(2).equals("")) {


Strings.isNullOrEmpty

coollog · 2018-06-22T19:23:04Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

+     * @throws NumberFormatException if any of the ports are in an invalid format or out of range
+     */
+    @VisibleForTesting
+    ImmutableList<String> expandPortRanges(List<String> ports) throws NumberFormatException {


Let's add a TODO for moving this to a class in frontend, so that BuildConfiguration can be kept free of user input parsing.

coollog · 2018-06-22T19:29:10Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

@@ -118,6 +127,15 @@ public void setContainerCmd(List<String> cmd) {
    config.Cmd = cmd;
  }

+  public void setContainerExposedPorts(List<String> exposedPorts) {


The template itself should probably be free of reproducibility logic. This can just be a TODO for now too.

coollog · 2018-06-22T19:31:18Z

...core/src/main/java/com/google/cloud/tools/jib/image/json/ContainerConfigurationTemplate.java

@@ -88,6 +94,9 @@

    /** Arguments to pass into main. */
    @Nullable private List<String> Cmd;
+
+    /** Network ports the container exposes. */
+    @Nullable private SortedMap<String, Map<?, ?>> ExposedPorts;


It might make sense to have this be a custom object that stores a list. That way, there doesn't have to be any logic in getContainerExposedPorts.

coollog · 2018-06-22T19:32:15Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -38,6 +39,9 @@

  public static class Builder {

+    /** Pattern used for parsing information out of exposed port configurations. */
+    private static Pattern portPattern = Pattern.compile("(\\d+)(?:-(\\d+))?(/tcp|/udp)?");


coollog · 2018-06-22T19:33:04Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -38,6 +39,9 @@

  public static class Builder {

+    /** Pattern used for parsing information out of exposed port configurations. */


The comment for examples on line 209-215 should go here

coollog · 2018-06-22T19:33:35Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -227,9 +230,9 @@ public BuildConfiguration build() {
        // Parse protocol
        int min = Integer.parseInt(matcher.group(1));
        int max = min;
-        if (!matcher.group(2).equals("")) {
+        if (!Strings.isNullOrEmpty(matcher.group(2))) {
          // Skip over the hyphen


this comment is not applicable anymore

…_ports_config

coollog · 2018-06-22T20:09:45Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

@@ -178,6 +198,63 @@ public BuildConfiguration build() {
          throw new IllegalStateException(errorMessage.toString());
      }
    }
+
+    /**
+     * TODO: Move this to a class in frontend


Let's file a bug to take care of these TODOs

TadCordle added 3 commits June 20, 2018 16:20

Add core changes for exposed port configuration

0b82cf6

Update integration tests

dd44f8b

Javadoc

fcfa21a

TadCordle requested review from loosebazooka and coollog June 20, 2018 20:56

Merge branch 'master' of github.com:GoogleContainerTools/jib into add…

39c34a4

…_ports_config

coollog reviewed Jun 21, 2018

View reviewed changes

TadCordle added 3 commits June 22, 2018 10:02

Merge branch 'master' of github.com:GoogleContainerTools/jib into add…

ac8d674

…_ports_config

Feedback

218bce1

Remove EmptyStruct

d60d7eb

TadCordle mentioned this pull request Jun 22, 2018

Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency #438

Merged

coollog reviewed Jun 22, 2018

View reviewed changes

TadCordle added 2 commits June 22, 2018 11:42

Feedback

6b7fba5

Add support for udp

fefd342

TadCordle commented Jun 22, 2018

View reviewed changes

TadCordle added 2 commits June 22, 2018 12:32

Use builders and expand imports

7fa64bb

i m p o r t s

a71e7bb

coollog reviewed Jun 22, 2018

View reviewed changes

Use regex for splitting

cc25cd4

coollog reviewed Jun 22, 2018

View reviewed changes

z

29b5757

coollog reviewed Jun 22, 2018

View reviewed changes

TadCordle added 4 commits June 22, 2018 15:56

TODOs

9122a6a

Merge branch 'master' of github.com:GoogleContainerTools/jib into add…

5a1c4c9

…_ports_config

Format

dc5a068

Fix examples

44f79f7

coollog approved these changes Jun 22, 2018

View reviewed changes

TadCordle merged commit f6028ca into master Jun 22, 2018

TadCordle deleted the add_ports_config branch June 22, 2018 20:17

		@@ -38,6 +39,9 @@

		public static class Builder {

		/** Pattern used for parsing information out of exposed port configurations. */

Add core changes for configuring exposed ports #432

Add core changes for configuring exposed ports #432

Conversation

TadCordle commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loosebazooka commented Jun 21, 2018

TadCordle commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment