Propagate base image's environment variable config to built image #716

TadCordle · 2018-07-25T18:12:13Z

Part of #595. Also did a small bit of refactoring/error checking for translating between environment variable list/map

…opagate-container-config

chanseokoh · 2018-07-25T19:20:53Z

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

@@ -96,8 +100,15 @@
          buildAndCacheApplicationLayerSteps) {
        imageBuilder.addLayer(NonBlockingSteps.get(buildAndCacheApplicationLayerStep));
      }
+
+      // Use environment from base image if not configured
+      if (buildConfiguration.getEnvironment() == null) {


Later we will have the ability to add (not overwrite) env variables, right?

Possibly, I'm not sure what the plan is for that.

Yea, I think environment variables should be overwritten only at the specific variable level, and not at the entire environment level.

So it could be like initialize imageBuilder with the base image, then apply overwrites from buildConfiguration.

chanseokoh · 2018-07-25T19:23:49Z

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

+    }
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    for (Map.Entry<String, String> environmentVariable : environment.entrySet()) {
+      builder.add(environmentVariable.getKey() + "=" + environmentVariable.getValue());


I see this PR does not change the current behavior, but I'm just wondering if any kind of escaping should also be considered. Do we have to convert this to a list? Maybe changing some API, we could pass the map and don't worry about escaping at all, although I admit it'd be very rare to have = in a variable name.

Yeah, I definitely think this can be more bullet proof. Actually I think this breaks if there's an = in the value, not just the name.

Edit: Not this, the opposite conversion in JsonToImageTranslator

Hmm I think we might want to guard this at the environment-setting level - so maybe we might need to have a separate Environment class to be this abstraction (like mentioned in #15)

We can handle this when we handle #15 then?

Maybe we can just throw an IllegalArgumentException for now?

coollog · 2018-07-25T21:17:42Z

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

+    }
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    for (Map.Entry<String, String> environmentVariable : environment.entrySet()) {
+      builder.add(environmentVariable.getKey() + "=" + environmentVariable.getValue());


Hmm I think we might want to guard this at the environment-setting level - so maybe we might need to have a separate Environment class to be this abstraction (like mentioned in #15)

coollog · 2018-07-25T21:21:03Z

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

+   * <p>Example matches: NAME=VALUE, A12345=$$$$$
+   */
+  @VisibleForTesting
+  static final Pattern environmentPattern = Pattern.compile("([A-Za-z][A-Za-z0-9_]+)=(.*)");


I believe environment variable names can actually contain any character (http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap08.html) - These strings have the form name=value; names shall not contain the character '='., so we should probably have this pattern be like (.*?)=(.*).

Here it says:

env: (array of strings, OPTIONAL) with the same semantics as IEEE Std 1003.1-2008's environ.

And in the link you shared:

Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit

Yep, for environment variables used in shell utilities (like bash), the names shouldn't consist of anything besides the alphanumerics, but for process environment variables, the definition is more lenient to allow any character besides =.

reading is hard

Shouldn't we make sure the name has at least one character though? I think what you gave will match something like "=ABC". Is that valid?

That’s a good point - let’s change the star to a plus.

That matches "====", I think ([^=].*?)=(.*) should work

Yea, I think having = in the value is okay

coollog · 2018-07-25T21:26:30Z

jib-core/src/test/java/com/google/cloud/tools/jib/builder/steps/BuildImageStepTest.java

@@ -46,6 +47,8 @@

  @Mock private BuildConfiguration mockBuildConfiguration;
  @Mock private BuildLogger mockBuildLogger;
+  @Mock private PullBaseImageStep mockPullBaseImageStep;
+  @Mock private Image<Layer> mockBaseImage;


We could prob just have a real Image since there's nothing in Image that needs to be mocked.

coollog · 2018-07-25T21:27:51Z

jib-core/src/test/java/com/google/cloud/tools/jib/image/json/JsonToImageTranslatorTest.java

@@ -110,6 +111,32 @@ public void testPortMapToList() throws BadContainerConfigurationFormatException
    }
  }

+  @Test
+  public void testJsonToImageTranslatorRegex() {
+    assertGoodPattern("NAME=VALUE", "NAME", "VALUE");


Might want to call this like assertGoodEnvironmentPattern

coollog · 2018-07-25T21:28:12Z

jib-gradle-plugin/CHANGELOG.md

@@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file.

 - Only builds non-empty layers ([#516](https://github.com/GoogleContainerTools/jib/pull/516/files))
 - Fixed slow image reference parsing ([#680](https://github.com/GoogleContainerTools/jib/pull/680))
+- Environment variable configuration is propagated from the base image ([#716](https://github.com/GoogleContainerTools/jib/pull/716))


nit: Propagates environment variables from base image...

…opagate-container-config

chanseokoh · 2018-07-25T22:55:17Z

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

+   *
+   * <p>Example matches: NAME=VALUE, A12345=$$$$$
+   */
+  @VisibleForTesting static final Pattern environmentPattern = Pattern.compile("([^=].*?)=(.*)");


This seems weird. ([^=].*?) matches any string that doesn't start with =. I think it's conceptually correct to say ([^=]+).

So as a concrete example,

Pattern pattern = Pattern.compile("([^=].*?)"); Matcher matcher = pattern.matcher("ab==c==d"); matcher.matches(); System.out.println(matcher.group(1));

It prints out ab==c==d.

Yeah I guess within the context of the rest of the expression, a name containing an = would only be detected if it started with an =. But yours makes more sense and is easier to read.

…opagate-container-config

briandealwis

Suggestions only, take 'em or leave 'em as you see fit.

briandealwis · 2018-07-26T03:23:41Z

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

+              "Invalid environment variable definition: " + environmentVariable);
+        }
+        String name = matcher.group(1);
+        String value = matcher.group(2);


When patterns are far away from their use, it can help to use a named group (e.g., matcher.group("name") and matcher.group("value")).

+1 We should probably do this for some of the other regex patterns too.

briandealwis · 2018-07-26T03:29:56Z

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

+      if (buildConfiguration.getEnvironment() != null) {
+        for (Map.Entry<String, String> entry : buildConfiguration.getEnvironment().entrySet()) {
+          imageBuilder.setEnvironmentVariable(entry.getKey(), entry.getValue());
+        }


I wonder if this could be simplified by instead having just a imageBuilder.addEnvironment():

+ // Initialize to environment from base image + imageBuilder.addEnvironment( + NonBlockingSteps.get(pullBaseImageStep).getBaseImage().getEnvironment()); + // override with configured environment + if (buildConfiguration.getEnvironment() != null) { + imageBuilder.addEnvironment(buildConfiguration.getEnvironment()); + }

briandealwis · 2018-07-26T03:32:24Z

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

+    for (Map.Entry<String, String> environmentVariable : environment.entrySet()) {
+      if (environmentVariable.getKey().contains("=")) {
+        throw new IllegalArgumentException("Environment variable name cannot contain '='");
+      }


This is exactly what Preconditions.checkArgument() does?

briandealwis · 2018-07-26T03:42:08Z

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

+      }
+      builder.add(environmentVariable.getKey() + "=" + environmentVariable.getValue());
+    }
+    return builder.build();


If you like streams, you could simplify this a bit:

Preconditions.checkArgument( Iterables.any(environment.keySet(), s -> s.contains("=")), "Environment variable name cannot contain '='"); return environment .entrySet() .stream() .map(entry -> entry.getKey() + "=" + entry.getValue()) .collect(ImmutableList.toImmutableList());

coollog · 2018-07-26T03:43:19Z

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

+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    for (Map.Entry<String, String> environmentVariable : environment.entrySet()) {
+      if (environmentVariable.getKey().contains("=")) {
+        throw new IllegalArgumentException("Environment variable name cannot contain '='");


It might be good to include the illegal name in the message.

coollog · 2018-07-26T03:47:03Z

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

+   *
+   * <p>Example matches: NAME=VALUE, A12345=$$$$$
+   */
+  @VisibleForTesting static final Pattern environmentPattern = Pattern.compile("([^=]+)=(.*)");


nit: constants should be in all caps

coollog

Gonna leave LGTM to after addressing @briandealwis 's comments.

chanseokoh · 2018-07-26T14:48:00Z

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

-        }
+    public Builder<T> addEnvironment(@Nullable Map<String, String> environment) {
+      if (environment != null) {
+        this.environmentBuilder.putAll(ImmutableMap.copyOf(environment));


I'm thinking this is no different from and just could be environmentBuilder.putAll(environment), am I right? You are just putting references to objects (Strings in this case) into the builder, not like you are doing a deep copy of String instances. (And Strings are immutable, so there is no point of deep copying, actually.) Am I right?

Sounds right to me.

chanseokoh

I think it looks good generally.

chanseokoh · 2018-07-26T14:49:44Z

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

+      return null;
+    }
+    Preconditions.checkArgument(
+        environment.keySet().stream().noneMatch(s -> s.contains("=")),


key -> key.contains("=") seems more readable.

chanseokoh · 2018-07-26T14:50:54Z

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

+   * <p>Example matches: NAME=VALUE, A12345=$$$$$
+   */
+  @VisibleForTesting
+  static final Pattern environmentPattern = Pattern.compile("(?<name>[^=]+)=(?<value>.*)");


ENVIRONMENT_PATTERN too.

chanseokoh · 2018-07-26T14:55:38Z

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

+      imageBuilder.addEnvironment(
+          NonBlockingSteps.get(pullBaseImageStep).getBaseImage().getEnvironment());
+      if (buildConfiguration.getEnvironment() != null) {
+        imageBuilder.addEnvironment(buildConfiguration.getEnvironment());


Since the parameter of addEnvironment() is @Nullable, we could probably remove the null check.

coollog

LGTM - will let someone else give final approval

coollog · 2018-07-26T15:21:21Z

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

+      return null;
+    }
+    Preconditions.checkArgument(
+        environment.keySet().stream().noneMatch(key -> key.contains("=")),


Or, even more clear may be s/key/variableName/

I like key more since it's descriptive about which part of the map itself it's dealing with. And we can still tell that the key corresponds to the name with the error message right below.

Approval delegated

TadCordle added 4 commits July 25, 2018 12:01

Progress

305f993

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

54560a9

…opagate-container-config

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

f922ee1

…opagate-container-config

Do environment variable conversion in translator classes

4ee9257

TadCordle requested review from briandealwis, coollog and chanseokoh July 25, 2018 18:13

TadCordle added this to the v0.9.8 milestone Jul 25, 2018

chanseokoh reviewed Jul 25, 2018

View reviewed changes

TadCordle added 2 commits July 25, 2018 16:25

Use regex for environ parsing

09c02aa

Changelog

1d78caf

coollog reviewed Jul 25, 2018

View reviewed changes

TadCordle added 3 commits July 25, 2018 18:15

Smaller feedback

3ed7011

Rework overwrite logic

4366e19

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

3304003

…opagate-container-config

chanseokoh reviewed Jul 25, 2018

View reviewed changes

TadCordle added 3 commits July 25, 2018 18:59

Fix regex and add illegal argument exception in case of bad map

d859764

Fix javadoc

6b3bdeb

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

26d83c8

…opagate-container-config

briandealwis reviewed Jul 26, 2018

View reviewed changes

coollog approved these changes Jul 26, 2018

View reviewed changes

coollog previously requested changes Jul 26, 2018

View reviewed changes

TadCordle added 2 commits July 26, 2018 10:41

Feedback

f165a74

More

a9b7397

chanseokoh reviewed Jul 26, 2018

View reviewed changes

TadCordle added 2 commits July 26, 2018 10:56

Simplify

16bba97

Feedback

0e7c811

coollog reviewed Jul 26, 2018

View reviewed changes

chanseokoh approved these changes Jul 26, 2018

View reviewed changes

TadCordle merged commit 82ce0a0 into master Jul 26, 2018

TadCordle deleted the propagate-container-config branch July 26, 2018 15:39

Propagate base image's environment variable config to built image #716

Propagate base image's environment variable config to built image #716

Conversation

TadCordle commented Jul 25, 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

TadCordle Jul 25, 2018 • edited Loading

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

TadCordle Jul 25, 2018 • edited Loading

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

briandealwis left a 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

coollog left a comment

Choose a reason for hiding this comment

chanseokoh Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh left a 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

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadCordle Jul 25, 2018 •

edited

Loading

TadCordle Jul 25, 2018 •

edited

Loading

chanseokoh Jul 26, 2018 •

edited

Loading