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.

Sign up for GitHub

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

WX-926 Support falling back to OCI Manifest Format #7003

Merged
merged 6 commits into from
Feb 7, 2023
Merged

WX-926 Support falling back to OCI Manifest Format #7003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
513110b
WX-926 Support falling back to OCI Manifest Format
kraefrei Feb 7, 2023
fb8f88c
First pass at recovery logic
kraefrei Feb 7, 2023
6b97559
Update test
kraefrei Feb 7, 2023
f52693f
Fix variable naming for clarity
kraefrei Feb 7, 2023
c7cc18a
Add comment explaining behavior
kraefrei Feb 7, 2023
3c67918
Fix typos and add media type comment
kraefrei Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 27 additions & 15 deletions dockerHashing/src/main/scala/cromwell/docker/registryv2/DockerRegistryV2Abstract.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -27,19 +27,27 @@ 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 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.
val ManifestV2MediaRange = MediaRange.parse(ManifestV2MediaType)
val DockerManifestV2MediaRange = MediaRange.parse(DockerManifestV2MediaType)
.unsafe("Cannot parse invalid manifest v2 content type. Please report this error.")
val ManifestListV2MediaRange = MediaRange.parse(ManifestListV2MediaType)
val DockerManifestListV2MediaRange = MediaRange.parse(DockerManifestListV2MediaType)
.unsafe("Cannot parse invalid manifest list v2 content type. Please report this error.")
val AcceptManifestV2Header = Accept.parse(ManifestV2MediaType)
val AcceptDockerManifestV2Header = Accept.parse(DockerManifestV2MediaType)
.unsafe("Cannot parse invalid manifest v2 Accept header. Please report this error.")

implicit val entityManifestDecoder = jsonEntityDecoder[DockerManifest](ManifestV2MediaRange)
implicit val entityManifestListDecoder = jsonEntityDecoder[DockerManifestList](ManifestListV2MediaRange)
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.")

implicit val entityManifestDecoder = jsonEntityDecoder[DockerManifest](DockerManifestV2MediaRange)
implicit val entityManifestListDecoder = jsonEntityDecoder[DockerManifestList](DockerManifestListV2MediaRange)
implicit val entityTokenDecoder = jsonOf[IO, DockerAccessToken]

/**
Expand Down Expand Up @@ -121,8 +129,12 @@ 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 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice to have - we have a function called firstSuccess that we use for certain WDL things. Could fit well here, and scales to arbitrary numbers of "things to try".

executeRequest(requestDockerManifest, handleManifestResponse(dockerInfoContext, token))
.handleErrorWith(tryOCIManifest)
}

/**
Expand Down Expand Up @@ -203,12 +215,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: _*
)
Expand Down Expand Up @@ -244,9 +256,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(DockerManifestV2MediaType)) =>
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(DockerManifestListV2MediaType)) =>
r.as[DockerManifestList].flatMap({ dockerManifestList =>
obtainManifestFromList(dockerManifestList, dockerImageIdentifier, token)
})
Expand All @@ -260,7 +272,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, 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")
Expand Down
6 changes: 3 additions & 3 deletions dockerHashing/src/test/scala/cromwell/docker/registryv2/DockerRegistryV2AbstractSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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.DockerManifestV2MediaType).toOption.get
val contentType: Header = `Content-Type`(mediaType)

val mockClient = Client({ _: Request[IO] =>
Expand Down