Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Do not fail on non RFC3339 labels but log warning #2084

Merged
merged 1 commit into from
May 23, 2019

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented May 23, 2019

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).

Addresses #2075, but in a stricter way.

@hiddeco hiddeco requested a review from squaremo May 23, 2019 16:31
image/image.go Outdated Show resolved Hide resolved
Copy link
Contributor

@2opremio 2opremio left a comment

Choose a reason for hiding this comment

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

Minor comment, otherwise LGTM

@2opremio
Copy link
Contributor

Some image vendors have not read the spec properly and pass along a timestamp in a different format than RFC3339

Do we know what wrong formats they typically use? As @squaremo mentioned in Slack, we could maybe try to accommodate those other formats as well, by trying to parse them as a fallback.

@hiddeco
Copy link
Member Author

hiddeco commented May 23, 2019

@2opremio CentOS images use a custom yyyymmdd format (a datestamp basically, if that would be a thing). I do not know what other images use the labels with a wrong format.

I am personally opposed to support anything different than what the spec defines, as it would stimulate people not following rules in a way, and the specs we support clearly state:

This label contains the Date/Time the image was built. The value SHOULD be formatted according to RFC 3339.

I also do not think this will be an issue for any user in case they do not have (direct) control over what is defined in the labels, as we safely fallback to the timestamp the registry gives us.

@2opremio
Copy link
Contributor

@2opremio CentOS images use a custom yyyymmdd format (a datestamp basically, if that would be a thing). I do not know what other images use the labels with a wrong format.

I am personally opposed to support anything different than what the spec defines, as it would stimulate people not following rules in a way, and the specs we support clearly state:

Makes sense. It may be worth reporting it to (or even creating a PR against) https://github.com/CentOS/CentOS-Dockerfiles

@2opremio
Copy link
Contributor

Actually, this is the repo https://github.com/CentOS/sig-cloud-instance-images

@hiddeco
Copy link
Member Author

hiddeco commented May 23, 2019

It has been mentioned in an issue there about a year ago, just did not find the time yet to submit a PR.

Some other reasons that came to mind about not supporting different timestamps:

  • we prioritize labels over the date from the registry, we can only do this if the given timestamp is more reliable, which is something we can not assume when someone does not follow the spec
  • it adds complexity to our codebase to support something that’s essentially wrong

@2opremio
Copy link
Contributor

Here's the culprit https://github.com/CentOS/sig-cloud-instance-build/blob/master/docker/containerbuild.sh#L74

@2opremio
Copy link
Contributor

Upstream PR CentOS/sig-cloud-instance-build#151

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).
@hiddeco hiddeco force-pushed the bug/2075-rfc-3339-ts branch from 3622a69 to 877a750 Compare May 23, 2019 20:14
@2opremio
Copy link
Contributor

LGTM

@hiddeco hiddeco merged commit 5b15a94 into master May 23, 2019
@hiddeco hiddeco deleted the bug/2075-rfc-3339-ts branch May 23, 2019 20:36
@hiddeco hiddeco added this to the v1.13.0 milestone May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants