implement env variable expansion for kaniko builds #4557

DanielSel · 2020-07-24T11:18:27Z

Adds environment variable expansion for kaniko builds (to address currently inconsistent behaviour between buildArgs and envs).
See #3974

codecov · 2020-07-24T11:29:33Z

Codecov Report

Merging #4557 into master will decrease coverage by 0.00%.
The diff coverage is 79.24%.

@@            Coverage Diff             @@
##           master    #4557      +/-   ##
==========================================
- Coverage   72.36%   72.36%   -0.01%     
==========================================
  Files         360      360              
  Lines       12474    12527      +53     
==========================================
+ Hits         9027     9065      +38     
- Misses       2789     2800      +11     
- Partials      658      662       +4

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100.00% <ø> (ø)`
pkg/skaffold/build/cluster/kaniko.go	`27.77% <64.51%> (+27.77%)`	⬆️
pkg/skaffold/build/cluster/pod.go	`86.95% <100.00%> (+0.38%)`	⬆️
pkg/skaffold/docker/image.go	`80.61% <100.00%> (+1.27%)`	⬆️
pkg/skaffold/docker/reference.go	`100.00% <100.00%> (ø)`
pkg/skaffold/util/env_template.go	`94.73% <0.00%> (-5.27%)`	⬇️
pkg/skaffold/build/kaniko/args.go	`92.85% <0.00%> (-2.39%)`	⬇️

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 12d64a2...2d0bdb7. Read the comment docs.

DanielSel · 2020-07-24T20:31:37Z

@tejal29 @gsquared94 it seems like some of the unit tests are flaky in the CI, but run flawless locally. Took me a few attempts to get the build to pass: https://github.com/GoogleContainerTools/skaffold/runs/907521989

Now I removed the dummy commit via force-push and the build status went from passing to failing without a rebuild. Do you have any ideas what I should do?

DanielSel · 2020-07-27T09:26:17Z

The Travis CI build is back to green. Thanks, in case anyone manually triggered the rebuild!

gsquared94 · 2020-07-28T01:41:23Z

@DanielSel sorry about the long time to reply; was traveling. I'd restarted the failed job manually, travis can be flaky that way sometimes.

DanielSel · 2020-07-28T09:11:24Z

I fixed the oversight in parsing image names (forgot the possibility of IP:PORT as registry url) and added a test.
@gsquared94 Could you re-run kokoro?

Thanks!

DanielSel · 2020-07-29T11:59:03Z

@gsquared94 one last kokoro run please :)

nkubala · 2020-07-30T23:58:27Z

pkg/skaffold/build/cluster/kaniko.go

+	return generatedEnvs, nil
+}
+
+func parseImageParts(imageStr string) (string, string, string, error) {


I think you should be able to use docker.ParseReference here instead. we do this a few other places in the code

That's how I started out until I realized that docker.ParseReference can't split out the image name from the repo path, which we need for our CI setup.

I could replace part of the function code with a call to docker.ParseReference and only leave the code to split IMAGE_NAME from IMAGE_REPO if you prefer.

What do you think @nkubala ?

sorry I totally missed this one. I was actually wrong on my original comment, we're using docker's (reference.Parse)[https://godoc.org/github.com/docker/distribution/reference#Parse] in skaffold. we finesse that into a ImageReference struct which should be able to split image name and repo path (we call it Domain) right?

if it isn't doing everything you need, I think that code should live somewhere near here so we can embed those pieces into the ImageReference struct.

@nkubala no worries. I'm not sure that it would make sense to refactor the ImageReference to change the way it splits image FQDN's since I assume there was a reason for this specific split.

However, I refactored my code to use docker.ParseReference (which returns the ImageReference you pointed out) instead of implementing it from scratch. Was that what you had in mind?

hey @DanielSel, I dug in a little more into ParseReference and how it creates the ImageReference. take the image gcr.io/k8s-skaffold/skaffold:foo as an example:

BaseName -> gcr.io/k8s-skaffold/skaffold
Domain -> gcr.io
Path -> k8s-skaffold/skaffold

the part you're interested in here is the "Repo", which would be gcr.io/k8s-skaffold. I think part of the confusion is that our terminology is not quite right here: BaseName is really "Unqualified Image Name" (to me at least), which is also "Repo" + "Image Name" (both of which are missing from this struct).

I propose that you take your logic here to split the image parts (which seems mostly fine to me as is) and move it into docker.ParseReference, and then embed "Repo" and "Image Name" into the ImageReference struct we already have. this won't change the existing usage of the struct around the codebase, but could be used later on if we wanted to, and is easier to find if we ever want to make changes to it. this isn't kaniko-specific logic, so it doesn't really belong in the kaniko package. WDYT?

(I think) I got you now :)
Code updated.

DanielSel · 2020-09-11T11:11:43Z

@tejal29 I took the freedom to rebase, I hope that's ok.

Could somebody trigger another kokoro run and make a decision about the requested changes please? This pull request is starting to gather dust :)

nkubala

@DanielSel thank you so much for sticking with us here, this is very close but I just want to make sure we put the code in the right place the first time so we don't have to move it later :)

pkg/skaffold/build/cluster/kaniko_test.go

nkubala · 2020-09-17T22:42:35Z

pkg/skaffold/build/cluster/kaniko.go

+	return generatedEnvs, nil
+}
+
+func parseImageParts(imageStr string) (string, string, string, error) {


hey @DanielSel, I dug in a little more into ParseReference and how it creates the ImageReference. take the image gcr.io/k8s-skaffold/skaffold:foo as an example:

BaseName -> gcr.io/k8s-skaffold/skaffold
Domain -> gcr.io
Path -> k8s-skaffold/skaffold

the part you're interested in here is the "Repo", which would be gcr.io/k8s-skaffold. I think part of the confusion is that our terminology is not quite right here: BaseName is really "Unqualified Image Name" (to me at least), which is also "Repo" + "Image Name" (both of which are missing from this struct).

I propose that you take your logic here to split the image parts (which seems mostly fine to me as is) and move it into docker.ParseReference, and then embed "Repo" and "Image Name" into the ImageReference struct we already have. this won't change the existing usage of the struct around the codebase, but could be used later on if we wanted to, and is easier to find if we ever want to make changes to it. this isn't kaniko-specific logic, so it doesn't really belong in the kaniko package. WDYT?

nkubala · 2020-09-18T19:09:46Z

@DanielSel yep this is more what I was thinking! looks like the error you introduced in ParseReference is causing test failures though - maybe because this is being called before the default-repo is substituted on the image? I don't think we want to error here either way, probably just leave the field unpopulated. it should still give you the result you want when you're using Repo.

DanielSel · 2020-09-18T23:44:36Z

@nkubala Sorry, my mistake. Fixed now.

pkg/skaffold/build/cluster/kaniko_test.go

MarlonGamez · 2020-10-19T18:18:29Z

Hey @DanielSel, Thanks for continuously working on this! If you get the chance, would you be able to rebase and fix file conflicts? We should be able to merge this in after that :)

…not set explicitly). resolves GoogleContainerTools#3229

google-cla · 2020-10-22T16:26:23Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-10-22T17:38:42Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

tejal29 · 2020-10-22T17:39:22Z

@googlebot I consent

google-cla · 2020-10-22T17:39:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

DanielSel · 2020-10-23T07:54:03Z

Hey @DanielSel, Thanks for continuously working on this! If you get the chance, would you be able to rebase and fix file conflicts? We should be able to merge this in after that :)

Done! @MarlonGamez

nkubala

@DanielSel thanks for all your hard work on this PR, and sticking with us to see it through!

DanielSel requested a review from a team as a code owner July 24, 2020 11:18

DanielSel requested a review from briandealwis July 24, 2020 11:18

pull-request-size bot added the size/L label Jul 24, 2020

googlebot added the cla: yes label Jul 24, 2020

DanielSel changed the title ~~Adds environment variable expansion for kaniko builds (to address currently inconsistent behaviour between buildArgs and envs).~~ implement env variable expansion for kaniko builds Jul 24, 2020

DanielSel mentioned this pull request Jul 24, 2020

implement env variable expansion for kaniko builds #3974

Closed

DanielSel force-pushed the feature/kaniko-env-expansion branch 2 times, most recently from e1271b7 to 9296c71 Compare July 24, 2020 20:26

DanielSel force-pushed the feature/kaniko-env-expansion branch from 1f537f4 to 9296c71 Compare July 24, 2020 20:34

gsquared94 added the kokoro:run runs the kokoro jobs on a PR label Jul 28, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 28, 2020

DanielSel force-pushed the feature/kaniko-env-expansion branch from 9296c71 to 1d54f66 Compare July 28, 2020 09:08

gsquared94 added the kokoro:run runs the kokoro jobs on a PR label Jul 28, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 28, 2020

DanielSel force-pushed the feature/kaniko-env-expansion branch from 1d54f66 to 0f9f51c Compare July 29, 2020 11:30

DanielSel mentioned this pull request Jul 29, 2020

(minor) fix vendoring #4585

Merged

briandealwis added the kokoro:run runs the kokoro jobs on a PR label Jul 29, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 29, 2020

nkubala suggested changes Jul 30, 2020

View reviewed changes

DanielSel requested a review from nkubala August 10, 2020 14:28

DanielSel force-pushed the feature/kaniko-env-expansion branch from d08e704 to bc5dbc2 Compare September 11, 2020 09:12

DanielSel force-pushed the feature/kaniko-env-expansion branch from bc5dbc2 to 640f41f Compare September 15, 2020 12:16

nkubala self-assigned this Sep 17, 2020

briandealwis added the kokoro:force-run forces a kokoro re-run on a PR label Sep 17, 2020

kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Sep 17, 2020

nkubala reviewed Sep 17, 2020

View reviewed changes

DanielSel force-pushed the feature/kaniko-env-expansion branch 2 times, most recently from 0393706 to acdcf5f Compare September 18, 2020 22:17

tejal29 reviewed Sep 21, 2020

View reviewed changes

pkg/skaffold/build/cluster/kaniko_test.go Outdated Show resolved Hide resolved

tejal29 approved these changes Sep 21, 2020

View reviewed changes

DanielSel and others added 5 commits October 22, 2020 17:06

implement env variable expansion for kaniko builds

b27d2b8

add IMAGE_REPO, IMAGE_NAME, IMAGE_TAG to generated env variables (if …

6e86e61

…not set explicitly). resolves GoogleContainerTools#3229

use docker.ParseReference for kaniko env templating

44cfce4

add Repo and Name attributes to ImageReference

2907eca

Update pkg/skaffold/build/cluster/kaniko_test.go

2d0bdb7

DanielSel force-pushed the feature/kaniko-env-expansion branch from 7bc0be7 to 2d0bdb7 Compare October 22, 2020 16:26

google-cla bot added cla: no and removed cla: yes labels Oct 22, 2020

google-cla bot added cla: yes and removed cla: no labels Oct 22, 2020

nkubala approved these changes Oct 23, 2020

View reviewed changes

nkubala merged commit 0ebebdf into GoogleContainerTools:master Oct 23, 2020

fmotrifork mentioned this pull request Nov 2, 2020

skaffold 1.16.0 fishworks/fish-food#1036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement env variable expansion for kaniko builds #4557

implement env variable expansion for kaniko builds #4557

DanielSel commented Jul 24, 2020

codecov bot commented Jul 24, 2020 •

edited

Loading

DanielSel commented Jul 24, 2020

DanielSel commented Jul 27, 2020

gsquared94 commented Jul 28, 2020

DanielSel commented Jul 28, 2020

DanielSel commented Jul 29, 2020

nkubala Jul 30, 2020

DanielSel Aug 10, 2020

nkubala Sep 14, 2020

DanielSel Sep 15, 2020

nkubala Sep 17, 2020

DanielSel Sep 18, 2020

DanielSel commented Sep 11, 2020

nkubala left a comment

nkubala Sep 17, 2020

nkubala commented Sep 18, 2020

DanielSel commented Sep 18, 2020

MarlonGamez commented Oct 19, 2020

google-cla bot commented Oct 22, 2020

googlebot commented Oct 22, 2020

tejal29 commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

DanielSel commented Oct 23, 2020 •

edited

Loading

nkubala left a comment

implement env variable expansion for kaniko builds #4557

implement env variable expansion for kaniko builds #4557

Conversation

DanielSel commented Jul 24, 2020

codecov bot commented Jul 24, 2020 • edited Loading

Codecov Report

DanielSel commented Jul 24, 2020

DanielSel commented Jul 27, 2020

gsquared94 commented Jul 28, 2020

DanielSel commented Jul 28, 2020

DanielSel commented Jul 29, 2020

nkubala Jul 30, 2020

Choose a reason for hiding this comment

DanielSel Aug 10, 2020

Choose a reason for hiding this comment

nkubala Sep 14, 2020

Choose a reason for hiding this comment

DanielSel Sep 15, 2020

Choose a reason for hiding this comment

nkubala Sep 17, 2020

Choose a reason for hiding this comment

DanielSel Sep 18, 2020

Choose a reason for hiding this comment

DanielSel commented Sep 11, 2020

nkubala left a comment

Choose a reason for hiding this comment

nkubala Sep 17, 2020

Choose a reason for hiding this comment

nkubala commented Sep 18, 2020

DanielSel commented Sep 18, 2020

MarlonGamez commented Oct 19, 2020

google-cla bot commented Oct 22, 2020

googlebot commented Oct 22, 2020

tejal29 commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

DanielSel commented Oct 23, 2020 • edited Loading

nkubala left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 24, 2020 •

edited

Loading

DanielSel commented Oct 23, 2020 •

edited

Loading