Skip to content

Disable fingerprint checking when storage is not available #3928

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

Merged
merged 1 commit into from
Dec 10, 2021
Merged

Disable fingerprint checking when storage is not available #3928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 17 additions & 2 deletions Sources/Commands/SwiftTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,20 @@ public class SwiftTool {
return .none
}
}

private func getSharedSecurityDirectory() throws -> AbsolutePath? {
do {
let fileSystem = localFileSystem
let sharedSecurityDirectory = fileSystem.swiftPMSecurityDirectory
if !fileSystem.exists(sharedSecurityDirectory) {
try fileSystem.createDirectory(sharedSecurityDirectory, recursive: true)
}
return sharedSecurityDirectory
} catch {
self.observabilityScope.emit(warning: "Failed creating shared security directory: \(error)")
return .none
}
}

/// Returns the currently active workspace.
func getActiveWorkspace() throws -> Workspace {
Expand All @@ -649,7 +663,8 @@ public class SwiftTool {

let delegate = ToolWorkspaceDelegate(self.outputStream, logLevel: self.logLevel, observabilityScope: self.observabilityScope)
let provider = GitRepositoryProvider(processSet: processSet)
let sharedCacheDirectory = try self.getSharedCacheDirectory()
let sharedSecurityDirectory = try self.getSharedSecurityDirectory()
let sharedCacheDirectory = try self.getSharedCacheDirectory()
let sharedConfigurationDirectory = try self.getSharedConfigurationDirectory()
let isXcodeBuildSystemEnabled = self.options.buildSystem == .xcode
let workspace = try Workspace(
Expand All @@ -658,7 +673,7 @@ public class SwiftTool {
workingDirectory: buildPath,
editsDirectory: self.editsDirectory(),
resolvedVersionsFile: self.resolvedVersionsFile(),
sharedSecurityDirectory: localFileSystem.swiftPMSecurityDirectory,
sharedSecurityDirectory: sharedSecurityDirectory,
sharedCacheDirectory: sharedCacheDirectory,
sharedConfigurationDirectory: sharedConfigurationDirectory
),
Expand Down
1 change: 0 additions & 1 deletion Sources/PackageFingerprint/Model.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,5 +65,4 @@ public typealias PackageFingerprints = [Version: [Fingerprint.Kind: Fingerprint]
public enum FingerprintCheckingMode: String {
case strict
case warn
case none
}
11 changes: 10 additions & 1 deletion Sources/PackageFingerprint/PackageFingerprintStorage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,16 @@ public extension PackageFingerprintStorage {
}
}

public enum PackageFingerprintStorageError: Error, Equatable {
public enum PackageFingerprintStorageError: Error, Equatable, CustomStringConvertible {
case conflict(given: Fingerprint, existing: Fingerprint)
case notFound

public var description: String {
switch self {
case .conflict(let given, let existing):
return "Fingerprint \(given) is different from previously recorded value \(existing)"
case .notFound:
return "Not found"
}
}
}
78 changes: 40 additions & 38 deletions Sources/PackageRegistry/RegistryClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,14 @@ public final class RegistryClient {
private let archiverProvider: (FileSystem) -> Archiver
private let httpClient: HTTPClient
private let authorizationProvider: HTTPClientAuthorizationProvider?
private let fingerprintStorage: PackageFingerprintStorage
private let fingerprintStorage: PackageFingerprintStorage?
private let fingerprintCheckingMode: FingerprintCheckingMode
private let jsonDecoder: JSONDecoder

public init(
configuration: RegistryConfiguration,
identityResolver: IdentityResolver,
fingerprintStorage: PackageFingerprintStorage,
fingerprintStorage: PackageFingerprintStorage? = .none,
fingerprintCheckingMode: FingerprintCheckingMode,
authorizationProvider: HTTPClientAuthorizationProvider? = .none,
customHTTPClient: HTTPClient? = .none,
Expand Down Expand Up @@ -423,27 +423,23 @@ public final class RegistryClient {
throw RegistryError.invalidSourceArchive
}

self.fingerprintStorage.put(package: package,
version: version,
fingerprint: .init(origin: .registry(registry.url), value: checksum),
observabilityScope: observabilityScope,
callbackQueue: callbackQueue) { storageResult in
switch storageResult {
case .success:
completion(.success(checksum))
case .failure(PackageFingerprintStorageError.conflict(_, let existing)):
switch self.fingerprintCheckingMode {
case .strict:
completion(.failure(RegistryError.checksumChanged(latest: checksum, previous: existing.value)))
case .warn:
observabilityScope.emit(warning: "The checksum \(checksum) from \(registry.url.absoluteString) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))")
if let fingerprintStorage = self.fingerprintStorage {
fingerprintStorage.put(package: package,
version: version,
fingerprint: .init(origin: .registry(registry.url), value: checksum),
observabilityScope: observabilityScope,
callbackQueue: callbackQueue) { storageResult in
switch storageResult {
case .success:
completion(.success(checksum))
case .none:
case .failure(let error):
// Don't throw write errors
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why do we no longer throw in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were doing two types of throw before:

  1. checksum has changed
  2. actual write failures (e.g., file system, serialization, etc.)

1 is sort of integrity check, and I think it's a bit strange to do it on a write operation when we are already doing it on read.

I suppose we can continue throwing 2 if that's the preference.

Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing a failure in Swift CI due to the removed none case at https://ci.swift.org/job/swift-PR-Linux-smoke-test/31790/consoleFull#6525590103122a513-f36a-4c87-8ed7-cbc36a1ec144:

/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-main/swiftpm/Tests/WorkspaceTests/RegistryPackageContainerTests.swift:421:39: error: type 'FingerprintCheckingMode' has no member 'none'
01:01:48             fingerprintCheckingMode: .none,
01:01:48                                      ~^~~~
01:01:48 error: fatalError

observabilityScope.emit(warning: "Failed to save checksum '\(checksum) from \(registry.url) to fingerprints storage: \(error)")
completion(.success(checksum))
}
case .failure(let error):
completion(.failure(error))
}
} else {
completion(.success(checksum))
}
} catch {
completion(.failure(RegistryError.failedRetrievingReleaseChecksum(error)))
Expand Down Expand Up @@ -535,8 +531,6 @@ public final class RegistryClient {
return completion(.failure(RegistryError.invalidChecksum(expected: expectedChecksum, actual: actualChecksum)))
case .warn:
observabilityScope.emit(warning: "The checksum \(actualChecksum) does not match previously recorded value \(expectedChecksum)")
case .none:
break
}
}

Expand Down Expand Up @@ -567,25 +561,33 @@ public final class RegistryClient {

// We either use a previously recorded checksum, or fetch it from the registry
func withExpectedChecksum(body: @escaping (Result<String, Error>) -> Void) {
self.fingerprintStorage.get(package: package,
version: version,
kind: .registry,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue) { result in
switch result {
case .success(let fingerprint):
body(.success(fingerprint.value))
case .failure(let error):
if error as? PackageFingerprintStorageError != .notFound {
observabilityScope.emit(error: "Failed to get registry fingerprint for \(package) \(version) from storage: \(error)")
if let fingerprintStorage = self.fingerprintStorage {
fingerprintStorage.get(package: package,
version: version,
kind: .registry,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue) { result in
switch result {
case .success(let fingerprint):
body(.success(fingerprint.value))
case .failure(let error):
if error as? PackageFingerprintStorageError != .notFound {
observabilityScope.emit(error: "Failed to get registry fingerprint for \(package) \(version) from storage: \(error)")
}
// Try fetching checksum from registry again no matter which kind of error it is
self.fetchSourceArchiveChecksum(package: package,
version: version,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue,
completion: body)
}
// Try fetching checksum from registry again no matter which kind of error it is
self.fetchSourceArchiveChecksum(package: package,
version: version,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue,
completion: body)
}
} else {
self.fetchSourceArchiveChecksum(package: package,
version: version,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue,
completion: body)
}
}
}
Expand Down
26 changes: 10 additions & 16 deletions Sources/Workspace/SourceControlPackageContainer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
private let manifestLoader: ManifestLoaderProtocol
private let toolsVersionLoader: ToolsVersionLoaderProtocol
private let currentToolsVersion: ToolsVersion
private let fingerprintStorage: PackageFingerprintStorage
private let fingerprintStorage: PackageFingerprintStorage?
private let fingerprintCheckingMode: FingerprintCheckingMode
private let observabilityScope: ObservabilityScope

Expand All @@ -79,7 +79,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
manifestLoader: ManifestLoaderProtocol,
toolsVersionLoader: ToolsVersionLoaderProtocol,
currentToolsVersion: ToolsVersion,
fingerprintStorage: PackageFingerprintStorage,
fingerprintStorage: PackageFingerprintStorage?,
fingerprintCheckingMode: FingerprintCheckingMode,
observabilityScope: ObservabilityScope
) throws {
Expand Down Expand Up @@ -150,6 +150,10 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
}

func checkIntegrity(version: Version, revision: Revision) throws {
guard let fingerprintStorage = self.fingerprintStorage else {
return
}

guard case .remoteSourceControl(let sourceControlURL) = self.package.kind else {
return
}
Expand All @@ -158,7 +162,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
let fingerprint: Fingerprint
do {
fingerprint = try temp_await {
self.fingerprintStorage.get(
fingerprintStorage.get(
package: packageIdentity,
version: version,
kind: .sourceControl,
Expand All @@ -172,7 +176,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
// Write to storage if fingerprint not yet recorded
do {
try temp_await {
self.fingerprintStorage.put(
fingerprintStorage.put(
package: packageIdentity,
version: version,
fingerprint: fingerprint,
Expand All @@ -181,16 +185,8 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
callback: $0
)
}
} catch PackageFingerprintStorageError.conflict(_, let existing) {
let message = "Revision \(revision.identifier) for \(self.package) version \(version) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))"
switch self.fingerprintCheckingMode {
case .strict:
throw StringError(message)
case .warn:
observabilityScope.emit(warning: message)
case .none:
return
}
} catch {
observabilityScope.emit(warning: "Failed to save revision '\(revision.identifier) from \(sourceControlURL) to fingerprints storage: \(error)")
}
} catch {
self.observabilityScope.emit(error: "Failed to get source control fingerprint for \(self.package) version \(version) from storage: \(error)")
Expand All @@ -205,8 +201,6 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
throw StringError(message)
case .warn:
observabilityScope.emit(warning: message)
case .none:
return
}
}
}
Expand Down
13 changes: 8 additions & 5 deletions Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ public class Workspace {
fileprivate let checksumAlgorithm: HashAlgorithm

/// The package fingerprint storage
fileprivate let fingerprintStorage: PackageFingerprintStorage
fileprivate let fingerprintStorage: PackageFingerprintStorage?

/// Enable prefetching containers in resolver.
fileprivate let resolverPrefetchingEnabled: Bool
Expand Down Expand Up @@ -259,6 +259,7 @@ public class Workspace {
/// - customHTTPClient: A custom http client.
/// - customArchiver: A custom archiver.
/// - customChecksumAlgorithm: A custom checksum algorithm.
/// - customFingerprintStorage: A custom fingerprint storage.
/// - additionalFileRules: File rules to determine resource handling behavior.
/// - resolverUpdateEnabled: Enables the dependencies resolver automatic version update check. Enabled by default. When disabled the resolver relies only on the resolved version file
/// - resolverPrefetchingEnabled: Enables the dependencies resolver prefetching based on the resolved version file. Enabled by default.
Expand Down Expand Up @@ -306,10 +307,12 @@ public class Workspace {
delegate: delegate.map(WorkspaceRepositoryManagerDelegate.init(workspaceDelegate:)),
cachePath: sharedRepositoriesCacheEnabled ? location.sharedRepositoriesCacheDirectory : .none
)
let fingerprintStorage = customFingerprintStorage ?? FilePackageFingerprintStorage(
fileSystem: fileSystem,
directoryPath: location.sharedFingerprintsDirectory
)
let fingerprintStorage = customFingerprintStorage ?? location.sharedFingerprintsDirectory.map {
FilePackageFingerprintStorage(
fileSystem: fileSystem,
directoryPath: $0
)
}

let registryClient = customRegistryClient ?? registries.map { configuration in
RegistryClient(
Expand Down
8 changes: 4 additions & 4 deletions Sources/Workspace/WorkspaceConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ extension Workspace {
public var resolvedVersionsFile: AbsolutePath

/// Path to the shared security directory
public var sharedSecurityDirectory: AbsolutePath
public var sharedSecurityDirectory: AbsolutePath?

/// Path to the shared cache directory
public var sharedCacheDirectory: AbsolutePath?
Expand Down Expand Up @@ -61,8 +61,8 @@ extension Workspace {
}

/// Path to the shared fingerprints directory.
public var sharedFingerprintsDirectory: AbsolutePath {
self.sharedSecurityDirectory.appending(component: "fingerprints")
public var sharedFingerprintsDirectory: AbsolutePath? {
self.sharedSecurityDirectory.map { $0.appending(component: "fingerprints") }
}

/// Path to the shared repositories cache.
Expand Down Expand Up @@ -103,7 +103,7 @@ extension Workspace {
workingDirectory: AbsolutePath,
editsDirectory: AbsolutePath,
resolvedVersionsFile: AbsolutePath,
sharedSecurityDirectory: AbsolutePath,
sharedSecurityDirectory: AbsolutePath?,
sharedCacheDirectory: AbsolutePath?,
sharedConfigurationDirectory: AbsolutePath?
) {
Expand Down
Loading