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.

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

Fix DateTimeParseException when created is not set in image config #8302

Merged

Conversation

SgtSilvio
Copy link
Contributor

Starting a testcontainer fails when using an image that does not set the "created" field in its config (for example hivemq/hivemq-swarm:4.25.0) with the following exception:

Text '' could not be parsed at index 0
java.time.format.DateTimeParseException: Text '' could not be parsed at index 0
	at [email protected]/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2046)
	at [email protected]/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1948)
	at [email protected]/java.time.ZonedDateTime.parse(ZonedDateTime.java:598)
	at [email protected]/java.time.ZonedDateTime.parse(ZonedDateTime.java:583)
	at app//org.testcontainers.images.ImageData.from(ImageData.java:22)
	at app//org.testcontainers.images.LocalImagesCache.refreshCache(LocalImagesCache.java:47)
	at app//org.testcontainers.images.AbstractImagePullPolicy.shouldPull(AbstractImagePullPolicy.java:24)
	at app//org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:70)
	at app//org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:28)
	at app//org.testcontainers.utility.LazyFuture.getResolvedValue(LazyFuture.java:20)
	at app//org.testcontainers.utility.LazyFuture.get(LazyFuture.java:41)
	at app//org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1365)
	at app//org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:362)
	at app//org.testcontainers.containers.GenericContainer.start(GenericContainer.java:333)

The "created" field is optional according to the OCI Image Format Specification (https://github.com/opencontainers/image-spec/blob/main/config.md#properties). Also, this used to work with older Docker engine versions.
Unfortunately, the "Created" field in the output of the docker inspect command changed in new versions:

  • Docker version 24.0.7: "0001-01-01T00:00:00Z"
  • Docker version 25.0.3: ""

This PR fixes this case by interpreting an empty string as an absent created time.
Additionally null is treated the same to cover a possible case when "Created" is also absent in the output of the docker inspect command.
The value Instant.EPOCH is used to represent an absent created time, because this is how docker represents it in the docker images list command.

@SgtSilvio
Copy link
Contributor Author

The moby project will add empty if the field was not set in the image config to the docker api spec so it should be expected to handle the empty string case: moby/moby#47374
So this fix is valid and it would be great to get it merged.

@eddumelendez
Copy link
Member

@SgtSilvio thanks for your contribution! Just one question, do you have any custom value for the API in docker-java.properties? Testcontainers should be using API version 1.32. You can enable trace logging for com.github.dockerjava package

@SgtSilvio
Copy link
Contributor Author

Thanks for your response @eddumelendez

do you have any custom value for the API in docker-java.properties?

No, no custom config for docker-java.

Unfortunately, the new Docker version returns the empty string timestamp on all api versions, so also 1.32. This will be fixed in an upcoming release of docker: moby/moby#47374
This change will nevertheless ensure that testcontainers will work on newer versions of the docker api. In the docker issue, they are also discussing to make the field optional, which is also covered by this change by the added null checks.

@eddumelendez
Copy link
Member

Unfortunately, the new Docker version returns the empty string timestamp on all api versions, so also 1.32.

Right! missed that part 👍🏽

@eddumelendez eddumelendez added this to the next milestone Feb 14, 2024
@kiview
Copy link
Member

kiview commented Feb 15, 2024

Hey @SgtSilvio, just to update you, we discussed it with the Docker Engine team and it is a breaking change in Docker Engine, that will be fixed with the next Docker Engine patch release. We are still discussing how to best move forward for now.

@SgtSilvio
Copy link
Contributor Author

SgtSilvio commented Feb 15, 2024

@kiview agreed.
Whatever change docker plans for future versions, the null checks that I added make sense in any case.

Copy link

@LukasBrand LukasBrand left a comment

Choose a reason for hiding this comment

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

Looks like a good safety check.

@SgtSilvio SgtSilvio force-pushed the bugifx/datetimeparseexception branch 2 times, most recently from f5aa035 to 8ff591c Compare March 1, 2024 16:08
@SgtSilvio
Copy link
Contributor Author

@kiview The Docker team has now made the decision to omit the Created field in the output of the docker inspect command.

The release notes of 25.0.4 state:

  • API: GET /images/{id}/json omits the Created field (previously it was 0001-01-01T00:00:00Z) if the Created field was missing from the image config. moby/moby#47451
  • API: Populate a missing Created field in GET /images/{id}/json with 0001-01-01T00:00:00Z for API versions <= 1.43. moby/moby#47387

This means:

  • For docker engine versions up to 24.0.x: Created="0001-01-01T00:00:00Z" for all API versions (max 1.43)
  • For docker engine versions 25.0.0-25.0.3: Created="" for all API versions (max 1.44) => this is considered a bug
  • For docker engine versions 25.0.4+:
    • Created="0001-01-01T00:00:00Z" for API versions up to 1.43 (backwards compatibility)
    • Created is absent for API versions 1.44+

The last point means that the null checks added in this PR are required for API versions 1.44+.
The empty string check is only required for docker engine versions 25.0.0-25.0.3.
So can we get this PR merged?

The output of docker inspect "Created" field changed:
Docker version 24.0.7: "0001-01-01T00:00:00Z"
Docker version 25.0.3: ""
@SgtSilvio SgtSilvio force-pushed the bugifx/datetimeparseexception branch from 8ff591c to 3f3ad2e Compare March 12, 2024 12:35
@eddumelendez eddumelendez modified the milestones: 1.19.6, next Mar 12, 2024
@eddumelendez eddumelendez merged commit 358f68a into testcontainers:main Mar 13, 2024
97 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @SgtSilvio !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants