Stream archived content #472

mtojek · 2020-05-18T13:41:03Z

This PR replaces serving archived artifacts from static resources, but supports creating them on the fly (on demand).

I added some unit tests for common cases.

elasticmachine · 2020-05-18T13:45:45Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #472 updated]
Start Time: 2020-05-25T09:37:29.250+0000
Duration: 5 min 27 sec

ruflin · 2020-05-18T14:46:35Z

artifacts.go

+// 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


Could we move this code under util so we can reuse it? Lets assume we have one day a tool elastic-package that allows us to validate a package and also build the .tar.gz file to upload manually to Kibana. Would be nice to share this code there. It will probably required to separate the handler code from the building the tar.gz code but I would assume this makes it easier to test if not already the case.

I'm ok with moving it to a different place except utils, commons, shared, etc. ;)

No strong opinion around the name. I assume this means we should also move the other utils code?

The entire project can be adjusted to the standard layout: https://github.com/golang-standards/project-layout

Moved to "archiver"

SGTM to cleanup to follow the standard layout but lets do it in small steps.

ruflin · 2020-05-18T14:49:26Z

artifacts.go

+	"github.com/pkg/errors"
+)
+
+const artifactsRouterPath = "/epr/{packageName}/{packageName:[a-z]+}-{packageVersion}.tar.gz"


I expected that we have some packages with an _ but couldn't find one: https://github.com/elastic/integrations/tree/master/dev/packages/beats

Right, php_fpm

Sounds like we need to support it then.

We should make sure that our validation code for package names (if we have one) validates for the same pattern. Perhaps we can even reuse it.

Sounds like we need to support it then.

Done

We should make sure that our validation code for package names (if we have one) validates for the same pattern. Perhaps we can even reuse it.

I don't see any. Do we have it?

Looks like we don't :-( https://github.com/elastic/package-registry/blob/master/util/package.go#L264

ruflin · 2020-05-18T14:51:18Z

testdata/artifacts/package/example/0.0.2/docs/README.md

@@ -0,0 +1,22 @@
+[role="xpack"]


Couldn't you reuse here one of the existing test package? https://github.com/elastic/package-registry/tree/master/testdata/package ?

I think this is possible now as I changed the way I compare package content (compare listings instead of created archive which was different depending on the platform).

Lets use an existing one if possible. If we can't lets at least use the same path to have all test packages in one place and then I would remove quite a few of the assets to just have 1-2 files per type.

ruflin · 2020-05-18T16:57:31Z

docs/api/example-0.0.2.tar.gz

@@ -0,0 +1,27 @@
+0 docs/


I like the idea of this file but wondering about the .tar.gz part. Any good alternative? Something like .tar.gz.sum (also not too good).

Renamed to example-0.0.2.tar.gz-preview.txt

ruflin · 2020-05-18T16:59:00Z

Don't forget the changelog ;-)

mtojek · 2020-05-19T09:28:40Z

Don't forget the changelog ;-)

Yes, already added.

ruflin

I tried to run this with my local Kibana setup and got the following error:

server    log   [10:20:46.524] [warning][plugins][taskManager][taskManager] Cancelling task vis_telemetry "oss_telemetry-vis_telemetry" as it expired at 2020-05-20T08:19:44.547Z after running for 06m 01s (with timeout set at 5m).
Unhandled Promise rejection detected:

Error: Cannot find asset system-0.1.0/dataset/syslog/fields/base-fields.yml
    at Object.getAsset (/Users/ruflin/Dev/elastic/kibana/x-pack/plugins/ingest_manager/server/services/epm/registry/index.ts:160:35)
    at map (/Users/ruflin/Dev/elastic/kibana/x-pack/plugins/ingest_manager/server/services/epm/packages/assets.ts:67:29)
    at Array.map (<anonymous>)
    at getAssetsData (/Users/ruflin/Dev/elastic/kibana/x-pack/plugins/ingest_manager/server/services/epm/packages/assets.ts:65:51)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Terminating process...
 server crashed  with status code 1

But checking the system package .tar.gz file the above file exists as expected. Not sure if the error is related to this PR or not. Could you try it on your end?

ruflin · 2020-05-20T08:15:50Z

archiver/archive.go

+		}
+	}()
+
+	err := filepath.Walk(packagePath, func(path string, info os.FileInfo, err error) error {


Should we have some logic to skip hidden files that might be created by the system like .DS_Store?

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.

This code is not only run inside docker but also locally.

ruflin · 2020-05-20T08:19:43Z

archiver/archive.go

+	}
+	defer func() {
+		err := f.Close()
+		if err != nil {


What about using a named return to still return an error in case this fails instead of writing it to the log file?

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.

Not sure I get This code is in defer, so it's not on the direct execution path. You can still assign the error with a named return. So the function that calls "writeFileContentToArchive" can decide if it wants to drop the error or not.

Yes, but on the other hand you can overwrite errors that caused the problem: https://play.golang.org/p/glEov8stXWq

I used multierror for this. Fixed.

ruflin · 2020-05-20T08:20:38Z

artifacts.go

+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"]


Do we need to validate that these entries exist?

Package name is restricted with regex:

const artifactsRouterPath = "/epr/{packageName}/{packageName:[a-z_]+}-{packageVersion}.tar.gz"

Package version is checked with semver.Parse:

_, err := semver.Parse(packageVersion) if err != nil { badRequest(w, "invalid package version") return }

I was more referring to check if the key exists: vars["packageName"] but as we have it in the router path I guess this should never happen as otherwise the routing would be wrong.

ruflin · 2020-05-20T08:20:59Z

artifacts.go

+
+		_, err := semver.Parse(packageVersion)
+		if err != nil {
+			badRequest(w, "invalid package version")


Would be nice to print out the package version that was invalid here.

The version is in URL and I don't expect that the user visits this endpoint anyway. I would leave it as is.

mtojek · 2020-05-20T08:49:47Z

I tried to run this with my local Kibana setup and got the following error:

I reproduced it, thanks!

kibana_1            | {"type":"log","@timestamp":"2020-05-20T08:46:33+00:00","tags":["error","plugins","ingestManager"],"pid":6,"message":"Cannot find asset system-0.1.0/dataset/auth/fields/base-fields.yml"}
kibana_1            | {"type":"log","@timestamp":"2020-05-20T08:46:33+00:00","tags":["error","plugins","ingestManager"],"pid":6,"message":"Error: Cannot find asset system-0.1.0/dataset/auth/fields/base-fields.yml\n    at Object.getAsset (/usr/share/kibana/x-pack/plugins/ingest_manager/server/services/epm/registry/index.js:171:35)\n    at map (/usr/share/kibana/x-pack/plugins/ingest_manager/server/services/epm/packages/assets.js:73:29)\n    at Array.map (<anonymous>)\n    at getAssetsData (/usr/share/kibana/x-pack/plugins/ingest_manager/server/services/epm/packages/assets.js:71:26)\n    at process._tickCallback (internal/process/next_tick.js:68:7)"}

Investigating now...

mtojek · 2020-05-20T09:10:32Z

Fixed. It was the issue with missing root in the package.

ruflin

LGTM

ruflin · 2020-05-25T09:16:55Z

archiver/archive.go

+		var me multierror.Errors
+
+		if err != nil {
+			me = append(me, err)


Nit: I stumbled over me on what it is. Perhaps use multiErr instead?

ruflin · 2020-05-25T09:17:28Z

archiver/archive.go

+			me = append(me, errors.Wrapf(err, "closing gzip writer failed"))
+		}
+
+		if me != nil {


Nice to see the full list of errors bubbling up. Will make debugging easier.

ruflin · 2020-05-25T09:18:21Z

archiver/archive.go

+
+	rootDir := fmt.Sprintf("%s-%s", properties.Name, properties.Version)
+
+	err = filepath.Walk(properties.Path, func(path string, info os.FileInfo, err error) error {


Out of curiosity: Will this follow symlinks?

I will quote the docs here:

// Walk does not follow symbolic links.

ruflin · 2020-05-25T09:19:20Z

archiver/archive.go

+
+	header.Name = relativePath
+	if info.IsDir() {
+		header.Name = header.Name + "/"


Is this / dependent on the platfrom or always /?

I've added an additional condition to check the /.

ruflin · 2020-05-25T09:21:37Z

artifacts.go

+			Path:    packagePath,
+		})
+		if err != nil {
+			log.Printf("archiving package path '%s' failed: %v", packagePath, err)


Is the error here already part of w so we don't need to have a badRequest here?

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).

dev/generator/main.go

mtojek added 11 commits May 15, 2020 14:06

Serve favicon as embedded resource

0c2c67e

Test favicon handler

2a94bc1

Generate index.json file

3df6329

Add unit test

f520a95

WIP

18ba79e

Merge branch 'master' into generate-index-json

3e818dd

Add CHANGELOG entry

6ac6dc3

Merge branch 'generate-index-json' into stream-archived-content-2

3a3e372

Merge branch 'master' into stream-archived-content-2

7ff83b9

Implement gzipping endpoint

fb0a872

Cover with tests

341f741

mtojek requested a review from ruflin May 18, 2020 13:41

mtojek self-assigned this May 18, 2020

mtojek marked this pull request as draft May 18, 2020 13:46

mtojek added 3 commits May 18, 2020 16:05

Copy example to artifacts

bc33bb7

Try: linux build

4b6a497

Fix: idempotent builds

aa885ab

ruflin reviewed May 18, 2020

View reviewed changes

mtojek marked this pull request as ready for review May 18, 2020 14:55

ruflin reviewed May 18, 2020

View reviewed changes

mtojek added 4 commits May 19, 2020 10:51

Address PR comments

d90c584

Address PR comments

f101104

Fix: typo

0ea1bbf

Add CHANGELOG

7c1e050

mtojek requested a review from ruflin May 19, 2020 09:26

ruflin mentioned this pull request May 19, 2020

Discuss: Move to zip instead of tar.gz for the format of the package. #474

Closed

ruflin reviewed May 20, 2020

View reviewed changes

Fix: add root inside the package

636dd19

Add comment

42dbdb1

mtojek requested a review from ruflin May 20, 2020 09:13

mtojek added 2 commits May 20, 2020 11:35

Use multierror

513371a

Fix: prevent panics

b9333a5

ruflin approved these changes May 25, 2020

View reviewed changes

Address PR comments

ce87023

mtojek merged commit 7ffb21f into elastic:master May 25, 2020

mtojek mentioned this pull request May 25, 2020

Build and stream archived package content #467

Closed


		rootDir := fmt.Sprintf("%s-%s", properties.Name, properties.Version)

		err = filepath.Walk(properties.Path, func(path string, info os.FileInfo, err error) error {

Stream archived content #472

Stream archived content #472

Conversation

mtojek commented May 18, 2020

elasticmachine commented May 18, 2020 • edited Loading

💚 Build Succeeded

Build stats

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

ruflin May 18, 2020 • 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

ruflin commented May 18, 2020

mtojek commented May 19, 2020

ruflin 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

mtojek May 20, 2020 • 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

mtojek commented May 20, 2020

mtojek commented May 20, 2020

ruflin 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

Choose a reason for hiding this comment

mtojek May 25, 2020 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented May 18, 2020 •

edited

Loading

ruflin May 18, 2020 •

edited

Loading

mtojek May 20, 2020 •

edited

Loading

mtojek May 25, 2020 •

edited

Loading