add support for multiple manifests per artifact

- oci expects any manifest to be referenceable not just the index
keygen-sh · Nov 15, 2024 · a4c8292 · a4c8292
1 parent 38ead71
commit a4c8292
Show file tree

Hide file tree

Showing 17 changed files with 124 additions and 45 deletions.
diff --git a/Gemfile b/Gemfile
@@ -59,6 +59,7 @@ gem 'verbose_migrations'
 gem 'temporary_tables'
 gem 'statement_timeout'
 gem 'union_of'
+gem 'order_as_specified'
 
 # Pattern matching
 gem 'rails-pattern_matching'

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -328,6 +328,8 @@ GEM
       rails (>= 6.0)
     oj (3.13.23)
     openssl (3.1.0)
+    order_as_specified (1.7)
+      activerecord (>= 5.0.0)
     parallel (1.23.0)
     parallel_tests (4.2.1)
       parallel
@@ -560,6 +562,7 @@ DEPENDENCIES
   null_association
   oj
   openssl (~> 3.1.0)
+  order_as_specified
   parallel_tests (~> 4.2.1)
   pg (~> 1.3.4)
   premailer (~> 1.23.0)

diff --git a/app/controllers/api/v1/release_engines/oci/manifests_controller.rb b/app/controllers/api/v1/release_engines/oci/manifests_controller.rb
@@ -10,7 +10,7 @@ class Oci::ManifestsController < Api::V1::BaseController
     def show
       authorize! package
 
-      manifest = package.manifests.find_by_reference!(params[:reference])
+      manifest = package.manifests.find_by_reference!(params[:reference], content_type: request.accepts.collect(&:to_s))
       authorize! manifest.artifact
 
       # for etag support

diff --git a/app/models/release_artifact.rb b/app/models/release_artifact.rb
@@ -38,6 +38,12 @@ class ReleaseArtifact < ApplicationRecord
     inverse_of: :artifacts,
     autosave: true,
     optional: true
+  has_many :manifests,
+    class_name: 'ReleaseManifest',
+    foreign_key: :release_artifact_id,
+    inverse_of: :artifact,
+    dependent: :delete_all
+  # NOTE(ezekg) not a strict has-one but this is a convenience
   has_one :manifest,
     class_name: 'ReleaseManifest',
     foreign_key: :release_artifact_id,

diff --git a/app/models/release_manifest.rb b/app/models/release_manifest.rb
@@ -10,6 +10,8 @@ class ReleaseManifest < ApplicationRecord
   include Environmental
   include Accountable
 
+  extend OrderAsSpecified
+
   belongs_to :artifact,
     class_name: 'ReleaseArtifact',
     foreign_key: :release_artifact_id,
@@ -27,7 +29,6 @@ class ReleaseManifest < ApplicationRecord
   has_account default: -> { artifact&.account_id }
 
   validates :artifact,
-    uniqueness: { message: 'already exists', scope: %i[release_artifact_id] },
     scope: { by: :account_id }
 
   validates :release,
@@ -50,13 +51,38 @@ class ReleaseManifest < ApplicationRecord
   def as_gemspec      = Gem::Specification.from_yaml(content)
   def as_package_json = JSON.parse(content)
 
-  def self.find_by_reference!(reference)
+  # find_by_reference is used for the oci engine to lookup a manifest by reference,
+  # which could be a digest, version, or tag, and also to request a specific media
+  # type in case of multiple manifests per-image.
+  def self.find_by_reference!(reference, content_type: nil)
     base = joins(:release)
 
-    base.where(content_digest: reference)
-        .or(base.where(release: { version: reference }))
-        .or(base.where(release: { tag: reference }))
-        .take!
+    manifests = base.where(content_digest: reference)
+                    .or(base.where(release: { version: reference }))
+                    .or(base.where(release: { tag: reference }))
+
+    # oci clients may want a specific media type in case of multiple manifests
+    unless content_type.blank?
+      manifests = case content_type
+                  in ['*/*', *] | '*/*' # has no preference i.e. accepts anything
+                    manifests.where.not(content_type: nil)
+                  in [*content_types, '*/*'] # has preference but accepts anything
+                    manifests.where(content_type: content_types)
+                             .or(manifests.where.not(content_type: nil))
+                             .order_as_specified(
+                               content_type: content_types,
+                             )
+                  in [*content_types] # has preferences
+                    manifests.where(content_type: content_types)
+                             .order_as_specified(
+                               content_type: content_types,
+                             )
+                  else # has preference
+                    manifests.where(content_type:)
+                  end
+    end
+
+    manifests.take!
   end
 
   def self.find_by_reference(...)

