Use FilePath for PublishSocket#1594
Conversation
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
Looks good, but we've added an absolutePath helper in containerization that we can now use, and I think things will be more maintainable if we add validation to the PublishSocket.init() calls so that a PublishSocket is at lexically correct by construction (we don't need to do semantic checks, such as file/directory existence in the initializers).
| containerPath: URL, | ||
| hostPath: URL, | ||
| containerPath: FilePath, | ||
| hostPath: FilePath, |
There was a problem hiding this comment.
What constraints/invariants exist on these paths today? If any do exist, should we enforce them in init() so that PublishSocket objects are correct by construction?
We should docc the parameters here and if there are constraints, describe them as well.
| /// that still decode these fields as `URL`. | ||
| public func encode(to encoder: any Encoder) throws { | ||
| var container = encoder.container(keyedBy: CodingKeys.self) | ||
| try container.encode(Self.encodePath(containerPath), forKey: .containerPath) |
There was a problem hiding this comment.
If we can get the change in before the end of the week or very early next week (1.0 release), we should do a breaking change on the XPC wire format and encode the absolute path but as the string representation of the path instead of as a URL.
The decoder should preserve compatibility (which it looks like you have with decodePath) for a few versions so that persisted config values from older releases continue to work.
We could in a separate PR add a persistent config migration at containers service startup in the API server.
There was a problem hiding this comment.
We can update the containerization dependency to 0.33.2 and use FilePathOps.absolutePath(path:).
Closes CHAOS-1459. Unblocks CHAOS-1463 (Parser).
chrisgeo
force-pushed
the
feat/chaos-1459-publish-socket-filepath
branch
from
b7299a8 to
1e7f495
Compare
- PublishSocket.init now throws and validates absolute paths so objects
are correct by construction; adds DocC for parameters and constraints
(addresses comment on PublishSocket.swift:33 and :72).
- Adds deprecation/migration note on the struct documenting that the
decoder accepts both new `FilePath` and legacy `URL` forms for one
release (addresses comment on PublishSocket.swift:21).
- Wire format pre-1.0 breaking change: encoder now emits the plain
absolute path (e.g. "/var/run/docker.sock") instead of the legacy
file-URL form ("file:///var/run/docker.sock"). The decoder still
accepts both forms so persisted bundles from earlier releases
continue to load; that compatibility will be removed in a later
release (addresses comment on PublishSocket.swift:58).
- Bumps containerization to 0.33.2 and replaces
URL(fileURLWithPath:).absoluteURL.path with
ContainerizationOS.FilePathOps.absolutePath in Parser.publishSocket,
removing the duplicate "must be absolute" check now that init()
enforces it (addresses comments on Parser.swift:754 and :762).
|
@jglogan I believe I addressed all concerns, please let me know if there's any I missed. |
3 participants
Type of Change
Motivation and Context
Migrates
PublishSocket.containerPathandPublishSocket.hostPathfromFoundation.URLtoSystemPackage.FilePath, consistent with the project-wide effort to replaceURL/Stringpaths with strongly-typedFilePath/SystemPathat API boundaries (#1480, #1493, #1518, #1522, #1523, #1570, #1574, #1580).PublishSocketwas the lastURL-typed path inContainerResource.URLis the wrong type for filesystem paths: it implies a navigable resource, permits non-file schemes, and forces consumers through percent-encoding when interoperating with POSIX APIs.FilePathis purpose-built,Sendable, and already used by both callers of this struct after the recent runtime reorg (#1577).Tracking issue: #1593
Wire-format compatibility
PublishSocketis serialized to disk as part of container bundles and over XPC. To keep this migration non-breaking on the wire:URL(filePath: path.string).absoluteString— byte-identical to the priorURL-typedJSONEncoderoutput (e.g."file:///var/run/docker.sock"). Existing bundles and any reader still decoding these fields asURLcontinue to work.file://form, plain absolute paths (forward-compat), and resolves percent-encoding. Empty, relative, malformed, or non-localhost-host inputs throwDecodingError.dataCorruptedso corrupt state fails loudly rather than silently producing an invalid socket path.Source compatibility
Source-breaking for any external consumer constructing
PublishSocketdirectly withURL. Acceptable given the pre-1.0 project status and the consistent direction of prior migrations.Testing
swift test --filter ContainerResourceTests— 50/50 passing)Tests/ContainerResourceTests/PublishSocketTests.swift, 14 cases covering:URLencoding (pinned via a siblingLegacyPublishSocketmirror)file://form, percent-encoded paths, andfile://localhost/...permissions"file:","file://", relative paths, and non-local-host file URLsBuild verified clean:
swift build --target ContainerRuntimeLinuxServer.