New tagging mechanism by dgageot · Pull Request #1482 · GoogleContainerTools/skaffold

dgageot · 2019-01-17T22:44:15Z

This completely rewrites the image tagging mechanism. Here's how it'll work:

Skaffold computes a tag for an image BEFORE it's built. This removes a lot of complexity.
Images pushed to registries will always be referenced by name, tag and digest in kubernetes manifests. That is to make sure the wrong image is not used and to remove this complexity from tagger.
Images used locally can't be referenced by digest (because we don't have it) or imageID (because k8s doesn't support that). So images used locally will be tagged with their imageID to make sure we produce a unique tag that can be used in kubernetes manifests
taggers (git, sha256, envTemplate, datetime) are all modified so that they can't use the image digest/imageID. It makes their code simpler but breaks the envTemplate tagger as it is.

codecov-io · 2019-01-17T22:53:09Z

Codecov Report

Merging #1482 into master will decrease coverage by 0.03%.
The diff coverage is 41.89%.

@@            Coverage Diff             @@
##           master    #1482      +/-   ##
==========================================
- Coverage   46.92%   46.88%   -0.04%     
==========================================
  Files         119      118       -1     
  Lines        5072     5042      -30     
==========================================
- Hits         2380     2364      -16     
+ Misses       2458     2446      -12     
+ Partials      234      232       -2

Impacted Files	Coverage Δ
pkg/skaffold/build/local/types.go	`0% <ø> (ø)`	⬆️
pkg/skaffold/build/gcb/cloud_build.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/deploy/helm.go	`61.47% <0%> (-3.37%)`	⬇️
pkg/skaffold/build/local/bazel.go	`30.3% <0%> (-0.47%)`	⬇️
pkg/skaffold/build/kaniko/kaniko.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/build/kaniko/run.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/build/local/jib_maven.go	`26.66% <0%> (+1.66%)`	⬆️
pkg/skaffold/build/local/jib_gradle.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/build/tag/git_commit.go	`79.16% <100%> (-5.21%)`	⬇️
pkg/skaffold/build/tag/sha256.go	`40% <100%> (-26.67%)`	⬇️
... and 9 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 bb700a0...5cee689. Read the comment docs.

balopat

I'm going to keep reviewing but - as we are in beta, we have to mark DIGEST related functionality as deprecated first, announce it, and then we can remove it in 3 months.

balopat · 2019-01-29T22:30:12Z

pkg/skaffold/build/tag/env_template.go

 // NewEnvTemplateTagger creates a new envTemplateTagger
 func NewEnvTemplateTagger(t string) (Tagger, error) {
+	if strings.Contains(t, "{{.DIGEST}}") {
+		return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be apppended to image tags")


Suggested change

return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be apppended to image tags")

return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be appended to image tags")

This has been fixed

balopat · 2019-01-29T22:31:08Z

pkg/skaffold/build/tag/env_template.go


 // NewEnvTemplateTagger creates a new envTemplateTagger
 func NewEnvTemplateTagger(t string) (Tagger, error) {
+	if strings.Contains(t, "{{.DIGEST}}") {


This wouldn't work if it has a space or do we normalize it somewhere? e.g {{ .DIGEST }}

This has been fixed

balopat · 2019-01-29T22:34:54Z

Otherwise everything looks good.
My recommendation would be to have

DIGEST = DIGEST
DIGEST_ALGO = sha256
DIGEST_HEX= DIGEST

and whenever these template params are used, we would print out a deprecation warning for 3 months.
WDYT?

priyawadhwa

For the build plugins to work, I think the tagger would need to be removed entirely from the build interface

Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*latest.Artifact) ([]Artifact, error)

would need to be

Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) ([]Artifact, error)

where maybe we store a tag in each artifact.

Would it be possible to resolve tagging before Build is called? (I totally realize that this could be a very big refactor so it doesn't have to happen in this PR, but I don't think there's anyway around it in the long run for the plugins).

cc @balopat

dgageot · 2019-01-30T11:36:19Z

@balopat how do you see the deprecation warning working?

dgageot · 2019-02-04T10:44:45Z

@priyawadhwa @balopat {{.DIGEST}} and friends should be handled better now.

In cases where it was used as an image name suffix (like for JenkinX), a deprecation warning is printed and this suffix is ignored because it'll be done automatically now.
In other cases, an error is printed and can't be recovered.

nkubala

one small typo but deprecation logic LGTM

nkubala · 2019-02-04T19:43:57Z

pkg/skaffold/build/tag/env_template.go

+	case strings.Contains(tag, "[DEPRECATED_DIGEST]") ||
+		strings.Contains(tag, "[DEPRECATED_DIGEST_ALGO]") ||
+		strings.Contains(tag, "[DEPRECATED_DIGEST_HEX]"):
+		return "", errors.New("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags")


s/apppended/appended

Let me fix that

nkubala

I think I'm cool with this, let's monitor for a few days after it gets merged to make sure we're not breaking people in ways we didn't expect

balopat · 2019-02-05T01:25:14Z

pkg/skaffold/build/tag/env_template.go

+	case strings.Contains(tag, "[DEPRECATED_DIGEST]") ||
+		strings.Contains(tag, "[DEPRECATED_DIGEST_ALGO]") ||
+		strings.Contains(tag, "[DEPRECATED_DIGEST_HEX]"):
+		return "", errors.New("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags")


I feel strongly about not throwing an error here either, we should not break user workflows.

Just a warning is enough, and we can leave DEPRECATED_* (without the brackets) in the image name. A seamless upgrade allows for deferred action on the user's side. Image name uniqueness won't be a problem as we are adding the @digest suffix anyway.

I am okay to take the blame for the messy image names, if they become an issue rather than facing angry users who have to change their skaffold.yaml-s without a notice, even though they were promised a stable beta!

This is fixed now

Signed-off-by: David Gageot <david@gageot.net>

Actually, we use: + imageID when the image is built locally + tag@sha256:abcabc when the image was pushed We assume here that taggers never append the digest to the tag themselves. (Which is not true) Signed-off-by: David Gageot <david@gageot.net>

Signed-off-by: David Gageot <david@gageot.net>

Pass the final tag to the builders and let them build and push that tag directly.

Signed-off-by: David Gageot <david@gageot.net>

Where digest is not available and imageID can’t be used in k8s manifests. Signed-off-by: David Gageot <david@gageot.net>

Fix GoogleContainerTools#1349 Signed-off-by: David Gageot <david@gageot.net>

Signed-off-by: David Gageot <david@gageot.net>

balopat · 2019-02-05T15:45:19Z

As dicussed let's change the "[" "]" chars to "_" so that the image names are compatible with docker, then we can merge it!

dgageot · 2019-02-05T16:04:43Z

Woot! All green 💚 🍏 📗
Let's merge!

dgageot added the wip label Jan 17, 2019

dgageot requested review from balopat, nkubala and r2d4 as code owners January 17, 2019 22:44

dgageot force-pushed the new-tagging branch 3 times, most recently from 7c8af68 to d925153 Compare January 24, 2019 05:42

googlebot added the cla: yes label Jan 24, 2019

dgageot force-pushed the new-tagging branch 7 times, most recently from 151c9d5 to ba3967c Compare January 29, 2019 05:38

balopat suggested changes Jan 29, 2019

View reviewed changes

balopat reviewed Jan 29, 2019

View reviewed changes

priyawadhwa reviewed Jan 29, 2019

View reviewed changes

priyawadhwa added this to the 1.0.0 milestone Jan 30, 2019

dgageot force-pushed the new-tagging branch 3 times, most recently from 515bf63 to d0c29e1 Compare January 31, 2019 06:59

priyawadhwa mentioned this pull request Feb 1, 2019

Builder plugin for docker in GCB #1577

Merged

dgageot force-pushed the new-tagging branch 2 times, most recently from 4be4ad6 to b77fab4 Compare February 4, 2019 08:57

nkubala reviewed Feb 4, 2019

View reviewed changes

dgageot force-pushed the new-tagging branch from bd56401 to 62b90c9 Compare February 4, 2019 20:02

nkubala approved these changes Feb 4, 2019

View reviewed changes

balopat reviewed Feb 5, 2019

View reviewed changes

dgageot force-pushed the new-tagging branch 2 times, most recently from 57f8005 to 0eb8ae0 Compare February 5, 2019 09:07

dgageot removed the wip label Feb 5, 2019

dgageot added 3 commits February 5, 2019 10:29

FakeApi produces more realistic imageID and digest

ed428eb

Signed-off-by: David Gageot <david@gageot.net>

Remove small optimization

fc25ff5

Signed-off-by: David Gageot <david@gageot.net>

dgageot force-pushed the new-tagging branch from 0eb8ae0 to a3cbf91 Compare February 5, 2019 09:29

dgageot added 9 commits February 5, 2019 11:52

Let’s assume taggers don’t need the digest

942b092

Signed-off-by: David Gageot <david@gageot.net>

Compute the tag before the build

b2de9dd

Signed-off-by: David Gageot <david@gageot.net>

Remove retagging

30e6794

Pass the final tag to the builders and let them build and push that tag directly.

Don’t pass the digest to taggers

fd24276

Signed-off-by: David Gageot <david@gageot.net>

Fix the case of the local dev

923f4de

Where digest is not available and imageID can’t be used in k8s manifests. Signed-off-by: David Gageot <david@gageot.net>

Don’t use local Docker to push Bazel images

31bde87

Fix GoogleContainerTools#1349 Signed-off-by: David Gageot <david@gageot.net>

Docker build should push images itself

66eb95f

Signed-off-by: David Gageot <david@gageot.net>

Fix test

1b02585

Signed-off-by: David Gageot <david@gageot.net>

Ignore proper usage of {{.DIGEST}} and print warning

1f9f4cb

Signed-off-by: David Gageot <david@gageot.net>

dgageot force-pushed the new-tagging branch from a3cbf91 to 1f9f4cb Compare February 5, 2019 10:52

balopat approved these changes Feb 5, 2019

View reviewed changes

Produce valid image names

5cee689

Signed-off-by: David Gageot <david@gageot.net>

dgageot merged commit 8efe309 into GoogleContainerTools:master Feb 5, 2019

elisiano mentioned this pull request May 21, 2019

Add support for unique-per-image tag (was DIGEST_HEX) #2162

Closed

nkubala mentioned this pull request Jun 19, 2019

sha256 tagger doesn't actually compute checksum #2290

Closed

morozov mentioned this pull request Aug 27, 2019

sha256 tagpolicy always produces latest #2713

Closed

balopat mentioned this pull request Sep 10, 2019

[proposal] Improve taggers #2804

Merged

	return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be apppended to image tags")
	return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be appended to image tags")

Conversation

dgageot commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

balopat Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

dgageot Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

balopat Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

dgageot Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

balopat commented Jan 29, 2019

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

dgageot commented Jan 30, 2019

Uh oh!

dgageot commented Feb 4, 2019

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

nkubala Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

dgageot Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

balopat Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgageot Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

balopat commented Feb 5, 2019

Uh oh!

dgageot commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dgageot commented Jan 17, 2019 •

edited

Loading

codecov-io commented Jan 17, 2019 •

edited

Loading

balopat Feb 5, 2019 •

edited

Loading