-
Notifications
You must be signed in to change notification settings - Fork 68
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
Stream archived content #472
Changes from 18 commits
0c2c67e
2a94bc1
3df6329
f520a95
18ba79e
3e818dd
6ac6dc3
3a3e372
7ff83b9
fb0a872
341f741
bc33bb7
4b6a497
aa885ab
d90c584
f101104
0ea1bbf
7c1e050
636dd19
42dbdb1
513371a
b9333a5
ce87023
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package archiver | ||
|
||
import ( | ||
"archive/tar" | ||
"compress/gzip" | ||
"io" | ||
"log" | ||
"os" | ||
"path/filepath" | ||
|
||
"github.com/pkg/errors" | ||
) | ||
|
||
// ArchivePackage method builds and streams an archive with package content. | ||
func ArchivePackage(w io.Writer, packagePath string) error { | ||
gzipWriter := gzip.NewWriter(w) | ||
tarWriter := tar.NewWriter(gzipWriter) | ||
defer func() { | ||
err := tarWriter.Close() | ||
if err != nil { | ||
log.Printf("Error occurred while closing tar writer: %v", err) | ||
} | ||
|
||
err = gzipWriter.Close() | ||
if err != nil { | ||
log.Printf("Error occurred while closing gzip writer: %v", err) | ||
} | ||
}() | ||
|
||
err := filepath.Walk(packagePath, func(path string, info os.FileInfo, err error) error { | ||
if err != nil { | ||
return err | ||
} | ||
|
||
relativePath, err := filepath.Rel(packagePath, path) | ||
if err != nil { | ||
return errors.Wrapf(err, "finding relative path failed (packagePath: %s, path: %s)", packagePath, path) | ||
} | ||
|
||
if relativePath == "." { | ||
return nil | ||
} | ||
|
||
header, err := buildArchiveHeader(info, relativePath) | ||
if err != nil { | ||
return errors.Wrapf(err, "building archive header failed (path: %s)", relativePath) | ||
} | ||
|
||
err = tarWriter.WriteHeader(header) | ||
if err != nil { | ||
return errors.Wrapf(err, "writing header failed (path: %s)", relativePath) | ||
} | ||
|
||
if !info.IsDir() { | ||
err = writeFileContentToArchive(path, tarWriter) | ||
if err != nil { | ||
return errors.Wrapf(err, "archiving file content failed (path: %s)", path) | ||
} | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
return errors.Wrapf(err, "processing package path '%s' failed", packagePath) | ||
} | ||
|
||
err = tarWriter.Flush() | ||
if err != nil { | ||
return errors.Wrap(err, "flushing tar writer failed") | ||
} | ||
|
||
err = gzipWriter.Flush() | ||
if err != nil { | ||
return errors.Wrap(err, "flushing gzip writer failed") | ||
} | ||
return nil | ||
} | ||
|
||
func buildArchiveHeader(info os.FileInfo, relativePath string) (*tar.Header, error) { | ||
header, err := tar.FileInfoHeader(info, "") | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "reading file info header failed (info: %s)", info.Name()) | ||
} | ||
|
||
header.Name = relativePath | ||
if info.IsDir() { | ||
header.Name = header.Name + "/" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added an additional condition to check the |
||
} | ||
return header, nil | ||
} | ||
|
||
func writeFileContentToArchive(path string, writer io.Writer) error { | ||
f, err := os.Open(path) | ||
if err != nil { | ||
return errors.Wrapf(err, "opening file failed (path: %s)", path) | ||
} | ||
defer func() { | ||
err := f.Close() | ||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about using a named return to still return an error in case this fails instead of writing it to the log file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is in defer, so it's not on the direct execution path. Also, if we've already started streaming and something really bad happens, we won't inform the use what happened, we can just cut the response stream. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but on the other hand you can overwrite errors that caused the problem: https://play.golang.org/p/glEov8stXWq There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used multierror for this. Fixed. |
||
log.Printf("Error occurred while closing file (path: %s): %v", path, err) | ||
} | ||
}() | ||
|
||
_, err = io.Copy(writer, f) | ||
if err != nil { | ||
return errors.Wrapf(err, "copying file content failed (path: %s)", path) | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package main | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we move this code under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with moving it to a different place except utils, commons, shared, etc. ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong opinion around the name. I assume this means we should also move the other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The entire project can be adjusted to the standard layout: https://github.com/golang-standards/project-layout There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to "archiver" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM to cleanup to follow the standard layout but lets do it in small steps. |
||
|
||
import ( | ||
"log" | ||
"net/http" | ||
"os" | ||
"path/filepath" | ||
"time" | ||
|
||
"github.com/blang/semver" | ||
"github.com/gorilla/mux" | ||
"github.com/pkg/errors" | ||
|
||
"github.com/elastic/package-registry/archiver" | ||
) | ||
|
||
const artifactsRouterPath = "/epr/{packageName}/{packageName:[a-z_]+}-{packageVersion}.tar.gz" | ||
|
||
var errArtifactNotFound = errors.New("artifact not found") | ||
|
||
func artifactsHandler(packagesBasePath string, cacheTime time.Duration) func(w http.ResponseWriter, r *http.Request) { | ||
return func(w http.ResponseWriter, r *http.Request) { | ||
vars := mux.Vars(r) | ||
packageName := vars["packageName"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to validate that these entries exist? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Package name is restricted with regex:
Package version is checked with _, err := semver.Parse(packageVersion)
if err != nil {
badRequest(w, "invalid package version")
return
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was more referring to check if the key exists: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
packageVersion := vars["packageVersion"] | ||
|
||
_, err := semver.Parse(packageVersion) | ||
if err != nil { | ||
badRequest(w, "invalid package version") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to print out the package version that was invalid here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The version is in URL and I don't expect that the user visits this endpoint anyway. I would leave it as is. |
||
return | ||
} | ||
|
||
packagePath := filepath.Join(packagesBasePath, packageName, packageVersion) | ||
_, err = os.Stat(packagePath) | ||
if os.IsNotExist(err) { | ||
notFoundError(w, errArtifactNotFound) | ||
return | ||
} | ||
if err != nil { | ||
log.Printf("stat package path '%s' failed: %v", packagePath, err) | ||
|
||
http.Error(w, "internal server error", http.StatusInternalServerError) | ||
return | ||
} | ||
|
||
w.Header().Set("Content-Type", "application/gzip") | ||
cacheHeaders(w, cacheTime) | ||
|
||
err = archiver.ArchivePackage(w, packagePath) | ||
if err != nil { | ||
log.Printf("archiving package path '%s' failed: %v", packagePath, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the error here already part of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the handler has already started streaming response (an archive) and an internal error occurs, you can't do anything in this situation. Headers are already sent. You can simply cut-off the stream and wait for your upstream dependency to retry the activity (as the package is corrupted). |
||
return | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
0 docs/ | ||
622 docs/README.md | ||
0 elasticsearch/ | ||
0 elasticsearch/ingest-pipeline/ | ||
892 elasticsearch/ingest-pipeline/pipeline-entry.json | ||
2071 elasticsearch/ingest-pipeline/pipeline-http.json | ||
887 elasticsearch/ingest-pipeline/pipeline-json.json | ||
3584 elasticsearch/ingest-pipeline/pipeline-plaintext.json | ||
900 elasticsearch/ingest-pipeline/pipeline-tcp.json | ||
0 img/ | ||
482070 img/kibana-envoyproxy.jpg | ||
1916 index.json | ||
0 kibana/ | ||
0 kibana/dashboard/ | ||
2221 kibana/dashboard/0c610510-5cbd-11e9-8477-077ec9664dbd.json | ||
0 kibana/index-pattern/ | ||
91348 kibana/index-pattern/filebeat.json | ||
0 kibana/infrastructure-ui-source/ | ||
797 kibana/infrastructure-ui-source/default.json | ||
0 kibana/visualization/ | ||
1863 kibana/visualization/0a994af0-5c9d-11e9-8477-077ec9664dbd.json | ||
1982 kibana/visualization/36f872a0-5c03-11e9-85b4-19d0072eb4f2.json | ||
2572 kibana/visualization/38f96190-5c99-11e9-8477-077ec9664dbd.json | ||
1995 kibana/visualization/7e4084e0-5c99-11e9-8477-077ec9664dbd.json | ||
1849 kibana/visualization/80844540-5c97-11e9-8477-077ec9664dbd.json | ||
1920 kibana/visualization/ab48c3f0-5ca6-11e9-8477-077ec9664dbd.json | ||
319 manifest.yml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
{ | ||
"service.name": "package-registry", | ||
"version": "0.4.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
invalid package version |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
artifact not found |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
artifact not found |
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.
Should we have some logic to skip hidden files that might be created by the system like
.DS_Store
?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 wouldn't pollute the code with references to files that are ".gitignored" anyway. The docker image is created and pushed in the CI environment, I don't think we can be afraid of presence of such files there.
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 code is not only run inside docker but also locally.