OCI registry support for Bicep extension publishing#18956
OCI registry support for Bicep extension publishing#18956willdavsmith wants to merge 4 commits intoAzure:mainfrom
Conversation
|
Related to #4884 |
|
I'm very happy to see this coming along! |
Adds ORAS-based transport for non-Azure OCI registries (GHCR, Docker Hub, etc). New experimental feature flag 'ociEnabled' gates the new code path. Includes priority-based registry provider routing, Docker credential support, and session-based push/pull/resolve operations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0bddcfd to
4afe9d0
Compare
Adds ORAS-based transport for non-Azure OCI registries (GHCR, Docker Hub, etc). New experimental feature flag 'ociEnabled' gates the new code path. Includes priority-based registry provider routing, Docker credential support, and session-based push/pull/resolve operations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4afe9d0 to
2d48764
Compare
|
Good news! I've successfully tested this to upload my extension to GitHub:
...I still need to test it restoring (without credentials) and I should test it with RedHat's Podmon instead of Docker. |
Signed-off-by: willdavsmith <willdavsmith@gmail.com>
Awesome! Thanks for trying it out and I'm glad it's useful for you. Feel free to update here, and if my commits break anything, please let me know. |
|
@willdavsmith successful demo with a dev build of the VS Code extension restoring the extension off GitHub for my Bicep file. |
|
@willdavsmith thank you for your work on this. this will make usage in an enterprise environment where self managed OCI registries exist finally a reality. |
|
|
||
| public IDirectoryHandle CacheRootDirectory => GetCacheRootDirectory(this.configuration.CacheRootDirectory); | ||
|
|
||
| public bool OciEnabled => this.configuration.ExperimentalFeaturesEnabled.OciEnabled || ReadBooleanEnvVar("BICEP_EXPERIMENTAL_OCI", defaultValue: false); |
There was a problem hiding this comment.
Do we need this env var at all? Generally we manage feature flags purely through bicepconfig, rather than duplicating.
There was a problem hiding this comment.
No, it was just set for testing. Removed.
Signed-off-by: willdavsmith <willdavsmith@gmail.com>
| var reference = ValidateReference(args.TargetExtensionReference); | ||
| var overwriteIfExists = args.Force; | ||
|
|
||
| logger.LogInformation("Preparing to publish extension \"{Reference}\" (force={Force}, indexFile={IndexFile}, binariesSpecified={BinaryCount}).", |
There was a problem hiding this comment.
The logger here will output everythign to stdout, whereas it looks more like the intention here is to add tracing.
I'd suggest either:
- Removing the logging
- Using trace methods instead (e.g.
Trace.WriteLine(...))
| "BCP367", | ||
| $"The \"{featureName}\" feature is temporarily disabled."); | ||
|
|
||
| public Diagnostic NonAzureOciRegistryRequiresExperimentalFeature(string registry) => CoreError( |
There was a problem hiding this comment.
[nit] Could you add this to the file in ascending code order (e.g. after BCP445)?
| { | ||
| var value = Environment.GetEnvironmentVariable(envVar); | ||
| if (value is null) | ||
| { | ||
| return defaultValue; | ||
| } | ||
|
|
||
| if (bool.TryParse(value, out var boolValue)) | ||
| { | ||
| return boolValue; | ||
| } | ||
|
|
||
| if (int.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var intValue)) | ||
| { | ||
| if (intValue == 1) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (intValue == 0) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
Is this change strictly necessary, now you've removed the use of env vars for feature flag enablement? If not, let's remove it.
| services.TryAddSingleton<IOciRegistryTransportFactory>(sp => | ||
| { | ||
| var providerFactory = sp.GetRequiredService<RegistryProviderFactory>(); | ||
| return new OciRegistryTransportFactory(providerFactory); | ||
| }); |
There was a problem hiding this comment.
Is this necessary? Can't you just write:
services.TryAddSingleton<IOciRegistryTransportFactory, OciRegistryTransportFactory>();
| services.TryAddSingleton<AzureContainerRegistryManager>(); | ||
| services.TryAddSingleton<DockerCredentialProvider>(); | ||
| services.TryAddSingleton<DockerCredentialSource>(); | ||
| services.TryAddSingleton<ICredentialSource>(sp => sp.GetRequiredService<DockerCredentialSource>()); |
There was a problem hiding this comment.
It doesn't seem right that requesting the generic interface ICredentialSource should always give you an instance of DockerCredentialSource. If that's intentional, should we rename ICredentialSource to IDockerCredentialSource to indicate that you are specifically requesting the Docker implementation?
| <ItemGroup> | ||
| <PackageReference Include="OrasProject.Oras" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
[nit] Add to the section with the other PackageReference blocks
| serviceProvider.GetRequiredService<IPublicModuleMetadataProvider>(), | ||
| serviceProvider.GetRequiredService<ICredentialChain>(), | ||
| serviceProvider.GetService<ILogger<OciArtifactRegistry>>() ?? NullLogger<OciArtifactRegistry>.Instance), | ||
| new TemplateSpecModuleRegistry(templateSpecRepositoryFactory), |
There was a problem hiding this comment.
Why not follow the standard DI pattern of passing these in as constructor arguments?
| { | ||
| foreach (var tag in manifestProps.Tags) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); |
There was a problem hiding this comment.
[nit] The explicit cancellation checks feel unnecessary, given that you're passing the token down to async methods correctly.
|
|
||
| // Fall back to authenticated client. | ||
| return await DownloadManifestInternalAsync(anonymousAccess: false); | ||
| return await DownloadManifestInternalAsync(anonymousAccess: false, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
[nit] we haven't been requiring this pattern in this repo, given it's a .NET 10 project.
| @@ -23,14 +24,38 @@ public static class TypesV1Archive | |||
| { | |||
| public static async Task<BinaryData> PackIntoBinaryData(IFileHandle typeIndexFile) | |||
There was a problem hiding this comment.
Why were the changes to this file necessary?
| private static readonly ImmutableHashSet<string> AzureRegistryHosts = | ||
| ImmutableHashSet.Create(StringComparer.OrdinalIgnoreCase, "mcr.microsoft.com"); |
There was a problem hiding this comment.
It feels odd to treat mcr.microsoft.com as an "Azure registry host", which I would assume means an "ACR". Is this correct?
| @@ -34,6 +38,20 @@ public static IServiceCollection AddBicepCore(this IServiceCollection services) | |||
| services.TryAddSingleton<INamespaceProvider, NamespaceProvider>(); | |||
| services.TryAddSingleton<IResourceTypeProviderFactory, ResourceTypeProviderFactory>(); | |||
| services.TryAddSingleton<IContainerRegistryClientFactory, ContainerRegistryClientFactory>(); | |||
There was a problem hiding this comment.
It looks like this interface and class are now ACR-specific - is that correct? If so, should we name them something specific to make this clearer?
|
|
||
| namespace Bicep.Core.Registry.Sessions; | ||
|
|
||
| public sealed record RegistryRef(string Host, string Repository, string? Tag, string? Digest); |
There was a problem hiding this comment.
How does this overlap with the existing IOciArtifactAddressComponents interface?
| IOciRegistryTransport GetTransport(OciArtifactReference reference); | ||
|
|
||
| IOciRegistryTransport GetTransport(string registry); | ||
|
|
||
| IRegistrySession CreateSession(RegistryRef reference, RegistryProviderContext context); |
There was a problem hiding this comment.
It looks like there's some overlap between IRegistrySession and IOciRegistryTransport, and the naming of these 2 interfaces doesn't make it super clear to me what the 2 intended responsibilities are. Could you clarify on this?
|
|
||
| namespace Bicep.Core.Registry.Sessions; | ||
|
|
||
| public sealed record RegistryRef(string Host, string Repository, string? Tag, string? Digest); |
There was a problem hiding this comment.
RegistryRef looks structurally identical to IOciArtifactAddressComponents (same fields, just Host renamed to Registry). The only usage in OciArtifactRegistry is CreateRegistryRef, which projects an OciArtifactReference into a RegistryRef — and AcrRegistrySession then immediately wraps it back into a private SessionOciArtifactReference that re-implements IOciArtifactReference. Could we just pass IOciArtifactAddressComponents (or OciArtifactReference) directly to IRegistrySession, and avoid the round-trip?
|
|
||
| namespace Bicep.Core.Registry.Sessions; | ||
|
|
||
| public sealed record RegistryArtifact( |
There was a problem hiding this comment.
RegistryArtifact looks like OciManifest minus SchemaVersion. After a pull via IRegistrySession.PullAsync, the result is immediately converted back to OciArtifactResult via ConvertToOciArtifactResult. Could we just use OciManifest for push input and OciArtifactResult for pull output, and avoid the conversion step?
|
|
||
| namespace Bicep.Core.Registry.Sessions; | ||
|
|
||
| public sealed record RegistryManifestInfo( |
There was a problem hiding this comment.
The fields here (Digest, MediaType, ArtifactType, Annotations) are a subset of what's already available from OciManifest + OciArtifactResult.ManifestDigest. Could ResolveAsync return OciArtifactResult instead, so we don't need a separate type?
|
|
||
| public override async Task<bool> CheckArtifactExists(ArtifactType artifactType, OciArtifactReference reference) | ||
| { | ||
| if (reference.ReferencingFile.Features.OciEnabled) |
There was a problem hiding this comment.
Every operation (PublishModule, PublishExtension, CheckArtifactExists, TryRestoreArtifactAsync, TryGetOciAnnotations) has an if (OciEnabled) { session path } else { transport path } fork. Since AcrRegistrySession already wraps AzureContainerRegistryManager, is there a reason the ACR path can't go through IRegistrySession unconditionally? It would simplify things a lot and mean we're not carrying dead code once the feature flag is removed.
|
|
||
| namespace Bicep.Core.Registry.Auth; | ||
|
|
||
| public interface ICredentialSource |
There was a problem hiding this comment.
There's only one ICredentialSource implementation (DockerCredentialSource), and CompositeCredentialChain just iterates a single-element list. Also, the AuthMetadata? challenge parameter is accepted by all the credential interfaces but never actually read in DockerCredentialSource. Could we simplify this — e.g. inject DockerCredentialProvider directly into OrasRegistrySession, and add back the composite pattern only if a second source is actually needed?
| /// <summary> | ||
| /// Represents a strategy for handling registry interactions for a specific host or group of hosts. | ||
| /// </summary> | ||
| public interface IRegistryProvider |
There was a problem hiding this comment.
The routing chain here is OciArtifactRegistry → IOciRegistryTransportFactory → RegistryProviderFactory → IRegistryProvider, and both OciRegistryTransportFactory and RegistryProviderFactory do essentially the same thing (resolve by hostname, delegate). The Name/Priority/CanHandle strategy pattern on IRegistryProvider feels heavyweight for a two-case dispatch (*.azurecr.io vs everything else). Could these layers be collapsed?
|
|
||
| public required CloudConfiguration Cloud { get; init; } | ||
|
|
||
| public CancellationToken CancellationToken { get; init; } |
There was a problem hiding this comment.
Passing CancellationToken through a context object is unusual in .NET — tokens normally flow through method arguments. Looking at the usage, TryCreateSession always passes default for the token, and every session method already accepts its own token directly. Could we remove the token from the context? Similarly, ILogger and CloudConfiguration feel like they should be constructor parameters on the session implementations (via DI) rather than passed in at session-creation time.
OCI Registry Support for Bicep Extension Publishing
Description
Adds support for publishing and restoring Bicep modules and extensions to/from non-Azure OCI-compliant container registries (GHCR, Docker Hub, etc.).
Previously,
publish,restore, andpublish-extensiononly worked with Azure Container Registry (ACR) because the transport was hard-coded to the Azure SDK'sContainerRegistryContentClient. This change introduces an alternative transport built on ORAS (OCI Registry As Storage), so Bicep artifacts can be stored in any compliant registry.Related to #4884.
User Experience
Feature flag
The feature is gated behind an experimental flag called
ociEnabled. Any one of the following enables it:--oci-enabledbefore the subcommand:bicep --oci-enabled publish myModule.bicep \ --target 'br:ghcr.io/myorg/bicep/modules/my-module:v1.0'BICEP_EXPERIMENTAL_OCI=1(ortrue)bicepconfig.json:{ "experimentalFeaturesEnabled": { "ociEnabled": true } }Authentication
When the flag is enabled, Bicep authenticates to non-Azure registries using Docker credentials:
~/.docker/config.jsonand invokes the configuredcredsStoreor per-registrycredHelpers(e.g.docker-credential-desktop,docker-credential-ecr-login).authentries — falls back to base64-encodedusername:passwordentries in theauthssection of~/.docker/config.json.ACR (
*.azurecr.io,mcr.microsoft.com) continues to use the existing Azure SDK auth path.Example: publish a module to GHCR
Example: publish an extension
bicep --oci-enabled publish-extension \ --target 'br:myregistry.example.com/bicep/extensions/my-ext:v1.0' \ --index-file ./out/index.jsonNew diagnostic: BCP440
Referencing a non-Azure registry without the flag produces:
Design
Registry providers
A new
IRegistryProviderinterface lets different strategies handle different registry hosts:RegistryProviderFactoryresolves the provider for a given hostname. ACR always wins for Azure hosts; the ORAS-based provider is the fallback.Transport abstraction
AzureContainerRegistryManageris now behindIOciRegistryTransport. A second implementation,OrasOciRegistryTransport, uses oras-dotnet to talk to generic registries.IOciRegistryTransportFactorypicks the right one based on hostname.AzureContainerRegistryManagerOrasOciRegistryTransportSession API
When
ociEnabledis active, push/pull/resolve go throughIRegistrySessioninstead of the transport directly:PushAsync— pushes config, layers, and manifestPullAsync— fetches manifest and layersResolveAsync— resolves a reference to manifest metadata (used byCheckArtifactExists)Two implementations:
AcrRegistrySession(wrapsAzureContainerRegistryManager) andOrasRegistrySession(ORAS library).Credential chain
DockerCredentialProviderimplements the Docker credential helper protocol — sends the registry hostname todocker-credential-<helper> getvia stdin, parses the JSON response. Supports username/password and identity token auth.New dependency
Project reference to oras-dotnet for the generic OCI transport.
Registry routing
Registries are classified as Azure (ACR/MCR) or generic based on hostname. Hosts matching *.azurecr.io/.cn/.us/.de/.gov or mcr.microsoft.com route through the existing Azure SDK path (
AzureContainerRegistryManager). All other registries route through the new ORAS-based transport with Docker credential resolution. ACR registries behind custom domains will fall through to the generic path, which works via
Docker credential helpers (e.g. docker-credential-acr-env). A future improvement could use OCI auth challenge detection to automatically identify the backing provider regardless of hostname.
What's unchanged
br:registry/path:tagformat for any registry.Checklist
Microsoft Reviewers: Open in CodeFlow