Merge branch 'sk/add-user-agent-to-container-registry-client' into 'master'

Add Gitlab User-Agent to ContainerRegistry::Client

See merge request gitlab-org/gitlab!29294

changelogs/unreleased/add-user-agent-to-container-registry-client.yml

+---
+title: Add Gitlab User-Agent to ContainerRegistry::Client
+merge_request: 29294
+author: Sashi Kumar
+type: other

ee/lib/ee/container_registry/client.rb

@@ -35,7 +35,7 @@ module EE
 
       # Pulls a blob from the Registry.
       # We currently use Faraday 0.12 which does not support streaming download yet
-      # Given that we aim to migrate to HTTP.rb client and that updating Faraday is potentialy
+      # Given that we aim to migrate to HTTP.rb client and that updating Faraday is potentially
       # dangerous, we use HTTP.rb here.
       #
       # @return {Object} Returns a Tempfile object or nil when no success
@@ -45,7 +45,10 @@ module EE
         blob_url = "/v2/#{name}/blobs/#{digest}"
 
         response = HTTP
-          .headers({ "Authorization" => "Bearer #{@options[:token]}" }) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          .headers({
+            'Authorization' => "Bearer #{@options[:token]}", # rubocop:disable Gitlab/ModuleWithInstanceVariables
+            'User-Agent' => "GitLab/#{::Gitlab::VERSION}"
+           })
           .get(::Gitlab::Utils.append_path(@base_uri, blob_url)) # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
         if response.status.redirect?
@@ -83,7 +86,7 @@ module EE
       # rubocop:disable Gitlab/ModuleWithInstanceVariables
       def faraday_raw
         strong_memoize(:faraday_raw) do
-          Faraday.new(@base_uri) do |conn|
+          faraday_base do |conn|
             initialize_connection(conn, @options, &method(:accept_raw_manifest))
           end
         end

ee/spec/lib/container_registry/client_spec.rb

@@ -6,6 +6,22 @@ describe ContainerRegistry::Client do
   let(:token) { '12345' }
   let(:options) { { token: token } }
   let(:client) { described_class.new("http://registry", options) }
+  let(:push_blob_headers) do
+    {
+      'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+      'Authorization' => 'bearer 12345',
+      'Content-Length' => '3',
+      'Content-Type' => 'application/octet-stream',
+      'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+    }
+  end
+  let(:headers_with_accept_types) do
+    {
+      'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+      'Authorization' => 'bearer 12345',
+      'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+    }
+  end
 
   describe '#push_blob' do
     let(:file) do
@@ -17,20 +33,11 @@ describe ContainerRegistry::Client do
 
     it 'PUT /v2/:name/blobs/uploads/url?digest=mytag' do
       stub_request(:put, "http://registry/v2/group/test/blobs/uploads/abcd?digest=mytag")
-        .with(
-          headers: {
-            'Authorization' => 'bearer 12345',
-            'Content-Length' => '3',
-            'Content-Type' => 'application/octet-stream'
-          })
+        .with(headers: push_blob_headers)
         .to_return(status: 200, body: "", headers: {})
 
       stub_request(:post, "http://registry/v2/group/test/blobs/uploads/")
-        .with(
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345'
-          })
+        .with(headers: headers_with_accept_types)
         .to_return(status: 200, body: "", headers: { 'Location' => 'http://registry/v2/group/test/blobs/uploads/abcd' })
 
       expect(client.push_blob('group/test', 'mytag', file.path)).to eq(true)
@@ -38,20 +45,11 @@ describe ContainerRegistry::Client do
 
     it 'raises error if response status is not 200' do
       stub_request(:put, "http://registry/v2/group/test/blobs/uploads/abcd?digest=mytag")
-        .with(
-          headers: {
-            'Authorization' => 'bearer 12345',
-            'Content-Length' => '3',
-            'Content-Type' => 'application/octet-stream'
-          })
+        .with(headers: push_blob_headers)
         .to_return(status: 404, body: "", headers: {})
 
       stub_request(:post, "http://registry/v2/group/test/blobs/uploads/")
-        .with(
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345'
-          })
+        .with(headers: headers_with_accept_types)
         .to_return(status: 200, body: "", headers: { 'Location' => 'http://registry/v2/group/test/blobs/uploads/abcd' })
 
       expect { client.push_blob('group/test', 'mytag', file.path) }
@@ -62,16 +60,21 @@ describe ContainerRegistry::Client do
   describe '#push_manifest' do
     let(:manifest) { 'manifest' }
     let(:manifest_type) { 'application/vnd.docker.distribution.manifest.v2+json' }
+    let(:manifest_headers) do
+      {
+          'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+          'Authorization' => 'bearer 12345',
+          'Content-Type' => 'application/vnd.docker.distribution.manifest.v2+json',
+          'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+      }
+    end
 
     it 'PUT v2/:name/manifests/:tag' do
       stub_request(:put, "http://registry/v2/group/test/manifests/my-tag")
         .with(
           body: "manifest",
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345',
-            'Content-Type' => 'application/vnd.docker.distribution.manifest.v2+json'
-          })
+          headers: manifest_headers
+        )
         .to_return(status: 200, body: "", headers: {})
 
       expect(client.push_manifest('group/test', 'my-tag', manifest, manifest_type)).to eq(true)
@@ -81,11 +84,8 @@ describe ContainerRegistry::Client do
       stub_request(:put, "http://registry/v2/group/test/manifests/my-tag")
         .with(
           body: "manifest",
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345',
-            'Content-Type' => 'application/vnd.docker.distribution.manifest.v2+json'
-          })
+          headers: manifest_headers
+        )
         .to_return(status: 404, body: "", headers: {})
 
       expect { client.push_manifest('group/test', 'my-tag', manifest, manifest_type) }
@@ -98,11 +98,7 @@ describe ContainerRegistry::Client do
 
     it 'returns true' do
       stub_request(:head, "http://registry/v2/group/test/blobs/digest")
-        .with(
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345'
-           })
+        .with(headers: headers_with_accept_types)
         .to_return(status: 200, body: "", headers: {})
 
       expect(client.blob_exists?('group/test', digest)).to eq(true)
@@ -110,11 +106,7 @@ describe ContainerRegistry::Client do
 
     it 'returns false' do
       stub_request(:head, "http://registry/v2/group/test/blobs/digest")
-        .with(
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345'
-           })
+        .with(headers: headers_with_accept_types)
         .to_return(status: 404, body: "", headers: {})
 
       expect(client.blob_exists?('group/test', digest)).to eq(false)
@@ -126,11 +118,7 @@ describe ContainerRegistry::Client do
 
     it 'GET "/v2/:name/manifests/:reference' do
       stub_request(:get, 'http://registry/v2/group/test/manifests/my-tag')
-        .with(
-          headers: {
-            'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
-            'Authorization' => 'bearer 12345'
-          })
+        .with(headers: headers_with_accept_types)
         .to_return(status: 200, body: manifest, headers: {})
 
       expect(client.repository_raw_manifest('group/test', 'my-tag')).to eq(manifest)
@@ -138,11 +126,16 @@ describe ContainerRegistry::Client do
   end
 
   describe '#pull_blob' do
-    let(:auth_headers) { { 'Authorization' => 'Bearer 12345' } }
+    let(:pull_blob_headers) do
+      {
+        'Authorization' => 'Bearer 12345',
+        'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+      }
+    end
 
     before do
       stub_request(:get, "http://registry/v2/group/test/blobs/e2312abc")
-          .with(headers: auth_headers)
+          .with(headers: pull_blob_headers)
           .to_return(status: 302, headers: { "Location" => 'http://download-link.com' })
     end
 
@@ -153,7 +146,7 @@ describe ContainerRegistry::Client do
       # With this stub we assert that there is no Authorization header in the request.
       # This also mimics the real case because Amazon s3 returns error too.
       stub_request(:get, "http://download-link.com/")
-        .with(headers: auth_headers)
+        .with(headers: pull_blob_headers)
         .to_return(status: 500)
 
       expect(client.pull_blob('group/test', 'e2312abc')).to be_a_kind_of(Tempfile)
@@ -168,7 +161,7 @@ describe ContainerRegistry::Client do
 
     it 'raises error when request is not authenticated' do
       stub_request(:get, "http://registry/v2/group/test/blobs/e2312abc")
-          .with(headers: auth_headers)
+          .with(headers: pull_blob_headers)
           .to_return(status: 401)
 
       expect { client.pull_blob('group/test', 'e2312abc') }.to raise_error(EE::ContainerRegistry::Client::Error)
@@ -179,7 +172,7 @@ describe ContainerRegistry::Client do
 
       it 'builds correct URL' do
         stub_request(:get, "http://registry//v2/group/test/blobs/e2312abc")
-          .with(headers: auth_headers)
+          .with(headers: pull_blob_headers)
           .to_return(status: 500)
 
         stub_request(:get, "http://download-link.com/")
@@ -192,7 +185,7 @@ describe ContainerRegistry::Client do
     context 'direct link to download, no redirect' do
       it 'downloads blob successfully' do
         stub_request(:get, "http://registry/v2/group/test/blobs/e2312abc")
-            .with(headers: auth_headers)
+            .with(headers: pull_blob_headers)
             .to_return(status: 200)
 
         expect(client.pull_blob('group/test', 'e2312abc')).to be_a_kind_of(Tempfile)

lib/container_registry/client.rb

@@ -159,13 +159,13 @@ module ContainerRegistry
     end
 
     def faraday
-      @faraday ||= Faraday.new(@base_uri) do |conn|
+      @faraday ||= faraday_base do |conn|
         initialize_connection(conn, @options, &method(:accept_manifest))
       end
     end
 
     def faraday_blob
-      @faraday_blob ||= Faraday.new(@base_uri) do |conn|
+      @faraday_blob ||= faraday_base do |conn|
         initialize_connection(conn, @options)
       end
     end
@@ -173,12 +173,16 @@ module ContainerRegistry
     # Create a new request to make sure the Authorization header is not inserted
     # via the Faraday middleware
     def faraday_redirect
-      @faraday_redirect ||= Faraday.new(@base_uri) do |conn|
+      @faraday_redirect ||= faraday_base do |conn|
         conn.request :json
         conn.adapter :net_http
       end
     end
 
+    def faraday_base(&block)
+      Faraday.new(@base_uri, headers: { user_agent: "GitLab/#{Gitlab::VERSION}" }, &block)
+    end
+
     def delete_if_exists(path)
       result = faraday.delete(path)
 

spec/lib/container_registry/client_spec.rb

@@ -6,6 +6,21 @@ describe ContainerRegistry::Client do
   let(:token) { '12345' }
   let(:options) { { token: token } }
   let(:client) { described_class.new("http://container-registry", options) }
+  let(:push_blob_headers) do
+    {
+        'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+        'Authorization' => "bearer #{token}",
+        'Content-Type' => 'application/octet-stream',
+        'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+    }
+  end
+  let(:headers_with_accept_types) do
+    {
+      'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+      'Authorization' => "bearer #{token}",
+      'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+    }
+  end
 
   shared_examples '#repository_manifest' do |manifest_type|
     let(:manifest) do
@@ -25,14 +40,15 @@ describe ContainerRegistry::Client do
             "size" => 2828661
           }
         ]
- }
+      }
     end
 
     it 'GET /v2/:name/manifests/mytag' do
       stub_request(:get, "http://container-registry/v2/group/test/manifests/mytag")
         .with(headers: {
-                'Accept' => described_class::ACCEPTED_TYPES.join(', '),
-                'Authorization' => "bearer #{token}"
+                'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+                'Authorization' => "bearer #{token}",
+                'User-Agent' => "GitLab/#{Gitlab::VERSION}"
               })
         .to_return(status: 200, body: manifest.to_json, headers: { content_type: manifest_type })
 
@@ -44,12 +60,23 @@ describe ContainerRegistry::Client do
   it_behaves_like '#repository_manifest', described_class::OCI_MANIFEST_V1_TYPE
 
   describe '#blob' do
+    let(:blob_headers) do
+      {
+          'Accept' => 'application/octet-stream',
+          'Authorization' => "bearer #{token}",
+          'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+      }
+    end
+
+    let(:redirect_header) do
+      {
+        'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+      }
+    end
+
     it 'GET /v2/:name/blobs/:digest' do
       stub_request(:get, "http://container-registry/v2/group/test/blobs/sha256:0123456789012345")
-        .with(headers: {
-               'Accept' => 'application/octet-stream',
-               'Authorization' => "bearer #{token}"
-             })
+        .with(headers: blob_headers)
         .to_return(status: 200, body: "Blob")
 
       expect(client.blob('group/test', 'sha256:0123456789012345')).to eq('Blob')
@@ -57,15 +84,14 @@ describe ContainerRegistry::Client do
 
     it 'follows 307 redirect for GET /v2/:name/blobs/:digest' do
       stub_request(:get, "http://container-registry/v2/group/test/blobs/sha256:0123456789012345")
-        .with(headers: {
-               'Accept' => 'application/octet-stream',
-               'Authorization' => "bearer #{token}"
-             })
+        .with(headers: blob_headers)
         .to_return(status: 307, body: "", headers: { Location: 'http://redirected' })
       # We should probably use hash_excluding here, but that requires an update to WebMock:
       # https://github.com/bblimke/webmock/blob/master/lib/webmock/matchers/hash_excluding_matcher.rb
       stub_request(:get, "http://redirected/")
-        .with { |request| !request.headers.include?('Authorization') }
+        .with(headers: redirect_header) do |request|
+          !request.headers.include?('Authorization')
+        end
         .to_return(status: 200, body: "Successfully redirected")
 
       response = client.blob('group/test', 'sha256:0123456789012345')
@@ -76,10 +102,11 @@ describe ContainerRegistry::Client do
 
   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)
+      .with(body: content, headers: push_blob_headers)
       .to_return(status: status, body: "", headers: {})
   end
 
@@ -136,11 +163,20 @@ describe ContainerRegistry::Client do
   end
 
   describe '#put_tag' do
+    let(:manifest_headers) do
+      {
+          'Accept' => 'application/vnd.docker.distribution.manifest.v2+json, application/vnd.oci.image.manifest.v1+json',
+          'Authorization' => "bearer #{token}",
+          'Content-Type' => 'application/vnd.docker.distribution.manifest.v2+json',
+          'User-Agent' => "GitLab/#{Gitlab::VERSION}"
+      }
+    end
+
     subject { client.put_tag('path', 'tagA', { foo: :bar }) }
 
     it 'uploads the manifest and returns the digest' do
       stub_request(:put, "http://container-registry/v2/path/manifests/tagA")
-        .with(body: "{\n  \"foo\": \"bar\"\n}")
+        .with(body: "{\n  \"foo\": \"bar\"\n}", headers: manifest_headers)
         .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:123' })
 
       expect(subject).to eq 'sha256:123'
@@ -153,6 +189,7 @@ describe ContainerRegistry::Client do
     context 'when the tag exists' do
       before do
         stub_request(:delete, "http://container-registry/v2/group/test/tags/reference/a")
+          .with(headers: headers_with_accept_types)
           .to_return(status: 200, body: "")
       end
 
@@ -162,6 +199,7 @@ describe ContainerRegistry::Client do
     context 'when the tag does not exist' do
       before do
         stub_request(:delete, "http://container-registry/v2/group/test/tags/reference/a")
+          .with(headers: headers_with_accept_types)
           .to_return(status: 404, body: "")
       end
 
@@ -171,6 +209,7 @@ describe ContainerRegistry::Client do
     context 'when an error occurs' do
       before do
         stub_request(:delete, "http://container-registry/v2/group/test/tags/reference/a")
+          .with(headers: headers_with_accept_types)
           .to_return(status: 500, body: "")
       end