Skip to content
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.

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

fix(publish): add OCI 1.0 fallback support for AWS ECR #11239

Merged
merged 7 commits into from Dec 5, 2023

Conversation

milas
Copy link
Member

@milas milas commented Dec 1, 2023

What I did
Currently, we publish Compose artifacts following the OCI 1.1 specification, which is still in the RC state.

As a result, not all registries support it yet. Most notably, AWS ECR will reject certain OCI 1.1-compliant requests with 405 Method Not Supported with cryptic Invalid JSON errors.

This adds initial support for Compose to generate either an OCI 1.0 or OCI 1.1 compatible manifest. Notably, the OCI 1.0 manifest will be missing the application/vnd.docker.compose.project artifact type, as that does not exist in that version of the spec. (Less importantly, it uses an empty ImageConfig instead of the newer application/vnd.oci.empty.v1+json media type for the config.)

Currently, this is not exposed as an option (via CLI flags or env vars). By default, OCI 1.1 is used unless the registry domain is amazonaws.com, which indicates an ECR registry, so Compose will instead use OCI 1.0.

Moving forward, we should decide how much we want to expose/ support different OCI versions and investigate if there's a more generic way to feature probe the registry to avoid maintaining a hardcoded list of domains, which is both tedious and insufficient.

Related issue
https://docker.atlassian.net/browse/ENV-344

(not mandatory) A picture of a cute animal, if possible in relation to what you did
family of meerkats

@milas milas added the kind/bug label Dec 1, 2023
@milas milas requested review from ndeloof and glours December 1, 2023 18:40
@milas milas self-assigned this Dec 1, 2023
@milas
Copy link
Member Author

milas commented Dec 1, 2023

fyi I want to add an E2E test for this but need to get an ECR registry set up with permissions for GHA to use so will likely do that as a follow-up PR

@ndeloof
Copy link
Contributor

ndeloof commented Dec 1, 2023

I don't think we should hide lack of OCI 1.1 support on ECR, better have an command line flag to select OCI spec level to be applied. This also will allow users to deploy on registries we haven't tested and control OCI spec version being used.

Currently, we publish Compose artifacts following the OCI 1.1
specification, which is still in the RC state.

As a result, not all registries support it yet. Most notably,
AWS ECR will reject certain OCI 1.1-compliant requests with
`405 Method Not Supported` with cryptic `Invalid JSON` errors.

This adds initial support for Compose to generate either an
OCI 1.0 or OCI 1.1 compatible manifest. Notably, the OCI 1.0
manifest will be missing the `application/vnd.docker.compose.project`
artifact type, as that does not exist in that version of the
spec. (Less importantly, it uses an empty `ImageConfig`
instead of the newer `application/vnd.oci.empty.v1+json` media
type for the config.)

Currently, this is not exposed as an option (via CLI flags or
env vars). By default, OCI 1.1 is used unless the registry
domain is `amazonaws.com`, which indicates an ECR registry, so
Compose will instead use OCI 1.0.

Moving forward, we should decide how much we want to expose/
support different OCI versions and investigate if there's a
more generic way to feature probe the registry to avoid
maintaining a hardcoded list of domains, which is both tedious
and insufficient.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 117 lines in your changes are missing coverage. Please review.

Comparison is base (8026d0e) 56.88% compared to head (b6437d0) 56.56%.

❗ Current head b6437d0 differs from pull request most recent head d1f9ea3. Consider uploading reports for the commit d1f9ea3 to get more accurate results

Files Patch % Lines
internal/ocipush/push.go 0.00% 101 Missing ⚠️
pkg/compose/publish.go 0.00% 15 Missing ⚠️
cmd/compose/publish.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11239      +/-   ##
==========================================
- Coverage   56.88%   56.56%   -0.32%     
==========================================
  Files         133      134       +1     
  Lines       11513    11553      +40     
==========================================
- Hits         6549     6535      -14     
- Misses       4326     4376      +50     
- Partials      638      642       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndeloof
Copy link
Contributor

ndeloof commented Dec 2, 2023

would it make sense we:

  • offer a flag to select OCI spec version to be used
  • if not set, first try to publish as OCI 1.1, then on failure warn user and have a second try as OCI 1.0

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

Sounds good to me, maybe a typo to fix first

pkg/compose/publish.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM but helper message should refer to OCI artifact spec, not image spec

| Name | Type | Default | Description |
|:--------------------------|:---------|:--------|:----------------------------------------------------------------------|
| `--dry-run` | | | Execute command in dry run mode |
| `--oci-version` | `string` | | OCI Image specification version (automatically determined by default) |
Copy link
Contributor

Choose a reason for hiding this comment

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

OCI artifact specification

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

really? So this is an non-secret plan to make all this think damn confusing as an "image" can now either be an image or an arbitrary artifact 🥸

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to say "OCI Image/Artifact specification" - does that sound OK?

[also asked internally in Slack if anyone knows what the suggested nomenclature is 🤷🏻]

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas
Copy link
Member Author

milas commented Dec 5, 2023

fyi my latest push also includes a slight change: 4264d53

there's comments and references to the OCI spec there, but now using a custom media type for the config descriptor in OCI 1.0 style pushes so that the manifest is still identifiable as a Compose project - that's the suggested fall back in the spec

@milas milas requested review from ndeloof and glours December 5, 2023 17:05
@ndeloof ndeloof enabled auto-merge (rebase) December 5, 2023 17:07
@ndeloof ndeloof merged commit 85a1aec into docker:main Dec 5, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants