From 091a00cb2ecf4505a79f5105dfdc37c2cdbcd071 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 14 Sep 2022 05:06:07 +0530 Subject: [PATCH] Add previous image on updates and latest tags Introduce new field in ImagePolicy status, observedPreviousImage, to store the previous image. This is needed to show image update version change in alerts. Introduce new field in ImageRepository status, latestTags, to store the 10 recent tags. This would help users debug image policy better. Signed-off-by: Sunny --- api/v1beta2/imagepolicy_types.go | 4 + api/v1beta2/imagerepository_types.go | 5 +- api/v1beta2/zz_generated.deepcopy.go | 5 + ...image.toolkit.fluxcd.io_imagepolicies.yaml | 4 + ...e.toolkit.fluxcd.io_imagerepositories.yaml | 4 + controllers/imagepolicy_controller.go | 36 +++++- controllers/imagepolicy_controller_test.go | 39 ++++++ controllers/imagerepository_controller.go | 24 +++- .../imagerepository_controller_test.go | 120 ++++++++++++++---- docs/api/image-reflector.md | 23 ++++ 10 files changed, 232 insertions(+), 32 deletions(-) diff --git a/api/v1beta2/imagepolicy_types.go b/api/v1beta2/imagepolicy_types.go index c53e2f1..a7369a7 100644 --- a/api/v1beta2/imagepolicy_types.go +++ b/api/v1beta2/imagepolicy_types.go @@ -105,6 +105,10 @@ type ImagePolicyStatus struct { // the image repository, when filtered and ordered according to // the policy. LatestImage string `json:"latestImage,omitempty"` + // ObservedPreviousImage is the observed previous LatestImage. It is used + // to keep track of the previous and current images. + // +optional + ObservedPreviousImage string `json:"observedPreviousImage,omitempty"` // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` // +optional diff --git a/api/v1beta2/imagerepository_types.go b/api/v1beta2/imagerepository_types.go index 53686c3..57d44a2 100644 --- a/api/v1beta2/imagerepository_types.go +++ b/api/v1beta2/imagerepository_types.go @@ -94,8 +94,9 @@ type ImageRepositorySpec struct { } type ScanResult struct { - TagCount int `json:"tagCount"` - ScanTime metav1.Time `json:"scanTime,omitempty"` + TagCount int `json:"tagCount"` + ScanTime metav1.Time `json:"scanTime,omitempty"` + LatestTags []string `json:"latestTags,omitempty"` } // ImageRepositoryStatus defines the observed state of ImageRepository diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index d30f295..29f2b40 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -328,6 +328,11 @@ func (in *NumericalPolicy) DeepCopy() *NumericalPolicy { func (in *ScanResult) DeepCopyInto(out *ScanResult) { *out = *in in.ScanTime.DeepCopyInto(&out.ScanTime) + if in.LatestTags != nil { + in, out := &in.LatestTags, &out.LatestTags + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ScanResult. diff --git a/config/crd/bases/image.toolkit.fluxcd.io_imagepolicies.yaml b/config/crd/bases/image.toolkit.fluxcd.io_imagepolicies.yaml index e1f9942..54302c0 100644 --- a/config/crd/bases/image.toolkit.fluxcd.io_imagepolicies.yaml +++ b/config/crd/bases/image.toolkit.fluxcd.io_imagepolicies.yaml @@ -391,6 +391,10 @@ spec: observedGeneration: format: int64 type: integer + observedPreviousImage: + description: ObservedPreviousImage is the observed previous LatestImage. + It is used to keep track of the previous and current images. + type: string type: object type: object served: true diff --git a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml index c8614fc..0b4baa6 100644 --- a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml +++ b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml @@ -432,6 +432,10 @@ spec: lastScanResult: description: LastScanResult contains the number of fetched tags. properties: + latestTags: + items: + type: string + type: array scanTime: format: date-time type: string diff --git a/controllers/imagepolicy_controller.go b/controllers/imagepolicy_controller.go index cbee108..c03f739 100644 --- a/controllers/imagepolicy_controller.go +++ b/controllers/imagepolicy_controller.go @@ -22,6 +22,7 @@ import ( "fmt" "time" + "github.com/google/go-containerregistry/pkg/name" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -192,10 +193,20 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) return } +// composeImagePolicyReadyMessage composes a Ready message for an ImagePolicy +// based on the results of applying the policy. +func composeImagePolicyReadyMessage(previousTag, latestTag, image string) string { + readyMsg := fmt.Sprintf("Latest image tag for '%s' resolved to %s", image, latestTag) + if previousTag != "" && previousTag != latestTag { + readyMsg = fmt.Sprintf("Latest image tag for '%s' updated from %s to %s", image, previousTag, latestTag) + } + return readyMsg +} + func (r *ImagePolicyReconciler) reconcile(ctx context.Context, obj *imagev1.ImagePolicy) (result ctrl.Result, retErr error) { oldObj := obj.DeepCopy() - var resultImage, resultTag string + var resultImage, resultTag, previousTag string // If there's no error and no requeue is requested, it's a success. Unlike // other reconcilers, this reconciler doesn't requeue on its own with a @@ -208,7 +219,8 @@ func (r *ImagePolicyReconciler) reconcile(ctx context.Context, obj *imagev1.Imag } defer func() { - readyMsg := fmt.Sprintf("Latest image tag for '%s' resolved to: %s", resultImage, resultTag) + readyMsg := composeImagePolicyReadyMessage(previousTag, resultTag, resultImage) + rs := pkgreconcile.NewResultFinalizer(isSuccess, readyMsg) retErr = rs.Finalize(obj, result, retErr) @@ -286,6 +298,26 @@ func (r *ImagePolicyReconciler) reconcile(ctx context.Context, obj *imagev1.Imag // Write the observations on status. obj.Status.LatestImage = repo.Spec.Image + ":" + latest + // If the old latest image and new latest image don't match, set the old + // image as the observed previous image. + // NOTE: The following allows the previous image to be set empty when + // there's a failure and a subsequent recovery from it. This behavior helps + // avoid creating an update event as there's no previous image to infer + // from. Recovery from a failure shouldn't result in an update event. + if oldObj.Status.LatestImage != obj.Status.LatestImage { + obj.Status.ObservedPreviousImage = oldObj.Status.LatestImage + } + // Parse the observed previous image if any and extract previous tag. This + // is used to determine image tag update path. + if obj.Status.ObservedPreviousImage != "" { + prevRef, err := name.NewTag(obj.Status.ObservedPreviousImage) + if err != nil { + e := fmt.Errorf("failed to parse previous image '%s': %w", obj.Status.ObservedPreviousImage, err) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, e.Error()) + result, retErr = ctrl.Result{}, e + } + previousTag = prevRef.TagStr() + } resultImage = repo.Spec.Image resultTag = latest diff --git a/controllers/imagepolicy_controller_test.go b/controllers/imagepolicy_controller_test.go index 5acd9c4..7161d9b 100644 --- a/controllers/imagepolicy_controller_test.go +++ b/controllers/imagepolicy_controller_test.go @@ -316,3 +316,42 @@ func TestImagePolicyReconciler_applyPolicy(t *testing.T) { }) } } + +func TestComposeImagePolicyReadyMessage(t *testing.T) { + testImage := "foo/bar" + + tests := []struct { + name string + previousTag string + latestTag string + image string + wantMessage string + }{ + { + name: "no previous tag", + latestTag: "1.0.0", + wantMessage: "Latest image tag for 'foo/bar' resolved to 1.0.0", + }, + { + name: "different previous tag", + previousTag: "1.0.0", + latestTag: "1.1.0", + wantMessage: "Latest image tag for 'foo/bar' updated from 1.0.0 to 1.1.0", + }, + { + name: "same previous and latest tags", + previousTag: "1.0.0", + latestTag: "1.0.0", + wantMessage: "Latest image tag for 'foo/bar' resolved to 1.0.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + result := composeImagePolicyReadyMessage(tt.previousTag, tt.latestTag, testImage) + g.Expect(result).To(Equal(tt.wantMessage)) + }) + } +} diff --git a/controllers/imagerepository_controller.go b/controllers/imagerepository_controller.go index 2200cc4..220548f 100644 --- a/controllers/imagerepository_controller.go +++ b/controllers/imagerepository_controller.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "regexp" + "sort" "strings" "time" @@ -57,6 +58,9 @@ import ( "github.com/fluxcd/image-reflector-controller/internal/secret" ) +// latestTagsCount is the number of tags to use as latest tags. +const latestTagsCount = 10 + // imageRepositoryOwnedConditions is a list of conditions owned by the // ImageRepositoryReconciler. var imageRepositoryOwnedConditions = []string{ @@ -462,8 +466,9 @@ func (r *ImageRepositoryReconciler) scan(ctx context.Context, obj *imagev1.Image scanTime := metav1.Now() obj.Status.LastScanResult = &imagev1.ScanResult{ - TagCount: len(filteredTags), - ScanTime: scanTime, + TagCount: len(filteredTags), + ScanTime: scanTime, + LatestTags: getLatestTags(filteredTags), } // If the reconcile request annotation was set, consider it @@ -551,6 +556,21 @@ func filterOutTags(tags []string, patterns []string) ([]string, error) { return filteredTags, nil } +// getLatestTags takes a slice of tags, sorts them in descending order of their +// values and returns the 10 latest tags. +func getLatestTags(tags []string) []string { + var result []string + sort.SliceStable(tags, func(i, j int) bool { return tags[i] > tags[j] }) + + if len(tags) >= latestTagsCount { + latestTags := tags[0:latestTagsCount] + result = append(result, latestTags...) + } else { + result = append(result, tags...) + } + return result +} + // isEqualSliceContent compares two string slices to check if they have the same // content. func isEqualSliceContent(a, b []string) bool { diff --git a/controllers/imagerepository_controller_test.go b/controllers/imagerepository_controller_test.go index e5a3b30..23e0986 100644 --- a/controllers/imagerepository_controller_test.go +++ b/controllers/imagerepository_controller_test.go @@ -440,37 +440,48 @@ func TestImageRepositoryReconciler_scan(t *testing.T) { defer registryServer.Close() tests := []struct { - name string - tags []string - exclusionList []string - annotation string - db *mockDatabase - wantErr bool - wantTags []string + name string + tags []string + exclusionList []string + annotation string + db *mockDatabase + wantErr bool + wantTags []string + wantLatestTags []string }{ { name: "no tags", wantErr: true, }, { - name: "simple tags", - tags: []string{"a", "b", "c", "d"}, - db: &mockDatabase{}, - wantTags: []string{"a", "b", "c", "d"}, + name: "simple tags", + tags: []string{"a", "b", "c", "d"}, + db: &mockDatabase{}, + wantTags: []string{"a", "b", "c", "d"}, + wantLatestTags: []string{"d", "c", "b", "a"}, }, { - name: "with single exclusion pattern", - tags: []string{"a", "b", "c", "d"}, - exclusionList: []string{"c"}, - db: &mockDatabase{}, - wantTags: []string{"a", "b", "d"}, + name: "simple tags, 10+", + tags: []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k"}, + db: &mockDatabase{}, + wantTags: []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k"}, + wantLatestTags: []string{"k", "j", "i", "h, g, f, e, d, c, b"}, }, { - name: "with multiple exclusion pattern", - tags: []string{"a", "b", "c", "d"}, - exclusionList: []string{"c", "a"}, - db: &mockDatabase{}, - wantTags: []string{"b", "d"}, + name: "with single exclusion pattern", + tags: []string{"a", "b", "c", "d"}, + exclusionList: []string{"c"}, + db: &mockDatabase{}, + wantTags: []string{"a", "b", "d"}, + wantLatestTags: []string{"d", "b", "a"}, + }, + { + name: "with multiple exclusion pattern", + tags: []string{"a", "b", "c", "d"}, + exclusionList: []string{"c", "a"}, + db: &mockDatabase{}, + wantTags: []string{"b", "d"}, + wantLatestTags: []string{"d", "b"}, }, { name: "bad exclusion pattern", @@ -485,11 +496,12 @@ func TestImageRepositoryReconciler_scan(t *testing.T) { wantErr: true, }, { - name: "with reconcile annotation", - tags: []string{"a", "b"}, - annotation: "foo", - db: &mockDatabase{}, - wantTags: []string{"a", "b"}, + name: "with reconcile annotation", + tags: []string{"a", "b"}, + annotation: "foo", + db: &mockDatabase{}, + wantTags: []string{"a", "b"}, + wantLatestTags: []string{"b", "a"}, }, } @@ -535,6 +547,62 @@ func TestImageRepositoryReconciler_scan(t *testing.T) { } } +func TestGetLatestTags(t *testing.T) { + tests := []struct { + name string + tags []string + wantLatestTags []string + }{ + { + name: "no tags", + wantLatestTags: nil, + }, + { + name: "few semver tags", + tags: []string{"1.0.0", "0.0.8", "1.2.5", "3.0.1", "1.0.1"}, + wantLatestTags: []string{"3.0.1", "1.2.5", "1.0.1", "1.0.0", "0.0.8"}, + }, + { + name: "10 semver tags", + tags: []string{"1.0.0", "0.0.8", "1.2.5", "3.0.1", "1.0.1", "5.1.1", "4.1.0", "4.5.0", "4.0.3", "2.2.2"}, + wantLatestTags: []string{"5.1.1", "4.5.0", "4.1.0", "4.0.3", "3.0.1", "2.2.2", "1.2.5", "1.0.1", "1.0.0", "0.0.8"}, + }, + { + name: "10+ semver tags", + tags: []string{"1.0.0", "0.0.8", "1.2.5", "3.0.1", "1.0.1", "5.1.1", "4.1.0", "4.5.0", "4.0.3", "2.2.2", "0.5.1", "0.1.0"}, + wantLatestTags: []string{"5.1.1", "4.5.0", "4.1.0", "4.0.3", "3.0.1", "2.2.2", "1.2.5", "1.0.1", "1.0.0", "0.5.1"}, + }, + { + name: "few numerical tags", + tags: []string{"-62", "-88", "73", "72", "15"}, + wantLatestTags: []string{"73", "72", "15", "-88", "-62"}, + }, + { + name: "few numerical tags", + tags: []string{"-62", "-88", "73", "72", "15", "16", "15", "29", "-33", "-91", "100", "101"}, + wantLatestTags: []string{"73", "72", "29", "16", "15", "15", "101", "100", "-91", "-88"}, + }, + { + name: "few word tags", + tags: []string{"aaa", "bbb", "ccc"}, + wantLatestTags: []string{"ccc", "bbb", "aaa"}, + }, + { + name: "few word tags", + tags: []string{"aaa", "bbb", "ccc", "ddd", "eee", "fff", "ggg", "hhh", "iii", "jjj", "kkk", "lll"}, + wantLatestTags: []string{"lll", "kkk", "jjj", "iii", "hhh", "ggg", "fff", "eee", "ddd", "ccc"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(getLatestTags(tt.tags)).To(Equal(tt.wantLatestTags)) + }) + } +} + func TestParseImageReference(t *testing.T) { tests := []struct { name string diff --git a/docs/api/image-reflector.md b/docs/api/image-reflector.md index 4b22458..b2e2fed 100644 --- a/docs/api/image-reflector.md +++ b/docs/api/image-reflector.md @@ -313,6 +313,19 @@ the policy.

+observedPreviousImage
+ +string + + + +(Optional) +

ObservedPreviousImage is the observed previous LatestImage. It is used +to keep track of the previous and current images.

+ + + + observedGeneration
int64 @@ -865,6 +878,16 @@ Kubernetes meta/v1.Time + + +latestTags
+ +[]string + + + + +