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

fix : Images not updated if registry or repository is different with same version #194

Merged
merged 2 commits into from Apr 26, 2021

Conversation

iamnoah
Copy link
Contributor

@iamnoah iamnoah commented Apr 14, 2021

Given this config:

imageList: foo=gcr.io/bar:>=1.0.0
foo.kustomize.image-name=bar

If my application is running bar:1.0.0 I'd expect an update to gcr.io/bar:1.0.0, however that was not happening because only tag was considered in the difference. To fix that, I had to add an extra "KustomizeImage" to also be considered for matching purposes, and add registry and name to the update check if this kustomize image is present.

I've added an additional test to show that once updated there should be no more updates (no endless loop of updates, etc.)

This is builds off of #192

@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #194 (d49f396) into master (37433e4) will decrease coverage by 0.33%.
The diff coverage is 59.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
- Coverage   66.99%   66.66%   -0.34%     
==========================================
  Files          20       20              
  Lines        1503     1515      +12     
==========================================
+ Hits         1007     1010       +3     
- Misses        405      412       +7     
- Partials       91       93       +2     
Impacted Files Coverage Δ
pkg/image/image.go 85.39% <0.00%> (-4.02%) ⬇️
pkg/image/options.go 71.42% <0.00%> (-2.44%) ⬇️
pkg/argocd/update.go 69.72% <50.00%> (-0.51%) ⬇️
pkg/argocd/argocd.go 63.63% <100.00%> (+0.35%) ⬆️

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 37433e4...d49f396. Read the comment docs.

Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR @iamnoah and sorry, but I'm having a hard time understanding the reasoning behind it.

The <alias>.kustomize.image-name is currently used to define which Kustomize image to override. With this change, the value of that annotation would be included to be checked for new versions, despite we want to check for updates to the overriden image? Why would we also want to check for the original image here?

Again sorry for not understanding it, but could you maybe elaborate a little on the use case?

results := make(image.ContainerImageList, len(splits))
for i, s := range splits {
results[i] = image.NewFromIdentifier(strings.TrimSpace(s))
func parseImageList(annotations map[string]string) *image.ContainerImageList {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand that moving complexity into this method makes sense, I think we should not make the method taking the app's annotation as an argument here. Main reason for that is, we are most likely going away from using annotations for configuration anytime soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with that, but I think I have demonstrated here that for non-trivial kustomize use cases, more config than just the image list is necessary, so some complex object or multiple parameters will have to be passed in.

@@ -91,8 +92,18 @@ func (img *ContainerImage) WithTag(newTag *tag.ImageTag) *ContainerImage {
return nimg
}

func (img *ContainerImage) DiffersFrom(other *ContainerImage, checkVersion bool) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if we should revert the logic here and use Equals() as a method name, because we have a similar method for ImageTag as well.

@iamnoah
Copy link
Contributor Author

iamnoah commented Apr 19, 2021

could you maybe elaborate a little on the use case?

Sure. The issue is having a existing deployment like this:

spec:
  template:
   spec:
    containers:
      - image: bar:latest

If I wasn't using image updater, I would add a kustomization like this:

kind: Kustomization
images:
- name: bar
  newName: gcr.io/bar
  newTag: 0.1.0

That would update the image to gcr.io/bar:0.1.0.

But .argocd-source.yaml will override anything I put in images with this:

images:
- bar:0.1.0

When what I wanted was:

images:
- bar=gcr.io/bar:0.1.0

So I am forced to tell image updater about the re-mapping via kustomize-name.

Why does this happen? Because when looking at a deployed app, image updater will check applicationImages.ContainsImage(applicationImage, false). There is no match because gcr.io/bar isn't bar.

So we need a different ContainerImage to check. What if we just look for bar instead, since that is what kustomize-name says to do?

  • If the application doesn't have an override yet, we will find bar and override it with gcr.io/bar as long as the tag is different. If they are the same tag, nothing happens.
  • If the image is overriden then looking for bar won't work because now the application contains only gcr.io/bar!

The problem is that the current state of application images can't tell you anything about what was overriden, and if we do fix the match, but have the same tags, no update happens. So we need two fixes:

  • Match either kustomize-name or the override name to handle both the case where the image has not been update and the one where it has.
  • If a kustomize-name is present, check if the desired registry is correct.

Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your thorough explanation @iamnoah and sorry that it took me a little longer to review, it was a busy week so far.

I have a couple of change requests, please see below.

pkg/argocd/argocd.go Outdated Show resolved Hide resolved
pkg/argocd/argocd.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now!

Thank you for your contribution and your patience, @iamnoah!

@jannfis jannfis merged commit 0267634 into argoproj-labs:master Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants