starting work on file assets builder #3085

chrislovecnm · 2017-07-28T23:14:43Z

I refactored to the dockerassets pkg to assetstasks, in order to not add yet another package. Added file copy task, that I have tested with s3 locally, but not certain how to add memfs tests.

Fixes: #3086

justinsb · 2017-08-01T04:22:50Z

pkg/assets/file_builder.go

+)
+
+// AssetBuilder discovers and remaps assets
+type FileAssetBuilder struct {


I think you could incorporate this into AssetBuilder, rather than needing a separate class, but we can see how it plays out..

justinsb · 2017-08-01T04:23:55Z

pkg/assets/file_builder.go

+	if f.AssetsLocation != nil && f.AssetsLocation.FileRepository != nil {
+		fileURL, err := url.Parse(file)
+		if err != nil {
+			return "", fmt.Errorf("unable to parse file url: %q, %v", file, err)


typically ...file url %q: %v but NBD

justinsb · 2017-08-01T04:24:42Z

pkg/assets/file_builder.go

+			fileRepo += "/"
+		}
+
+		fileAsset.File = fileRepo + "/" + fileURL.Path


Presumably we don't want the + "/" here given we just added it above?

justinsb · 2017-08-01T04:25:40Z

pkg/assets/file_builder_test.go

+		kopsAssets *kops.Assets
+	}{
+		{
+			// FIXME - need https://s3.amazonaws.com/k8s-for-greeks-kops/kubernetes-release/release/v1.7.2/bin/linux/amd64/kubelet


I realized that - specifically for S3 - we can use a signed URL

justinsb · 2017-08-01T04:26:41Z

pkg/assets/file_builder.go

+}
+
+// TODO SHA
+// TODO need to map file in the same manner and include vfs and http location


Are these still outstanding?

Open question I have, and I updated the comment, is do we have the SHA url with the file URL.

justinsb · 2017-08-01T04:29:36Z

upup/pkg/fi/assettasks/copyfile.go

+}
+
+func transferFile(source string, target string, sourceSHA string, sourceSHALocation string) error {
+	data, err := vfs.Context.ReadFile(source)


Ouch... these files could be big!

testing worked ok, do we have another method?

Do you want me to write the file to disk? I think it needs to be in memory in order to compare the SHAs anyways.

It's fine for now, but streaming is going to be preferable for big files, and that probably does mean a tempfile if you effectively have to do two passes (compare SHAs and then copy to a destination).

justinsb · 2017-08-01T04:30:42Z

upup/pkg/fi/assettasks/copyfile.go

+
+		if sha != "" {
+
+			trimmedSHA := strings.TrimSuffix(sha, "\n")


TrimSpace might be easier

justinsb · 2017-08-01T04:33:58Z

upup/pkg/fi/assettasks/copyfile.go

+				return err
+			}
+
+			b := bytes.NewBufferString(sha)


I think you can just use []byte(sha)

justinsb · 2017-08-01T04:34:45Z

upup/pkg/fi/assettasks/copyfile.go

+			trimmedSHA := strings.TrimSuffix(sha, "\n")
+
+			in := bytes.NewReader(data)
+			dataHash, err := hashing.HashesForResource(in, []hashing.HashAlgorithm{hashing.HashAlgorithmSHA1})


You can use hashing.HashAlgorithmSHA1.Hash(in)

justinsb · 2017-08-01T04:38:07Z

upup/pkg/fi/assettasks/copyfile.go

+	//path := strings.Join(pathSlice, "/")
+
+	// TODO I am not a huge fan of this, but it would work
+	// TODO @justinsb should we require an URL?


Yes, I would just have the task expect "real" vfs path strings, i.e. ones that would work with vfs.Context.BuildVfsPath. From a vfs.Path you can get the path using Path()

Need more context please

justinsb · 2017-08-01T04:40:08Z

A few simplifications possible, and it would be great to get at least one usage of RemapFile so we can see whether FileAssetBuilder should be merged with AssetBuilder

chrislovecnm · 2017-08-01T19:41:20Z

@justinsb I can refactor to just use on builder. Doing that now, and I have some questions

k8s-github-robot · 2017-08-04T00:01:58Z

@chrislovecnm PR needs rebase

justinsb · 2017-08-06T03:40:48Z

pkg/assets/builder.go

-	Assets []*Asset
+	ContainerAssets []*ContainerAsset
+	FileAssets      []*FileAsset
+	AssetsLocation  *kops.Assets


Probably assetsLocation as I would guess no need to export (?), but NBD.

justinsb · 2017-08-06T03:42:29Z

pkg/assets/builder.go

+		}
+
+		fileRepo := strings.TrimSuffix(*a.AssetsLocation.FileRepository, "/")
+		fileAsset.File = fileRepo + fileURL.Path


It may turn out we want the Host in here also, as otherwise we risk collisions (and it'll be more self explanation with the Host I would guess), but we can see how it goes...

justinsb · 2017-08-06T03:45:52Z

pkg/assets/builder_test.go

+		kopsAssets *kops.Assets
+	}{
+		{
+			// FIXME - need https://s3.amazonaws.com/k8s-for-greeks-kops/kubernetes-release/release/v1.7.2/bin/linux/amd64/kubelet


We do, but this only applies to nodeup, right? Because AIUI once we are in nodeup we can (more) easily download from a secured bucket. But nodeup is tricky, and we either want to remap or use a signed URL.

We can also remap on the upload, i.e. we can recognize that a write to https://s3.amazonaws.com/k8s-for-greeks-kops/ becomes a write to s3://k8s-for-greeks-kops/, using a public ACL.

To my mind, a signed URL is going to be best, because I don't think people that want to run mirrors are going to love the idea of public buckets.

Not sure if this FIXME is a blocker for merge? I'm guessing not, because it doesn't cause any regression in existing behaviour (i.e. this doesn't work yet anyway)?

justinsb · 2017-08-06T03:48:34Z

upup/pkg/fi/assettasks/copyfile.go

+	SourceFile *string
+	TargetFile *string
+	// @justinsb not sure I like this but the problem is knowing if the sha is a file or a string
+	// so I did a location or a sha.  We have both SHAs in remote file locations, and SHAs in strings


I'm saying when you build CopyFile, you can the SHA URL at that time, so that CopyFile always has a SHA directly in a string value.

justinsb · 2017-08-06T03:50:20Z

upup/pkg/fi/assettasks/copyfile.go

+}
+
+func transferFile(source string, target string, sourceSHA string, sourceSHALocation string) error {
+	data, err := vfs.Context.ReadFile(source)


It's fine for now, but streaming is going to be preferable for big files, and that probably does mean a tempfile if you effectively have to do two passes (compare SHAs and then copy to a destination).

justinsb · 2017-08-06T03:54:19Z

upup/pkg/fi/assettasks/copyfile.go

+
+			b := bytes.NewBufferString(sha)
+			if err := writeFile(shaVFS, b.Bytes()); err != nil {
+				return fmt.Errorf("Error uploading file %q: %v", shaVFS, err)


Writing the target file before writing the target SHA is going to be safer..

justinsb · 2017-08-06T03:58:12Z

A few suggestions but can be done as the code evolves.

I believe this is additive - only comes in to play if users configure an assetLocation, so LGTM

/lgtm

k8s-github-robot · 2017-08-11T03:44:44Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @justinsb

k8s-github-robot · 2017-08-12T00:03:31Z

@chrislovecnm PR needs rebase

justinsb · 2017-08-14T11:59:23Z

Needs rebase again but then LGTM

chrislovecnm · 2017-08-18T19:05:04Z

@justinsb needs another lgtm, as I do not like self merging :(

justinsb · 2017-08-20T14:49:01Z

/lgtm

k8s-github-robot · 2017-08-20T14:49:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm,justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-20T14:49:28Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-20T15:21:06Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 28, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

justinsb reviewed Aug 1, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2017

justinsb reviewed Aug 6, 2017

View reviewed changes

k8s-ci-robot assigned justinsb Aug 6, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2017

chrislovecnm force-pushed the file-asset-tasks branch from 6a77fa7 to 15a2c9f Compare August 11, 2017 03:44

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 11, 2017

starting work on file assets builder

ee17e65

chrislovecnm force-pushed the file-asset-tasks branch from 15a2c9f to ee17e65 Compare August 18, 2017 19:04

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2017

k8s-github-robot merged commit 9c86800 into kubernetes:master Aug 20, 2017

chrislovecnm deleted the file-asset-tasks branch December 30, 2017 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

starting work on file assets builder #3085

starting work on file assets builder #3085

chrislovecnm commented Jul 28, 2017 •

edited

justinsb Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

chrislovecnm Aug 1, 2017

justinsb Aug 1, 2017

chrislovecnm Aug 1, 2017

chrislovecnm Aug 1, 2017

justinsb Aug 6, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

justinsb Aug 1, 2017

chrislovecnm Aug 1, 2017

justinsb commented Aug 1, 2017

chrislovecnm commented Aug 1, 2017

k8s-github-robot commented Aug 4, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb commented Aug 6, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 12, 2017

justinsb commented Aug 14, 2017

chrislovecnm commented Aug 18, 2017

justinsb commented Aug 20, 2017

k8s-github-robot commented Aug 20, 2017

k8s-github-robot commented Aug 20, 2017

k8s-github-robot commented Aug 20, 2017

starting work on file assets builder #3085

starting work on file assets builder #3085

Conversation

chrislovecnm commented Jul 28, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 1, 2017

chrislovecnm commented Aug 1, 2017

k8s-github-robot commented Aug 4, 2017

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

justinsb commented Aug 6, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 12, 2017

justinsb commented Aug 14, 2017

chrislovecnm commented Aug 18, 2017

justinsb commented Aug 20, 2017

k8s-github-robot commented Aug 20, 2017

k8s-github-robot commented Aug 20, 2017

k8s-github-robot commented Aug 20, 2017

chrislovecnm commented Jul 28, 2017 •

edited