Skip to content

OCI builder: fixes from review of #2602 - #2603

Open
jnunn-aws wants to merge 6 commits into
normj/aws-oci-builderfrom
jonatnun/oci-builder-review-fixes
Open

jnunn-aws wants to merge 6 commits into
normj/aws-oci-builderfrom
jonatnun/oci-builder-review-fixes

Conversation

@jnunn-aws

Copy link
Copy Markdown
Contributor

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.

  • a6a72dd Exempt only ECR hosts and the configured proxy from the connect guard
  • 04185e1 Treat only 404 as absent and report registry denials as authentication errors
  • 821b9a9 Add FileManifest.AddDirectory for folder entries
  • f88fe01 Write layers as ustar so they are reproducible across processes
  • b78ea5a Add LayerPushProgressQueue for polling push progress
  • b3c3263 Expose the resolved base image digests

Tests and checks:

  • The library suite passes on net10.0, with tests added for each change.
  • A real push to a scratch ECR repository on pwsh 7.4 (.NET 8) and pwsh 7.6 (.NET 10) covered:
    • a push;
    • a re-push, where every layer was skipped;
    • an invalid token, rejected in under a second with no base-layer download.
  • Layers built in separate processes give the same digests on each runtime. GNU tar and Python's tarfile both read them.
  • Proxies: a local proxy, and one set at run time through HttpClient.DefaultProxy, both receive CONNECT. 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:

  • The connect guard no longer checks ECR hosts or the configured proxy. Every other host, including caller-supplied base registries, is still checked.
  • Exception types:
    • Denials after the handshake are RegistryAuthenticationException (now with StatusCode), not RegistryRequestException.
    • Transport failures are RegistryException, not HttpRequestException.
  • Existence checks: a HEAD redirect now counts as present, and only 404 counts as absent.
  • Layer bytes change because entries are ustar now. Builds are reproducible per .NET major version, not across versions, because zlib and the ustar uid/gid formatting differ between .NET 8 and 10.

Internal test seams: RegistryClient.Create takes proxy and resolveHost, there's a RegistryClient constructor overload with anonymous, FromBaseImageAsync has an overload that takes the source client factory, and Layer.CreateEntry/FitsUstar are internal.

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.
@jnunn-aws
jnunn-aws requested review from a team as code owners October 29, 2026 04:27
@jnunn-aws
jnunn-aws requested review from boblodgett and normj and removed request for a team October 29, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant