DeleteExpiredPackageBlob handles packages in registry

matt-royal · tcdowney · tcdowney · commit d622d924fdfa · 2020-10-08T14:22:20.000-07:00
[#175080280]

Co-authored-by: Matt Royal &lt;mroyal@pivotal.io&gt;
Co-authored-by: Tim Downey &lt;tdowney@vmware.com&gt;
diff --git a/app/actions/app_delete.rb b/app/actions/app_delete.rb
@@ -1,4 +1,4 @@
-require 'jobs/runtime/blobstore_delete.rb'
+require 'jobs/runtime/blobstore_delete'
 require 'jobs/v3/buildpack_cache_delete'
 require 'actions/package_delete'
 require 'actions/task_delete'
diff --git a/app/jobs/kubernetes/registry_delete.rb b/app/jobs/kubernetes/registry_delete.rb
@@ -0,0 +1,22 @@
+module VCAP::CloudController
+  module Jobs
+    module Kubernetes
+      class RegistryDelete < VCAP::CloudController::Jobs::CCJob
+        def initialize(image_reference)
+          @image_reference = image_reference
+        end
+
+        attr_reader :image_reference
+
+        def perform
+          client = CloudController::DependencyLocator.instance.registry_buddy_client
+          client.delete_image(image_reference)
+        end
+
+        def max_attempts
+          3
+        end
+      end
+    end
+  end
+end
diff --git a/app/jobs/runtime/delete_expired_package_blob.rb b/app/jobs/runtime/delete_expired_package_blob.rb
@@ -1,3 +1,6 @@
+require 'jobs/kubernetes/registry_delete'
+require 'jobs/runtime/blobstore_delete'
+
 module VCAP::CloudController
   module Jobs
     module Runtime
@@ -14,8 +17,11 @@ def perform
           package = PackageModel.find(guid: package_guid)
           return unless package
 
-          # TODO: skip this if we're using an image registry?
-          BlobstoreDelete.new(package_guid, :package_blobstore).perform
+          if VCAP::CloudController::Config.config.package_image_registry_configured?
+            VCAP::CloudController::Jobs::Kubernetes::RegistryDelete.new(package.bits_image_reference).perform
+          else
+            BlobstoreDelete.new(package_guid, :package_blobstore).perform
+          end
           package.update(package_hash: nil, sha256_checksum: nil)
         end
 
diff --git a/app/models/runtime/package_model.rb b/app/models/runtime/package_model.rb
@@ -40,6 +40,16 @@ def image
       docker_image
     end
 
+    def bits_image_reference
+      config = VCAP::CloudController::Config.config
+      raise 'Package Registry is not configured' unless config.package_image_registry_configured?
+      raise 'Package type must be bits' unless bits?
+
+      package_registry_base_path = config.get(:packages, :image_registry, :base_path)
+
+      "#{package_registry_base_path}/#{guid}@sha256:#{sha256_checksum}"
+    end
+
     def bits?
       type == BITS_TYPE
     end
diff --git a/lib/cloud_controller/config_schemas/worker_schema.rb b/lib/cloud_controller/config_schemas/worker_schema.rb
@@ -101,6 +101,11 @@ class WorkerSchema < VCAP::Config
             fog_aws_storage_options: Hash
           },
 
+          optional(:package_image_uploader) => {
+            host: String,
+            port: Integer
+          },
+
           db_encryption_key: enum(String, NilClass),
 
           optional(:database_encryption) => {
diff --git a/lib/cloud_controller/dependency_locator.rb b/lib/cloud_controller/dependency_locator.rb
@@ -366,6 +366,13 @@ def packer
       end
     end
 
+    def registry_buddy_client
+      PackageImageUploader::Client.new(
+        VCAP::CloudController::Config.config.get(:package_image_uploader, :host),
+        VCAP::CloudController::Config.config.get(:package_image_uploader, :port),
+      )
+    end
+
     def statsd_client
       @dependencies[:statsd_client] ||
         register(:statsd_client, Statsd.new(config.get(:statsd_host), config.get(:statsd_port)))
diff --git a/lib/cloud_controller/kpack/stager.rb b/lib/cloud_controller/kpack/stager.rb
@@ -93,9 +93,8 @@ def image_resource(staging_details, builder_spec)
 
     def configure_source(staging_details)
       if config.package_image_registry_configured?
-        package_registry_base_path = config.get(:packages, :image_registry, :base_path)
         { registry: {
-          image: "#{package_registry_base_path}/#{staging_details.package.guid}@sha256:#{staging_details.package.sha256_checksum}"
+          image: staging_details.package.bits_image_reference
         } }
       else
         { blob: { url: blobstore_url_generator.package_download_url(staging_details.package) } }
diff --git a/lib/cloud_controller/packager/registry_bits_packer.rb b/lib/cloud_controller/packager/registry_bits_packer.rb
@@ -11,11 +11,7 @@ class RegistryBitsPacker
       def send_package_to_blobstore(package_guid, uploaded_package_zip, cached_files_fingerprints)
         Dir.mktmpdir('registry_bits_packer', packages_tmp_dir) do |root_path|
           complete_package_path = match_resources_and_validate_package(root_path, uploaded_package_zip, cached_files_fingerprints)
-
-          client = PackageImageUploader::Client.new(
-            VCAP::CloudController::Config.config.get(:package_image_uploader, :host),
-            VCAP::CloudController::Config.config.get(:package_image_uploader, :port),
-          )
+          client = CloudController::DependencyLocator.instance.registry_buddy_client
 
           registry = VCAP::CloudController::Config.config.get(:packages, :image_registry, :base_path)
           response = client.post_package(package_guid, complete_package_path, registry)
diff --git a/lib/package_image_uploader/client.rb b/lib/package_image_uploader/client.rb
@@ -7,16 +7,30 @@ def initialize(host, port)
     end
 
     def post_package(package_guid, zip_file_path, registry)
-      response = with_request_error_handling do
+      response = with_request_error_handling 200 do
         client.post('/packages',
-          body: { 'package_zip_path' => zip_file_path,
+          body: {
             'package_guid' => package_guid,
-            'registry_base_path' => registry }
+            'package_zip_path' => zip_file_path,
+            'registry_base_path' => registry,
+          }
         )
       end
       JSON.parse(response.body)
     end
 
+    def delete_image(image_reference)
+      with_request_error_handling 202 do
+        client.delete('/images',
+          body: {
+            image_reference: image_reference
+          }
+        )
+      end
+
+      nil
+    end
+
     private
 
     def client
@@ -27,11 +41,11 @@ def logger
       @logger ||= Steno.logger('cc.package_image_uploader')
     end
 
-    def with_request_error_handling(&_block)
+    def with_request_error_handling(successful_status)
       response = yield
 
       case response.status
-      when 200
+      when successful_status
         response
       when 400
         logger.error("PackageImageUploader returned: #{response.status} with #{response.body}")
diff --git a/spec/unit/jobs/kubernetes/registry_delete_spec.rb b/spec/unit/jobs/kubernetes/registry_delete_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+module VCAP::CloudController
+  module Jobs::Kubernetes
+    RSpec.describe RegistryDelete, job_context: :worker do
+      let(:image_reference) { 'path/to/image' }
+      let(:registry_buddy_client) { instance_double(PackageImageUploader::Client) }
+
+      subject(:job) do
+        RegistryDelete.new(image_reference)
+      end
+
+      describe '#perform' do
+        it 'sends a request to the registry buddy to delete the package' do
+          allow(CloudController::DependencyLocator.instance).to receive(:registry_buddy_client).
+            and_return(registry_buddy_client)
+          allow(registry_buddy_client).to receive(:delete_image)
+
+          job.perform
+
+          expect(registry_buddy_client).to have_received(:delete_image).with(image_reference)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/unit/jobs/runtime/delete_expired_package_blob_spec.rb b/spec/unit/jobs/runtime/delete_expired_package_blob_spec.rb
@@ -8,15 +8,40 @@ module Jobs::Runtime
 
       it { is_expected.to be_a_valid_job }
 
-      it 'delegates to blobstore delete job' do
-        expect_any_instance_of(BlobstoreDelete).to receive(:perform)
-        job.perform
+      context 'when using a package registry' do
+        let(:registry_delete) { instance_double(VCAP::CloudController::Jobs::Kubernetes::RegistryDelete) }
+
+        before do
+          TestConfig.override(packages: { image_registry: { base_path: 'hub.example.com/user' } })
+          allow(VCAP::CloudController::Jobs::Kubernetes::RegistryDelete).to receive(:new).and_return(registry_delete)
+          allow(registry_delete).to receive(:perform)
+        end
+
+        it 'delegates to registry delete job' do
+          job.perform
+
+          expect(VCAP::CloudController::Jobs::Kubernetes::RegistryDelete).to have_received(:new).with(package.bits_image_reference)
+          expect(registry_delete).to have_received(:perform)
+        end
+
+        it 'nils the package_hash and sha256_checksum values' do
+          expect { job.perform }.to change {
+            [package.reload.package_hash, package.reload.sha256_checksum]
+          }.to([nil, nil])
+        end
       end
 
-      it 'nils the package_hash and sha256_checksum values' do
-        expect { job.perform }.to change {
-          [package.reload.package_hash, package.reload.sha256_checksum]
-        }.to([nil, nil])
+      context 'when not using a package registry' do
+        it 'delegates to blobstore delete job' do
+          expect_any_instance_of(BlobstoreDelete).to receive(:perform)
+          job.perform
+        end
+
+        it 'nils the package_hash and sha256_checksum values' do
+          expect { job.perform }.to change {
+            [package.reload.package_hash, package.reload.sha256_checksum]
+          }.to([nil, nil])
+        end
       end
 
       context 'when the package does not exist' do
diff --git a/spec/unit/lib/package_image_uploader/client_spec.rb b/spec/unit/lib/package_image_uploader/client_spec.rb
@@ -5,7 +5,7 @@ module PackageImageUploader
   RSpec.describe Client do
     let(:package_image_uploader_host) { '127.0.0.1' }
     let(:package_image_uploader_port) { 8080 }
-    subject { PackageImageUploader::Client.new(package_image_uploader_host, package_image_uploader_port) }
+    subject(:client) { PackageImageUploader::Client.new(package_image_uploader_host, package_image_uploader_port) }
 
     describe '#post_package' do
       let(:status) { 200 }
@@ -25,7 +25,7 @@ module PackageImageUploader
         let(:response) { '{"hash":{"algorithm":"sha256","hex":"sha-2-5-6-hex"}}' }
 
         it 'returns the OCI Image sha' do
-          expect(subject.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker')).to eq(
+          expect(client.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker')).to eq(
             'hash' => { 'algorithm' => 'sha256', 'hex' => 'sha-2-5-6-hex' }
           )
         end
@@ -36,7 +36,7 @@ module PackageImageUploader
         let(:response) { 'unprocessable entity' }
 
         it 'raises a PackageImageUploader::Error' do
-          expect { subject.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker') }.
+          expect { client.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker') }.
             to raise_error(PackageImageUploader::Error, /Unprocessable Entity error/)
         end
       end
@@ -46,7 +46,7 @@ module PackageImageUploader
         let(:response) { 'bad request' }
 
         it 'raises a PackageImageUploader::Error' do
-          expect { subject.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker') }.
+          expect { client.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker') }.
             to raise_error(PackageImageUploader::Error, /Bad Request error/)
         end
       end
@@ -56,7 +56,66 @@ module PackageImageUploader
         let(:response) { 'bad response' }
 
         it 'raises a PackageImageUploader::Error' do
-          expect { subject.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker') }.
+          expect { client.post_package('a-package-guid', '/path/to/package.zip', 'docker.io/cfcapidocker') }.
+            to raise_error(PackageImageUploader::Error, /Server error/)
+        end
+      end
+    end
+
+    describe '#delete_image' do
+      let(:status) { 202 }
+      let(:response) { '{}' }
+
+      let(:image_reference) { 'reference-to-my-image' }
+
+      before do
+        stub_request(:delete, "http://#{package_image_uploader_host}:#{package_image_uploader_port}/images").
+          to_return(status: status, body: response)
+      end
+
+      it 'sends the request to the server' do
+        client.delete_image(image_reference)
+
+        expect(WebMock).to have_requested(:delete,
+          "http://#{package_image_uploader_host}:#{package_image_uploader_port}/images"
+        ).with(body: { 'image_reference' => image_reference })
+      end
+
+      context 'when the request succeeds' do
+        let(:status) { 202 }
+        let(:response) { '{}' }
+
+        it 'returns nil' do
+          expect(client.delete_image(image_reference)).to be_nil
+        end
+      end
+
+      context 'when PackageImageUploader returns a 422' do
+        let(:status) { 422 }
+        let(:response) { 'unprocessable entity' }
+
+        it 'raises a PackageImageUploader::Error' do
+          expect { client.delete_image(image_reference) }.
+            to raise_error(PackageImageUploader::Error, /Unprocessable Entity error/)
+        end
+      end
+
+      context 'when PackageImageUploader returns a 400' do
+        let(:status) { 400 }
+        let(:response) { 'bad request' }
+
+        it 'raises a PackageImageUploader::Error' do
+          expect { client.delete_image(image_reference) }.
+            to raise_error(PackageImageUploader::Error, /Bad Request error/)
+        end
+      end
+
+      context 'when PackageImageUploader returns a 500' do
+        let(:status) { 500 }
+        let(:response) { 'bad response' }
+
+        it 'raises a PackageImageUploader::Error' do
+          expect { client.delete_image(image_reference) }.
             to raise_error(PackageImageUploader::Error, /Server error/)
         end
       end
diff --git a/spec/unit/models/runtime/package_model_spec.rb b/spec/unit/models/runtime/package_model_spec.rb
@@ -21,6 +21,40 @@ module VCAP::CloudController
       end
     end
 
+    describe 'bits_image_reference' do
+      let(:package_sha256_checksum) { 'sha256-checksum' }
+      let(:type) { PackageModel::BITS_TYPE }
+      let(:package) { PackageModel.make(sha256_checksum: package_sha256_checksum, type: type) }
+
+      context 'when using a package registry' do
+        before do
+          TestConfig.override({ packages: { image_registry: { base_path: 'hub.example.com/user' } } })
+        end
+
+        it 'returns the image reference for the package' do
+          expect(package.bits_image_reference).to eq("hub.example.com/user/#{package.guid}@sha256:#{package_sha256_checksum}")
+        end
+
+        context 'when the package is type DOCKER' do
+          let(:type) { PackageModel::DOCKER_TYPE }
+
+          it 'raises an error' do
+            expect {
+              package.bits_image_reference
+            }.to raise_error('Package type must be bits')
+          end
+        end
+      end
+
+      context 'when not using a package registry' do
+        it 'raises an error' do
+          expect {
+            package.bits_image_reference
+          }.to raise_error('Package Registry is not configured')
+        end
+      end
+    end
+
     describe 'checksum_info' do
       let(:package) { PackageModel.new(state: PackageModel::READY_STATE) }
       context 'when the package has a sha1 hash and not a sha256 hash' do

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-require 'jobs/runtime/blobstore_delete.rb'`
	`1`	`+require 'jobs/runtime/blobstore_delete'`
`2`	`2`	`require 'jobs/v3/buildpack_cache_delete'`
`3`	`3`	`require 'actions/package_delete'`
`4`	`4`	`require 'actions/task_delete'`