Duplicated tagSuffix with images

[mathieu-benoit]

When using the tagSuffix for updating the images in my kustomization.yaml file, the tagSuffix is actually duplicated on the final rendering.

Here is my test to reproduce it:

cat <<EOF> deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: deploysuffix
spec:
  template:
    spec:
      containers:
      - image: redis:6.2.6
        name: redis
EOF
cat <<EOF> kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- deployment.yaml
images:
- name: redis
  tagSuffix: -alpine
EOF
kubectl kustomize .

Output:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: deploysuffix
spec:
  template:
    spec:
      containers:
      - image: redis:6.2.6-alpine-alpine
        name: redis

kubectl version:

Client Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.1", GitCommit:"e4d4e1ab7cf1bf15273ef97303551b279f0920a9", GitTreeState:"clean", BuildDate:"2022-09-14T19:49:27Z", GoVersion:"go1.19.1", Compiler:"gc", Platform:"linux/amd64"}
Kustomize Version: v4.5.7

Wondering how this += behaves: https://github.com/kubernetes-sigs/kustomize/blob/master/api/filters/imagetag/updater.go#L51, seems to be called twice?

And apparently this unit test https://github.com/kubernetes-sigs/kustomize/blob/master/api/filters/imagetag/imagetag_test.go#L843 is not catching this?

[annasong20]

I confirm that I see the same behavior running kustomize v4.5.7 on my mac. I also agree that this is a bug. We should fix it and add the appropriate regression test for ImageTagTransformer.

Thanks, @mathieu-benoit, for including the line numbers! The problem lies in the fact that the function enclosing the line you linked, SetImageValue() is called twice, first by the LegacyFilter and second by the normal Filter. Only tagSuffix sees this behavior because it's the only field in that function operated on by a +=, as @mathieu-benoit pointed out.

[annasong20]

/triage triage/accepted

[k8s-ci-robot]

@annasong20: The label(s) triage/triage/accepted cannot be applied, because the repository doesn't have them.

In response to this:

/triage triage/accepted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[annasong20]

/triage/accepted

[annasong20]

/triage accepted

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[Serializator]

/remove-lifecycle rotten

[DiptoChakrabarty]

I will pick this up
/assign

[DiptoChakrabarty]

hey @annasong20 I have implemented a PR which just makes the tagsuffix field empty before jumping to the Filter

kustomize/plugin/builtin/imagetagtransformer/ImageTagTransformer.go

Line 36 in 76f8d28

return m.ApplyFilter(imagetag.Filter{

HoweverI wanted to discuss if there were better methods to tackle this and wanted some info , I was experimenting with the ImageTagTransformer
I noticed that m.ApplyFilter(imagetag.LegacyFilter) and m.ApplyFilter(imagetag.Filter) call the same Filter function and removing the m.ApplyFilter(imagetag.LegacyFilter) piece of code produces the same results (tested with newName, digest and tagSuffix)

I am guessing the LegacyFilter is used to validate the image tag and then in case of no errors the filter is used to apply the new tag in the fieldspecs , but in this case the filter is invoked twice

[annasong20]

@DiptoChakrabarty Thank you for raising this concern! We definitely should attend to the legacy filter.

My understanding is that the non-legacy filter was supposed to be the sole ImageTagTransformer filter. However, the legacy filter was added in #2931 because the non-legacy filter requires FieldSpecs when the transformer is invoked via the Kustomize transformers field. See the linked issue for more details.

removing the m.ApplyFilter(imagetag.LegacyFilter) piece of code produces the same results (tested with newName, digest and tagSuffix)

As much as I'd love for this to be true :), when I commented out the legacy filter, tests like ArbitraryPath failed for me. You can see the complete list of failed tests by running all the Kustomize presubmit tests. The failed tests reflect why we need the legacy filter - to transform images in the absence of FieldSpecs.

[annasong20]

After discussion with @natasha41575, we agree that in the long run we want to deprecate the legacy filter. However, before we can do so, we need to pass the default FieldSpecs to the non-legacy filter when the ImageTagTransformer is invoked by the Kustomize transformers field. In other words, this issue is blocked by #4404.

Until then, as a temporary fix to the current issue, we can add a check to the non-legacy filter to not apply the image transformations if the field is covered by the legacy filter. Feel free brainstorm your own solutions! Because this solution is a hack, we should comment that it is temporary until we can deprecate the legacy filter.