Repository: fluxcd/flux
Pull request #2594 (2019-11-08 10:29:35)

Ignore timestamp labels during sorting and release of images
In #2176 a whitelist option for the usage of image timestamps from
labels was implemented. This PR did however not remove the
`CreatedTS` method, with as a result that some elements of Flux like
`fluxctl` would still consult the timestamp parsed from labels, rather
than relying on the cache decorated `info.CreatedAt`.

This commit removes the method, and replaces any calls made to it
with `info.CreatedAt`, so that a timestamp from a label is not
mistakenly used while the image has not been whitelisted.

2opremio: Can you add a test which would had showed the problem this PR is fixing?
hiddeco: @2opremio I will try to find some time.
hiddeco: @2opremio the problem is fairly simple, labels should always be ignored and only be [used by the cache decorator in case an image has been whitelisted to replace the `CreatedAt` date](https://github.com/fluxcd/flux/blob/07b3d3d608dc53738d9778d6dd23355e8f50e550/pkg/registry/cache/registry.go#L51). I have updated the `OrderByCreationDate` test to show the labels are now ignored while present in the metadata.

Patch:
diff --git a/cmd/fluxctl/list_images_cmd.go b/cmd/fluxctl/list_images_cmd.go
index b2ece425..099e67b7 100644
--- a/cmd/fluxctl/list_images_cmd.go
+++ b/cmd/fluxctl/list_images_cmd.go
@@ -130,8 +130,8 @@ func (opts *imageListOpts) RunE(cmd *cobra.Command, args []string) error {
 				}
 				if printLine {
 					createdAt := ""
-					if !available.CreatedTS().IsZero() {
-						createdAt = available.CreatedTS().Format(time.RFC822)
+					if !available.CreatedAt.IsZero() {
+						createdAt = available.CreatedAt.Format(time.RFC822)
 					}
 					fmt.Fprintf(out, "\t\t%s %s\t%s\n", running, tag, createdAt)
 				}
diff --git a/pkg/daemon/images.go b/pkg/daemon/images.go
index c2fd6e61..32002ccc 100644
--- a/pkg/daemon/images.go
+++ b/pkg/daemon/images.go
@@ -102,13 +102,13 @@ func calculateChanges(logger log.Logger, candidateWorkloads resources, workloads
 					continue containers
 				}
 				current := repoMetadata.FindImageWithRef(currentImageID)
-				if pattern.RequiresTimestamp() && (current.CreatedTS().IsZero() || latest.CreatedTS().IsZero()) {
-					logger.Log("warning", "image with zero created timestamp", "current", fmt.Sprintf("%s (%s)", current.ID, current.CreatedTS()), "latest", fmt.Sprintf("%s (%s)", latest.ID, latest.CreatedTS()), "action", "skip container")
+				if pattern.RequiresTimestamp() && (current.CreatedAt.IsZero() || latest.CreatedAt.IsZero()) {
+					logger.Log("warning", "image with zero created timestamp", "current", fmt.Sprintf("%s (%s)", current.ID, current.CreatedAt), "latest", fmt.Sprintf("%s (%s)", latest.ID, latest.CreatedAt), "action", "skip container")
 					continue containers
 				}
 				newImage := currentImageID.WithNewTag(latest.ID.Tag)
 				changes.Add(workload.ID, container, newImage)
-				logger.Log("info", "added update to automation run", "new", newImage, "reason", fmt.Sprintf("latest %s (%s) > current %s (%s)", latest.ID.Tag, latest.CreatedTS(), currentImageID.Tag, current.CreatedTS()))
+				logger.Log("info", "added update to automation run", "new", newImage, "reason", fmt.Sprintf("latest %s (%s) > current %s (%s)", latest.ID.Tag, latest.CreatedAt, currentImageID.Tag, current.CreatedAt))
 			}
 		}
 	}
diff --git a/pkg/image/image.go b/pkg/image/image.go
index 5f9a49af..c3fecb5f 100644
--- a/pkg/image/image.go
+++ b/pkg/image/image.go
@@ -351,27 +351,6 @@ func (im *Info) UnmarshalJSON(b []byte) error {
 	return err
 }
 
