Merge pull request #21963 from vespa-engine/mpolden/zip-improvements

Unify zip/tar archive reading [run-systemtest]

4 files changed, 108 insertions, 108 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index 7a754dd84cd..cb1c1a461e3 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -1053,7 +1053,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
 
     private File decompressApplication(InputStream in, String contentType, File tempDir) {
         try (CompressedApplicationInputStream application =
-                     CompressedApplicationInputStream.createFromCompressedStream(in, contentType)) {
+                     CompressedApplicationInputStream.createFromCompressedStream(in, contentType, configserverConfig.maxApplicationPackageSize())) {
             return decompressApplication(application, tempDir);
         } catch (IOException e) {
             throw new IllegalArgumentException("Unable to decompress data in body", e);
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStream.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStream.java
index 0672f13fd6a..443ab47e786 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStream.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStream.java
@@ -1,23 +1,21 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.application;
 
-import com.google.common.io.ByteStreams;
+import com.yahoo.compress.ArchiveStreamReader;
+import com.yahoo.compress.ArchiveStreamReader.Options;
+import com.yahoo.vespa.config.server.http.BadRequestException;
+import com.yahoo.vespa.config.server.http.InternalServerException;
+import com.yahoo.vespa.config.server.http.v2.ApplicationApiHandler;
 
 import java.io.File;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.logging.Level;
-import com.yahoo.vespa.config.server.http.BadRequestException;
-import com.yahoo.vespa.config.server.http.InternalServerException;
-import com.yahoo.vespa.config.server.http.v2.ApplicationApiHandler;
-import org.apache.commons.compress.archivers.ArchiveEntry;
-import org.apache.commons.compress.archivers.ArchiveInputStream;
-import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
-import org.apache.commons.compress.archivers.zip.ZipArchiveInputStream;
-
 import java.util.logging.Logger;
-import java.util.zip.GZIPInputStream;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
 
@@ -29,53 +27,43 @@ import static com.yahoo.yolean.Exceptions.uncheck;
 public class CompressedApplicationInputStream implements AutoCloseable {
 
     private static final Logger log = Logger.getLogger(CompressedApplicationInputStream.class.getPackage().getName());
-    private final ArchiveInputStream ais;
+
+    private final ArchiveStreamReader reader;
+
+    private CompressedApplicationInputStream(ArchiveStreamReader reader) {
+        this.reader = reader;
+    }
 
     /**
      * Create an instance of a compressed application from an input stream.
      *
      * @param is   the input stream containing the compressed files.
      * @param contentType the content type for determining what kind of compressed stream should be used.
+     * @param maxSizeInBytes the maximum allowed size of the decompressed content
      * @return An instance of an unpacked application.
      */
-    public static CompressedApplicationInputStream createFromCompressedStream(InputStream is, String contentType) {
+    public static CompressedApplicationInputStream createFromCompressedStream(InputStream is, String contentType, long maxSizeInBytes) {
         try {
-            ArchiveInputStream ais = getArchiveInputStream(is, contentType);
-            return createFromCompressedStream(ais);
-        } catch (IOException e) {
+            Options options = Options.standard().maxSize(maxSizeInBytes).allowDotSegment(true);
+            switch (contentType) {
+                case ApplicationApiHandler.APPLICATION_X_GZIP:
+                    return new CompressedApplicationInputStream(ArchiveStreamReader.ofTarGzip(is, options));
+                case ApplicationApiHandler.APPLICATION_ZIP:
+                    return new CompressedApplicationInputStream(ArchiveStreamReader.ofZip(is, options));
+                default:
+                    throw new BadRequestException("Unable to decompress");
+            }
+        } catch (UncheckedIOException e) {
             throw new InternalServerException("Unable to create compressed application stream", e);
         }
     }
 
-    static CompressedApplicationInputStream createFromCompressedStream(ArchiveInputStream ais) {
-        return new CompressedApplicationInputStream(ais);
-    }
-
-    private static ArchiveInputStream getArchiveInputStream(InputStream is, String contentTypeHeader) throws IOException {
-        ArchiveInputStream ais;
-        switch (contentTypeHeader) {
-            case ApplicationApiHandler.APPLICATION_X_GZIP:
-                ais = new TarArchiveInputStream(new GZIPInputStream(is));
-                break;
-            case ApplicationApiHandler.APPLICATION_ZIP:
-                ais = new ZipArchiveInputStream(is);
-                break;
-            default:
-                throw new BadRequestException("Unable to decompress");
-        }
-        return ais;
-    }
-
-    private CompressedApplicationInputStream(ArchiveInputStream ais) {
-        this.ais = ais;
-    }
-
     /**
      * Close this stream.
      * @throws IOException if the stream could not be closed
      */
     public void close() throws IOException {
-        ais.close();
+        reader.close();
     }
 
     File decompress() throws IOException {
@@ -83,45 +71,44 @@ public class CompressedApplicationInputStream implements AutoCloseable {
     }
 
     public File decompress(File dir) throws IOException {
-        decompressInto(dir);
+        decompressInto(dir.toPath());
         dir = findActualApplicationDir(dir);
         return dir;
     }
 
-    private void decompressInto(File application) throws IOException {
-        log.log(Level.FINE, () -> "Application is in " + application.getAbsolutePath());
+    private void decompressInto(Path dir) throws IOException {
+        if (!Files.isDirectory(dir)) throw new IllegalArgumentException("Not a directory: " + dir.toAbsolutePath());
+        log.log(Level.FINE, () -> "Application is in " + dir.toAbsolutePath());
         int entries = 0;
-        ArchiveEntry entry;
-        while ((entry = ais.getNextEntry()) != null) {
-            log.log(Level.FINE, "Unpacking %s", entry.getName());
-            File outFile = new File(application, entry.getName());
-            // FIXME/TODO: write more tests that break this logic. I have a feeling it is not very robust.
-            if (entry.isDirectory()) {
-                if (!(outFile.exists() && outFile.isDirectory())) {
-                    log.log(Level.FINE, () -> "Creating dir: " + outFile.getAbsolutePath());
-                    boolean res = outFile.mkdirs();
-                    if (!res) {
-                        log.log(Level.WARNING, "Could not create dir " + entry.getName());
-                    }
-                }
-            } else {
-                log.log(Level.FINE, () -> "Creating output file: " + outFile.getAbsolutePath());
-
-                // Create parent dir if necessary
-                String parent = outFile.getParent();
-                new File(parent).mkdirs();
-
-                FileOutputStream fos = new FileOutputStream(outFile);
-                ByteStreams.copy(ais, fos);
-                fos.close();
+        Path tmpFile = null;
+        OutputStream tmpStream = null;
+        try {
+            tmpFile = createTempFile(dir);
+            tmpStream = Files.newOutputStream(tmpFile);
+            ArchiveStreamReader.ArchiveFile file;
+            while ((file = reader.readNextTo(tmpStream)) != null) {
+                tmpStream.close();
+                log.log(Level.FINE, "Creating output file: " + file.path());
+                Path dstFile = dir.resolve(file.path().toString()).normalize();
+                Files.createDirectories(dstFile.getParent());
+                Files.move(tmpFile, dstFile);
+                tmpFile = createTempFile(dir);
+                tmpStream = Files.newOutputStream(tmpFile);
+                entries++;
             }
-            entries++;
+        } finally {
+            if (tmpStream != null) tmpStream.close();
+            if (tmpFile != null) Files.deleteIfExists(tmpFile);
         }
         if (entries == 0) {
-            log.log(Level.WARNING, "Not able to read any entries from " + application.getName());
+            log.log(Level.WARNING, "Not able to decompress any entries to " + dir);
         }
     }
 
+    private static Path createTempFile(Path applicationDir) throws IOException {
+        return Files.createTempFile(applicationDir, "application", null);
+    }
+
     private File findActualApplicationDir(File application) {
         // If application is in e.g. application/, use that as root for UnpackedApplication
         // TODO: Vespa 8: Remove application/ directory support
@@ -131,4 +118,5 @@ public class CompressedApplicationInputStream implements AutoCloseable {
         }
         return application;
     }
+
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationApiHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationApiHandler.java
index 1a8b36ee19f..c8953d5996c 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationApiHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationApiHandler.java
@@ -49,9 +49,11 @@ public class ApplicationApiHandler extends SessionHandler {
     public final static String MULTIPART_PARAMS = "prepareParams";
     public final static String MULTIPART_APPLICATION_PACKAGE = "applicationPackage";
     public final static String contentTypeHeader = "Content-Type";
+
     private final TenantRepository tenantRepository;
     private final Duration zookeeperBarrierTimeout;
     private final Zone zone;
+    private final long maxApplicationPackageSize;
 
     @Inject
     public ApplicationApiHandler(Context ctx,
@@ -61,6 +63,7 @@ public class ApplicationApiHandler extends SessionHandler {
         super(ctx, applicationRepository);
         this.tenantRepository = applicationRepository.tenantRepository();
         this.zookeeperBarrierTimeout = Duration.ofSeconds(configserverConfig.zookeeper().barrierTimeout());
+        this.maxApplicationPackageSize = configserverConfig.maxApplicationPackageSize();
         this.zone = zone;
     }
 
@@ -85,14 +88,14 @@ public class ApplicationApiHandler extends SessionHandler {
                 log.log(Level.FINE, "Deploy parameters: [{0}]", new String(params, StandardCharsets.UTF_8));
                 prepareParams = PrepareParams.fromJson(params, tenantName, zookeeperBarrierTimeout);
                 Part appPackagePart = parts.get(MULTIPART_APPLICATION_PACKAGE);
-                compressedStream = createFromCompressedStream(appPackagePart.getInputStream(), appPackagePart.getContentType());
+                compressedStream = createFromCompressedStream(appPackagePart.getInputStream(), appPackagePart.getContentType(), maxApplicationPackageSize);
             } catch (IOException e) {
                 log.log(Level.WARNING, "Unable to parse multipart in deploy", e);
                 throw new BadRequestException("Request contains invalid data");
             }
         } else {
             prepareParams = PrepareParams.fromHttpRequest(request, tenantName, zookeeperBarrierTimeout);
-            compressedStream = createFromCompressedStream(request.getData(), request.getHeader(contentTypeHeader));
+            compressedStream = createFromCompressedStream(request.getData(), request.getHeader(contentTypeHeader), maxApplicationPackageSize);
         }
 
         PrepareResult result = applicationRepository.deploy(compressedStream, prepareParams);
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStreamTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStreamTest.java
index 23444ac53d6..1e8005c8af6 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStreamTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/CompressedApplicationInputStreamTest.java
@@ -2,10 +2,10 @@
 package com.yahoo.vespa.config.server.application;
 
 import com.google.common.io.ByteStreams;
+import com.yahoo.vespa.config.server.http.InternalServerException;
+import com.yahoo.yolean.Exceptions;
 import org.apache.commons.compress.archivers.ArchiveOutputStream;
-import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
 import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream;
-import org.apache.commons.compress.archivers.zip.ZipArchiveInputStream;
 import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream;
 import org.junit.Test;
 
@@ -15,11 +15,10 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
-import java.util.zip.GZIPInputStream;
 import java.util.zip.GZIPOutputStream;
 
-import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -64,9 +63,10 @@ public class CompressedApplicationInputStreamTest {
     @Test
     public void require_that_valid_tar_application_can_be_unpacked() throws IOException {
         File outFile = createTarFile();
-        CompressedApplicationInputStream unpacked = CompressedApplicationInputStream.createFromCompressedStream(new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(outFile))));
-        File outApp = unpacked.decompress();
-        assertTestApp(outApp);
+        try (CompressedApplicationInputStream unpacked = streamFromTarGz(outFile)) {
+            File outApp = unpacked.decompress();
+            assertTestApp(outApp);
+        }
     }
 
     @Test
@@ -91,48 +91,39 @@ public class CompressedApplicationInputStreamTest {
 
         archiveOutputStream.close();
 
-        CompressedApplicationInputStream unpacked = CompressedApplicationInputStream.createFromCompressedStream(new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(outFile))));
-        File outApp = unpacked.decompress();
-        assertEquals("application", outApp.getName()); // gets the name of the subdir
-        assertTestApp(outApp);
+        try (CompressedApplicationInputStream unpacked = streamFromTarGz(outFile)) {
+            File outApp = unpacked.decompress();
+            assertEquals("application", outApp.getName()); // gets the name of the subdir
+            assertTestApp(outApp);
+        }
     }
 
     @Test
     public void require_that_valid_zip_application_can_be_unpacked() throws IOException {
         File outFile = createZipFile();
-        CompressedApplicationInputStream unpacked = CompressedApplicationInputStream.createFromCompressedStream(
-                new ZipArchiveInputStream(new FileInputStream(outFile)));
-        File outApp = unpacked.decompress();
-        assertTestApp(outApp);
+        try (CompressedApplicationInputStream unpacked = streamFromZip(outFile)) {
+            File outApp = unpacked.decompress();
+            assertTestApp(outApp);
+        }
     }
 
     @Test
     public void require_that_gnu_tared_file_can_be_unpacked() throws IOException, InterruptedException {
-        File tmpTar = File.createTempFile("myapp", ".tar");
-        Process p = new ProcessBuilder("tar", "-C", "src/test/resources/deploy/validapp", "--exclude=.svn", "-cvf", tmpTar.getAbsolutePath(), ".").start();
-        p.waitFor();
-        p = new ProcessBuilder("gzip", tmpTar.getAbsolutePath()).start();
-        p.waitFor();
-        File gzFile = new File(tmpTar.getAbsolutePath() + ".gz");
+        File gzFile = createTarGz("src/test/resources/deploy/validapp");
         assertTrue(gzFile.exists());
-        CompressedApplicationInputStream unpacked = CompressedApplicationInputStream.createFromCompressedStream(
-                new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(gzFile))));
+        CompressedApplicationInputStream unpacked = CompressedApplicationInputStream.createFromCompressedStream(new FileInputStream(gzFile), "application/x-gzip", Long.MAX_VALUE);
         File outApp = unpacked.decompress();
         assertTestApp(outApp);
     }
 
     @Test
     public void require_that_nested_app_can_be_unpacked() throws IOException, InterruptedException {
-        File tmpTar = File.createTempFile("myapp", ".tar");
-        Process p = new ProcessBuilder("tar", "-C", "src/test/resources/deploy/advancedapp", "--exclude=.svn", "-cvf", tmpTar.getAbsolutePath(), ".").start();
-        p.waitFor();
-        p = new ProcessBuilder("gzip", tmpTar.getAbsolutePath()).start();
-        p.waitFor();
-        File gzFile = new File(tmpTar.getAbsolutePath() + ".gz");
+        File gzFile = createTarGz("src/test/resources/deploy/advancedapp");
         assertTrue(gzFile.exists());
-        CompressedApplicationInputStream unpacked = CompressedApplicationInputStream.createFromCompressedStream(
-                new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(gzFile))));
-        File outApp = unpacked.decompress();
+        File outApp;
+        try (CompressedApplicationInputStream unpacked = streamFromTarGz(gzFile)) {
+            outApp = unpacked.decompress();
+        }
         List<File> files = Arrays.asList(outApp.listFiles());
         assertEquals(5, files.size());
         assertTrue(files.contains(new File(outApp, "services.xml")));
@@ -164,11 +155,29 @@ public class CompressedApplicationInputStreamTest {
         assertEquals(new File(bar, "lol").getAbsolutePath(), bar.listFiles()[0].getAbsolutePath());
     }
 
-
-    @Test(expected = IOException.class)
-    public void require_that_invalid_application_returns_error_when_unpacked() throws IOException {
+    @Test(expected = InternalServerException.class)
+    public void require_that_invalid_application_returns_error_when_unpacked() throws Exception {
         File app = new File("src/test/resources/deploy/validapp/services.xml");
-        CompressedApplicationInputStream.createFromCompressedStream(
-                new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(app))));
+        streamFromTarGz(app).close();
+    }
+
+    private static File createTarGz(String appDir) throws IOException, InterruptedException {
+        File tmpTar = File.createTempFile("myapp", ".tar");
+        Process p = new ProcessBuilder("tar", "-C", appDir, "-cvf", tmpTar.getAbsolutePath(), ".").start();
+        p.waitFor();
+        p = new ProcessBuilder("gzip", tmpTar.getAbsolutePath()).start();
+        p.waitFor();
+        File gzFile = new File(tmpTar.getAbsolutePath() + ".gz");
+        assertTrue(gzFile.exists());
+        return gzFile;
+    }
+
+    private static CompressedApplicationInputStream streamFromZip(File zipFile) {
+        return Exceptions.uncheck(() -> CompressedApplicationInputStream.createFromCompressedStream(new FileInputStream(zipFile), "application/zip", Long.MAX_VALUE));
+    }
+
+    private static CompressedApplicationInputStream streamFromTarGz(File tarFile) {
+        return Exceptions.uncheck(() -> CompressedApplicationInputStream.createFromCompressedStream(new FileInputStream(tarFile), "application/x-gzip", Long.MAX_VALUE));
     }
+
 }