Autogenerate k8s manifests in skaffold init #3703

nkubala · 2020-02-19T21:12:07Z

NOTE: this feature is hidden behind a feature flag, --XXenableManifestGeneration, to allow for further iteration and improvement before exposing publicly.

this change adds support for autogenerating k8s manifests in skaffold init for projects that do not have any. skaffold will generate a k8s yaml for each found image that is not referenced by an existing k8s yaml in the project directory, and each generated yaml file will contain a deployment for the image, and a service exposing the deployment.

in the event that skaffold init finds a builder configuration in a project directory, but no k8s yaml referencing this image, we don't actually have any name for the image, so skaffold will generate a name. this name will either be:

the name of the encompassing directory, in the event that the builder is found in a nested directory; or,
the name of the builder file itself, in the event that only one builder configuration is found
for each image, a yaml file will be generated containing kubernetes resource definitions for a service and a deployment. this file will either be:

<directory_name>/deployment.yaml, for each builder that is found in a nested directory; or,
<image_name>-deployment.yaml, if we only have one builder configuration found

so for the microservices example, we would generate

.
├── leeroy-app
│   ├── app.go
│   ├── deployment.yaml <-- generated
│   └── Dockerfile
├── leeroy-web
│   ├── deployment.yaml <-- generated
│   ├── Dockerfile
│   └── web.go
├── README.md
└── skaffold.yaml <-- (also generated)

whereas for the getting-started example, we would generate

.
├── deployment.yaml <-- generated
├── Dockerfile
├── main.go
├── README.md
└── skaffold.yaml <-- generated

Future Work

figure out how to handle ports in the generated yaml - this is currently unsupported, so port forwarding will not work
figure out the right behavior for resolved and unresolved builder configurations - if we have a preexisting image1 being referenced in k8s.yaml, but also another builder configuration, should we always generate?
remove feature flag

codecov · 2020-02-19T21:19:45Z

Codecov Report

Merging #3703 into master will decrease coverage by 0.43%.
The diff coverage is 53.9%.

Impacted Files	Coverage Δ
pkg/skaffold/initializer/build/init.go	`51.72% <0%> (-29.23%)`	⬇️
pkg/skaffold/initializer/build/builders.go	`57.69% <0%> (ø)`	⬆️
pkg/skaffold/initializer/prompt/prompt.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/initializer/build/cli.go	`75% <0%> (ø)`	⬆️
cmd/skaffold/app/cmd/init.go	`100% <100%> (ø)`	⬆️
pkg/skaffold/initializer/build/util.go	`100% <100%> (ø)`	⬆️
pkg/skaffold/initializer/init.go	`38.63% <27.27%> (-11.37%)`	⬇️
pkg/skaffold/initializer/deploy/init.go	`68.96% <50%> (-21.04%)`	⬇️
pkg/skaffold/initializer/deploy/kubectl.go	`85.18% <55.55%> (-14.82%)`	⬇️
pkg/skaffold/kubernetes/generator/generate.go	`60% <60%> (ø)`
... and 18 more

pkg/skaffold/initializer/build/init.go

pkg/skaffold/initializer/build/builders.go

pkg/skaffold/initializer/init.go

pkg/skaffold/initializer/prompt/prompt.go

pkg/skaffold/initializer/init.go

integration/testdata/init/hello/deployment.yaml.expected

pkg/skaffold/initializer/build/resolve.go

pkg/skaffold/initializer/init.go

integration/init_test.go

balopat

it seems to me that when generating manifests, the "unable to automatically resolve builder/image pairs; run 'skaffold init' without '--force' to manually resolve ambiguities" error path shouldn't even be exercised...there is nothing to match up!!! It is not the force that's broken -- I think this PR should contain the logic that when there is manifest generation, we know the pairing... especially the microservices case, when all the docker images are known and matched with exactly one manifest. but maybe I'm missing something.

nkubala · 2020-02-24T23:29:33Z

@balopat the reason I left the commented out test in there is because we currently conflate two concepts in one flag:

