Add core ko builder implementation #6054

halvards · 2021-06-22T13:19:56Z

Description

This adds the Build() method for building artifacts using ko. It supports both publishing the resulting image to a registry, and sideloading it to the local Docker daemon.

The Skaffold docker client is used in the ko builder. This ensures that any Minikube config set by Skaffold is used. The ko builder uses the docker client when sideloading images to the docker daemon.

The temporary.go file contains the structs intended to be added to the schema.

The additions to the design proposal explain image naming for the ko builder, specifically how container images are named and how Go import paths are resolved when using the proposed ko builder.

This commit includes ko builder unit tests for non-current-working-directory workspace dirs. These verify that the ko builder works even if the context specified in skaffold.yaml differs from the current working directory.

Tracking: #6041

Follow-up Work

This implementation is still missing the following features:

integration test
dependencies (for file watching)
insecure registries
debug mode
support for go flags and environment variables (based on Add support for Go build flags ko-build/ko#340)
actually plumbing the builder into the Skaffold CLI and API :-)

codecov · 2021-06-22T13:32:49Z

Codecov Report

Merging #6054 (5158e66) into main (73d1ec9) will increase coverage by 0.02%.
The diff coverage is 74.28%.

@@            Coverage Diff             @@
##             main    #6054      +/-   ##
==========================================
+ Coverage   70.38%   70.40%   +0.02%     
==========================================
  Files         501      505       +4     
  Lines       22755    22825      +70     
==========================================
+ Hits        16015    16070      +55     
- Misses       5696     5702       +6     
- Partials     1044     1053       +9

Impacted Files	Coverage Δ
pkg/skaffold/build/ko/build.go	`60.00% <60.00%> (ø)`
pkg/skaffold/build/ko/publisher.go	`80.00% <80.00%> (ø)`
pkg/skaffold/build/ko/builder.go	`100.00% <100.00%> (ø)`
pkg/skaffold/build/ko/ko.go	`100.00% <100.00%> (ø)`
pkg/skaffold/docker/parse.go	`87.39% <0.00%> (-0.85%)`	⬇️
pkg/skaffold/util/tar.go	`65.51% <0.00%> (+2.29%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d1ec9...5158e66. Read the comment docs.

briandealwis

Looking good!

pkg/skaffold/build/ko/build.go

briandealwis · 2021-06-29T15:24:24Z

pkg/skaffold/build/ko/build.go

+// If the image name does _not_ start with `ko://`, determine the Go import
+// path of the image workspace directory.


Would it make sense to allow the user to specify the import-path? There are users with a monorepo that contains multiple images. So something like:

build: artifacts: - image: gcr.io/project/backend # ~my/image/one~ builder: ko importPath: cmd/main1 - image: gcr.io/project/frontend # ~my/image/two~ builder: ko importPath: cmd/main2

(implicit context: .)

(Updated to use explicit and different image names)

Yes! The idea is that users can pass this in the image name using the ko:// prefix like this:

build: artifacts: - image: ko://example.com/my/image/one ko: {} - image: ko://example.com/my/image/two ko: {}

For the same context: .. WDYT?

I'll add some unit tests to show this.

What should happen for non-ko:// image references?

Ko needs an import path to build an image. So for non-ko:// image references, getImportPath() determines the Go import path from the context/workspace directory. This works with arbitrary image names in skaffold.yaml - in other words, a non-ko:// image name doesn't need to match the import path.

The former supports current ko users moving to Skaffold, as they likely already have Kubernetes manifests with ko image references in place of image names. The latter matches Skaffold behavior for other builders, and would be more familiar for existing Skaffold users who want to start using ko.

Added some unit tests to show non-cwd context/workspace builds.

Using the context to specify the importPath is a bit unorthodox

The proposal uses context to specify the directory of execution.

precludes users who use a different project layout such as with mixed languages

Does it? For the multi-go.mod example, this configuration would work:

build: artifacts: - image: root context: . ko: {} - image: pkg-foo context: ./pkg/foo ko: {}

The root image in that example would use the go.mod from the same directory as skaffold.yaml, while the pkg-foo image would use ./pkg/foo/go.mod. The ko builder runs go list and go build in the context directory. This is the ko logic, the ko builder passes the context directory as the dir argument to the moduleInfo() function: https://github.com/google/ko/blob/690533235ad4209fab6c88f44314760b102ce829/pkg/build/gobuild.go#L153 and https://github.com/google/ko/blob/690533235ad4209fab6c88f44314760b102ce829/pkg/build/gobuild.go#L365

