Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kustomize aliases without newTag defined result in a nil pointer dereference and application crash #181

Closed
amalagaura opened this issue Mar 31, 2021 · 14 comments
Labels
bug Something isn't working
Milestone

Comments

@amalagaura
Copy link

amalagaura commented Mar 31, 2021

Describe the bug
We wish to have a generic kustomization.yaml without an image tag. We wish to use the argocd image updater to set the image tags. We use a generic image name called image. We first tried without the image alias in kustomization.yaml. In that case the image updater fails to update or find the image alias. Apparently Kustomize needs to do a first pass to change the image alias to the final image. So then we add the image newName. But without a newTag the application crashes.

To Reproduce
A kustomization.yaml with the contents:

images:
- name: image
  newName: some-registry.com/some-image

with Argo Application with annotations:

argocd-image-updater.argoproj.io/image-list: image=some-registry.com/some-image
argocd-image-updater.argoproj.io/image.kustomize.image-name: some-registry.com/some-image
argocd-image-updater.argoproj.io/image.update-strategy: latest

This creates a panic and the application crashes

Expected behavior
Image updater should allow newTag to be empty because it is valid according to Kustomize. Kustomize applies latest as the image tag.

Additional context
We use a generic image name in our deployment.yaml files and rely on kustomize image aliases to set the correct image. We are also not clear with the argocd-image-updater.argoproj.io/image.kustomize.image-name is supposed to designate.

Version
v99.9.9+ccd78aa

Logs

 panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x196e453]
goroutine 22 [running]:
github.com/argoproj-labs/argocd-image-updater/pkg/tag.(*ImageTag).IsDigest(...)
        /src/argocd-image-updater/pkg/tag/tag.go:96
github.com/argoproj-labs/argocd-image-updater/pkg/tag.(*ImageTag).Equals(...)
        /src/argocd-image-updater/pkg/tag/tag.go:102
github.com/argoproj-labs/argocd-image-updater/pkg/argocd.UpdateApplication(0xc000c73b78, 0xc000c45310, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
        /src/argocd-image-updater/pkg/argocd/update.go:252 +0xc43
main.runImageUpdater.func1(0xc000193e50, 0xc0001a57a0, 0xc000c35ff0, 0xc000c45310, 0xc000050ae0, 0x1, 0xc000c35ff4, 0xc00071fd40, 0x14, 0xc0007253b0, ...)
        /src/argocd-image-updater/cmd/main.go:160 +0x23c
created by main.runImageUpdater
        /src/argocd-image-updater/cmd/main.go:147 +0xa3d
@amalagaura amalagaura added the bug Something isn't working label Mar 31, 2021
@amalagaura
Copy link
Author

amalagaura commented Mar 31, 2021

My main confusion in all this is that the newTag must exist. I can eliminate the panic and runtime error by just setting a non existent tag like newTag: latest. But then our application is always in an error state because we don't have latest as a valid tag.

Does the newTag need to exist and then it gets overridden by the image updater? When I check ArgoCD the parameters are not getting overridden by Image Updater. I only see kustomize images in the parameters list.

@jannfis
Copy link
Contributor

jannfis commented Apr 1, 2021

Thanks @amalagaura, this is indeed a bug.

To explain the background: The Image Updater inspects the Argo CD Application for running images, and does expect a tag in that information. If you don't set a tag in your kustomization.yaml or in your manifests, and apply this to the cluster, the image information just doesn't have a tag attached to it. Probably, any pod set up this way would probably be in an ImagePullBackoff error state as well.

I have just committed #182 to master which fixes this issue.

@amalagaura
Copy link
Author

Thanks @jannfis this is helpful. From a bootstrap point of view it would be nice to set the image information only in one place. But I understand how the purpose of the image updater is to refine the restrictions on the images set from the manifests and not necessarily to override completely. We are doing a complete override because we are trying to set image information only in one place in order to have very generic templates that teams override in one kustomization.yaml file only.
Thank you for your assistance.

@jannfis
Copy link
Contributor

jannfis commented Apr 1, 2021

Yes, I think I understand the use-case. The behavior of Image Updater supports that now with #182 merged,

When you now apply manifests without a tag, the pod will probably not spin up. However, when Image Updater comes around, it treats the non-existing tag as reason for an update, and sets the image's tag to the one it considers latest according to your update strategy.

Please give it a spin and let me know whether it worked out for you.

@amalagaura
Copy link
Author

amalagaura commented Apr 1, 2021

@jannfis I tried from master and it seems to be working without the newTag now. So this bug report should be solved.

But I am still confused regarding this. I tried removing the images section completely from the kustomization.yaml but it failed with the error message "image some-registry/some-where/image seems not to be live in this application" so it means it does not do any sort of replacement like kustomize does.

So it did not search for the alias which was image. It searched for the full form. What does alias in the image updater mean for kustomize resources? Kustomize changes expands an image and maybe it is my confusion that I thought that is an "alias". So when we say image=some-registry.com/some-image does the image only refer to an identifier in the subsequent annotations like argocd-image-updater.argoproj.io/image.update-strategy? In which case it has no relation to the kustomize image name? I don't understand why the docs says alias is mandatory in the section for kustomize.

Thank you

@jannfis
Copy link
Contributor

jannfis commented Apr 6, 2021

I don't understand why the docs says alias is mandatory in the section for kustomize.

The docs are outdated and wrong, sorry for that. See #184 for a similar discussion. So, to wrap it up

  • The image alias is mandatory if you wish to set additional configuration for any image
  • If you wish to use the original image name for Kustomize (e.g. oldimage=some/new/image), use the argocd-image-updater.argoproj.io/<image_alias>.kustomize.image-name: <image_name> annotation (which is unfortunately underdocumented).

@amalagaura
Copy link
Author

amalagaura commented Apr 8, 2021

Apologies in advance. I read through all the discussions but I am still not able to get things working the way I think they should. I tried many different combinations but I don't think I have the configuration correct.

If I understand correctly, adding the the <image_alias>.kustomize.image-name will create the ArgoCD parameter override. I am trying to verify this in the ArgoCD web interface in the parameters tab. But I only see the parameter image=image overrides coming through, no matter what I set for that that annotation.

Maybe I am trying to simplify this too far but I saw the other comment did not have an image override in the kustomization.yaml so I think the follow should work:

Deployment.yaml

    spec:
      containers:
      - image: image
        name: our-container
argocd-image-updater.argoproj.io/image-list: image=some/new/image
argocd-image-updater.argoproj.io/image.update-strategy: latest
argocd-image-updater.argoproj.io/image.kustomize.image-name: image

I am hopelessly confusing myself at this point between aliases and images but the nginx example also made it difficult. Do I need to introduce something like the following?

argocd-image-updater.argoproj.io/image-list: somealias=some/new/image
argocd-image-updater.argoproj.io/somealias.update-strategy: latest
argocd-image-updater.argoproj.io/somealias.kustomize.image-name: image

But I tried this too with no change to the ArgoCD parameters. The parameters are not getting updated in the ArgoCD web UI beyond image=image. I have not enabled the Git writeback because I wanted to get the the basic functionality working first.

It is not a big deal to set the image override in kustomize but I feel like I am missing something here.

@jannfis
Copy link
Contributor

jannfis commented Apr 8, 2021

Hey @amalagaura - I can understand your confusion, as I found myself a little confused too :)

But I think there's indeed a bug - or a not yet existing feature - in the update loop for this special scenario, which tbh, I have not considered yet. So, this is how the update loop currently works:

  • Argo CD Image Updater will take a look at the container images actually defined in the cluster (e.g. in a pod's .spec.containers definition, exported in the .status field of an Argo CD Application CR)
  • The image-list annotation specifies the name of the container images that Image Updater should look for. In this case, it is some/new/image
  • Since this image is not defined yet in the cluster (it is still just image), the Image Updater will ignore it

To implement this and overcome this almost "chicken-egg" problem, we would to need to also consider scanning for the image defined in somealias.kustomize.image-name, which in this case would be image.

@jannfis jannfis added this to the v0.10.0 milestone Apr 8, 2021
@iamnoah
Copy link
Contributor

iamnoah commented Apr 9, 2021

@jannfis sorry to pile on, but I've just run into the "seems not to be live in this application" message trying to update a PodTemplate. Is there a way around the container not being in the status?

@jannfis
Copy link
Contributor

jannfis commented Apr 9, 2021

Hey, we were just discussing a similar requirement in #187

jannfis pushed a commit that referenced this issue Apr 13, 2021
…189)

* allow image updates to be forced if container not present #181

* explain new code, refactor image parsing and test
@amalagaura
Copy link
Author

amalagaura commented Jun 21, 2021

Thanks for our attention @jannfis. This issue has resurfaced in the 0.9.x series.

time="2021-06-15T16:31:22Z" level=debug msg="Processing application webui"
time="2021-06-15T16:31:22Z" level=debug msg="Considering this image for update" alias=image application=webui image_name=plan/webui image_tag="<nil>" registry=registry.com
time="2021-06-15T16:31:22Z" level=debug msg="Using no version constraint when looking for a new tag" alias=image application=webui image_name=plan/webui image_tag="<nil>" registry=registry.com
time="2021-06-15T16:31:22Z" level=debug msg="found 2 from 2 tags eligible for consideration" image=registry.com/plan/webui

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x196ca5e]
 goroutine 47045 [running]:
github.com/argoproj-labs/argocd-image-updater/pkg/argocd.UpdateApplication(0xc000a57b80, 0xc0014a2ea0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
/src/argocd-image-updater/pkg/argocd/update.go:232 +0xb1e
main.runImageUpdater.func1(0xc0014981e0, 0xc000128360, 0xc00146531f, 0xc0014a2ea0, 0xc0003de840, 0x1, 0xc001465330, 0xc0003c8d00, 0x1a, 0xc00031b4e0, ...)
/src/argocd-image-updater/cmd/main.go:158 +0x21f
created by main.runImageUpdater

/src/argocd-image-updater/cmd/main.go:146 +0xa3d

@jannfis
Copy link
Contributor

jannfis commented Jun 22, 2021

Hey @amalagaura, this has never been fixed in the 0.9.x release branch, I think. The change was only made in the master branch so far.

That being said, the release 0.10 is overdue and I really should take some time to do the release. This will also contain the fix, along with a couple of new features.

@jannfis
Copy link
Contributor

jannfis commented Dec 16, 2021

Is this still reproducible with v0.11.0?

@jannfis jannfis modified the milestones: v0.11.0, v0.12.0 Dec 16, 2021
@jannfis
Copy link
Contributor

jannfis commented Mar 14, 2022

Feel free to re-open this issue if it the problem persists.

@jannfis jannfis closed this as completed Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants