Repository navigation
OCI builder: fixes from review of #2602 - #2603
Conversation
The connect guard refused every loopback or private address, including the proxy connection, so pushes failed behind any local or corporate proxy, and ECR behind an interface VPC endpoint (which resolves to private addresses) was refused too. - Skip the address check for an Amazon ECR registry host and for the configured proxy: the target of the handler's CONNECT tunnel request, or the proxy that IWebProxy.GetProxy selects (not when it returns the destination itself). Every other host, including caller-supplied base registries, is still checked. - Match the ECR host pattern against the host alone, anchored with \z. - Report transport failures (DNS, connection, TLS, proxy, the guard) as RegistryException with the HttpRequestException as the inner exception. - RegistryClient.Create takes internal proxy and resolveHost parameters so tests can drive a local proxy and simulate private DNS.
…n errors BlobExistsAsync and ManifestExistsAsync returned false for any status other than 200. With an expired ECR token a push treated every base layer as missing and downloaded it before failing on the upload. - A HEAD success or redirect means present, 404 means absent, and anything else throws. - 401, 403 and ECR's 400 DENIED after the handshake become RegistryAuthenticationException, which now carries StatusCode. ECR answers a HEAD with a bare 400, so a GET of /v2/ is used to read the DENIED error. - The message for a client that sends no credentials (base-image pulls) points at a missing repository or tag rather than credentials.
A layer has no entries for the parent folders of mapped files, and some image consumers reject an image whose new folders have files but no entries of their own. AddDirectory adds a 0755 folder entry without a local source. Building a layer now also rejects a file or folder beneath a path that is mapped as a file.
.NET's PAX writer names each extended header PaxHeaders.<process id>, so the same files built in two processes gave different layer and image digests, and every build uploaded a new layer. Entries are now ustar, which has no atime or ctime fields, with fixed times and owners. An entry ustar cannot hold (a name too long for its fields, or a file of 8 GiB or more) falls back to PAX; whether it fits is decided by asking .NET's ustar writer. Directory names longer than 100 bytes drop their trailing slash so the name field is never left empty. GNU entries were not used: .NET 8 writes the current time into their atime and ctime, and the epoch written there makes Python's tarfile misread names.
System.Progress<T> raises callbacks on thread-pool threads when there is no synchronization context, which breaks PowerShell scriptblocks (no runspace) and any sink that is not thread-safe. LayerPushProgressQueue queues events for the caller to drain with TryDequeue or DequeueAll. The EcrPushOptions docs and README now describe Progress<T> accurately and show the queue.
OciImageBuilder.BaseImageDigest is the digest of the platform manifest the build is based on, and BaseImageIndexDigest the manifest list it was selected from (null for a single-platform reference). An internal FromBaseImageAsync overload takes the source client factory for tests.
| string registryHost, | ||
| IRegistryCredentialSource credentialSource, | ||
| TimeSpan? timeout = null, | ||
| IWebProxy? proxy = null, |
There was a problem hiding this comment.
I don't see how the user can take advantage of the proxy support. Do we need to add an option in EcrPushOptions for user to be able to specify the proxy and timeout?
There was a problem hiding this comment.
Yes. EcrPushOptions now has Proxy, next to the existing UploadTimeout. The pull needed the same, so a new FromBaseImageAsync(baseImage, platform, BaseImagePullOptions) overload takes Proxy and Timeout. That client also downloads the base layers during the push, so its options cover those requests too. Both default to the old behaviour (HttpClient.DefaultProxy, no client timeout), and the connect guard exempts whichever proxy is passed. The existing FromBaseImageAsync signature is unchanged. Fixed in dda9398.
| /// folders the base image already has, or their mode is reset to <c>0755</c>. | ||
| /// </remarks> | ||
| /// <param name="containerPath">The absolute folder path inside the container (for example <c>/opt/app/lib</c>).</param> | ||
| public void AddDirectory(string containerPath) |
There was a problem hiding this comment.
I would prefer AddEmptyDirectory to call out this is a special API to add something that doesn't map to something public.
There was a problem hiding this comment.
Renamed to AddEmptyDirectory, and the Directories property to EmptyDirectories to match. Fixed in 40cf91d.
| /// writer, against a throwaway stream, rather than re-implementing its rule for splitting a long name into the | ||
| /// prefix and name fields (it does not accept every split the format allows). | ||
| /// </summary> | ||
| internal static bool FitsUstar(TarEntryType type, string name, long size) |
There was a problem hiding this comment.
I was nervous about adding this complexity for the reproducible output and figured in our case most build images would change. Looking at your change for this it doesn't look too bad. I'm not fan of using the exception handling for control flow and worry if MSFT changes where the exception is thrown we would break. What do you think of this version:
// ustar header limits: a 100-byte name field, and an 11-digit octal size field, so the
// largest size is 8^11 - 1 (one byte under 8 GiB).
private const int UstarNameFieldLength = 100;
private const long UstarMaxSize = (1L << 33) - 1;
/// <summary>
/// Whether an entry can be written as ustar: its size fits the size field and its UTF-8 name fits
/// the name field. Longer names use PAX. ustar's 155-byte prefix field isn't used, because .NET
/// decides how to split a name between the two fields and doesn't document how.
/// </summary>
internal static bool FitsUstar(string name, long size)
=> size <= UstarMaxSize && Encoding.UTF8.GetByteCount(name) <= UstarNameFieldLength;
We also wouldn't need the HeaderProbeStream type as well with this approach.
There was a problem hiding this comment.
Agreed on the exception handling. FitsUstar now checks the field limits itself, and HeaderProbeStream is gone. This keeps the prefix field, though. .NET names each PAX extended header PaxHeaders.<process id>, so a PAX entry gives the layer a new digest in every process. 47 of the 615 entries in the PowerShell 7.6 Linux tarball are over 100 bytes, so that layer would be uploaded again on every build. The check splits at the last /, with a prefix of up to 155 bytes and a name of up to 100. .NET tries that split first, so it always accepts these names. A new test round-trips 2,000 generated names through TarWriter and TarReader to catch a change in how .NET splits them. Fixed in 5de4198.
normj
added
the
Release Not Needed
The method adds a folder entry that maps nothing from the local filesystem, unlike the other Add methods, and the name now says so. The Directories property is renamed to EmptyDirectories to match.
FitsUstar no longer writes a throwaway header and catches the exception; it checks the size field limit and the name and prefix field lengths itself, and HeaderProbeStream is removed. A name over 100 bytes stays ustar when its split at the last '/' gives a prefix of up to 155 bytes and a non-empty name of up to 100 bytes. .NET tries that split first, so it always accepts these names. Sending every name over 100 bytes to PAX instead would make those layers differ across processes, because PAX headers carry the process id; 47 of the 615 entries in the pwsh 7.6 tarball are over 100 bytes. A test generates 2,000 names, writes each one FitsUstar accepts with .NET's ustar writer and reads them back with TarReader, which catches a change in how .NET splits names.
A caller could set a proxy only process-wide, through HTTPS_PROXY or HttpClient.DefaultProxy, and could not set a timeout for the pull. EcrPushOptions.Proxy sets the proxy for requests to ECR, next to the existing UploadTimeout. A new FromBaseImageAsync overload takes BaseImagePullOptions with Proxy and Timeout for the base image's registry; that client also downloads base layers during the push. Both default to today's behaviour, and the connect guard exempts whichever proxy is used. The existing FromBaseImageAsync signature is unchanged.
These are fixes for the larger items in my review of #2602, one commit per finding, each with its tests. Merge, cherry-pick or close as you see fit; each commit builds and passes on its own. The smaller items are suggested changes on #2602 instead.
Tests and checks:
tarfileboth read them.HttpClient.DefaultProxy, both receiveCONNECT. The ECR VPC endpoint case is covered by a test that resolves the ECR host to a private address. It hasn't been run in a real VPC.Behavior changes worth a look:
RegistryAuthenticationException(now withStatusCode), notRegistryRequestException.RegistryException, notHttpRequestException.Internal test seams:
RegistryClient.CreatetakesproxyandresolveHost, there's aRegistryClientconstructor overload withanonymous,FromBaseImageAsynchas an overload that takes the source client factory, andLayer.CreateEntry/FitsUstarare internal.