"--force-resolve" - the idea that we should force a resolution of builders to images, even if there are ambiguities found by skaffold
"--force-write" - the idea that skaffold should write out the skaffold.yaml it generates without user confirmation

clearly we don't want to support the first concept here if we're generating manifests, but we need to support the second concept to run the tests that actually call skaffold init without having to pass confirmation to the test to write the file. this has been fine for the existing test because it doesn't have any ambiguities in the builder/image pairs - but adding a test where there is an ambiguity (i.e. we need to generate a manifest and an image name), is now impossible to pass because I can't tell skaffold to force write the config. the test will hang and timeout.

I agree we should fix these flags, but I don't think that should happen in this PR.

pkg/skaffold/initializer/build/builders.go

tejal29 · 2020-02-25T22:25:25Z

pkg/skaffold/initializer/build/resolve.go

 	return nil
 }
+
+func getGeneratedBuilderPair(b InitBuilder) GeneratedBuilderImagePair {


Can we add some tests for this method?
filepath.Dir() will return all but last component meaning it will contain \, . which are not valid image name chars

https://play.golang.org/p/huk3EmORFw5

this is implicitly tested through TestResolveBuilderImages and TestAutoSelectBuilders. because of the way we detect builders in the analyze step of the initializer, the last 5 cases in your linked snippet will never happen.

I am not sure if this true.
I run checked out your branch and ran into this issue

Project tree:

examples/getting-started <say this is root of the repo> |__app |__ deploy |---- Dockerfile |- main.go

Currently, the code assumes you are running skaffold init from inside examples/app. This logic is very sensitive to run context.

tejaldesai@@helm-deployment (init-k8sgen)$ ../../out/skaffold init --XXenableManifestGeneration adding manifest path app/deploy/deployment.yaml for image app/deploy apiVersion: skaffold/v2alpha4 kind: Config metadata: name: helm-deployment deploy: kubectl: manifests: - app/deploy/deployment.yaml app/deploy/deployment.yaml - apiVersion: v1 kind: Service metadata: name: app/deploy labels: app: app/deploy spec: clusterIP: None selector: app: app/deploy --- apiVersion: apps/v1 kind: Deployment metadata: name: app/deploy labels: app: app/deploy spec: replicas: 1 selector: matchLabels: app: app/deploy template: metadata: labels: app: app/deploy spec: containers: - name: app/deploy image: app/deploy Do you want to write this configuration, along with the generated k8s manifests, to skaffold.yaml? [y/n]: y Generated manifest app/deploy/deployment.yaml was written Configuration skaffold.yaml was written You can now run [skaffold build] to build the artifacts or [skaffold run] to build and deploy or [skaffold dev] to enter development mode, with auto-redeploy

so I dug into this one more, and skaffold init actually doesn't even work with this directory structure :/

i'm going to add sanitization to this PR, and open an issue to fix this separately.

pkg/skaffold/initializer/build/resolve.go

dgageot · 2020-02-28T20:22:52Z

@tejal29 @balopat did @nkubala address all your feedback?

nkubala · 2020-02-28T20:35:53Z

@dgageot I'm going to add sanitization for the generated image name since it can contain illegal characters and then this should be good to go in

balopat · 2020-03-01T18:41:56Z

@balopat the reason I left the commented out test in there is because we currently conflate two concepts in one flag:

"--force-resolve" - the idea that we should force a resolution of builders to images, even if there are ambiguities found by skaffold

"--force-write" - the idea that skaffold should write out the skaffold.yaml it generates without user confirmation

clearly we don't want to support the first concept here if we're generating manifests, but we need to support the second concept to run the tests that actually call skaffold init without having to pass confirmation to the test to write the file. this has been fine for the existing test because it doesn't have any ambiguities in the builder/image pairs - but adding a test where there is an ambiguity (i.e. we need to generate a manifest and an image name), is now impossible to pass because I can't tell skaffold to force write the config. the test will hang and timeout.

I agree we should fix these flags, but I don't think that should happen in this PR.

Thanks for the explanation. I'm okay to go forward with this.

addressed feedback

nkubala requested review from balopat, dgageot and tejal29 as code owners February 19, 2020 21:12

pull-request-size bot added the size/XL label Feb 19, 2020

googlebot added the cla: yes label Feb 19, 2020

dgageot approved these changes Feb 20, 2020

View reviewed changes

dgageot self-assigned this Feb 20, 2020

tejal29 reviewed Feb 20, 2020

View reviewed changes

pkg/skaffold/initializer/build/init.go Outdated Show resolved Hide resolved

tejal29 previously requested changes Feb 20, 2020

View reviewed changes

tejal29 reviewed Feb 20, 2020

View reviewed changes

integration/testdata/init/hello/deployment.yaml.expected Show resolved Hide resolved

tejal29 reviewed Feb 21, 2020

View reviewed changes

pkg/skaffold/initializer/build/resolve.go Show resolved Hide resolved

tejal29 reviewed Feb 21, 2020

View reviewed changes

pkg/skaffold/initializer/init.go Outdated Show resolved Hide resolved

balopat reviewed Feb 22, 2020

View reviewed changes

integration/init_test.go Show resolved Hide resolved

balopat suggested changes Feb 22, 2020

View reviewed changes

nkubala added 2 commits February 24, 2020 15:20

Autogenerate k8s manifests in skaffold init

25b9ddd

feedback

8d4812f

nkubala force-pushed the init-k8sgen branch from 7da9e48 to 8d4812f Compare February 24, 2020 23:23

nkubala force-pushed the init-k8sgen branch from 1d3173e to 2b381fb Compare February 25, 2020 18:12

tejal29 reviewed Feb 25, 2020

View reviewed changes

pkg/skaffold/initializer/build/resolve.go Show resolved Hide resolved

nkubala force-pushed the init-k8sgen branch 2 times, most recently from 6c5d54c to ec97f6c Compare February 26, 2020 00:17

defer manifest generation to build initializer

286d11d

nkubala force-pushed the init-k8sgen branch from ec97f6c to 286d11d Compare February 26, 2020 00:56

nkubala added 2 commits February 28, 2020 13:13

fix adding builder pairs after generating manifests

efccc62

add sanitization for image name

42846ba

balopat approved these changes Mar 1, 2020

View reviewed changes

nkubala merged commit 0a024c4 into GoogleContainerTools:master Mar 2, 2020

nkubala deleted the init-k8sgen branch March 2, 2020 18:03

fmotrifork mentioned this pull request Mar 11, 2020

skaffold 1.5.0 fishworks/fish-food#614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autogenerate k8s manifests in skaffold init #3703

Autogenerate k8s manifests in skaffold init #3703

nkubala commented Feb 19, 2020

codecov bot commented Feb 19, 2020 •

edited

Loading

balopat left a comment •

edited

Loading

nkubala commented Feb 24, 2020

tejal29 Feb 25, 2020

nkubala Feb 26, 2020

tejal29 Feb 26, 2020

nkubala Feb 28, 2020

dgageot commented Feb 28, 2020

nkubala commented Feb 28, 2020

balopat commented Mar 1, 2020

Autogenerate k8s manifests in skaffold init #3703

Autogenerate k8s manifests in skaffold init #3703

Conversation

nkubala commented Feb 19, 2020

Future Work

codecov bot commented Feb 19, 2020 • edited Loading

Codecov Report

balopat left a comment • edited Loading

Choose a reason for hiding this comment

nkubala commented Feb 24, 2020

tejal29 Feb 25, 2020

Choose a reason for hiding this comment

nkubala Feb 26, 2020

Choose a reason for hiding this comment

tejal29 Feb 26, 2020

Choose a reason for hiding this comment

nkubala Feb 28, 2020

Choose a reason for hiding this comment

dgageot commented Feb 28, 2020

nkubala commented Feb 28, 2020

balopat commented Mar 1, 2020

codecov bot commented Feb 19, 2020 •

edited

Loading

balopat left a comment •

edited

Loading