Download and cache package image contents in package manager #1795

hasheddan · 2020-10-01T18:30:34Z

Description of your changes

Updates the package manager to download static package images and cache them in a mounted volume. This PR will remain in draft while the following items are addressed:

Fix incompatible module dependencies (more info: Azure go-autorest causing ambiguous import Azure/go-autorest#414) (this was fixed by removing a bunch of the docker machinery)
Complete unit test implementation (this is mostly done, but might follow up with some additional tests)
Allow for customizing volume mount in helm chart
Clean commit history

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Manual testing details to follow.

apis/pkg/v1alpha1/interfaces.go

cmd/crossplane/core/core.go

pkg/cache/cache.go

pkg/controller/pkg/revision/imageback.go

pkg/controller/pkg/revision/reconciler.go

pkg/controller/pkg/revision/imageback.go

pkg/cache/cache.go

hasheddan

@negz I have removed the [WIP] status but am keeping in draft so that we don't merge this and stomp on @muvaf work in #1798. Once I rebase on his changes and do a bit of refactoring then this should be ready for merge. I'll add full workflow to PR body for an understanding of how CLI works.

hasheddan · 2020-10-03T11:40:41Z

cmd/crank/main.go

 	ctx := kong.Parse(&cli,
 		kong.Name("crank"),
 		kong.Description("A tool for building platforms on Crossplane."),
+		kong.Bind(child),


This binds child, making it possible to pass it to command Run functions.

hasheddan · 2020-10-03T11:42:04Z

cmd/crank/main.go

+// strChild is a string value child argument.
+type strChild string
+
+func (c strChild) AfterApply(arg *childArg) error { // nolint:unparam
+	arg.strVal = string(c)
+	return nil
 }


Any time an arg or flag of type strChild is traversed in the node tree, AfterApply will be invoked. I am using this to propagate values from child to parent commands.

hasheddan · 2020-10-03T11:43:31Z

cmd/crank/push.go

+}
+
+// Run runs the push cmd.
+func (c *pushCmd) Run(child *childArg) error {


Logic in the run function of a command with child node subcommands will be invoked after the subcommands are. This means that we can use common functionality in the parents and just use the child leaf nodes for value setting, etc.

Refactors crank <package-type> install <args> to crank install <package-type> <args> to more closely match kubectl semantics. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Refactors crank build <package-type> commands to build static packages from a scratch image using go-containerregistry. These packages are single layer with the layer being a tarball containing only a package.yaml, which has the contents of the package in a YAML stream. Note that the package images are saved with the .xpkg extension, which identifies them as Crossplane packages. Crossplane packages can be loaded into a container runtime, such as Docker, just as any OCI image tarball can. Importantly, Crossplane package images are not tagged on build, but are instead tagged on push before being written to a registry. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Refactors the crank push command to use go-containerregistry to write xpkg tarballs to registry. Also updates crank CLI name in usage to be kubectl crossplane such that users are not confused with command syntax. Note there is a large diff in the go.mod in this commit because we no longer rely on much of the docker machinery that was previously introduced. A few of the k8s dependencies were also updated because of the transitive dependencies in go-containerregistry. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Crossplane packages are opionated OCI image tarballs with the .xpkg extension. This pkg contains utilities for building, naming, and parsing both the on-disk crossplane package format and .xpkg tarballs directly. If other crossplane pkgs are moved to an internal directory in the future we should consider keeping this one public. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

The xpkg image cache is a filesystem-backed cache that can be used to store crossplane packages for controllers that require regular access to the packages. It is safe to pass to multiple concurrent controllers. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Package revisions will now cache their packages in the container filesystem instead of reading the logs of an install pod. This reference will no longer be used. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Now that install pods are no longer used by package revisions there is no need for the package controllers to create a pod only to fetch the digest of the package image. The pod manager is now replaced with a lighter weight digester which fetches an image digest used for generating a package revision's name. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Replaces the pod log parser backend with an image backend that first attempts to fetch an image from the xpkg cache, and if unsuccessful will retrieve it from a registry. The xpkg cache is pluggable, meaning that you can modify the image backend behavior by providing a cache with different properties. This is most clearly illustrated by providing a NopCache, which will cause the image backend to re-fetch the package image on every initialization. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the package revision controllers to use the image backend for parsing and to apply a finalizer that is only removed after package contents are removed from the xpkg cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Build the package cache based on the cache-dir flag passed to crossplane and passes the cache to the package revision controllers. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Adds an emptyDir volume and mount to the crossplane deployment that is to be used for caching xpkg images. The emptyDir medium and size is configurable by passing values to the helm chart. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates the helm chart README and crossplane docs with info on how to configure the package cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

cmd/crank/build.go

cluster/charts/crossplane/templates/deployment.yaml

In order to allow for passing of more specific arguments to the push and build commands we use separate types for binding each. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Gives more informative errors and allows for overriding the filesystem used for retrieving and writing build artifacts. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Implements some basic tests that ensure an xpkg is written to memory mapped fs when command executes successfully. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Use the kong context bound stdout for writing such that it can be overridden during command tree parsing. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Plumbs fs from pushChild to push command such that it can be used with alternate file systems than just os.Fs. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Bumps crossplane-runtime to latest version on master in order to make use of the generic linter functionality in crank and the revision controllers. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Moves linter helpers that are specific to crossplane packages to the xpkg pkg and makes use of them alongside the crossplane-runtime linter functionality in the package revision controllers. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Extracts the fetching behavior of the image parser backend to a separate interface such that it is pluggable and can easily be tested. Also adds tests for image parser backend. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates build logic to accept parser and linter such that packages are validated when they are built. Build will fail if the wrong package type is supplied as a child command. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

The same meta and object schemes are almost always used for building crossplane packages, so they have been moved to public functions in the xpkg pkg. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan · 2020-10-06T02:51:30Z

@negz I believe I have addressed all feedback from our synchronous review today, plus quite a bit more in terms of testing and API cleaning (including adding linting to the build commands). I still anticipate further improvements will be made in the future in terms of how we structure and test crank, but I don't feel like we have any critical paths that are untested right now (besides my comment on the afero bug).

hasheddan · 2020-10-06T02:54:26Z

pkg/controller/pkg/revision/imageback_test.go

+		// TODO(hasheddan): re-enable after https://github.com/spf13/afero/pull/268 is merged.
+		// "ErrEmptyImage": {
+		// 	reason: "Should return error if image is empty.",
+		// 	args: args{
+		// 		c: xpkg.NewNopCache(),
+		// 		f: &MockFetcher{
+		// 			MockFetch: func() (v1.Image, error) {
+		// 				return empty.Image, nil
+		// 			},
+		// 		},
+		// 		opts: []parser.BackendOption{Package("test/test:latest")},
+		// 	},
+		// 	want: errors.Wrap(&os.PathError{Op: "open", Path: "package.yaml", Err: syscall.ENOENT}, errOpenPackageStream),
+		// },


This test causes a panic because of the way the afero is handling tarfs initialization when an empty tar reader is passed. I have opened spf13/afero#268 to fix. In practice, this would certainly be an edge case (i.e. totally empty package), but we obviously don't opportunity for panics in our code. I will work with afero maintainers to get this addressed prior to Crossplane release, or introduce a check and error early to avoid this execution branch.

negz

Awesome work @hasheddan! It's been a winding path, but I'm glad we landed here. This feels like a simplification. I've added a few comments but they're all nitpicks; feel free to ignore any you don't agree with or would prefer to defer, and merge at your leisure.

cluster/charts/crossplane/templates/deployment.yaml

cmd/crossplane/core/core.go

cmd/crank/build.go

negz · 2020-10-06T03:26:36Z

cmd/crank/build.go

+	pkgName := child.name
+	if pkgName == "" {
+		metaPath := filepath.Join(root, xpkg.MetaFile)
+		pkgName, err = xpkg.ParseNameFromMeta(child.fs, metaPath)


Is there any value in running these paths through filepath.Clean? The gosec linter has recently been recommending this to guard against http://cwe.mitre.org/data/definitions/22.html.

@negz I believe filepath.Join() also runs filepath.Clean() on the returned result https://golang.org/pkg/path/filepath/#Join

cmd/crank/build_test.go

negz · 2020-10-06T04:13:45Z

pkg/controller/pkg/revision/reconciler.go

+	objects  Establisher
+	parser   parser.Parser
+	linter   parser.Linter
+	backend  parser.Backend


I wonder if parser is the right name for the crossplane-runtime package. It's not immediately obvious what it's a parser for (i.e. packages), and parser.Parser stutters. xpkg would make sense if it didn't conflict with the local package of that name. Perhaps package.Parser, package.Linter, and package.Backend?

Yeah I struggled with this a bit. Similarly if it was named package we would have package.Package. I think parser actually makes sense for Parser (despite stuttering) and Backend (i.e. it the backend for a parser). Linter is linting the output of a parser, which would be a package so that is kind of strange. I guess we could name it pkg, which would make the general crossplane-runtime library pkg and the crossplane specific additions xpkg... If we make this change I will likely do so in follow-up PR

negz · 2020-10-06T04:16:23Z

pkg/xpkg/build.go

+	_, err = io.Copy(buf, r)
+	if err != nil {
+		return nil, errors.Wrap(err, errCopyStream)
+	}


Nit: This (and the instance below) could be inlined to if _, err := ...; err != nil. (Feels like there should be a linter for this.)

pkg/xpkg/find.go

pkg/xpkg/name.go

Changes flag type for package cache-dir to ExistingVarDir so that we panic early if cache directory does not exist. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Updates a few code comments and command outputs to be more descriptive of their functionality. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

negz reviewed Oct 1, 2020

View reviewed changes

hasheddan force-pushed the artifacts branch from c94cff8 to 1ccb5aa Compare October 2, 2020 21:31

hasheddan mentioned this pull request Oct 2, 2020

Update to v2 package format crossplane/provider-template#5

Open

hasheddan commented Oct 3, 2020

View reviewed changes

hasheddan changed the title ~~[WIP] Download and cache package image contents in package manager~~ Download and cache package image contents in package manager Oct 3, 2020

This was referenced Oct 3, 2020

Move common package linter functionality to crossplane-runtime crossplane/crossplane-runtime#211

Merged

Consider using OCI Artifacts for packaging #1799

Closed

Build provider package with static content crossplane-contrib/provider-aws#365

Merged

hasheddan force-pushed the artifacts branch from 1ccb5aa to 4ed41d2 Compare October 5, 2020 16:45

hasheddan added 12 commits October 5, 2020 11:45

Refactor crank install commands to match kubectl pattern

8702a9f

Refactors crank <package-type> install <args> to crank install <package-type> <args> to more closely match kubectl semantics. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Remove installPodRef from package revisions

e1c7394

Package revisions will now cache their packages in the container filesystem instead of reading the logs of an install pod. This reference will no longer be used. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Build xpkg cache when initializing manager and pass to revisions

0780f03

Build the package cache based on the cache-dir flag passed to crossplane and passes the cache to the package revision controllers. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Update helm chart docs with package cache config fields

ff92e85

Updates the helm chart README and crossplane docs with info on how to configure the package cache. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan force-pushed the artifacts branch from 4ed41d2 to ff92e85 Compare October 5, 2020 16:45

hasheddan commented Oct 5, 2020

View reviewed changes

cmd/crank/build.go Outdated Show resolved Hide resolved

hasheddan marked this pull request as ready for review October 5, 2020 17:31

hasheddan commented Oct 5, 2020

View reviewed changes

cluster/charts/crossplane/templates/deployment.yaml Show resolved Hide resolved

hasheddan added 5 commits October 5, 2020 21:34

Use separate child args for push and build

abc1246

In order to allow for passing of more specific arguments to the push and build commands we use separate types for binding each. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Wrap build errors and pass fs in buildChild

900b1d7

Gives more informative errors and allows for overriding the filesystem used for retrieving and writing build artifacts. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Add basic testing for build command

ac3647a

Implements some basic tests that ensure an xpkg is written to memory mapped fs when command executes successfully. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Use stdout bound to kong context in install commands

a1f72b2

Use the kong context bound stdout for writing such that it can be overridden during command tree parsing. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Allow for overriding fs used for pushing xpkg images

7a3c53b

Plumbs fs from pushChild to push command such that it can be used with alternate file systems than just os.Fs. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan added 5 commits October 5, 2020 21:41

Bump crossplane-runtime to pick up generic linter machinery

682402b

Bumps crossplane-runtime to latest version on master in order to make use of the generic linter functionality in crank and the revision controllers. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Separate fetch logic from image parser backend

b1587e4

Extracts the fetching behavior of the image parser backend to a separate interface such that it is pluggable and can easily be tested. Also adds tests for image parser backend. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Lint packages during build

7dd8eb5

Updates build logic to accept parser and linter such that packages are validated when they are built. Build will fail if the wrong package type is supplied as a child command. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Move parser scheme building helpers to xpkg

9137c3b

The same meta and object schemes are almost always used for building crossplane packages, so they have been moved to public functions in the xpkg pkg. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan requested a review from negz October 6, 2020 02:49

hasheddan commented Oct 6, 2020

View reviewed changes

negz approved these changes Oct 6, 2020

View reviewed changes

hasheddan added 2 commits October 6, 2020 08:51

Require that cache-dir directory exists on flag parsing

043d189

Changes flag type for package cache-dir to ExistingVarDir so that we panic early if cache directory does not exist. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Clean up package building and management comments and output

f1369d6

Updates a few code comments and command outputs to be more descriptive of their functionality. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan merged commit f0869fa into crossplane:master Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download and cache package image contents in package manager #1795

Download and cache package image contents in package manager #1795

hasheddan commented Oct 1, 2020 •

edited

Loading

hasheddan left a comment

hasheddan Oct 3, 2020

hasheddan Oct 3, 2020

hasheddan Oct 3, 2020

hasheddan commented Oct 6, 2020

hasheddan Oct 6, 2020

negz left a comment

negz Oct 6, 2020

hasheddan Oct 6, 2020

negz Oct 6, 2020

hasheddan Oct 6, 2020

negz Oct 6, 2020

Download and cache package image contents in package manager #1795

Download and cache package image contents in package manager #1795

Conversation

hasheddan commented Oct 1, 2020 • edited Loading

Description of your changes

How has this code been tested

hasheddan 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

hasheddan commented Oct 6, 2020

Choose a reason for hiding this comment

negz 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

hasheddan commented Oct 1, 2020 •

edited

Loading