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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 26, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4113ea
update image even if only difference is the registry
iamnoah Apr 14, 2021
d49f396
refactor: use annotation methods which are more consistent
iamnoah Apr 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 18 additions & 14 deletions pkg/argocd/argocd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,12 @@ func FilterApplicationsForUpdate(apps []v1alpha1.Application, patterns []string)

// Check whether application has our annotation set
annotations := app.GetAnnotations()
if updateImage, ok := annotations[common.ImageUpdaterAnnotation]; !ok {
if _, ok := annotations[common.ImageUpdaterAnnotation]; !ok {
log.Tracef("skipping app '%s' of type '%s' because required annotation is missing", app.GetName(), app.Status.SourceType)
continue
} else {
log.Tracef("processing app '%s' of type '%v'", app.GetName(), app.Status.SourceType)
imageList := parseImageList(updateImage)
imageList := parseImageList(annotations)
appImages := ApplicationImages{}
appImages.Application = app
appImages.Images = *imageList
Expand All @@ -183,11 +183,17 @@ func FilterApplicationsForUpdate(apps []v1alpha1.Application, patterns []string)
return appsForUpdate, nil
}

func parseImageList(updateImage string) *image.ContainerImageList {
splits := strings.Split(updateImage, ",")
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.

results := make(image.ContainerImageList, 0)
if updateImage, ok := annotations[common.ImageUpdaterAnnotation]; ok {
splits := strings.Split(updateImage, ",")
for _, s := range splits {
img := image.NewFromIdentifier(strings.TrimSpace(s))
if kustomizeImage, ok := annotations[fmt.Sprintf(common.KustomizeApplicationNameAnnotation, img.ImageAlias)]; ok {
iamnoah marked this conversation as resolved.
Show resolved Hide resolved
img.KustomizeImage = image.NewFromIdentifier(kustomizeImage)
}
results = append(results, img)
}
}
return &results
}
Expand Down Expand Up @@ -425,13 +431,11 @@ func GetImagesFromApplication(app *v1alpha1.Application) image.ContainerImageLis
// The Application may wish to update images that don't create a container we can detect.
// Check the image list for images with a force-update annotation, and add them if they are not already present.
annotations := app.Annotations
if updateImage, ok := annotations[common.ImageUpdaterAnnotation]; ok {
for _, img := range *parseImageList(updateImage) {
img.ImageTag = nil // the tag from the image list will be a version constraint, which isn't a valid tag
if forceStr, force := annotations[fmt.Sprintf(common.ForceUpdateOptionAnnotation, img.ImageAlias)]; force && strings.ToLower(forceStr) == "true" {
if images.ContainsImage(img, false) == nil {
images = append(images, img)
}
for _, img := range *parseImageList(annotations) {
img.ImageTag = nil // the tag from the image list will be a version constraint, which isn't a valid tag
if forceStr, force := annotations[fmt.Sprintf(common.ForceUpdateOptionAnnotation, img.ImageAlias)]; force && strings.ToLower(forceStr) == "true" {
iamnoah marked this conversation as resolved.
Show resolved Hide resolved
if images.ContainsImage(img, false) == nil {
images = append(images, img)
}
}
}
Expand Down
19 changes: 15 additions & 4 deletions pkg/argocd/argocd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ func Test_GetImagesFromApplication(t *testing.T) {
imageList := GetImagesFromApplication(application)
require.Len(t, imageList, 1)
assert.Equal(t, "nginx", imageList[0].ImageName)
assert.Nil(t, imageList[0].ImageTag)
})
}

Expand Down Expand Up @@ -724,8 +725,18 @@ func TestKubernetesClient_UpdateSpec_Conflict(t *testing.T) {
}

func Test_parseImageList(t *testing.T) {
assert.Equal(t, []string{"foo", "bar"}, parseImageList(" foo, bar ").Originals())
// should whitespace inside the spec be preserved?
assert.Equal(t, []string{"foo", "bar", "baz = qux"}, parseImageList(" foo, bar,baz = qux ").Originals())
assert.Equal(t, []string{"foo", "bar", "baz=qux"}, parseImageList("foo,bar,baz=qux").Originals())
t.Run("Test basic parsing", func(t *testing.T) {
assert.Equal(t, []string{"foo", "bar"}, parseImageList(map[string]string{common.ImageUpdaterAnnotation: " foo, bar "}).Originals())
// should whitespace inside the spec be preserved?
assert.Equal(t, []string{"foo", "bar", "baz = qux"}, parseImageList(map[string]string{common.ImageUpdaterAnnotation: " foo, bar,baz = qux "}).Originals())
assert.Equal(t, []string{"foo", "bar", "baz=qux"}, parseImageList(map[string]string{common.ImageUpdaterAnnotation: "foo,bar,baz=qux"}).Originals())
})
t.Run("Test kustomize override", func(t *testing.T) {
imgs := *parseImageList(map[string]string{
common.ImageUpdaterAnnotation: "foo=bar",
fmt.Sprintf(common.KustomizeApplicationNameAnnotation, "foo"): "baz",
})
assert.Equal(t, "bar", imgs[0].ImageName)
assert.Equal(t, "baz", imgs[0].KustomizeImage.ImageName)
})
}
13 changes: 10 additions & 3 deletions pkg/argocd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ func UpdateApplication(updateConf *UpdateConfiguration, state *SyncIterationStat
AddField("image_tag", updateableImage.ImageTag).
AddField("alias", applicationImage.ImageAlias)

if updateableImage.KustomizeImage != nil {
imgCtx.AddField("kustomize_image", updateableImage.KustomizeImage)
}

imgCtx.Debugf("Considering this image for update")

rep, err := registry.GetRegistryEndpoint(updateableImage.RegistryURL)
Expand Down Expand Up @@ -256,9 +260,7 @@ func UpdateApplication(updateConf *UpdateConfiguration, state *SyncIterationStat
}
}

// If the latest tag does not match image's current tag, it means we have
// an update candidate.
if !updateableImage.ImageTag.Equals(latest) {
if needsUpdate(updateableImage, applicationImage, latest) {

imgCtx.Infof("Setting new image to %s", updateableImage.WithTag(latest).String())
needUpdate = true
Expand Down Expand Up @@ -340,6 +342,11 @@ func UpdateApplication(updateConf *UpdateConfiguration, state *SyncIterationStat
return result
}

func needsUpdate(updateableImage *image.ContainerImage, applicationImage *image.ContainerImage, latest *tag.ImageTag) bool {
// If the latest tag does not match image's current tag or the kustomize image is different, it means we have an update candidate.
return !updateableImage.ImageTag.Equals(latest) || applicationImage.KustomizeImage != nil && applicationImage.DiffersFrom(updateableImage, false)
}

func setAppImage(app *v1alpha1.Application, img *image.ContainerImage) error {
var err error
if appType := GetApplicationType(app); appType == ApplicationTypeKustomize {
Expand Down
75 changes: 66 additions & 9 deletions pkg/argocd/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,17 +320,16 @@ func Test_UpdateApplication(t *testing.T) {
kubeClient := kube.KubernetesClient{
Clientset: fake.NewFakeKubeClient(),
}
imageList := "foobar=gcr.io/jannfis/foobar:>=1.0.1"
annotations := map[string]string{
common.ImageUpdaterAnnotation: "foobar=gcr.io/jannfis/foobar:>=1.0.1",
fmt.Sprintf(common.KustomizeApplicationNameAnnotation, "foobar"): "jannfis/foobar",
}
appImages := &ApplicationImages{
Application: v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Name: "guestbook",
Namespace: "guestbook",
Annotations: map[string]string{
common.ImageUpdaterAnnotation: imageList,
fmt.Sprintf(common.KustomizeApplicationNameAnnotation, "foobar"): "jannfis/foobar",
fmt.Sprintf(common.ForceUpdateOptionAnnotation, "foobar"): "true",
},
Name: "guestbook",
Namespace: "guestbook",
Annotations: annotations,
},
Spec: v1alpha1.ApplicationSpec{
Source: v1alpha1.ApplicationSource{
Expand All @@ -350,7 +349,7 @@ func Test_UpdateApplication(t *testing.T) {
},
},
},
Images: *parseImageList(imageList),
Images: *parseImageList(annotations),
}
res := UpdateApplication(&UpdateConfiguration{
NewRegFN: mockClientFn,
Expand All @@ -366,6 +365,64 @@ func Test_UpdateApplication(t *testing.T) {
assert.Equal(t, 1, res.NumImagesUpdated)
})

t.Run("Test not updated because kustomize image is the same", func(t *testing.T) {
mockClientFn := func(endpoint *registry.RegistryEndpoint, username, password string) (registry.RegistryClient, error) {
regMock := regmock.RegistryClient{}
regMock.On("Tags", mock.Anything).Return([]string{"1.0.1"}, nil)
return &regMock, nil
}

argoClient := argomock.ArgoCD{}
argoClient.On("UpdateSpec", mock.Anything, mock.Anything).Return(nil, nil)

kubeClient := kube.KubernetesClient{
Clientset: fake.NewFakeKubeClient(),
}
annotations := map[string]string{
common.ImageUpdaterAnnotation: "foobar=gcr.io/jannfis/foobar:>=1.0.1",
fmt.Sprintf(common.KustomizeApplicationNameAnnotation, "foobar"): "jannfis/foobar",
}
appImages := &ApplicationImages{
Application: v1alpha1.Application{
ObjectMeta: v1.ObjectMeta{
Name: "guestbook",
Namespace: "guestbook",
Annotations: annotations,
},
Spec: v1alpha1.ApplicationSpec{
Source: v1alpha1.ApplicationSource{
Kustomize: &v1alpha1.ApplicationSourceKustomize{
Images: v1alpha1.KustomizeImages{
"jannfis/foobar:1.0.1",
},
},
},
},
Status: v1alpha1.ApplicationStatus{
SourceType: v1alpha1.ApplicationSourceTypeKustomize,
Summary: v1alpha1.ApplicationSummary{
Images: []string{
"gcr.io/jannfis/foobar:1.0.1",
},
},
},
},
Images: *parseImageList(annotations),
}
res := UpdateApplication(&UpdateConfiguration{
NewRegFN: mockClientFn,
ArgoClient: &argoClient,
KubeClient: &kubeClient,
UpdateApp: appImages,
DryRun: false,
}, NewSyncIterationState())
assert.Equal(t, 0, res.NumErrors)
assert.Equal(t, 0, res.NumSkipped)
assert.Equal(t, 1, res.NumApplicationsProcessed)
assert.Equal(t, 1, res.NumImagesConsidered)
assert.Equal(t, 0, res.NumImagesUpdated)
})

t.Run("Test skip because of match-tag pattern doesn't match", func(t *testing.T) {
meta := make([]*schema1.SignedManifest, 4)
for i := 0; i < 4; i++ {
Expand Down
11 changes: 11 additions & 0 deletions pkg/image/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ type ContainerImage struct {
ImageAlias string
HelmParamImageName string
HelmParamImageVersion string
KustomizeImage *ContainerImage
original string
}

Expand Down Expand Up @@ -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.

return img.RegistryURL != other.RegistryURL || img.ImageName != other.ImageName || (checkVersion && img.ImageTag.TagName != other.ImageTag.TagName)
}

// ContainsImage checks whether img is contained in a list of images
func (list *ContainerImageList) ContainsImage(img *ContainerImage, checkVersion bool) *ContainerImage {
// if there is a KustomizeImage override, check it for a match first
if img.KustomizeImage != nil {
if kustomizeMatch := list.ContainsImage(img.KustomizeImage, checkVersion); kustomizeMatch != nil {
return kustomizeMatch
}
}
for _, image := range *list {
if img.ImageName == image.ImageName && image.RegistryURL == img.RegistryURL {
if !checkVersion || image.ImageTag.TagName == img.ImageTag.TagName {
Expand Down