search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
11.05k stars 2.25k forks source link

Duplicated `tagSuffix` with `images` #4814

Open mathieu-benoit opened 2 years ago

mathieu-benoit commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

/triage triage/accepted

k8s-ci-robot commented 2 years ago

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

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/4814#issuecomment-1268946215): >/triage triage/accepted Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
annasong20 commented 2 years ago

/triage/accepted

annasong20 commented 2 years ago

/triage accepted

k8s-triage-robot commented 1 year ago

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:

You can:

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

/lifecycle stale

k8s-triage-robot commented 1 year ago

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:

You can:

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

/lifecycle rotten

Serializator commented 1 year ago

/remove-lifecycle rotten

DiptoChakrabarty commented 1 year ago

I will pick this up /assign

DiptoChakrabarty commented 1 year ago

hey @annasong20 I have implemented a PR which just makes the tagsuffix field empty before jumping to the Filter https://github.com/kubernetes-sigs/kustomize/blob/76f8d2828bd72abf7de3dbe31027af2603fa9afa/plugin/builtin/imagetagtransformer/ImageTagTransformer.go#L36

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 commented 1 year ago

@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 commented 1 year ago

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.