Full import paths (e.g., github.com/org/repo aren't required in skaffold.yaml, and I don't think we need to ask users to provide them. Supporting ko://-prefixed import paths as image names in skaffold.yaml is specifically just for existing ko users who use ko to render/populate image names in Kubernetes manifests. For these users, this feature simplifies Skaffold adoption, where Skaffold takes over rendering responsibilities, with no change required to their existing k8s manifest templates.

.ko.yaml is only read from the current process working directory, or from KO_CONFIG_PATH. It supports overriding the default base image, and the base image per import path prefix: https://github.com/google/ko#configuration

I didn't explain myself well. Here the issue is:

build: artifacts: - image: gcr.io/k8s-project/image # this is the image name context: . # use the root go.mod

How do I specify that the image should use the main() in ./pkg/foo?

I see. Given this directory structure:

./go.mod ./skaffold.yaml ./pkg/foo/main.go # <- main() is here

The Skaffold config would be:

build: artifacts: - image: gcr.io/k8s-project/image context: ./pkg/foo

ko (and go build) executes in the ./pkg/foo directory, so context specifies the directory of execution.

this feels a little bit like a paradigm rift between the way we use context for the other builders - if you tried to simply "plug and play" a different builder (e.g. GCB) using this specified context, I don't think it would work since skaffold generally treats this as the root of all the required source files for building the artifact.

that said, since ko is a little different I don't think we should keep blocking this implementation since we can always go back and change this later. I still think this would be clearer for users as an importPath field in the build configuration (especially users who don't know anything about ko except it's "a good way to build go apps"), but we can revisit as an enhancement later.

Yes, you're right. It'll fail for non-local builders.

I'll dig into what the solution here could be and create another PR. Since users can specify import path via the image field (as ko://import.path/here/pkg/foo), adding an importPath field would result in two ways to provide the import path, and that could be confusing.

Google Cloud Buildpacks fail if there are two main() functions in different directories where both are not the context directory. There, the GOOGLE_BUILDABLE envvar must be set: https://github.com/GoogleCloudPlatform/buildpacks/blob/main/README.md#configuration

briandealwis · 2021-06-29T15:27:23Z

pkg/skaffold/build/ko/build_test.go

+)
+
+// koImportPath is the import path of this package, with the ko scheme prefix.
+const koImportPath = "ko://github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/ko"


Aside: we seem to have broken this ko:// behaviour (#5505).

I can take a look at this - feel free to assign me (I don't have permission to assign myself).

Fixed in #6286

pkg/skaffold/build/ko/schema/temporary.go

When embedding ko, it may be necessary to override the docker client. This adds a PublishOption to inject a docker client created elsewhere. Ko will use this client to interact with the docker daemon. Context: GoogleContainerTools/skaffold#6054 (comment)

briandealwis · 2021-07-09T17:23:45Z

Marking this as blocked pending ko-build/ko#378.

When embedding ko, it may be necessary to override the docker client. This adds a PublishOption to inject a docker client created elsewhere. Ko will use this client to interact with the docker daemon. Context: GoogleContainerTools/skaffold#6054 (comment)

halvards · 2021-07-22T00:08:57Z

Friendly ping 🙂

pkg/skaffold/build/ko/build_test.go

briandealwis · 2021-07-27T19:47:20Z

pkg/skaffold/build/ko/build_test.go

+			importpath:          "ko://github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/docker",
+			imageNameFromConfig: "ko://github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/docker",
+			workspace:           "..",
+		},


I found this test hard to puzzle through: it wasn't clear what are the inputs vs expected test results, and what are test incidentals. test.ref seems an incidental? It might help to embed the Artifact definition in the test structure?

{ description: ..., artifact: &latestV1.Artifact{ ArtifactType: latestV1.ArtifactType{KoArtifact: &latestV1.KoArtifact{}}, ImageName: "ko://github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/ko", Workspace: ... }, },

I agree, it's not a great test. Since it's a unit test, I didn't want to actually build or publish - that's for the integration test.

That means the test covers wiring the components together, and a few bits of functionality: determining the Go import path, ensuring the image name is printed to the out writer, and returning the image identifier correctly for pushed vs non-pushed images.

All conditional logic are in separate functions, so I'll replace this test with individual tests for those functions. I'll leave one test case for the Build() function, just to check that everything is wired together.

halvards · 2021-07-28T09:19:40Z

I pushed a new commit with improvements to the confusing unit test.

docs/design_proposals/ko-builder.md

briandealwis · 2021-08-04T17:57:22Z

pkg/skaffold/build/ko/build.go

+// If the image name does _not_ start with `ko://`, determine the Go import
+// path of the image workspace directory.


I didn't explain myself well. Here the issue is:

build: artifacts: - image: gcr.io/k8s-project/image # this is the image name context: . # use the root go.mod

How do I specify that the image should use the main() in ./pkg/foo?

pkg/skaffold/build/ko/build_test.go

halvards · 2021-08-09T12:42:18Z

Friendly ping 😀

nkubala

left a comment about the importPath vs. context, but I think we should move forward with this so it doesn't sit in limbo forever. one other nit about the temporary schema changes (which I think is a great idea) and otherwise this LGTM.

thanks @briandealwis for the thoughtful reviews on this and @halvards for sticking with us while we get it right :)

pkg/skaffold/build/ko/build.go

pkg/skaffold/build/ko/build_test.go

This adds the `Build()` method for building artifacts using ko. It supports both publishing the resulting image to a registry, and sideloading it to the local Docker daemon. The Skaffold docker client is used in the ko builder. This ensures that any Minikube config set by Skaffold is used. The ko builder uses the docker client when sideloading images to the docker daemon. The `temporary.go` file contains the structs intended to be added to the schema. The additions to the design proposal explain image naming for the ko builder, specifically how container images are named and how Go import paths are resolved when using the proposed ko builder. This commit includes ko builder unit tests for non-current-working-directory workspace dirs. These verify that the ko builder works even if the context specified in `skaffold.yaml` differs from the current working directory. This implementation is still missing the following features: - integration test - dependencies (for file watching) - insecure registries - debug mode - support for `go` flags and environment variables (based on ko-build/ko#340) - actually plumbing the builder into the Skaffold CLI and API :-) Tracking: GoogleContainerTools#6041

Split out the confusing end-to-end unit test with tests for individual functions.

Also fix typo in design proposal.

Reminders to update the import path once the contents of `pkg/skaffold/build/ko/schema` have been added to the real schema in `pkg/skaffold/schema/latest/v1`.

halvards · 2021-08-11T09:35:47Z

Thanks for the review @briandealwis and @nkubala !

Adding the TODO in the import block resulted in gci linter errors: https://app.travis-ci.com/github/GoogleContainerTools/skaffold/jobs/530642848#L313

Decide on how ko builder users should specify the location of `package main` for their images. Previous discussion: GoogleContainerTools#6054 (comment) Tracking: GoogleContainerTools#6041 Related: GoogleContainerTools#6054, GoogleContainerTools#6286

* Refine ko builder behavior for main packages Decide on how ko builder users should specify the location of `package main` for their images. Previous discussion: #6054 (comment) Tracking: #6041 Related: #6054, #6286 * Improve explanation of the Target config field Clarify how a blank value works, and also how users can use a pattern with wildcards.

This implements support for the Target config field for the ko builder. The Target field allows users to specify a target for `go build`. This is necessary where the main package is not in the context directory. Tracking: GoogleContainerTools#6041 Related: GoogleContainerTools#6054, GoogleContainerTools#6437

This implements support for the Target config field for the ko builder. The Target field allows users to specify a target for `go build`. This is necessary where the main package is not in the context directory. Tracking: #6041 Related: #6054, #6437

halvards requested a review from a team as a code owner June 22, 2021 13:19

halvards requested a review from nkubala June 22, 2021 13:19

google-cla bot added the cla: yes label Jun 22, 2021

pull-request-size bot added the size/XL label Jun 22, 2021

tejal29 assigned nkubala Jun 23, 2021

briandealwis reviewed Jun 29, 2021

View reviewed changes

halvards mentioned this pull request Jul 2, 2021

Enable overriding docker client ko-build/ko#378

Merged

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Jul 7, 2021

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 7, 2021

briandealwis added the !! blocked !! this issue/PR is blocked by another issue label Jul 9, 2021

halvards force-pushed the ko-build-core branch from c3fae69 to 64d3d30 Compare July 22, 2021 02:00

briandealwis removed the !! blocked !! this issue/PR is blocked by another issue label Jul 23, 2021

halvards force-pushed the ko-build-core branch from 64d3d30 to 227e428 Compare July 27, 2021 01:22

briandealwis reviewed Jul 27, 2021

View reviewed changes

halvards force-pushed the ko-build-core branch from 1817348 to e248f7a Compare July 28, 2021 09:18

halvards force-pushed the ko-build-core branch from e248f7a to 1855d7e Compare July 29, 2021 07:56

briandealwis reviewed Aug 4, 2021

View reviewed changes

halvards force-pushed the ko-build-core branch from ddb997e to e8a7e4e Compare August 5, 2021 00:44

MarlonGamez added the kokoro:run runs the kokoro jobs on a PR label Aug 10, 2021

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Aug 10, 2021

nkubala approved these changes Aug 11, 2021

View reviewed changes

pkg/skaffold/build/ko/build.go Show resolved Hide resolved

pkg/skaffold/build/ko/build_test.go Show resolved Hide resolved

halvards added 3 commits August 11, 2021 19:24

Improve ko Builder unit tests

7173991

Split out the confusing end-to-end unit test with tests for individual functions.

Reorder test inputs and expected outputs

4254556

Also fix typo in design proposal.

Add TODO to update import path

84777c7

Reminders to update the import path once the contents of `pkg/skaffold/build/ko/schema` have been added to the real schema in `pkg/skaffold/schema/latest/v1`.

halvards force-pushed the ko-build-core branch from e8a7e4e to 84777c7 Compare August 11, 2021 09:36

Fix lint errors for TODO comments in import block

5158e66

Adding the TODO in the import block resulted in gci linter errors: https://app.travis-ci.com/github/GoogleContainerTools/skaffold/jobs/530642848#L313

tejal29 merged commit e6a7a03 into GoogleContainerTools:main Aug 11, 2021

halvards deleted the ko-build-core branch August 12, 2021 00:41

halvards mentioned this pull request Aug 16, 2021

Refine ko builder behavior for main packages #6437

Merged

halvards mentioned this pull request Aug 17, 2021

Implement Target support for ko builder #6447

Merged

halvards mentioned this pull request Aug 24, 2021

Add Dir config field to ko builder #6496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add core ko builder implementation #6054

Add core ko builder implementation #6054

halvards commented Jun 22, 2021 •

edited

Loading

codecov bot commented Jun 22, 2021 •

edited

Loading

briandealwis left a comment

briandealwis Jun 29, 2021 •

edited

Loading

halvards Jul 1, 2021

briandealwis Jul 6, 2021

halvards Jul 7, 2021

halvards Jul 7, 2021

halvards Jul 28, 2021

briandealwis Aug 4, 2021

halvards Aug 5, 2021 •

edited

Loading

nkubala Aug 11, 2021

halvards Aug 11, 2021

briandealwis Jun 29, 2021

halvards Jul 7, 2021

halvards Jul 28, 2021

briandealwis commented Jul 9, 2021

halvards commented Jul 22, 2021

briandealwis Jul 27, 2021

halvards Jul 28, 2021

halvards commented Jul 28, 2021

briandealwis Aug 4, 2021

halvards commented Aug 9, 2021

nkubala left a comment

halvards commented Aug 11, 2021

		// If the image name does _not_ start with `ko://`, determine the Go import
		// path of the image workspace directory.

Add core ko builder implementation #6054

Add core ko builder implementation #6054

Conversation

halvards commented Jun 22, 2021 • edited Loading

codecov bot commented Jun 22, 2021 • edited Loading

Codecov Report

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Jun 29, 2021 • 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

halvards Aug 5, 2021 • 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

briandealwis commented Jul 9, 2021

halvards commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halvards commented Jul 28, 2021

Choose a reason for hiding this comment

halvards commented Aug 9, 2021

nkubala left a comment

Choose a reason for hiding this comment

halvards commented Aug 11, 2021

halvards commented Jun 22, 2021 •

edited

Loading

codecov bot commented Jun 22, 2021 •

edited

Loading

briandealwis Jun 29, 2021 •

edited

Loading

halvards Aug 5, 2021 •

edited

Loading