Fix: Change condition for the behaviour when --no-push=true without setting --destinations

[JeromeJu]

Fixes #1613

Description

This commit changes the condition check for the behavior when --no-push is set to true while destinations are needed. Prior this change, users would have to set destinations even when noPush option is set to true.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes unit tests
• [n/a] Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

• The code flow looks good.
• Unit tests and or integration tests added.

[aaron-prindle]

In testing with this PR, it seems that when --no-push and --tar-path are set w/ this PR, there is no longer an error message but the tar is not generated. This likely not the desired outcome for users setting --no-push and --tar-path, they will expect a tar generated at the path

test command:

    docker run \
        -v "$HOME"/.config/gcloud:/root/.config/gcloud \
        -v "$context":/workspace \
        gcr.io/kaniko-project/executor:latest \
        --dockerfile "${dockerfile}" --context dir:///workspace/ \
        --cache="${cache}" --no-push --tarPath=/image.tar

output @ HEAD (w/o this PR)

INFO[0107] Applying label retention=prune               
error pushing image: must provide at least one destination when tarPath is specified

# ^ error message is printed, kaniko exits with failure and no tar is generated

output w/ this PR

INFO[0109] LABEL retention="prune"                      
INFO[0109] Applying label retention=prune               
INFO[0109] Skipping push to container registry due to --no-push flag 

# ^ kaniko exits with success BUT NO TAR IS GENERATED <-- undesirable UX

This makes sense as with this change, we short circuit push logic here:

kaniko/pkg/executor/push.go

Line 210 in f0c2ff4

return nil

Which means we don't get to the tarPath logic here:

kaniko/pkg/executor/push.go

Lines 213 to 227 in f0c2ff4

	if opts.TarPath != "" {
	tagToImage := map[name.Tag]v1.Image{}
	if len(destRefs) == 0 {
	return errors.New("must provide at least one destination when tarPath is specified")
	}
	for _, destRef := range destRefs {
	tagToImage[destRef] = image
	}
	err := tarball.MultiWriteToFile(opts.TarPath, tagToImage)
	if err != nil {
	return errors.Wrap(err, "writing tarball to file failed")
	}
	}

Fixing this is somewhat non-trivial as currently the tarPath logic requires usage of a map that is empty when no destination is set:

kaniko/pkg/executor/push.go

Line 221 in f0c2ff4

tagToImage[destRef] = image

While the logic here could be tweaked to not require a destRef mapping, it is additionally complicated as currently the tar file kaniko creates via --tar-path has a manifest.json that has RepoTags values set to the --destination value(s). Will need to check if this is an optional field (so we can leave it empty and that is fine) or if a dummy value could be supplied to keep the schema valid.

manifest.json in the resulting filename.tar file (eg: --tar-path=filename.tar)

[{"Config":"sha256:05810c0cff6417cb92934f04e341d790ecdb01a4ffcff6aa7f266463067c34ed","RepoTags":["gcr.io/aprindle-test-cluster/kaniko-test:latest"],"Layers":["396c31837116ac290458afcb928f68b6cc1c7bdd6963fc72f52f365a2a89c1b5.tar.gz","4b77badfe5220d7b5256b7208fee352615c948cd252ee0d4b909ca1d531b7a97.tar.gz"]}]a

From this, the simplest fix would likely be to modify the kaniko logic s.t. when --tar-path is used w/o --destination, a "dummy" destination is set. This would allow for the currently necessary tagToImage[destRef] = image line to work as expected as a dummy destRef would exist in the case none were set. In that case the original statement ordering would likely be desirable

[JeromeJu]

Thanks @aaron-prindle , I'm trying to get a better understanding of different combinations of opts here, wondering if the following makes sense:

opts	Case i	Case ii	Case iii
--no-push	True	True	False
--destinations	Empty	Empty	Empty
--tar-path	Not empty	Empty	Either empty or not
Behaviour	Generate a tar at TarPath, use the dummy destRef workaround	Short circuit in DoPush	Return the previous Error(error pushing image: must provide at least one destination when tarPath is specified) where destinations are missing

[aaron-prindle]

@JeromeJu

Thanks @aaron-prindle , I'm trying to get a better understanding of different combinations of opts here, wondering if the following makes sense:

opts Case i Case ii Case iii
--no-push True True False
--destinations Empty Empty Empty
--tar-path Not empty Empty Either empty or not
Behaviour Generate a tar at TarPath, use the dummy destRef workaround Short circuit in DoPush Return the previous Error(error pushing image: must provide at least one destination when tarPath is specified) where destinations are missing

Yes +1, the behaviour for the three cases you outlined is exactly what is desired

[aaron-prindle]

Just tried it and it works great! I left one small comment regarding a test case name being a dupe. After that is resolved we should be good to merge! 🎊

As expected the dummy value is present in the manifest.json

aprindle@aprindle-ssd ~/kaniko-tar-path/kaniko cat manifest.json 
[{"Config":"sha256:fcbf14c4f26da0e408cd84d3ff07a09db17eb56dcc9ff3c4deaf7a8f98b90ce7","RepoTags":["docker.io/unset-repo/unset-image-name:latest"],"Layers":["396c31837116ac290458afcb928f68b6cc1c7bdd6963fc72f52f365a2a89c1b5.tar.gz","ddd8d01266dd2012b35dd4adfea327d1dd5cbf1431e02a28d32fa6d709b5eff8.tar.gz"]}]aprindle@aprindle-ssd

and as discussed the dummy value existing in the tar should not be an issue for users ommitting the --destination flag. User's who rely on a specific value veing in the manifest.json for RepoPaths can still set the now optional --destination as they did previously

[JeromeJu]

Thanks for the review @aaron-prindle . Updated.

[aaron-prindle]

LGTM! Thanks for the PR @JeromeJu! 🎊