diff --git a/app/workers/process_npm_package_worker.rb b/app/workers/process_npm_package_worker.rb
@@ -64,6 +64,7 @@ def perform(artifact_id)
         content_digest: "sha512-#{Digest::SHA512.hexdigest(content)}",
         content_type: 'application/vnd.npm.install-v1+json',
         content_length: content.bytesize,
+        content_path: 'package.json',
         content:,
       )
     end

diff --git a/app/workers/process_oci_image_worker.rb b/app/workers/process_oci_image_worker.rb
@@ -43,22 +43,29 @@ def perform(artifact_id)
         next unless
           item.exists?
 
-        case item
-        in OciImageLayout::Index => index
+        # store manifests as a manifest for easy reference without hitting the storage provider
+        if item in OciImageLayout::Index | OciImageLayout::Manifest => manifest
           raise ImageNotAcceptableError, 'manifest is too big' if
-            index.size > MAX_MANIFEST_SIZE
+            manifest.size > MAX_MANIFEST_SIZE
 
           ReleaseManifest.create!(
             account_id: artifact.account_id,
             environment_id: artifact.environment_id,
             release_id: artifact.release_id,
             release_artifact_id: artifact.id,
-            content_type: index.media_type,
-            content_digest: index.digest,
-            content_length: index.size,
-            content: index.read,
+            content_type: manifest.media_type,
+            content_digest: manifest.digest,
+            content_length: manifest.size,
+            content_path: manifest.path,
+            content: manifest.read,
           )
-        in OciImageLayout::Blob => blob
+
+          # still need to store it as a blob
+          manifest.rewind
+        end
+
+        # store blobs as a descriptor pointing to the storage provider
+        if item in OciImageLayout::Blob => blob
           raise ImageNotAcceptableError, 'blob is too big' if
             blob.size > MAX_BLOB_SIZE
 

diff --git a/app/workers/process_ruby_gem_worker.rb b/app/workers/process_ruby_gem_worker.rb
@@ -43,6 +43,7 @@ def perform(artifact_id)
       content_digest: "sha256-#{Digest::SHA256.hexdigest(yaml)}",
       content_type: 'application/x-yaml',
       content_length: yaml.bytesize,
+      content_path: gemspec.file_name,
       content: yaml,
     )
 

diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb
@@ -5,11 +5,6 @@
   application/vnd.keygen+json
 ]
 
-Mime::Type.register 'application/vnd.oci+json', :oci, %W[
-  application/vnd.oci.image.manifest.v1+json
-  application/vnd.oci.image.index.v1+json
-]
-
 # adds pattern matching
 class Mime::Type
   def deconstruct_keys(*) = { string:, symbol:, synonyms: }

diff --git a/config/routes.rb b/config/routes.rb
@@ -108,7 +108,7 @@
 
   concern :oci do
     # NOTE(ezekg) /v2 namespace is handled outside of this because docker wants it to always be at the root...
