Adds entries for all directories in layer tarball. #772

coollog · 2018-08-01T21:13:25Z

Fixes #523
Works towards #727

This makes sure that permissions for directories in the container filesystem will all be the default 755.

coollog · 2018-08-01T21:14:12Z

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

@@ -44,6 +45,12 @@
      throws IOException {
    List<TarArchiveEntry> tarArchiveEntries = new ArrayList<>();

+    // Adds the extraction path itself.
+    TarArchiveEntry extractionPathDirectoryEntry =
+        new TarArchiveEntry(Paths.get(".").toFile(), layerEntry.getExtractionPath());


Not sure how to do this better than just using the current directory .. Otherwise, we would have to either have a directory to use, or duplicate a lot of the logic in TarArchiveEntry's constructor.

Maybe add a more explicit comment about this? (e.g. // Adds the extraction path itself; to do this, we can just add the current directory named as the extraction path)

TadCordle · 2018-08-02T14:10:30Z

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

@@ -45,7 +45,8 @@
      throws IOException {
    List<TarArchiveEntry> tarArchiveEntries = new ArrayList<>();

-    // Adds the extraction path itself.
+    // Adds the extraction path itself; to do this, are adding the current directory to act as the


to do this, we are adding

…erTools/jib into add-extraction-path-to-tar

chanseokoh · 2018-08-02T15:48:04Z

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

+    // Adds the extraction path itself; to do this, we are adding the current directory to act as
+    // the file input (since all directories are treated the same in TarArchiveEntry).
+    TarArchiveEntry extractionPathDirectoryEntry =
+        new TarArchiveEntry(Paths.get(".").toFile(), layerEntry.getExtractionPath());


Given #727 (comment), I guess the extraction path is like /app/classes, this directory gets the readable permission, but /app does not.

Hmm, yea that might be it. Will try to think of a good way to add parent directories of the extraction too.

Will need to add parent directories too.

loosebazooka

Man it'd be nice if redhat didn't use ancient docker.

loosebazooka · 2018-08-02T21:15:16Z

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

+        directoriesToAdd.add(parentDirectory.toString());
+      }
+    }
+    // We are using the current directory to act as the file input (since all directories are


I'm not sure I understand this comment

The TarArchiveEntry constructor can use a directory and sets up defaults like normalizing the path, normalizing the end separator (adds / if not present), and sets default permissions. It just needs to check file.isDirectory, so any File that is a directory will do.

I think we could think about trying to find a better TarArchiveEntry or implement our own...

chanseokoh · 2018-08-02T21:34:41Z

Unfortunately, this still doesn't fix #727 (comment) :(

coollog · 2018-08-02T21:42:42Z

@chanseokoh Yea, perhaps we need to just set default permissions for all files.

looks like fix failed

coollog · 2018-08-07T18:52:52Z

This can be merged and I'll file a followup PR for adding all directories to have default directory mode.

chanseokoh · 2018-08-07T18:58:39Z

I thought this was to set the 755 permissions to the directories (which turned out to work only for /app, /app/classes, app/resources, app/lib, ... and not for directories nested in them). So, if you are going to have a PR that will make all directories to have the default directory mode, I think this PR is not necessary? Or, is this PR still necessary and fixing something?

coollog · 2018-08-07T19:08:52Z

@chanseokoh That is true, I'll just modify this PR then.

loosebazooka · 2018-08-13T21:02:02Z

is this PR ready?

coollog · 2018-08-13T21:26:03Z

This PR is in terms of what it does, but it still doesn't seem to fix #727 (comment)

coollog · 2018-08-27T14:34:00Z

This fixes #523 (#523 (comment)) and can be merged.

briandealwis · 2018-08-27T14:50:42Z

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

+      for (TarArchiveEntry tarArchiveEntry : tarArchiveEntries) {
+        // Adds all directories along extraction paths to explicitly set permissions for those
+        // directories.
+        for (Path parentDirectory : getParentDirectories(Paths.get(tarArchiveEntry.getName()))) {


I wonder if this wouldn't be better in TarStreamBuilder. This approach also generates a lot of garbage since for only the first file per parent actually needs to walk the parents.

An alternative would be to first sort the file list, then walk the list and add directory elements all at once to the TarStreamBuilder, and then add the files.

Hmm, that's a good point. I'll change this to add parents by iterating backwards along the path and adding the parents in a post-order manner. This way, the entries don't need to be sorted before the parent directories are added and each parent list is only walked fully once.

briandealwis · 2018-08-27T14:58:04Z

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

+  // are treated the same in TarArchiveEntry).
+  private static final File DIRECTORY_FILE = Paths.get(".").toFile();
+
+  /** Decorates an object with a separate object for {@link #hashCode}. */


I'd appreciate detail as to why this class is required. Is Path#hashCode() possibly inconsistent? (UnixPath#hashCode() looks fine)

This was to have TarArchiveEntry be hashed by the entry name. I'll actually remove this since it is more confusing than it needs to be.

briandealwis · 2018-08-27T18:21:26Z

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

+  // are treated the same in TarArchiveEntry).
+  private static final File DIRECTORY_FILE = Paths.get(".").toFile();
+
+  /** Holds a list of {@link TarArchiveEntry}s with unique extraction paths. */


Add a note that: Additions are assumed to be files. Directory entries are added as required.

Added a note, though the additions do not necessary need to be regular files.

briandealwis · 2018-08-27T18:24:15Z

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

+    List<LayerEntry> layerEntries = this.layerEntries.build();
+    for (LayerEntry layerEntry : layerEntries) {
+      // Converts layerEntry to list of TarArchiveEntrys and adds to uniqueTarArchiveEntries.
+      buildAsTarArchiveEntries(layerEntry).forEach(uniqueTarArchiveEntries::add);


Would it make sense to break into two parts? This is quite a complex statement.

briandealwis · 2018-08-28T02:29:54Z

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

+  private static final File DIRECTORY_FILE = Paths.get(".").toFile();
+
+  /**
+   * Holds a list of {@link TarArchiveEntry}s with unique extraction paths. The list also consists


s/consists of/includes/

briandealwis · 2018-08-28T02:31:06Z

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

@@ -34,6 +38,48 @@
 */
 public class ReproducibleLayerBuilder {

+  // Uses the current directory to act as the file input to TarArchiveEntry (since all directories
+  // are treated the same in TarArchiveEntry).
+  private static final File DIRECTORY_FILE = Paths.get(".").toFile();


Move into UniqueTarArchiveEntries? Make as a java doc and include a link to TarArchiveEntry constructor as a @seealso?

briandealwis · 2018-08-28T13:46:30Z

👍

This reverts commit 54e1e2b.

) This reverts commit 54e1e2b.

Adds extraction path directory to layer tarball.

209f33b

coollog added this to the v0.9.9 milestone Aug 1, 2018

coollog requested a review from a team August 1, 2018 21:13

coollog commented Aug 1, 2018

View reviewed changes

This was referenced Aug 1, 2018

/app not readable when running on Openshift #523

Closed

Cannot run image running with container generated by jib as non-root in k8s #727

Closed

TadCordle approved these changes Aug 2, 2018

View reviewed changes

coollog added 2 commits August 2, 2018 10:06

Adds comment.

8b48d1b

Merge branch 'master' into add-extraction-path-to-tar

dae67cf

TadCordle reviewed Aug 2, 2018

View reviewed changes

coollog added 3 commits August 2, 2018 10:15

we

f61f1b2

Merge branch 'add-extraction-path-to-tar' of github.com:GoogleContain…

ed9ba9e

…erTools/jib into add-extraction-path-to-tar

newline

30c7b34

TadCordle previously approved these changes Aug 2, 2018

View reviewed changes

chanseokoh reviewed Aug 2, 2018

View reviewed changes

coollog added 3 commits August 2, 2018 13:52

Adds parent directories as well.

edc1ee8

Merge branch 'master' into add-extraction-path-to-tar

3bdcbe3

Merge branch 'master' into add-extraction-path-to-tar

8ee56d4

loosebazooka previously approved these changes Aug 2, 2018

View reviewed changes

Merge branch 'master' into add-extraction-path-to-tar

eb5d936

googlebot added the cla: yes label Aug 7, 2018

Adds all parent directories.

efdb39d

coollog changed the title ~~Adds extraction path directory to layer tarball.~~ Adds entries for all directories in layer tarball. Aug 8, 2018

coollog added 2 commits August 8, 2018 14:09

Merge branch 'master' into add-extraction-path-to-tar

880650b

Remove extra code.

d9f4547

TadCordle modified the milestones: v0.9.9, v0.9.10 Aug 20, 2018

Merge branch 'master' into add-extraction-path-to-tar

1a84864

TadCordle approved these changes Aug 27, 2018

View reviewed changes

coollog requested a review from briandealwis August 27, 2018 14:55

briandealwis reviewed Aug 27, 2018

View reviewed changes

Reduces redundant parent directory walk.

2f94e64

briandealwis approved these changes Aug 27, 2018

View reviewed changes

coollog added 2 commits August 27, 2018 15:05

Some better comments.

8a1109b

better comments

ae5f7ce

coollog added the kokoro:run label Aug 27, 2018

kokoro-team removed the kokoro:run label Aug 27, 2018

coollog added 2 commits August 27, 2018 22:05

Merge branch 'master' into add-extraction-path-to-tar

c151964

Updates CHANGELOG.

baf4b30

briandealwis approved these changes Aug 28, 2018

View reviewed changes

javadoc

545214f

coollog merged commit 54e1e2b into master Aug 28, 2018

coollog deleted the add-extraction-path-to-tar branch August 28, 2018 14:00

coollog added a commit that referenced this pull request Aug 28, 2018

Revert "Adds entries for all directories in layer tarball. (#772)"

f31a6aa

This reverts commit 54e1e2b.

coollog added a commit that referenced this pull request Aug 28, 2018

Revert "Adds entries for all directories in layer tarball. (#772)" (#888

1e59755

) This reverts commit 54e1e2b.

coollog restored the add-extraction-path-to-tar branch August 28, 2018 14:47

coollog deleted the add-extraction-path-to-tar branch August 28, 2018 14:48

coollog mentioned this pull request Aug 28, 2018

Adds entries for all directories in layer tarball. #891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds entries for all directories in layer tarball. #772

Adds entries for all directories in layer tarball. #772

coollog commented Aug 1, 2018 •

edited

Loading

coollog Aug 1, 2018

TadCordle Aug 2, 2018 •

edited

Loading

TadCordle Aug 2, 2018

chanseokoh Aug 2, 2018

coollog Aug 2, 2018

loosebazooka left a comment

loosebazooka Aug 2, 2018

coollog Aug 2, 2018

coollog Aug 2, 2018

chanseokoh commented Aug 2, 2018

coollog commented Aug 2, 2018

coollog commented Aug 7, 2018

chanseokoh commented Aug 7, 2018 •

edited

Loading

coollog commented Aug 7, 2018

loosebazooka commented Aug 13, 2018

coollog commented Aug 13, 2018

coollog commented Aug 27, 2018

briandealwis Aug 27, 2018

coollog Aug 27, 2018

briandealwis Aug 27, 2018

coollog Aug 27, 2018

briandealwis Aug 27, 2018

coollog Aug 27, 2018

briandealwis Aug 27, 2018

briandealwis Aug 28, 2018

briandealwis Aug 28, 2018

briandealwis commented Aug 28, 2018

Adds entries for all directories in layer tarball. #772

Adds entries for all directories in layer tarball. #772

Conversation

coollog commented Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

TadCordle Aug 2, 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

loosebazooka 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

chanseokoh commented Aug 2, 2018

coollog commented Aug 2, 2018

coollog commented Aug 7, 2018

chanseokoh commented Aug 7, 2018 • edited Loading

coollog commented Aug 7, 2018

loosebazooka commented Aug 13, 2018

coollog commented Aug 13, 2018

coollog commented Aug 27, 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

briandealwis commented Aug 28, 2018

coollog commented Aug 1, 2018 •

edited

Loading

TadCordle Aug 2, 2018 •

edited

Loading

chanseokoh commented Aug 7, 2018 •

edited

Loading