From 877a75097dd2666b0ffd2b01a3c8837acc283add Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Thu, 23 May 2019 16:48:47 +0200
Subject: [PATCH] Do not fail on non RFC3339 labels but log warning

Some image vendors have not read the spec properly and pass along
a timestamp in a different format than RFC3339. Before this change
the unmarshal of the JSON would fail for those labels and the affected
images would be excluded from our cache.

We now include those images in our cache and collect all labels that
failed parsing in a custom error, which we detect and log (instead
of bailing out).
---
 image/image.go                     | 32 +++++++++++++++++++++++++-----
 image/image_test.go                | 17 ++++++++++++++++
 registry/cache/repocachemanager.go |  5 ++++-
 registry/client.go                 | 13 +++++++++---
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/image/image.go b/image/image.go
index 0fbe3ef90..d580c5ed5 100644
--- a/image/image.go
+++ b/image/image.go
@@ -223,6 +223,17 @@ func (i Ref) WithNewTag(t string) Ref {
 	return img
 }
 
+type LabelTimestampFormatError struct {
+	Labels []string
+}
+
+func (e *LabelTimestampFormatError) Error() string {
+	return fmt.Sprintf(
+		"failed to parse %d timestamp label(s) as RFC3339 (%s); ask the repository administrator to correct this as it conflicts with the spec",
+		len(e.Labels),
+		strings.Join(e.Labels, ","))
+}
+
 // Labels has all the image labels we are interested in for an image
 // ref, the JSON struct tag keys should be equal to the label.
 type Labels struct {
@@ -261,12 +272,23 @@ func (l *Labels) UnmarshalJSON(b []byte) error {
 		Created   string `json:"org.opencontainers.image.created,omitempty"`
 	}{}
 	json.Unmarshal(b, &unencode)
-
-	var err error
-	if err = decodeTime(unencode.BuildDate, &l.BuildDate); err == nil {
-		err = decodeTime(unencode.Created, &l.Created)
+	labelErr := LabelTimestampFormatError{}
+	if err := decodeTime(unencode.BuildDate, &l.BuildDate); err != nil {
+		if _, ok := err.(*time.ParseError); !ok {
+			return err
+		}
+		labelErr.Labels = append(labelErr.Labels, "org.label-schema.build-date")
 	}
-	return err
+	if err := decodeTime(unencode.Created, &l.Created); err != nil {
+		if _, ok := err.(*time.ParseError); !ok {
+			return err
+		}
+		labelErr.Labels = append(labelErr.Labels, "org.opencontainers.image.created")
+	}
+	if len(labelErr.Labels) >= 1 {
+		return &labelErr
+	}
+	return nil
 }
 
 // Info has the metadata we are able to determine about an image ref,
diff --git a/image/image_test.go b/image/image_test.go
index f4697d59b..818ed124a 100644
--- a/image/image_test.go
+++ b/image/image_test.go
@@ -153,6 +153,23 @@ func TestImageLabelsSerialisation(t *testing.T) {
 	assert.Equal(t, labels, labels1)
 }
 
+func TestNonRFC3339ImageLabelsUnmarshal(t *testing.T) {
+	str := `{
+	"org.label-schema.build-date": "20190523",
+	"org.opencontainers.image.created": "20190523"
+}`
+
+	var labels Labels
+	err := json.Unmarshal([]byte(str), &labels)
+	lpe, ok := err.(*LabelTimestampFormatError)
+	if !ok {
+		t.Fatalf("Got %v, but expected LabelTimestampFormatError", err)
+	}
+	if lc := len(lpe.Labels); lc != 2 {
+		t.Errorf("Got error for %v labels, expected 2", lc)
+	}
+}
+
 func TestImageLabelsZeroTime(t *testing.T) {
 	labels := Labels{}
 	bytes, err := json.Marshal(labels)
diff --git a/registry/cache/repocachemanager.go b/registry/cache/repocachemanager.go
index bade231d4..416bae284 100644
--- a/registry/cache/repocachemanager.go
+++ b/registry/cache/repocachemanager.go
@@ -254,7 +254,10 @@ func (c *repoCacheManager) updateImage(ctx context.Context, update imageToUpdate
 		if ctx.Err() == context.DeadlineExceeded {
 			return registry.ImageEntry{}, c.clientTimeoutError()
 		}
-		return registry.ImageEntry{}, err
+		if _, ok := err.(*image.LabelTimestampFormatError); !ok {
+			return registry.ImageEntry{}, err
+		}
+		c.logger.Log("err", err, "ref", imageID)
 	}
 
 	refresh := update.previousRefresh
diff --git a/registry/client.go b/registry/client.go
index d2540cd04..4434a4ef4 100644
--- a/registry/client.go
+++ b/registry/client.go
@@ -120,6 +120,7 @@ interpret:
 		return ImageEntry{}, fetchErr
 	}
 
+	var labelErr error
 	info := image.Info{ID: a.repo.ToRef(ref), Digest: manifestDigest.String()}
 
 	// TODO(michael): can we type switch? Not sure how dependable the
@@ -139,7 +140,10 @@ interpret:
 		}
 
 		if err = json.Unmarshal([]byte(man.History[0].V1Compatibility), &v1); err != nil {
-			return ImageEntry{}, err
+			if _, ok := err.(*image.LabelTimestampFormatError); !ok {
+				return ImageEntry{}, err
+			}
+			labelErr = err
 		}
 		// This is not the ImageID that Docker uses, but assumed to
 		// identify the image as it's the topmost layer.
@@ -162,7 +166,10 @@ interpret:
 			} `json:"container_config"`
 		}
 		if err = json.Unmarshal(configBytes, &config); err != nil {
-			return ImageEntry{}, err
+			if _, ok := err.(*image.LabelTimestampFormatError); !ok {
+				return ImageEntry{}, err
+			}
+			labelErr = err
 		}
 		// This _is_ what Docker uses as its Image ID.
 		info.ImageID = man.Config.Digest.String()
@@ -184,5 +191,5 @@ interpret:
 		t := reflect.TypeOf(manifest)
 		return ImageEntry{}, errors.New("unknown manifest type: " + t.String())
 	}
-	return ImageEntry{Info: info}, nil
+	return ImageEntry{Info: info}, labelErr
 }