-// CreatedTS returns the created at timestamp for an image,
-// prioritizing user defined timestamps from labels over the ones we
-// receive from a Docker registry API.
-//
-// The reason for this is registry vendors have different
-// interpretations of what a creation  date is, and we want the user to
-// be in control when required.
-//
-// In addition we prioritize the `Created` label over the `BuildDate`,
-// as the Label Schema Spec has been deprecated in favour of the OCI
-// Spec (but is still well known and widely used).
-func (im Info) CreatedTS() time.Time {
-	if !im.Labels.Created.IsZero() {
-		return im.Labels.Created
-	}
-	if !im.Labels.BuildDate.IsZero() {
-		return im.Labels.BuildDate
-	}
-	return im.CreatedAt
-}
-
 // RepositoryMetadata contains the image metadata information found in an
 // image repository.
 //
@@ -425,10 +404,10 @@ func decodeTime(s string, t *time.Time) error {
 // NewerByCreated returns true if lhs image should be sorted
 // before rhs with regard to their creation date descending.
 func NewerByCreated(lhs, rhs *Info) bool {
-	if lhs.CreatedTS().Equal(rhs.CreatedTS()) {
+	if lhs.CreatedAt.Equal(rhs.CreatedAt) {
 		return lhs.ID.String() < rhs.ID.String()
 	}
-	return lhs.CreatedTS().After(rhs.CreatedTS())
+	return lhs.CreatedAt.After(rhs.CreatedAt)
 }
 
 // NewerBySemver returns true if lhs image should be sorted


Test patch:
diff --git a/pkg/image/image_test.go b/pkg/image/image_test.go
index 09044bd3..eb43db4d 100644
--- a/pkg/image/image_test.go
+++ b/pkg/image/image_test.go
@@ -235,12 +235,14 @@ func TestImageInfoCreatedAtZero(t *testing.T) {
 func TestImage_OrderByCreationDate(t *testing.T) {
 	time0 := testTime.Add(time.Second)
 	time2 := testTime.Add(-time.Second)
+	ignoredTime := testTime.Add(+time.Minute)
+	ignoredTime2 := testTime.Add(+time.Hour)
 	imA := mustMakeInfo("my/Image:2", testTime)
-	imB := mustMakeInfo("my/Image:0", time.Time{}).setLabels(Labels{Created: time0})
-	imC := mustMakeInfo("my/Image:3", time.Time{}).setLabels(Labels{BuildDate: time2})
-	imD := mustMakeInfo("my/Image:4", time.Time{})                                      // test nil
-	imE := mustMakeInfo("my/Image:1", time.Time{}).setLabels(Labels{Created: testTime}) // test equal
-	imF := mustMakeInfo("my/Image:5", time.Time{})                                      // test nil equal
+	imB := mustMakeInfo("my/Image:0", time0).setLabels(Labels{Created: ignoredTime2})  // label time should be ignored
+	imC := mustMakeInfo("my/Image:3", time2).setLabels(Labels{BuildDate: ignoredTime}) // label time should be ignored
+	imD := mustMakeInfo("my/Image:4", time.Time{})                                     // test nil
+	imE := mustMakeInfo("my/Image:1", testTime).setLabels(Labels{Created: testTime})   // test equal
+	imF := mustMakeInfo("my/Image:5", time.Time{})                                     // test nil equal
 	imgs := []Info{imA, imB, imC, imD, imE, imF}
 	Sort(imgs, NewerByCreated)
 	checkSorted(t, imgs)
@@ -256,7 +258,7 @@ func checkSorted(t *testing.T, imgs []Info) {
 	for i, im := range imgs {
 		if strconv.Itoa(i) != im.ID.Tag {
 			for j, jim := range imgs {
-				t.Logf("%v: %v %s", j, jim.ID.String(), jim.CreatedTS())
+				t.Logf("%v: %v %s", j, jim.ID.String(), jim.CreatedAt)
 			}
 			t.Fatalf("Not sorted in expected order: %#v", imgs)
 		}