ContainerRegistry: Add a struct to represent a registry operation (#29)

euanh · web-flow · commit 483ad76a5739 · 2024-10-23T14:54:26.000+01:00
### Motivation

`RegistryClient.executeRequestThrowing` currently takes an `HTTPRequest` struct parameter and executes it using the underlying `HTTPClient`.   It augments `HTTPClient` by handling authentication challenges and decodes JSON error responses, but the use of `HTTPRequest` means that the caller is responsible for details such as generating the full distribution endpoint URL.   Giving `RegistryClient` its own abstract operation type frees the caller from dealing with these internal details.

The struct can also be passed easily anywhere the details of the operation are required, making future improvements easier.   For instance, if an operation causes an error, the original operation struct could be attached to the error making all its details available to the error handler.

### Modifications

Add a struct which holds all the necessary information about a requested operation on the registry, and refactor `RegistryClient` to use it.

### Result

No functional change.

This refactoring makes the separation of RegistryClient and HTTPClient's responsibilities clearer, and allows us to centralise the generation of registry URLs.  In the future it will make further improvements easier, such as changing how authentication is handled and providing more detailed error messages.

### Test Plan

Automated tests continue to pass; also tested manually.
diff --git a/Sources/ContainerRegistry/Blobs.swift b/Sources/ContainerRegistry/Blobs.swift
@@ -41,7 +41,7 @@ extension RegistryClient {
         // - Do not include the digest.
         // Response will include a 'Location' header telling us where to PUT the blob data.
         let httpResponse = try await executeRequestThrowing(
-            .post(registryURLForPath("/v2/\(repository)/blobs/uploads/")),
+            .post(repository, path: "blobs/uploads/"),
             expectingStatus: .accepted,  // expected response code for a "two-shot" upload
             decodingErrors: [.notFound]
         )
@@ -63,7 +63,7 @@ public extension RegistryClient {
 
         do {
             let _ = try await executeRequestThrowing(
-                .head(registryURLForPath("/v2/\(repository)/blobs/\(digest)")),
+                .head(repository, path: "blobs/\(digest)"),
                 decodingErrors: [.notFound]
             )
             return true
@@ -82,7 +82,7 @@ public extension RegistryClient {
         precondition(digest.count > 0, "digest must not be an empty string")
 
         return try await executeRequestThrowing(
-            .get(registryURLForPath("/v2/\(repository)/blobs/\(digest)"), accepting: ["application/octet-stream"]),
+            .get(repository, path: "blobs/\(digest)", accepting: ["application/octet-stream"]),
             decodingErrors: [.notFound]
         )
         .data
@@ -105,7 +105,7 @@ public extension RegistryClient {
         precondition(digest.count > 0, "digest must not be an empty string")
 
         return try await executeRequestThrowing(
-            .get(registryURLForPath("/v2/\(repository)/blobs/\(digest)"), accepting: ["application/octet-stream"]),
+            .get(repository, path: "blobs/\(digest)", accepting: ["application/octet-stream"]),
             decodingErrors: [.notFound]
         )
         .data
@@ -136,10 +136,9 @@ public extension RegistryClient {
         let digest = digest(of: data)
         location.queryItems = (location.queryItems ?? []) + [URLQueryItem(name: "digest", value: "\(digest.utf8)")]
         guard let uploadURL = location.url else { throw RegistryClientError.invalidUploadLocation("\(location)") }
-
         let httpResponse = try await executeRequestThrowing(
             // All blob uploads have Content-Type: application/octet-stream on the wire, even if mediatype is different
-            .put(uploadURL, contentType: "application/octet-stream"),
+            .put(repository, url: uploadURL, contentType: "application/octet-stream"),
             uploading: data,
             expectingStatus: .created,
             decodingErrors: [.badRequest, .notFound]
diff --git a/Sources/ContainerRegistry/CheckAPI.swift b/Sources/ContainerRegistry/CheckAPI.swift
@@ -22,8 +22,10 @@ public extension RegistryClient {
         // but this is not required and some do not.
         // The registry may require authentication on this endpoint.
         do {
+            // Using the bare HTTP client because this is the only endpoint which does not include a repository path
             let _ = try await executeRequestThrowing(
-                .get(registryURLForPath("/v2/")),
+                .get("", url: registryURL.distributionEndpoint),
+                expectingStatus: .ok,
                 decodingErrors: [.unauthorized, .notFound]
             )
             return true
diff --git a/Sources/ContainerRegistry/HTTPClient.swift b/Sources/ContainerRegistry/HTTPClient.swift
@@ -159,40 +159,4 @@ extension HTTPRequest {
         //    https://developer.apple.com/forums/thread/89811
         if let authorization { headerFields[.authorization] = authorization }
     }
-
-    static func get(
-        _ url: URL,
-        accepting: [String] = [],
-        contentType: String? = nil,
-        withAuthorization authorization: String? = nil
-    ) -> HTTPRequest {
-        .init(method: .get, url: url, accepting: accepting, contentType: contentType, withAuthorization: authorization)
-    }
-
-    static func head(
-        _ url: URL,
-        accepting: [String] = [],
-        contentType: String? = nil,
-        withAuthorization authorization: String? = nil
-    ) -> HTTPRequest {
-        .init(method: .head, url: url, accepting: accepting, contentType: contentType, withAuthorization: authorization)
-    }
-
-    static func put(
-        _ url: URL,
-        accepting: [String] = [],
-        contentType: String? = nil,
-        withAuthorization authorization: String? = nil
-    ) -> HTTPRequest {
-        .init(method: .put, url: url, accepting: accepting, contentType: contentType, withAuthorization: authorization)
-    }
-
-    static func post(
-        _ url: URL,
-        accepting: [String] = [],
-        contentType: String? = nil,
-        withAuthorization authorization: String? = nil
-    ) -> HTTPRequest {
-        .init(method: .post, url: url, accepting: accepting, contentType: contentType, withAuthorization: authorization)
-    }
 }
diff --git a/Sources/ContainerRegistry/Manifests.swift b/Sources/ContainerRegistry/Manifests.swift
@@ -21,7 +21,8 @@ public extension RegistryClient {
         let httpResponse = try await executeRequestThrowing(
             // All blob uploads have Content-Type: application/octet-stream on the wire, even if mediatype is different
             .put(
-                registryURLForPath("/v2/\(repository)/manifests/\(reference)"),
+                repository,
+                path: "manifests/\(reference)",
                 contentType: manifest.mediaType ?? "application/vnd.oci.image.manifest.v1+json"
             ),
             uploading: manifest,
@@ -35,8 +36,9 @@ public extension RegistryClient {
         // ECR does not set this header at all.
         // If the header is not present, create a suitable value.
         // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests
-        return try httpResponse.response.headerFields[.location]
-            ?? registryURLForPath("/v2/\(repository)/manifests/\(manifest.digest)").absoluteString
+        return httpResponse.response.headerFields[.location]
+            ?? registryURL.distributionEndpoint(forRepository: repository, andEndpoint: "manifests/\(manifest.digest)")
+            .absoluteString
     }
 
     func getManifest(repository: String, reference: String) async throws -> ImageManifest {
@@ -46,7 +48,8 @@ public extension RegistryClient {
 
         return try await executeRequestThrowing(
             .get(
-                registryURLForPath("/v2/\(repository)/manifests/\(reference)"),
+                repository,
+                path: "manifests/\(reference)",
                 accepting: [
                     "application/vnd.oci.image.manifest.v1+json",
                     "application/vnd.docker.distribution.manifest.v2+json",
@@ -63,7 +66,8 @@ public extension RegistryClient {
 
         return try await executeRequestThrowing(
             .get(
-                registryURLForPath("/v2/\(repository)/manifests/\(reference)"),
+                repository,
+                path: "manifests/\(reference)",
                 accepting: [
                     "application/vnd.oci.image.index.v1+json",
                     "application/vnd.docker.distribution.manifest.list.v2+json",
diff --git a/Sources/ContainerRegistry/RegistryClient.swift b/Sources/ContainerRegistry/RegistryClient.swift
@@ -115,33 +115,166 @@ public struct RegistryClient {
         let urlsession = URLSession(configuration: .ephemeral)
         try await self.init(registry: registryURL, client: urlsession, auth: auth)
     }
+}
 
-    func registryURLForPath(_ path: String) throws -> URL {
-        var components = URLComponents()
-        components.path = path
-        guard let url = components.url(relativeTo: registryURL) else {
-            throw RegistryClientError.invalidRegistryPath(path)
-        }
-        return url
+extension URL {
+    /// The base distribution endpoint URL
+    var distributionEndpoint: URL { self.appendingPathComponent("/v2/") }
+
+    /// The URL for a particular endpoint relating to a particular repository
+    /// - Parameters:
+    ///   - repository: The name of the repository.   May include path separators.
+    ///   - endpoint: The distribution endpoint e.g. "tags/list"
+    /// - Returns: A fully-qualified URL for the endpoint.
+    func distributionEndpoint(forRepository repository: String, andEndpoint endpoint: String) -> URL {
+        self.appendingPathComponent("/v2/\(repository)/\(endpoint)")
     }
 }
 
 extension RegistryClient {
+    /// Represents an operation to be executed on the registry.
+    struct RegistryOperation {
+        enum Destination {
+            case subpath(String)  // Repository subpath on the registry
+            case url(URL)  // Full destination URL, for example from a Location header returned by the registry
+        }
+
+        var method: HTTPRequest.Method  // HTTP method
+        var repository: String  // Repository path on the registry
+        var destination: Destination  // Destination of the operation: can be a subpath or remote URL
+        var accepting: [String] = []  // Acceptable response types
+        var contentType: String? = nil  // Request data type
+
+        func url(relativeTo registry: URL) -> URL {
+            switch destination {
+            case .url(let url): return url
+            case .subpath(let path):
+                let subpath = registry.distributionEndpoint(forRepository: repository, andEndpoint: path)
+                return subpath
+            }
+        }
+
+        // Convenience constructors
+        static func get(
+            _ repository: String,
+            path: String,
+            actions: [String]? = nil,
+            accepting: [String] = [],
+            contentType: String? = nil
+        ) -> RegistryOperation {
+            .init(
+                method: .get,
+                repository: repository,
+                destination: .subpath(path),
+                accepting: accepting,
+                contentType: contentType
+            )
+        }
+
+        static func get(
+            _ repository: String,
+            url: URL,
+            actions: [String]? = nil,
+            accepting: [String] = [],
+            contentType: String? = nil
+        ) -> RegistryOperation {
+            .init(
+                method: .get,
+                repository: repository,
+                destination: .url(url),
+                accepting: accepting,
+                contentType: contentType
+            )
+        }
+
+        static func head(
+            _ repository: String,
+            path: String,
+            actions: [String]? = nil,
+            accepting: [String] = [],
+            contentType: String? = nil
+        ) -> RegistryOperation {
+            .init(
+                method: .head,
+                repository: repository,
+                destination: .subpath(path),
+                accepting: accepting,
+                contentType: contentType
+            )
+        }
+
+        /// This handles the 'put' case where the registry gives us a location URL which we must not alter, aside from adding the digest to it
+        static func put(
+            _ repository: String,
+            url: URL,
+            actions: [String]? = nil,
+            accepting: [String] = [],
+            contentType: String? = nil
+        ) -> RegistryOperation {
+            .init(
+                method: .put,
+                repository: repository,
+                destination: .url(url),
+                accepting: accepting,
+                contentType: contentType
+            )
+        }
+
+        static func put(
+            _ repository: String,
+            path: String,
+            actions: [String]? = nil,
+            accepting: [String] = [],
+            contentType: String? = nil
+        ) -> RegistryOperation {
+            .init(
+                method: .put,
+                repository: repository,
+                destination: .subpath(path),
+                accepting: accepting,
+                contentType: contentType
+            )
+        }
+
+        static func post(
+            _ repository: String,
+            path: String,
+            actions: [String]? = nil,
+            accepting: [String] = [],
+            contentType: String? = nil
+        ) -> RegistryOperation {
+            .init(
+                method: .post,
+                repository: repository,
+                destination: .subpath(path),
+                accepting: accepting,
+                contentType: contentType
+            )
+        }
+    }
+
     /// Execute an HTTP request with no request body.
     /// - Parameters:
-    ///   - request: The HTTP request to execute.
+    ///   - operation: The Registry operation to execute.
     ///   - success: The HTTP status code expected if the request is successful.
     ///   - errors: Expected error codes for which the registry sends structured error messages.
     /// - Returns: An asynchronously-delivered tuple that contains the raw response body as a Data instance, and a HTTPURLResponse.
     /// - Throws: If the server response is unexpected or indicates that an error occurred.
     ///
     /// A plain Data version of this function is required because Data is Decodable and decodes from base64.
     /// Plain blobs are not encoded in the registry, so trying to decode them will fail.
-    public func executeRequestThrowing(
-        _ request: HTTPRequest,
+    func executeRequestThrowing(
+        _ operation: RegistryOperation,
         expectingStatus success: HTTPResponse.Status = .ok,
         decodingErrors errors: [HTTPResponse.Status]
     ) async throws -> (data: Data, response: HTTPResponse) {
+        let request = HTTPRequest(
+            method: operation.method,
+            url: operation.url(relativeTo: registryURL),
+            accepting: operation.accepting,
+            contentType: operation.contentType
+        )
+
         do {
             let authenticatedRequest = auth?.auth(for: request) ?? request
             return try await client.executeRequestThrowing(authenticatedRequest, expectingStatus: success)
@@ -166,8 +299,8 @@ extension RegistryClient {
     ///   - errors: Expected error codes for which the registry sends structured error messages.
     /// - Returns: An asynchronously-delivered tuple that contains the raw response body as a Data instance, and a HTTPURLResponse.
     /// - Throws: If the server response is unexpected or indicates that an error occurred.
-    public func executeRequestThrowing<Response: Decodable>(
-        _ request: HTTPRequest,
+    func executeRequestThrowing<Response: Decodable>(
+        _ request: RegistryOperation,
         expectingStatus success: HTTPResponse.Status = .ok,
         decodingErrors errors: [HTTPResponse.Status]
     ) async throws -> (data: Response, response: HTTPResponse) {
@@ -182,7 +315,7 @@ extension RegistryClient {
 
     /// Execute an HTTP request uploading a request body.
     /// - Parameters:
-    ///   - request: The HTTP request to execute.
+    ///   - operation: The Registry operation to execute.
     ///   - payload: The request body to upload.
     ///   - success: The HTTP status code expected if the request is successful.
     ///   - errors: Expected error codes for which the registry sends structured error messages.
@@ -191,12 +324,19 @@ extension RegistryClient {
     ///
     /// A plain Data version of this function is required because Data is Encodable and encodes to base64.
     /// Accidentally encoding data blobs will cause digests to fail and runtimes to be unable to run the images.
-    public func executeRequestThrowing(
-        _ request: HTTPRequest,
+    func executeRequestThrowing(
+        _ operation: RegistryOperation,
         uploading payload: Data,
         expectingStatus success: HTTPResponse.Status,
         decodingErrors errors: [HTTPResponse.Status]
     ) async throws -> (data: Data, response: HTTPResponse) {
+        let request = HTTPRequest(
+            method: operation.method,
+            url: operation.url(relativeTo: registryURL),
+            accepting: operation.accepting,
+            contentType: operation.contentType
+        )
+
         do {
             let authenticatedRequest = auth?.auth(for: request) ?? request
             return try await client.executeRequestThrowing(
@@ -224,20 +364,20 @@ extension RegistryClient {
 
     /// Execute an HTTP request uploading a Codable request body.
     /// - Parameters:
-    ///   - request: The HTTP request to execute.
+    ///   - operation: The Registry operation to execute.
     ///   - payload: The request body to upload.
     ///   - success: The HTTP status code expected if the request is successful.
     ///   - errors: Expected error codes for which the registry sends structured error messages.
     /// - Returns: An asynchronously-delivered tuple that contains the raw response body as a Data instance, and a HTTPURLResponse.
     /// - Throws: If the server response is unexpected or indicates that an error occurred.
-    public func executeRequestThrowing<Body: Encodable>(
-        _ request: HTTPRequest,
+    func executeRequestThrowing<Body: Encodable>(
+        _ operation: RegistryOperation,
         uploading payload: Body,
         expectingStatus success: HTTPResponse.Status,
         decodingErrors errors: [HTTPResponse.Status]
     ) async throws -> (data: Data, response: HTTPResponse) {
         try await executeRequestThrowing(
-            request,
+            operation,
             uploading: try encoder.encode(payload),
             expectingStatus: success,
             decodingErrors: errors
diff --git a/Sources/ContainerRegistry/Tags.swift b/Sources/ContainerRegistry/Tags.swift
@@ -16,10 +16,7 @@ public extension RegistryClient {
     func getTags(repository: String) async throws -> Tags {
         // See https://github.com/opencontainers/distribution-spec/blob/main/spec.md#listing-tags
         precondition(repository.count > 0, "repository must not be an empty string")
-        return try await executeRequestThrowing(
-            .get(registryURLForPath("/v2/\(repository)/tags/list")),
-            decodingErrors: [.notFound]
-        )
-        .data
+
+        return try await executeRequestThrowing(.get(repository, path: "tags/list"), decodingErrors: [.notFound]).data
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -16,10 +16,7 @@ public extension RegistryClient {`
`16`	`16`	`func getTags(repository: String) async throws -> Tags {`
`17`	`17`	`// See https://github.com/opencontainers/distribution-spec/blob/main/spec.md#listing-tags`
`18`	`18`	`precondition(repository.count > 0, "repository must not be an empty string")`
`19`		`- return try await executeRequestThrowing(`
`20`		`- .get(registryURLForPath("/v2/\(repository)/tags/list")),`
`21`		`- decodingErrors: [.notFound]`
`22`		`- )`
`23`		`- .data`
	`19`	`+`
	`20`	`+ return try await executeRequestThrowing(.get(repository, path: "tags/list"), decodingErrors: [.notFound]).data`
`24`	`21`	`}`
`25`	`22`	`}`