Merge branch '227808-improve-container-registry-client-supports-tag-delete' into 'master'

Improve the container registry client

See merge request gitlab-org/gitlab!46989

changelogs/unreleased/227808-improve-container-registry-client-supports-tag-delete.yml

+---
+title: Improve the container registry client tags delete method
+merge_request: 46989
+author:
+type: changed

lib/container_registry/client.rb

@@ -15,6 +15,7 @@ module ContainerRegistry
     CONTAINER_IMAGE_V1_TYPE = 'application/vnd.docker.container.image.v1+json'
     REGISTRY_VERSION_HEADER = 'gitlab-container-registry-version'
     REGISTRY_FEATURES_HEADER = 'gitlab-container-registry-features'
+    REGISTRY_TAG_DELETE_FEATURE = 'tag_delete'
 
     ACCEPTED_TYPES = [DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE, OCI_MANIFEST_V1_TYPE].freeze
 
@@ -25,8 +26,6 @@ module ContainerRegistry
       registry_config = Gitlab.config.registry
       return false unless registry_config.enabled && registry_config.api_url.present?
 
-      return true if ::Gitlab.com?
-
       token = Auth::ContainerRegistryAuthenticationService.access_token([], [])
       client = new(registry_config.api_url, token: token)
       client.supports_tag_delete?
@@ -81,6 +80,9 @@ module ContainerRegistry
     # the DELETE method in the Allow header. Others reply with an 404 Not Found.
     def supports_tag_delete?
       strong_memoize(:supports_tag_delete) do
+        registry_features = Gitlab::CurrentSettings.container_registry_features || []
+        next true if ::Gitlab.com? && registry_features.include?(REGISTRY_TAG_DELETE_FEATURE)
+
         response = faraday.run_request(:options, '/v2/name/tags/reference/tag', '', {})
         response.success? && response.headers['allow']&.include?('DELETE')
       end

spec/lib/container_registry/client_spec.rb

@@ -3,9 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe ContainerRegistry::Client do
+  using RSpec::Parameterized::TableSyntax
+
   let(:token) { '12345' }
   let(:options) { { token: token } }
-  let(:client) { described_class.new("http://container-registry", options) }
+  let(:registry_api_url) { 'http://container-registry' }
+  let(:client) { described_class.new(registry_api_url, options) }
   let(:push_blob_headers) do
     {
         'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
@@ -101,16 +104,6 @@ RSpec.describe ContainerRegistry::Client do
     end
   end
 
-  def stub_upload(path, content, digest, status = 200)
-    stub_request(:post, "http://container-registry/v2/#{path}/blobs/uploads/")
-      .with(headers: headers_with_accept_types)
-      .to_return(status: status, body: "", headers: { 'location' => 'http://container-registry/next_upload?id=someid' })
-
-    stub_request(:put, "http://container-registry/next_upload?digest=#{digest}&id=someid")
-      .with(body: content, headers: push_blob_headers)
-      .to_return(status: status, body: "", headers: {})
-  end
-
   describe '#upload_blob' do
     subject { client.upload_blob('path', 'content', 'sha256:123') }
 
@@ -221,28 +214,36 @@ RSpec.describe ContainerRegistry::Client do
   describe '#supports_tag_delete?' do
     subject { client.supports_tag_delete? }
 
-    context 'when the server supports tag deletion' do
-      before do
-        stub_request(:options, "http://container-registry/v2/name/tags/reference/tag")
-          .to_return(status: 200, body: "", headers: { 'Allow' => 'DELETE' })
-      end
-
-      it { is_expected.to be_truthy }
+    where(:registry_tags_support_enabled, :is_on_dot_com, :container_registry_features, :expect_registry_to_be_pinged, :expected_result) do
+      true  | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | true
+      true  | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | true  | true
+      true  | true  | []                                                       | true  | true
+      true  | false | []                                                       | true  | true
+      false | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | true
+      false | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | true  | false
+      false | true  | []                                                       | true  | false
+      false | false | []                                                       | true  | false
     end
 
-    context 'when the server does not support tag deletion' do
+    with_them do
       before do
-        stub_request(:options, "http://container-registry/v2/name/tags/reference/tag")
-          .to_return(status: 404, body: "")
+        allow(::Gitlab).to receive(:com?).and_return(is_on_dot_com)
+        stub_registry_tags_support(registry_tags_support_enabled)
+        stub_application_setting(container_registry_features: container_registry_features)
       end
 
-      it { is_expected.to be_falsey }
+      it 'returns the expected result' do
+        if expect_registry_to_be_pinged
+          expect_next_instance_of(Faraday::Connection) do |connection|
+            expect(connection).to receive(:run_request).and_call_original
           end
+        else
+          expect(Faraday::Connection).not_to receive(:new)
         end
 
-  def stub_registry_info(headers: {}, status: 200)
-    stub_request(:get, 'http://container-registry/v2/')
-      .to_return(status: status, body: "", headers: headers)
+        expect(subject).to be expected_result
+      end
+    end
   end
 
   describe '#registry_info' do
@@ -291,45 +292,78 @@ RSpec.describe ContainerRegistry::Client do
   end
 
   describe '.supports_tag_delete?' do
-    let(:registry_enabled) { true }
-    let(:registry_api_url) { 'http://sandbox.local' }
-    let(:registry_tags_support_enabled) { true }
-    let(:is_on_dot_com) { false }
-
     subject { described_class.supports_tag_delete? }
 
+    where(:registry_api_url, :registry_enabled, :registry_tags_support_enabled, :is_on_dot_com, :container_registry_features, :expect_registry_to_be_pinged, :expected_result) do
+      'http://sandbox.local' | true  | true  | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | true
+      'http://sandbox.local' | true  | true  | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | true  | true
+      'http://sandbox.local' | true  | false | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | true
+      'http://sandbox.local' | true  | false | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | true  | false
+      'http://sandbox.local' | false | true  | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      'http://sandbox.local' | false | true  | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      'http://sandbox.local' | false | false | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      'http://sandbox.local' | false | false | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      'http://sandbox.local' | true  | true  | true  | []                                                       | true  | true
+      'http://sandbox.local' | true  | true  | false | []                                                       | true  | true
+      'http://sandbox.local' | true  | false | true  | []                                                       | true  | false
+      'http://sandbox.local' | true  | false | false | []                                                       | true  | false
+      'http://sandbox.local' | false | true  | true  | []                                                       | false | false
+      'http://sandbox.local' | false | true  | false | []                                                       | false | false
+      'http://sandbox.local' | false | false | true  | []                                                       | false | false
+      'http://sandbox.local' | false | false | false | []                                                       | false | false
+      ''                     | true  | true  | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | true  | true  | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | true  | false | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | true  | false | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | false | true  | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | false | true  | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | false | false | true  | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | false | false | false | [ContainerRegistry::Client::REGISTRY_TAG_DELETE_FEATURE] | false | false
+      ''                     | true  | true  | true  | []                                                       | false | false
+      ''                     | true  | true  | false | []                                                       | false | false
+      ''                     | true  | false | true  | []                                                       | false | false
+      ''                     | true  | false | false | []                                                       | false | false
+      ''                     | false | true  | true  | []                                                       | false | false
+      ''                     | false | true  | false | []                                                       | false | false
+      ''                     | false | false | true  | []                                                       | false | false
+      ''                     | false | false | false | []                                                       | false | false
+    end
+
+    with_them do
       before do
         allow(::Gitlab).to receive(:com?).and_return(is_on_dot_com)
         stub_container_registry_config(enabled: registry_enabled, api_url: registry_api_url, key: 'spec/fixtures/x509_certificate_pk.key')
         stub_registry_tags_support(registry_tags_support_enabled)
+        stub_application_setting(container_registry_features: container_registry_features)
       end
 
-    context 'with the registry enabled' do
-      it { is_expected.to be true }
-
-      context 'without an api url' do
-        let(:registry_api_url) { '' }
-
-        it { is_expected.to be false }
+      it 'returns the expected result' do
+        if expect_registry_to_be_pinged
+          expect_next_instance_of(Faraday::Connection) do |connection|
+            expect(connection).to receive(:run_request).and_call_original
           end
-
-      context 'on .com' do
-        let(:is_on_dot_com) { true }
-
-        it { is_expected.to be true }
+        else
+          expect(Faraday::Connection).not_to receive(:new)
         end
 
-      context 'when registry server does not support tag deletion' do
-        let(:registry_tags_support_enabled) { false }
-
-        it { is_expected.to be false }
+        expect(subject).to be expected_result
+      end
     end
   end
 
-    context 'with the registry disabled' do
-      let(:registry_enabled) { false }
+  def stub_upload(path, content, digest, status = 200)
+    stub_request(:post, "#{registry_api_url}/v2/#{path}/blobs/uploads/")
+      .with(headers: headers_with_accept_types)
+      .to_return(status: status, body: "", headers: { 'location' => "#{registry_api_url}/next_upload?id=someid" })
 
-      it { is_expected.to be false }
+    stub_request(:put, "#{registry_api_url}/next_upload?digest=#{digest}&id=someid")
+      .with(body: content, headers: push_blob_headers)
+      .to_return(status: status, body: "", headers: {})
+  end
+
+  def stub_registry_info(headers: {}, status: 200)
+    stub_request(:get, "#{registry_api_url}/v2/")
+      .to_return(status: status, body: "", headers: headers)
   end
 
   def stub_registry_tags_support(supported = true)
@@ -341,5 +375,4 @@ RSpec.describe ContainerRegistry::Client do
         headers: { 'Allow' => 'DELETE' }
       )
   end
-  end
 end