From 513110bec3fdb8e38d66ab2cc4e5edfb94f16839 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 7 Feb 2023 10:36:37 -0500 Subject: [PATCH 1/6] WX-926 Support falling back to OCI Manifest Format --- .../registryv2/DockerRegistryV2Abstract.scala | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala index b8f78476eac..94e1474ccb6 100644 --- a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala +++ b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala @@ -5,8 +5,8 @@ import cats.effect.IO import cats.syntax.either._ import common.validation.Validation._ import cromwell.docker.DockerInfoActor._ +import cromwell.docker._ import cromwell.docker.registryv2.DockerRegistryV2Abstract._ -import cromwell.docker.{DockerHashResult, DockerImageIdentifier, DockerRegistry, DockerRegistryConfig} import io.circe.Decoder import io.circe.generic.auto._ import org.http4s.Uri.{Authority, Scheme} @@ -27,17 +27,23 @@ object DockerRegistryV2Abstract { } val DigestHeaderName = CaseInsensitiveString("Docker-Content-Digest") - val ManifestV2MediaType = "application/vnd.docker.distribution.manifest.v2+json" - val ManifestListV2MediaType = "application/vnd.docker.distribution.manifest.list.v2+json" + val ManifestDockerV2MediaType = "application/vnd.docker.distribution.manifest.v2+json" + val ManifestDockerListV2MediaType = "application/vnd.docker.distribution.manifest.list.v2+json" + val ManifestOCIIndexV1MediaType = "application/vnd.oci.image.index.v1+json" // If one of those fails it means someone changed one of the strings above to an invalid one. - val ManifestV2MediaRange = MediaRange.parse(ManifestV2MediaType) + val ManifestV2MediaRange = MediaRange.parse(ManifestDockerV2MediaType) .unsafe("Cannot parse invalid manifest v2 content type. Please report this error.") - val ManifestListV2MediaRange = MediaRange.parse(ManifestListV2MediaType) + val ManifestListV2MediaRange = MediaRange.parse(ManifestDockerListV2MediaType) .unsafe("Cannot parse invalid manifest list v2 content type. Please report this error.") - val AcceptManifestV2Header = Accept.parse(ManifestV2MediaType) + val AcceptManifestV2Header = Accept.parse(ManifestDockerV2MediaType) .unsafe("Cannot parse invalid manifest v2 Accept header. Please report this error.") + val OCIManifestV1MediaRange = MediaRange.parse(ManifestOCIIndexV1MediaType) + .unsafe("Cannot parse invalid oci manifest v1 content type. Please report this error.") + val AcceptOCIManifestV1Header = Accept.parse(ManifestOCIIndexV1MediaType) + .unsafe("Cannot parse invalid oci manifest v1 Accept header. Please report this error.") + implicit val entityManifestDecoder = jsonEntityDecoder[DockerManifest](ManifestV2MediaRange) implicit val entityManifestListDecoder = jsonEntityDecoder[DockerManifestList](ManifestListV2MediaRange) implicit val entityTokenDecoder = jsonOf[IO, DockerAccessToken] @@ -244,9 +250,9 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi * If that assumption turns out to be incorrect, a smarter decision may need to be made to choose the manifest to lookup. */ private def parseManifest(dockerImageIdentifier: DockerImageIdentifier, token: Option[String])(response: Response[IO])(implicit client: Client[IO]): IO[Option[DockerManifest]] = response match { - case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(ManifestV2MediaType)) => + case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(ManifestDockerV2MediaType)) => r.as[DockerManifest].map(Option.apply) - case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(ManifestListV2MediaType)) => + case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(ManifestDockerListV2MediaType)) => r.as[DockerManifestList].flatMap({ dockerManifestList => obtainManifestFromList(dockerManifestList, dockerImageIdentifier, token) }) From fb8f88c1748042e76a9f399a9c9688666e141102 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 7 Feb 2023 11:21:31 -0500 Subject: [PATCH 2/6] First pass at recovery logic --- .../registryv2/DockerRegistryV2Abstract.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala index 94e1474ccb6..e56c97c43ae 100644 --- a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala +++ b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala @@ -127,8 +127,11 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi * @return docker info response */ protected def getDockerResponse(token: Option[String], dockerInfoContext: DockerInfoContext)(implicit client: Client[IO]): IO[DockerInfoSuccessResponse] = { - val request = manifestRequest(token, dockerInfoContext.dockerImageID) - executeRequest(request, handleManifestResponse(dockerInfoContext, token)) + val requestV2 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptManifestV2Header) + def requestV1 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptOCIManifestV1Header) + def tryOCIManifest(err: Throwable) = executeRequest(requestV1, handleManifestResponse(dockerInfoContext, token)) + executeRequest(requestV2, handleManifestResponse(dockerInfoContext, token)) + .handleErrorWith(tryOCIManifest) } /** @@ -209,12 +212,12 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi /** * Request to get the manifest, using the auth token if provided */ - private def manifestRequest(token: Option[String], imageId: DockerImageIdentifier): IO[Request[IO]] = { + private def manifestRequest(token: Option[String], imageId: DockerImageIdentifier, manifestHeader: Accept): IO[Request[IO]] = { val authorizationHeader = token.map(t => Authorization(Credentials.Token(AuthScheme.Bearer, t))) val request = Method.GET( buildManifestUri(imageId), List( - Option(AcceptManifestV2Header), + Option(manifestHeader), authorizationHeader ).flatten: _* ) @@ -266,7 +269,7 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi .map(_.digest) .map(dockerImageIdentifier.swapReference) match { case Some(identifierWithNewHash) => - val request = manifestRequest(token, identifierWithNewHash) + val request = manifestRequest(token, identifierWithNewHash, AcceptManifestV2Header) executeRequest(request, parseManifest(dockerImageIdentifier, token)) case None => logger.error(s"The manifest list for ${dockerImageIdentifier.fullName} was empty. Cannot proceed to obtain the size of image") From 6b975598ee47c30280637b35b68871f22a82a527 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 7 Feb 2023 11:50:01 -0500 Subject: [PATCH 3/6] Update test --- .../docker/registryv2/DockerRegistryV2AbstractSpec.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala b/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala index e567aa540cd..1461069ebd1 100644 --- a/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala +++ b/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala @@ -3,10 +3,10 @@ package cromwell.docker.registryv2 import cats.effect.{IO, Resource} import common.assertion.CromwellTimeoutSpec import cromwell.docker.DockerInfoActor.{DockerInfoContext, DockerInfoFailedResponse} -import cromwell.docker.{DockerImageIdentifier, DockerInfoActor, DockerInfoRequest, DockerRegistryConfig} +import cromwell.docker._ +import org.http4s._ import org.http4s.client.Client import org.http4s.headers.`Content-Type` -import org.http4s.{Header, Headers, MediaType, Request, Response} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -20,7 +20,7 @@ class DockerRegistryV2AbstractSpec extends AnyFlatSpec with CromwellTimeoutSpec override protected def buildTokenRequestHeaders(dockerInfoContext: DockerInfoActor.DockerInfoContext) = List.empty } - val mediaType = MediaType.parse(DockerRegistryV2Abstract.ManifestV2MediaType).toOption.get + val mediaType = MediaType.parse(DockerRegistryV2Abstract.ManifestDockerV2MediaType).toOption.get val contentType: Header = `Content-Type`(mediaType) val mockClient = Client({ _: Request[IO] => From f52693f2833e482ba2eb91b500de853f382a9afc Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 7 Feb 2023 13:59:57 -0500 Subject: [PATCH 4/6] Fix variable naming for clarity --- .../registryv2/DockerRegistryV2Abstract.scala | 34 +++++++++---------- .../DockerRegistryV2AbstractSpec.scala | 2 +- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala index e56c97c43ae..49139173ce9 100644 --- a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala +++ b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala @@ -27,25 +27,25 @@ object DockerRegistryV2Abstract { } val DigestHeaderName = CaseInsensitiveString("Docker-Content-Digest") - val ManifestDockerV2MediaType = "application/vnd.docker.distribution.manifest.v2+json" - val ManifestDockerListV2MediaType = "application/vnd.docker.distribution.manifest.list.v2+json" - val ManifestOCIIndexV1MediaType = "application/vnd.oci.image.index.v1+json" + val DockerManifestV2MediaType = "application/vnd.docker.distribution.manifest.v2+json" + val DockerManifestListV2MediaType = "application/vnd.docker.distribution.manifest.list.v2+json" + val OCIIndexV1MediaType = "application/vnd.oci.image.index.v1+json" // If one of those fails it means someone changed one of the strings above to an invalid one. - val ManifestV2MediaRange = MediaRange.parse(ManifestDockerV2MediaType) + val DockerManifestV2MediaRange = MediaRange.parse(DockerManifestV2MediaType) .unsafe("Cannot parse invalid manifest v2 content type. Please report this error.") - val ManifestListV2MediaRange = MediaRange.parse(ManifestDockerListV2MediaType) + val DockerManifestListV2MediaRange = MediaRange.parse(DockerManifestListV2MediaType) .unsafe("Cannot parse invalid manifest list v2 content type. Please report this error.") - val AcceptManifestV2Header = Accept.parse(ManifestDockerV2MediaType) + val AcceptDockerManifestV2Header = Accept.parse(DockerManifestV2MediaType) .unsafe("Cannot parse invalid manifest v2 Accept header. Please report this error.") - val OCIManifestV1MediaRange = MediaRange.parse(ManifestOCIIndexV1MediaType) - .unsafe("Cannot parse invalid oci manifest v1 content type. Please report this error.") - val AcceptOCIManifestV1Header = Accept.parse(ManifestOCIIndexV1MediaType) - .unsafe("Cannot parse invalid oci manifest v1 Accept header. Please report this error.") + val OCIIdexV1MediaRange = MediaRange.parse(OCIIndexV1MediaType) + .unsafe("Cannot parse invalid oci index v1 content type. Please report this error.") + val AcceptOCIIndexV1Header = Accept.parse(OCIIndexV1MediaType) + .unsafe("Cannot parse invalid oci index v1 Accept header. Please report this error.") - implicit val entityManifestDecoder = jsonEntityDecoder[DockerManifest](ManifestV2MediaRange) - implicit val entityManifestListDecoder = jsonEntityDecoder[DockerManifestList](ManifestListV2MediaRange) + implicit val entityManifestDecoder = jsonEntityDecoder[DockerManifest](DockerManifestV2MediaRange) + implicit val entityManifestListDecoder = jsonEntityDecoder[DockerManifestList](DockerManifestListV2MediaRange) implicit val entityTokenDecoder = jsonOf[IO, DockerAccessToken] /** @@ -127,8 +127,8 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi * @return docker info response */ protected def getDockerResponse(token: Option[String], dockerInfoContext: DockerInfoContext)(implicit client: Client[IO]): IO[DockerInfoSuccessResponse] = { - val requestV2 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptManifestV2Header) - def requestV1 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptOCIManifestV1Header) + val requestV2 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptDockerManifestV2Header) + def requestV1 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptOCIIndexV1Header) def tryOCIManifest(err: Throwable) = executeRequest(requestV1, handleManifestResponse(dockerInfoContext, token)) executeRequest(requestV2, handleManifestResponse(dockerInfoContext, token)) .handleErrorWith(tryOCIManifest) @@ -253,9 +253,9 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi * If that assumption turns out to be incorrect, a smarter decision may need to be made to choose the manifest to lookup. */ private def parseManifest(dockerImageIdentifier: DockerImageIdentifier, token: Option[String])(response: Response[IO])(implicit client: Client[IO]): IO[Option[DockerManifest]] = response match { - case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(ManifestDockerV2MediaType)) => + case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(DockerManifestV2MediaType)) => r.as[DockerManifest].map(Option.apply) - case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(ManifestDockerListV2MediaType)) => + case Status.Successful(r) if r.headers.exists(_.value.equalsIgnoreCase(DockerManifestListV2MediaType)) => r.as[DockerManifestList].flatMap({ dockerManifestList => obtainManifestFromList(dockerManifestList, dockerImageIdentifier, token) }) @@ -269,7 +269,7 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi .map(_.digest) .map(dockerImageIdentifier.swapReference) match { case Some(identifierWithNewHash) => - val request = manifestRequest(token, identifierWithNewHash, AcceptManifestV2Header) + val request = manifestRequest(token, identifierWithNewHash, AcceptDockerManifestV2Header) executeRequest(request, parseManifest(dockerImageIdentifier, token)) case None => logger.error(s"The manifest list for ${dockerImageIdentifier.fullName} was empty. Cannot proceed to obtain the size of image") diff --git a/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala b/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala index 1461069ebd1..e99383ac131 100644 --- a/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala +++ b/dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala @@ -20,7 +20,7 @@ class DockerRegistryV2AbstractSpec extends AnyFlatSpec with CromwellTimeoutSpec override protected def buildTokenRequestHeaders(dockerInfoContext: DockerInfoActor.DockerInfoContext) = List.empty } - val mediaType = MediaType.parse(DockerRegistryV2Abstract.ManifestDockerV2MediaType).toOption.get + val mediaType = MediaType.parse(DockerRegistryV2Abstract.DockerManifestV2MediaType).toOption.get val contentType: Header = `Content-Type`(mediaType) val mockClient = Client({ _: Request[IO] => From c7cc18a8b22da7d5a735f2df9a3fcbd3578a3c85 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 7 Feb 2023 14:01:07 -0500 Subject: [PATCH 5/6] Add comment explaining behavior --- .../cromwell/docker/registryv2/DockerRegistryV2Abstract.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala index 49139173ce9..4486a3dcbb1 100644 --- a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala +++ b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala @@ -130,6 +130,7 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi val requestV2 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptDockerManifestV2Header) def requestV1 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptOCIIndexV1Header) def tryOCIManifest(err: Throwable) = executeRequest(requestV1, handleManifestResponse(dockerInfoContext, token)) + // Try to execute a request using the Docker Manifest format, and if that fails, try using the newer OCI manifest format executeRequest(requestV2, handleManifestResponse(dockerInfoContext, token)) .handleErrorWith(tryOCIManifest) } From 3c67918e14218ffc8a8ad7a3eff7354d3a03e7bb Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 7 Feb 2023 14:58:24 -0500 Subject: [PATCH 6/6] Fix typos and add media type comment --- .../registryv2/DockerRegistryV2Abstract.scala | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala index 4486a3dcbb1..1a5a02d95bd 100644 --- a/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala +++ b/dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala @@ -29,6 +29,8 @@ object DockerRegistryV2Abstract { val DigestHeaderName = CaseInsensitiveString("Docker-Content-Digest") val DockerManifestV2MediaType = "application/vnd.docker.distribution.manifest.v2+json" val DockerManifestListV2MediaType = "application/vnd.docker.distribution.manifest.list.v2+json" + // See https://github.com/opencontainers/image-spec/blob/main/image-index.md + // This is the media type that current images of Ubuntu use, https://github.com/docker-library/official-images/pull/13950 val OCIIndexV1MediaType = "application/vnd.oci.image.index.v1+json" // If one of those fails it means someone changed one of the strings above to an invalid one. @@ -39,10 +41,10 @@ object DockerRegistryV2Abstract { val AcceptDockerManifestV2Header = Accept.parse(DockerManifestV2MediaType) .unsafe("Cannot parse invalid manifest v2 Accept header. Please report this error.") - val OCIIdexV1MediaRange = MediaRange.parse(OCIIndexV1MediaType) - .unsafe("Cannot parse invalid oci index v1 content type. Please report this error.") + val OCIIndexV1MediaRange = MediaRange.parse(OCIIndexV1MediaType) + .unsafe("Cannot parse invalid OCI index v1 content type. Please report this error.") val AcceptOCIIndexV1Header = Accept.parse(OCIIndexV1MediaType) - .unsafe("Cannot parse invalid oci index v1 Accept header. Please report this error.") + .unsafe("Cannot parse invalid OCI index v1 Accept header. Please report this error.") implicit val entityManifestDecoder = jsonEntityDecoder[DockerManifest](DockerManifestV2MediaRange) implicit val entityManifestListDecoder = jsonEntityDecoder[DockerManifestList](DockerManifestListV2MediaRange) @@ -127,11 +129,11 @@ abstract class DockerRegistryV2Abstract(override val config: DockerRegistryConfi * @return docker info response */ protected def getDockerResponse(token: Option[String], dockerInfoContext: DockerInfoContext)(implicit client: Client[IO]): IO[DockerInfoSuccessResponse] = { - val requestV2 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptDockerManifestV2Header) - def requestV1 = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptOCIIndexV1Header) - def tryOCIManifest(err: Throwable) = executeRequest(requestV1, handleManifestResponse(dockerInfoContext, token)) + val requestDockerManifest = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptDockerManifestV2Header) + lazy val requestOCIManifest = manifestRequest(token, dockerInfoContext.dockerImageID, AcceptOCIIndexV1Header) + def tryOCIManifest(err: Throwable) = executeRequest(requestOCIManifest, handleManifestResponse(dockerInfoContext, token)) // Try to execute a request using the Docker Manifest format, and if that fails, try using the newer OCI manifest format - executeRequest(requestV2, handleManifestResponse(dockerInfoContext, token)) + executeRequest(requestDockerManifest, handleManifestResponse(dockerInfoContext, token)) .handleErrorWith(tryOCIManifest) }