-    scope module: :oci, defaults: { format: :oci } do
+    scope module: :oci, defaults: { format: :binary } do
       # see: https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests
       match ':package/manifests/:reference', via: %i[head get], to: 'manifests#show', as: :oci_manifest, constraints: {
         package: /[^\/]*/,

diff --git a/db/migrate/20241114163421_add_content_attributes_to_release_manifests.rb b/db/migrate/20241114163421_add_content_attributes_to_release_manifests.rb
@@ -2,7 +2,7 @@ class AddContentAttributesToReleaseManifests < ActiveRecord::Migration[7.2]
   disable_ddl_transaction!
 
   def change
-    # FIXME(ezekg) eventually make these non-null?
+    add_column :release_manifests, :content_path,   :string, null: true
     add_column :release_manifests, :content_type,   :string, null: true
     add_column :release_manifests, :content_length, :bigint, null: true
     add_column :release_manifests, :content_digest, :string, null: true

diff --git a/db/migrate/20241114214652_remove_unique_index_from_release_manifests.rb b/db/migrate/20241114214652_remove_unique_index_from_release_manifests.rb
@@ -0,0 +1,12 @@
+class RemoveUniqueIndexFromReleaseManifests < ActiveRecord::Migration[7.2]
+  disable_ddl_transaction!
+
+  def change
+    remove_index :release_manifests, %i[release_artifact_id],
+      algorithm: :concurrently,
+      unique: true
+
+    add_index :release_manifests, %i[release_artifact_id],
+      algorithm: :concurrently
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2024_11_14_163421) do
+ActiveRecord::Schema[7.2].define(version: 2024_11_14_214652) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "btree_gin"
   enable_extension "pg_stat_statements"
@@ -610,13 +610,14 @@
     t.jsonb "metadata"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.string "content_path"
     t.string "content_type"
     t.bigint "content_length"
     t.string "content_digest"
     t.index ["account_id", "created_at"], name: "index_release_manifests_on_account_id_and_created_at", order: { created_at: :desc }
     t.index ["environment_id"], name: "index_release_manifests_on_environment_id"
     t.index ["release_artifact_id", "content_digest"], name: "idx_on_release_artifact_id_content_digest_d7f016852f", unique: true
-    t.index ["release_artifact_id"], name: "index_release_manifests_on_release_artifact_id", unique: true
+    t.index ["release_artifact_id"], name: "index_release_manifests_on_release_artifact_id"
     t.index ["release_id"], name: "index_release_manifests_on_release_id"
   end
 

diff --git a/lib/oci_image_layout.rb b/lib/oci_image_layout.rb
@@ -155,6 +155,7 @@ def initialize(io, media_type:, digest:, size:, path:, fs:)
 
     def exists?   = io.present?
     def read(...) = (io.read(...) if exists?)
+    def seek(...) = (io.seek(...) if exists?)
     def rewind    = (io.rewind if exists?)
     def close     = (io.close if exists?)
     def closed?   = (io.closed? if exists?)

diff --git a/spec/workers/process_npm_package_worker_spec.rb b/spec/workers/process_npm_package_worker_spec.rb
@@ -52,6 +52,8 @@
       it 'should process package' do
         expect { subject.perform_async(artifact.id) }.to change { artifact.reload.manifest }
 
+        expect(artifact.manifest.content_type).to eq 'application/vnd.npm.install-v1+json'
+        expect(artifact.manifest.content_path).to eq 'package.json'
         expect(artifact.manifest.content).to eq minified_package_json
         expect(artifact.status).to eq 'UPLOADED'
       end

diff --git a/spec/workers/process_oci_image_worker_spec.rb b/spec/workers/process_oci_image_worker_spec.rb
@@ -17,14 +17,6 @@
   context 'when artifact is a valid image' do
     let(:image_fixture) { 'alpine-3.20.3.tar' }
     let(:image_tarball) { file_fixture(image_fixture).open }
-    let(:image_index) {
-      tarball = file_fixture(image_fixture).open
-
-      Minitar::Reader.open tarball do |archive|
-        archive.find { _1.file? && _1.name in 'index.json' }
-               .read
-      end
-    }
 
     before do
       Aws.config = { s3: { stub_responses: { get_object: [{ body: image_tarball }] } } }
@@ -33,10 +25,12 @@
     context 'when artifact is waiting' do
       let(:artifact) { create(:artifact, :oci_image, :waiting, account:) }
 
+      it 'should not change artifact status' do
+        expect { subject.perform_async(artifact.id) }.to_not change { artifact.reload.status }
+      end
+
       it 'should not store manifest' do
         expect { subject.perform_async(artifact.id) }.to not_change { artifact.reload.manifest }
-
-        expect(artifact.status).to eq 'WAITING'
       end
 
       it 'should not upload blobs' do
@@ -47,14 +41,20 @@
     context 'when artifact is processing' do
       let(:artifact) { create(:artifact, :oci_image, :processing, account:) }
 
-      it 'should store manifest' do
-        expect { subject.perform_async(artifact.id) }.to change { artifact.reload.manifest }
+      it 'should change artifact status' do
+        expect { subject.perform_async(artifact.id) }.to change { artifact.reload.status }.to('UPLOADED')
+      end
 
-        expect(artifact.manifest.content_digest).to eq 'sha256:355eee6af939abf5ba465c9be69c3b725f8d3f19516ca9644cf2a4fb112fd83b'
-        expect(artifact.manifest.content_type).to eq 'application/vnd.oci.image.index.v1+json'
-        expect(artifact.manifest.content_length).to eq 441
-        expect(artifact.manifest.content).to eq image_index
-        expect(artifact.status).to eq 'UPLOADED'
+      it 'should store manifests' do
+        expect { subject.perform_async(artifact.id) }.to change { artifact.reload.manifests }
+
+        expect(artifact.manifests).to satisfy { |manifests|
+          manifests in [
+            ReleaseManifest(content_path: 'index.json',                                                                    content_digest: 'sha256:355eee6af939abf5ba465c9be69c3b725f8d3f19516ca9644cf2a4fb112fd83b', content_type: 'application/vnd.oci.image.index.v1+json',                   content_length: 441,  content: String),
+            ReleaseManifest(content_path: 'blobs/sha256/beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d', content_digest: 'sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d', content_type: 'application/vnd.docker.distribution.manifest.list.v2+json', content_length: 1853, content: String),
+            ReleaseManifest(content_path: 'blobs/sha256/33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735', content_digest: 'sha256:33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735', content_type: 'application/vnd.docker.distribution.manifest.v2+json',      content_length: 528,  content: String),
+          ]
+        }
       end
 
       context 'when blobs are not uploaded' do
@@ -68,6 +68,7 @@
           expect(artifact.descriptors).to satisfy { |descriptors|
             descriptors in [
               ReleaseDescriptor(content_path: 'oci-layout',                                                                    content_type: 'application/vnd.oci.layout.header.v1+json',                 content_digest: 'sha256:18f0797eab35a4597c1e9624aa4f15fd91f6254e5538c1e0d193b2a95dd4acc6', content_length: 30),
+              ReleaseDescriptor(content_path: 'index.json',                                                                    content_type: 'application/vnd.oci.image.index.v1+json',                   content_digest: 'sha256:355eee6af939abf5ba465c9be69c3b725f8d3f19516ca9644cf2a4fb112fd83b', content_length: 441),
               ReleaseDescriptor(content_path: 'blobs/sha256/beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d', content_type: 'application/vnd.docker.distribution.manifest.list.v2+json', content_digest: 'sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d', content_length: 1853),
               ReleaseDescriptor(content_path: 'blobs/sha256/33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735', content_type: 'application/vnd.docker.distribution.manifest.v2+json',      content_digest: 'sha256:33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735', content_length: 528),
               ReleaseDescriptor(content_path: 'blobs/sha256/43c4264eed91be63b206e17d93e75256a6097070ce643c5e8f0379998b44f170', content_type: 'application/vnd.docker.image.rootfs.diff.tar.gzip',         content_digest: 'sha256:43c4264eed91be63b206e17d93e75256a6097070ce643c5e8f0379998b44f170', content_length: 3623807),
@@ -79,6 +80,7 @@
         it 'should upload blobs' do
           expect { subject.perform_async(artifact.id) }.to upload(
             { key: %r{oci-layout},                                                                    content_type: 'application/vnd.oci.layout.header.v1+json',                 content_length: 30 },
+            { key: %r{index.json},                                                                    content_type: 'application/vnd.oci.image.index.v1+json',                   content_length: 441 },
             { key: %r{blobs/sha256/beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d}, content_type: 'application/vnd.docker.distribution.manifest.list.v2+json', content_length: 1853 },
             { key: %r{blobs/sha256/33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735}, content_type: 'application/vnd.docker.distribution.manifest.v2+json',      content_length: 528 },
             { key: %r{blobs/sha256/43c4264eed91be63b206e17d93e75256a6097070ce643c5e8f0379998b44f170}, content_type: 'application/vnd.docker.image.rootfs.diff.tar.gzip',         content_length: 3623807 },
@@ -92,6 +94,21 @@
           Aws.config[:s3][:stub_responses][:head_object] = []
         end
 
+        it 'should store blobs' do
+          expect { subject.perform_async(artifact.id) }.to change { artifact.reload.descriptors }
+
+          expect(artifact.descriptors).to satisfy { |descriptors|
+            descriptors in [
+              ReleaseDescriptor(content_path: 'oci-layout',                                                                    content_type: 'application/vnd.oci.layout.header.v1+json',                 content_digest: 'sha256:18f0797eab35a4597c1e9624aa4f15fd91f6254e5538c1e0d193b2a95dd4acc6', content_length: 30),
+              ReleaseDescriptor(content_path: 'index.json',                                                                    content_type: 'application/vnd.oci.image.index.v1+json',                   content_digest: 'sha256:355eee6af939abf5ba465c9be69c3b725f8d3f19516ca9644cf2a4fb112fd83b', content_length: 441),
+              ReleaseDescriptor(content_path: 'blobs/sha256/beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d', content_type: 'application/vnd.docker.distribution.manifest.list.v2+json', content_digest: 'sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d', content_length: 1853),
+              ReleaseDescriptor(content_path: 'blobs/sha256/33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735', content_type: 'application/vnd.docker.distribution.manifest.v2+json',      content_digest: 'sha256:33735bd63cf84d7e388d9f6d297d348c523c044410f553bd878c6d7829612735', content_length: 528),
+              ReleaseDescriptor(content_path: 'blobs/sha256/43c4264eed91be63b206e17d93e75256a6097070ce643c5e8f0379998b44f170', content_type: 'application/vnd.docker.image.rootfs.diff.tar.gzip',         content_digest: 'sha256:43c4264eed91be63b206e17d93e75256a6097070ce643c5e8f0379998b44f170', content_length: 3623807),
+              ReleaseDescriptor(content_path: 'blobs/sha256/91ef0af61f39ece4d6710e465df5ed6ca12112358344fd51ae6a3b886634148b', content_type: 'application/vnd.docker.container.image.v1+json',            content_digest: 'sha256:91ef0af61f39ece4d6710e465df5ed6ca12112358344fd51ae6a3b886634148b', content_length: 1471),
+            ]
+          }
+        end
+
         it 'should not reupload blobs' do
           expect { subject.perform_async(artifact.id) }.to_not upload
         end
@@ -101,10 +118,12 @@
     context 'when artifact is uploaded' do
       let(:artifact) { create(:artifact, :oci_image, :uploaded, account:) }
 
+      it 'should not change artifact status' do
+        expect { subject.perform_async(artifact.id) }.to_not change { artifact.reload.status }
+      end
+
       it 'should not store manifest' do
         expect { subject.perform_async(artifact.id) }.to not_change { artifact.reload.manifest }
-
-        expect(artifact.status).to eq 'UPLOADED'
       end
 
       it 'should not upload blobs' do
@@ -115,10 +134,12 @@
     context 'when artifact is failed' do
       let(:artifact) { create(:artifact, :oci_image, :failed, account:) }
 
+      it 'should not change artifact status' do
+        expect { subject.perform_async(artifact.id) }.to_not change { artifact.reload.status }
+      end
+
       it 'should not store manifest' do
         expect { subject.perform_async(artifact.id) }.to not_change { artifact.reload.manifest }
-
-        expect(artifact.status).to eq 'FAILED'
       end
 
       it 'should not upload blobs' do

diff --git a/spec/workers/process_ruby_gem_worker_spec.rb b/spec/workers/process_ruby_gem_worker_spec.rb
@@ -37,6 +37,8 @@
       it 'should process gem' do
         expect { subject.perform_async(artifact.id) }.to change { artifact.reload.manifest }
 
+        expect(artifact.manifest.content_type).to eq 'application/x-yaml'
+        expect(artifact.manifest.content_path).to eq gemspec.file_name
         expect(artifact.manifest.content).to eq gemspec.to_yaml
         expect(artifact.status).to eq 'UPLOADED'
       end