From 0c5419f9795f0a25dbc806b573c7bc6f6add629b Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Thu, 26 Mar 2026 18:00:41 +0100 Subject: [PATCH 1/3] Add WebDAV support to storage-cli client and improve test quality Enable WebDAV/DAV provider support in StorageCliClient by adding 'dav' to STORAGE_CLI_TYPES and mapping legacy 'webdav' provider name to 'dav'. Test improvements: - Add comprehensive tests for all legacy provider mappings (AzureRM, AWS, Google, aliyun, webdav) - Add test for native 'dav' storage-cli type - Use begin/ensure blocks to prevent tempfile leaks on test failures - Add helper methods (write_config_file, stub_config_for_droplets) to reduce repetition - Stub additional_flags in #run_cli tests for stability - Fix test descriptions for accuracy --- .../storage_cli/storage_cli_client.rb | 12 +- .../storage_cli/storage_cli_client_spec.rb | 397 ++++++++++-------- 2 files changed, 230 insertions(+), 179 deletions(-) diff --git a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb index e9fb97b10a2..fe2fcc28f3e 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb @@ -17,16 +17,16 @@ class StorageCliClient < BaseClient 'resource_pool' => :storage_cli_config_file_resource_pool }.freeze - # Native storage-cli type names supported by CC (dav intentionally excluded for now) - STORAGE_CLI_TYPES = %w[azurebs alioss s3 gcs].freeze + # Native storage-cli type names supported by CC + STORAGE_CLI_TYPES = %w[azurebs alioss s3 gcs dav].freeze # DEPRECATED: Legacy fog provider names (remove after migration window) LEGACY_PROVIDER_TO_STORAGE_CLI_TYPE = { 'AzureRM' => 'azurebs', 'aliyun' => 'alioss', 'AWS' => 's3', - 'Google' => 'gcs' - # 'webdav' => 'dav', # intentionally not enabled yet + 'Google' => 'gcs', + 'webdav' => 'dav' }.freeze def initialize(directory_key:, resource_type:, root_dir:, min_size: nil, max_size: nil) @@ -39,10 +39,6 @@ def initialize(directory_key:, resource_type:, root_dir:, min_size: nil, max_siz provider = cfg['provider']&.to_s raise BlobstoreError.new("No provider specified in config file: #{File.basename(config_file_path)}") if provider.nil? || provider.empty? - # Explicitly block unfinished webdav storage-cli support to avoid confusion and wasted effort on debugging - # unsupported providers. Remove this check when webdav support is added. - raise "provider '#{provider}' is not supported yet" if %w[webdav dav].include?(provider) - @storage_type = if STORAGE_CLI_TYPES.include?(provider) provider diff --git a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb index ead5a6e64f8..789e5435a15 100644 --- a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb @@ -4,154 +4,240 @@ module CloudController module Blobstore RSpec.describe StorageCliClient do + # Helper methods + def write_config_file(hash) + file = Tempfile.new(['storage-cli', '.json']) + file.write(hash.to_json) + file.flush + file + end + + def stub_config_for_droplets(path) + config_double = instance_double(VCAP::CloudController::Config) + allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) + allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(path) + allow(Steno).to receive(:logger).and_return(double(info: nil, error: nil, debug: nil)) + end + describe 'client init' do # DEPRECATED: Legacy fog provider tests - remove after migration window # START LEGACY FOG SUPPORT TESTS - it 'init the correct client when JSON has provider AzureRM (legacy fog name)' do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write({ provider: 'AzureRM', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json) - droplets_cfg.flush - - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) - - client = StorageCliClient.new( - directory_key: 'dummy-key', - root_dir: 'dummy-root', - resource_type: 'droplets' + it 'maps AzureRM legacy provider to azurebs storage-cli type' do + droplets_cfg = write_config_file( + provider: 'AzureRM', + account_key: 'bommelkey', + account_name: 'bommel', + container_name: 'bommelcontainer', + environment: 'BommelCloud' ) - expect(client.instance_variable_get(:@storage_type)).to eq('azurebs') - expect(client.instance_variable_get(:@resource_type)).to eq('droplets') - expect(client.instance_variable_get(:@root_dir)).to eq('dummy-root') - expect(client.instance_variable_get(:@directory_key)).to eq('dummy-key') + begin + stub_config_for_droplets(droplets_cfg.path) - droplets_cfg.close! + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('azurebs') + expect(client.instance_variable_get(:@resource_type)).to eq('droplets') + expect(client.instance_variable_get(:@root_dir)).to eq('dummy-root') + expect(client.instance_variable_get(:@directory_key)).to eq('dummy-key') + ensure + droplets_cfg.close! + end end - # END LEGACY FOG SUPPORT TESTS - it 'init the correct client when JSON has provider azurebs (native storage-cli type)' do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write({ provider: 'azurebs', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json) - droplets_cfg.flush - - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) - - client = StorageCliClient.new( - directory_key: 'dummy-key', - root_dir: 'dummy-root', - resource_type: 'droplets' + it 'maps AWS legacy provider to s3 storage-cli type' do + droplets_cfg = write_config_file( + provider: 'AWS', + bucket_name: 'test-bucket', + access_key_id: 'key', + secret_access_key: 'secret' ) - expect(client.instance_variable_get(:@storage_type)).to eq('azurebs') - expect(client.instance_variable_get(:@resource_type)).to eq('droplets') - expect(client.instance_variable_get(:@root_dir)).to eq('dummy-root') - expect(client.instance_variable_get(:@directory_key)).to eq('dummy-key') + begin + stub_config_for_droplets(droplets_cfg.path) - droplets_cfg.close! + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('s3') + ensure + droplets_cfg.close! + end end - # DEPRECATED: Legacy fog provider tests - remove after migration window - # START LEGACY FOG SUPPORT TESTS - it 'raises an error for an unknown legacy provider' do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write( - { provider: 'UnknownProvider', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json + it 'maps Google legacy provider to gcs storage-cli type' do + droplets_cfg = write_config_file( + provider: 'Google', + bucket_name: 'test-bucket', + json_key: '{}' ) - droplets_cfg.flush + begin + stub_config_for_droplets(droplets_cfg.path) - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('gcs') + ensure + droplets_cfg.close! + end + end - expect do - StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') - end.to raise_error(RuntimeError, /Unknown provider: UnknownProvider/) + it 'maps aliyun legacy provider to alioss storage-cli type' do + droplets_cfg = write_config_file( + provider: 'aliyun', + access_key_id: 'key', + access_key_secret: 'secret', + endpoint: 'aliyun.com', + bucket_name: 'bucket' + ) + begin + stub_config_for_droplets(droplets_cfg.path) - droplets_cfg.close! + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('alioss') + ensure + droplets_cfg.close! + end end - it 'blocks webdav/dav provider explicitly' do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write( - { provider: 'webdav', - account_key: 'bommelkey' }.to_json + it 'maps webdav legacy provider to dav storage-cli type' do + droplets_cfg = write_config_file( + provider: 'webdav', + endpoint: 'https://webdav.example.com', + user: 'testuser', + password: 'testpass' ) - droplets_cfg.flush + begin + stub_config_for_droplets(droplets_cfg.path) - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('dav') + ensure + droplets_cfg.close! + end + end - expect do - StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') - end.to raise_error(RuntimeError, /is not supported yet/) + it 'raises an error for an unknown legacy provider' do + droplets_cfg = write_config_file( + provider: 'UnknownProvider', + account_key: 'bommelkey', + account_name: 'bommel', + container_name: 'bommelcontainer', + environment: 'BommelCloud' + ) + begin + stub_config_for_droplets(droplets_cfg.path) - droplets_cfg.close! + expect do + StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') + end.to raise_error(RuntimeError, /Unknown provider: UnknownProvider/) + ensure + droplets_cfg.close! + end end # END LEGACY FOG SUPPORT TESTS - it 'raises an error for an unknown storage-cli type' do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write( - { provider: 'unknown_type', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json + it 'init the correct client when JSON has provider azurebs (native storage-cli type)' do + droplets_cfg = write_config_file( + provider: 'azurebs', + account_key: 'bommelkey', + account_name: 'bommel', + container_name: 'bommelcontainer', + environment: 'BommelCloud' ) - droplets_cfg.flush + begin + stub_config_for_droplets(droplets_cfg.path) - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('azurebs') + expect(client.instance_variable_get(:@resource_type)).to eq('droplets') + expect(client.instance_variable_get(:@root_dir)).to eq('dummy-root') + expect(client.instance_variable_get(:@directory_key)).to eq('dummy-key') + ensure + droplets_cfg.close! + end + end - expect do - StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') - end.to raise_error(RuntimeError, /Unknown provider: unknown_type/) + it 'init the correct client when JSON has provider dav (native storage-cli type)' do + droplets_cfg = write_config_file( + provider: 'dav', + endpoint: 'https://webdav.example.com', + user: 'testuser', + password: 'testpass' + ) + begin + stub_config_for_droplets(droplets_cfg.path) - droplets_cfg.close! + client = StorageCliClient.new( + directory_key: 'dummy-key', + root_dir: 'dummy-root', + resource_type: 'droplets' + ) + expect(client.instance_variable_get(:@storage_type)).to eq('dav') + expect(client.instance_variable_get(:@resource_type)).to eq('droplets') + expect(client.instance_variable_get(:@root_dir)).to eq('dummy-root') + expect(client.instance_variable_get(:@directory_key)).to eq('dummy-key') + ensure + droplets_cfg.close! + end end - # DEPRECATED: Legacy fog provider test - remove after migration window - # START LEGACY FOG SUPPORT TESTS - it 'raises an error when provider is missing' do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write( - { account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json + it 'raises an error for an unknown storage-cli type' do + droplets_cfg = write_config_file( + provider: 'unknown_type', + account_key: 'bommelkey', + account_name: 'bommel', + container_name: 'bommelcontainer', + environment: 'BommelCloud' ) - droplets_cfg.flush + begin + stub_config_for_droplets(droplets_cfg.path) - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) + expect do + StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') + end.to raise_error(RuntimeError, /Unknown provider: unknown_type/) + ensure + droplets_cfg.close! + end + end - expect do - StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') - end.to raise_error(BlobstoreError, /No provider specified/) + it 'raises an error when provider is missing' do + droplets_cfg = write_config_file( + account_key: 'bommelkey', + account_name: 'bommel', + container_name: 'bommelcontainer', + environment: 'BommelCloud' + ) + begin + stub_config_for_droplets(droplets_cfg.path) - droplets_cfg.close! + expect do + StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets') + end.to raise_error(BlobstoreError, /No provider specified/) + ensure + droplets_cfg.close! + end end - # END LEGACY FOG SUPPORT TESTS - # After removal, change error message expectation to /No storage_type specified/ - it 'raise when no resource type' do + it 'raises when resource_type is missing' do expect do StorageCliClient.new(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: nil) end.to raise_error(RuntimeError, 'Missing resource_type') @@ -161,21 +247,12 @@ module Blobstore describe 'resource_type → config file selection & validation' do let(:config_double) { instance_double(VCAP::CloudController::Config) } - let(:droplets_cfg) { Tempfile.new(['droplets', '.json']) } - let(:buildpacks_cfg) { Tempfile.new(['buildpacks', '.json']) } - let(:packages_cfg) { Tempfile.new(['packages', '.json']) } - let(:resource_pool_cfg) { Tempfile.new(['resource_pool', '.json']) } + let(:droplets_cfg) { write_config_file(provider: 'azurebs', account_key: 'key', account_name: 'acc', container_name: 'cont', environment: 'Cloud') } + let(:buildpacks_cfg) { write_config_file(provider: 'azurebs', account_key: 'key', account_name: 'acc', container_name: 'cont', environment: 'Cloud') } + let(:packages_cfg) { write_config_file(provider: 'azurebs', account_key: 'key', account_name: 'acc', container_name: 'cont', environment: 'Cloud') } + let(:resource_pool_cfg) { write_config_file(provider: 'azurebs', account_key: 'key', account_name: 'acc', container_name: 'cont', environment: 'Cloud') } before do - [droplets_cfg, buildpacks_cfg, packages_cfg, resource_pool_cfg].each do |f| - f.write({ provider: 'AzureRM', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json) - f.flush - end - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) allow(config_double).to receive(:get) do |key| @@ -264,22 +341,10 @@ def build_client(resource_type) describe 'client helper operations' do describe 'Json operations' do - let(:droplets_cfg) do - f = Tempfile.new(['droplets', '.json']) - f.write({ provider: 'AzureRM', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json) - f.flush - f - end - - let(:config_double) { instance_double(VCAP::CloudController::Config) } + let(:droplets_cfg) { write_config_file(provider: 'azurebs', account_key: 'key', account_name: 'acc', container_name: 'cont', environment: 'Cloud') } before do - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) + stub_config_for_droplets(droplets_cfg.path) end after do @@ -302,19 +367,9 @@ def build_client(resource_type) end describe 'with valid client' do + let(:droplets_cfg) { write_config_file(provider: 'azurebs', account_key: 'key', account_name: 'acc', container_name: 'cont', environment: 'Cloud') } let(:client) do - droplets_cfg = Tempfile.new(['droplets', '.json']) - droplets_cfg.write({ provider: 'AzureRM', - account_key: 'bommelkey', - account_name: 'bommel', - container_name: 'bommelcontainer', - environment: 'BommelCloud' }.to_json) - droplets_cfg.flush - - config_double = instance_double(VCAP::CloudController::Config) - allow(VCAP::CloudController::Config).to receive(:config).and_return(config_double) - allow(config_double).to receive(:get).with(:storage_cli_config_file_droplets).and_return(droplets_cfg.path) - + stub_config_for_droplets(droplets_cfg.path) StorageCliClient.new( directory_key: 'dummy-key', root_dir: 'dummy-root', @@ -322,6 +377,10 @@ def build_client(resource_type) ) end + after do + droplets_cfg.close! + end + it '#local? returns false' do expect(client.local?).to be false end @@ -340,14 +399,13 @@ def build_client(resource_type) describe 'client operations' do let!(:tmp_cfg) do - f = Tempfile.new(['storage_cli_config', '.json']) - f.write({ provider: 'AzureRM', - account_name: 'some-account-name', - account_key: 'some-access-key', - container_name: directory_key, - environment: 'AzureCloud' }.to_json) - f.flush - f + write_config_file( + provider: 'azurebs', + account_name: 'some-account-name', + account_key: 'some-access-key', + container_name: directory_key, + environment: 'AzureCloud' + ) end before do @@ -371,13 +429,6 @@ def build_client(resource_type) subject(:client) { StorageCliClient.new(directory_key: directory_key, resource_type: resource_type, root_dir: 'bommel') } let(:directory_key) { 'my-bucket' } let(:resource_type) { 'resource_pool' } - let(:downloaded_file) do - Tempfile.open('') do |tmpfile| - tmpfile.write('downloaded file content') - tmpfile - end - end - let(:deletable_blob) { StorageCliBlob.new('deletable-blob') } let(:dest_path) { File.join(Dir.mktmpdir, SecureRandom.uuid) } @@ -397,7 +448,7 @@ def build_client(resource_type) allow(VCAP::CloudController::Config.config).to receive(:get).with(:storage_cli_optional_flags).and_return('-log-level warn -log-file some/path/storage-cli.log') end - it('returns empty list') { + it('returns list with parsed flags') { expect(client.send(:additional_flags)).to eq(['-log-level', 'warn', '-log-file', 'some/path/storage-cli.log']) } end @@ -465,7 +516,7 @@ def build_client(resource_type) describe '#blob' do let(:properties_json) { '{"etag": "test-etag", "last_modified": "2024-10-01T00:00:00Z", "content_length": 1024}' } - it 'returns a list of StorageCliBlob instances for a given key' do + it 'returns a StorageCliBlob for a given key' do allow(client).to receive(:run_cli).with('properties', 'bommel/va/li/valid-blob').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) allow(client).to receive(:run_cli).with('sign', 'bommel/va/li/valid-blob', 'get', '3600s').and_return(['some-url', instance_double(Process::Status, exitstatus: 0)]) @@ -493,9 +544,13 @@ def build_client(resource_type) end describe '#run_cli' do + before do + allow(client).to receive(:additional_flags).and_return([]) + end + it 'returns output and status on success' do status = instance_double(Process::Status, success?: true, exitstatus: 0) - allow(Open3).to receive(:capture3).with(anything, '-s', anything, '-c', anything, 'list', 'arg1').and_return(['ok', '', status]) + allow(Open3).to receive(:capture3).and_return(['ok', '', status]) output, returned_status = client.send(:run_cli, 'list', 'arg1') expect(output).to eq('ok') @@ -504,7 +559,7 @@ def build_client(resource_type) it 'raises an error on failure' do status = instance_double(Process::Status, success?: false, exitstatus: 1) - allow(Open3).to receive(:capture3).with(anything, '-s', anything, '-c', anything, 'list', 'arg1').and_return(['', 'error message', status]) + allow(Open3).to receive(:capture3).and_return(['', 'error message', status]) expect do client.send(:run_cli, 'list', 'arg1') @@ -513,7 +568,7 @@ def build_client(resource_type) it 'allows exit code 3 if specified' do status = instance_double(Process::Status, success?: false, exitstatus: 3) - allow(Open3).to receive(:capture3).with(anything, '-s', anything, '-c', anything, 'list', 'arg1').and_return(['', 'error message', status]) + allow(Open3).to receive(:capture3).and_return(['', 'error message', status]) output, returned_status = client.send(:run_cli, 'list', 'arg1', allow_exit_code_three: true) expect(output).to eq('') From 40ca610913489abd4737b0f27dff5ccc3e9e3fca Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 11 May 2026 13:15:09 +0200 Subject: [PATCH 2/3] Add lazy signing support for WebDAV StorageCliClient Implements on-demand URL signing for WebDAV blobstore to support dual endpoints (internal for Diego, public for external users). - Add supports_lazy_signing? method to detect DAV provider - Add sign_internal_url and sign_public_url methods for DAV - StorageCliBlob generates URLs on-demand when lazy signing enabled - Non-DAV providers continue using eager signing (pre-generated URLs) - Add comprehensive test coverage for lazy signing functionality This maintains backward compatibility with existing fog/webdav behavior where internal and public signed URLs are generated on-demand with different endpoints. --- .../blobstore/storage_cli/storage_cli_blob.rb | 10 +- .../storage_cli/storage_cli_client.rb | 25 +- .../storage_cli/storage_cli_blob_spec.rb | 129 +++++++++- .../storage_cli/storage_cli_client_spec.rb | 222 ++++++++++++++++-- 4 files changed, 364 insertions(+), 22 deletions(-) diff --git a/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb b/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb index b16f9f19777..8c4c98563c2 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb @@ -3,18 +3,26 @@ module Blobstore class StorageCliBlob < Blob attr_reader :key - def initialize(key, properties: nil, signed_url: nil) + def initialize(key, properties: nil, signed_url: nil, storage_cli_client: nil, expires_in_seconds: 3600) @key = key @signed_url = signed_url if signed_url + @storage_cli_client = storage_cli_client + @expires_in_seconds = expires_in_seconds # Set properties to an empty hash if nil to avoid nil errors @properties = properties || {} end def internal_download_url + # For DAV with lazy signing support, generate URL on-demand + return @storage_cli_client.sign_internal_url(@key, verb: 'get', expires_in_seconds: @expires_in_seconds) if @storage_cli_client&.supports_lazy_signing? + signed_url end def public_download_url + # For DAV with lazy signing support, generate URL on-demand + return @storage_cli_client.sign_public_url(@key, verb: 'get', expires_in_seconds: @expires_in_seconds) if @storage_cli_client&.supports_lazy_signing? + signed_url end diff --git a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb index fe2fcc28f3e..f532fd86c75 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb @@ -168,8 +168,29 @@ def blob(key) properties = properties(key) return nil if properties.nil? || properties.empty? - signed_url = sign_url(partitioned_key(key), verb: 'get', expires_in_seconds: 3600) - StorageCliBlob.new(key, properties:, signed_url:) + # For DAV with lazy signing support, pass client reference for on-demand signing + # For other providers, generate signed URL eagerly + if supports_lazy_signing? + StorageCliBlob.new(key, properties: properties, storage_cli_client: self, expires_in_seconds: 3600) + else + signed_url = sign_url(partitioned_key(key), verb: 'get', expires_in_seconds: 3600) + StorageCliBlob.new(key, properties:, signed_url:) + end + end + + def supports_lazy_signing? + # Only DAV with external signer needs lazy signing for internal vs public endpoints + @storage_type == 'dav' + end + + def sign_internal_url(key, verb:, expires_in_seconds:) + stdout, _status = run_cli('sign-internal', partitioned_key(key), verb.to_s.downcase, "#{expires_in_seconds}s") + stdout.strip + end + + def sign_public_url(key, verb:, expires_in_seconds:) + stdout, _status = run_cli('sign-public', partitioned_key(key), verb.to_s.downcase, "#{expires_in_seconds}s") + stdout.strip end def files_for(prefix, _ignored_directory_prefixes=[]) diff --git a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb index 1595f3caa7a..3fecb0549c3 100644 --- a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb @@ -18,15 +18,134 @@ module Blobstore end describe 'download_urls' do - it 'returns the internal download URL of the blob' do - expect(blob.internal_download_url).to eq(signed_url) + context 'with pre-generated signed URL (eager signing for non-DAV providers)' do + it 'returns the internal download URL of the blob' do + expect(blob.internal_download_url).to eq(signed_url) + end + + it 'returns the public download URL of the blob' do + expect(blob.public_download_url).to eq(signed_url) + end + end + + context 'with lazy signing (for DAV provider)' do + let(:storage_cli_client) { double('StorageCliClient') } + + before do + allow(storage_cli_client).to receive(:supports_lazy_signing?).and_return(true) + end + + subject(:lazy_blob) do + StorageCliBlob.new( + 'dr/op/droplet-guid', + properties: properties, + storage_cli_client: storage_cli_client, + expires_in_seconds: 3600 + ) + end + + describe '#internal_download_url' do + it 'calls sign_internal_url on the storage_cli_client' do + expect(storage_cli_client).to receive(:sign_internal_url).with( + 'dr/op/droplet-guid', + verb: 'get', + expires_in_seconds: 3600 + ).and_return('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789') + + url = lazy_blob.internal_download_url + + expect(url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789') + end + + it 'generates URL on-demand each time it is called' do + call_count = 0 + allow(storage_cli_client).to receive(:sign_internal_url) do + call_count += 1 + "https://blobstore.internal/url-#{call_count}" + end + + url1 = lazy_blob.internal_download_url + url2 = lazy_blob.internal_download_url + + expect(url1).to eq('https://blobstore.internal/url-1') + expect(url2).to eq('https://blobstore.internal/url-2') + expect(call_count).to eq(2) + end + end + + describe '#public_download_url' do + it 'calls sign_public_url on the storage_cli_client' do + expect(storage_cli_client).to receive(:sign_public_url).with( + 'dr/op/droplet-guid', + verb: 'get', + expires_in_seconds: 3600 + ).and_return('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=public456&expires=999') + + url = lazy_blob.public_download_url + + expect(url).to eq('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=public456&expires=999') + end + + it 'generates URL on-demand each time it is called' do + call_count = 0 + allow(storage_cli_client).to receive(:sign_public_url) do + call_count += 1 + "https://blobstore.public/url-#{call_count}" + end + + url1 = lazy_blob.public_download_url + url2 = lazy_blob.public_download_url + + expect(url1).to eq('https://blobstore.public/url-1') + expect(url2).to eq('https://blobstore.public/url-2') + expect(call_count).to eq(2) + end + end + + it 'uses custom expires_in_seconds when provided' do + custom_blob = StorageCliBlob.new( + 'test-key', + properties: properties, + storage_cli_client: storage_cli_client, + expires_in_seconds: 7200 + ) + + expect(storage_cli_client).to receive(:sign_internal_url).with( + 'test-key', + verb: 'get', + expires_in_seconds: 7200 + ).and_return('url') + + custom_blob.internal_download_url + end end - it 'returns the public download URL of the blob' do - expect(blob.public_download_url).to eq(signed_url) + context 'when storage_cli_client does not support lazy signing' do + let(:storage_cli_client) { double('StorageCliClient') } + + before do + allow(storage_cli_client).to receive(:supports_lazy_signing?).and_return(false) + end + + subject(:non_lazy_blob) do + StorageCliBlob.new( + 'test-blob', + properties: properties, + signed_url: signed_url, + storage_cli_client: storage_cli_client + ) + end + + it 'falls back to using pre-generated signed_url for internal_download_url' do + expect(non_lazy_blob.internal_download_url).to eq(signed_url) + end + + it 'falls back to using pre-generated signed_url for public_download_url' do + expect(non_lazy_blob.public_download_url).to eq(signed_url) + end end - context 'when signed_url is not provided' do + context 'when signed_url is not provided and no storage_cli_client' do subject(:blob_without_signed_url) { StorageCliBlob.new('test-blob', properties:) } it 'raises an error when accessing internal_download_url' do diff --git a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb index 789e5435a15..d5cfc2f8a9e 100644 --- a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb @@ -516,20 +516,80 @@ def build_client(resource_type) describe '#blob' do let(:properties_json) { '{"etag": "test-etag", "last_modified": "2024-10-01T00:00:00Z", "content_length": 1024}' } - it 'returns a StorageCliBlob for a given key' do - allow(client).to receive(:run_cli).with('properties', 'bommel/va/li/valid-blob').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) - allow(client).to receive(:run_cli).with('sign', 'bommel/va/li/valid-blob', 'get', '3600s').and_return(['some-url', instance_double(Process::Status, exitstatus: 0)]) - - blob = client.blob('valid-blob') - expect(blob).to be_a(StorageCliBlob) - expect(blob.key).to eq('valid-blob') - expect(blob.attributes(:etag, :last_modified, :content_length)).to eq({ - etag: 'test-etag', - last_modified: '2024-10-01T00:00:00Z', - content_length: 1024 - }) - expect(blob.internal_download_url).to eq('some-url') - expect(blob.public_download_url).to eq('some-url') + context 'for non-DAV providers (eager signing)' do + it 'returns a StorageCliBlob with pre-generated signed URL' do + allow(client).to receive(:run_cli).with('properties', 'bommel/va/li/valid-blob').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) + allow(client).to receive(:run_cli).with('sign', 'bommel/va/li/valid-blob', 'get', '3600s').and_return(['some-url', instance_double(Process::Status, exitstatus: 0)]) + + blob = client.blob('valid-blob') + expect(blob).to be_a(StorageCliBlob) + expect(blob.key).to eq('valid-blob') + expect(blob.attributes(:etag, :last_modified, :content_length)).to eq({ + etag: 'test-etag', + last_modified: '2024-10-01T00:00:00Z', + content_length: 1024 + }) + expect(blob.internal_download_url).to eq('some-url') + expect(blob.public_download_url).to eq('some-url') + end + end + + context 'for DAV provider (lazy signing)' do + let!(:dav_cfg) do + write_config_file( + provider: 'dav', + endpoint: 'https://blobstore.internal:4443/admin/cc-droplets', + public_endpoint: 'https://blobstore.example.com/admin/cc-droplets', + user: 'testuser', + password: 'testpass', + signed_url_format: 'external-nginx-secure-link-signer' + ) + end + + let(:dav_client) do + stub_config_for_droplets(dav_cfg.path) + StorageCliClient.new( + directory_key: 'cc-droplets', + root_dir: nil, + resource_type: 'droplets' + ) + end + + after { dav_cfg.close! } + + it 'returns a StorageCliBlob with storage_cli_client reference for lazy signing' do + allow(dav_client).to receive(:run_cli).with('properties', 'dr/op/droplet-guid').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) + + blob = dav_client.blob('droplet-guid') + expect(blob).to be_a(StorageCliBlob) + expect(blob.key).to eq('droplet-guid') + expect(blob.instance_variable_get(:@storage_cli_client)).to eq(dav_client) + expect(blob.instance_variable_get(:@signed_url)).to be_nil + end + + it 'generates internal URL on-demand when internal_download_url is called' do + allow(dav_client).to receive(:run_cli).with('properties', 'dr/op/droplet-guid').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) + allow(dav_client).to receive(:run_cli).with('sign-internal', 'dr/op/droplet-guid', 'get', + '3600s').and_return(['https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=abc&expires=123', + instance_double(Process::Status, exitstatus: 0)]) + + blob = dav_client.blob('droplet-guid') + internal_url = blob.internal_download_url + + expect(internal_url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=abc&expires=123') + end + + it 'generates public URL on-demand when public_download_url is called' do + allow(dav_client).to receive(:run_cli).with('properties', 'dr/op/droplet-guid').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) + allow(dav_client).to receive(:run_cli).with('sign-public', 'dr/op/droplet-guid', 'get', + '3600s').and_return(['https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=xyz&expires=456', + instance_double(Process::Status, exitstatus: 0)]) + + blob = dav_client.blob('droplet-guid') + public_url = blob.public_download_url + + expect(public_url).to eq('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=xyz&expires=456') + end end it 'raises an error if the cli output is empty' do @@ -543,6 +603,140 @@ def build_client(resource_type) end end + describe '#supports_lazy_signing?' do + context 'for DAV provider' do + let!(:dav_cfg) do + write_config_file( + provider: 'dav', + endpoint: 'https://blobstore.internal:4443', + user: 'testuser', + password: 'testpass' + ) + end + + let(:dav_client) do + stub_config_for_droplets(dav_cfg.path) + StorageCliClient.new( + directory_key: 'cc-droplets', + root_dir: nil, + resource_type: 'droplets' + ) + end + + after { dav_cfg.close! } + + it 'returns true' do + expect(dav_client.supports_lazy_signing?).to be true + end + end + + context 'for non-DAV providers' do + let!(:s3_cfg) do + write_config_file( + provider: 's3', + bucket_name: 'test-bucket', + access_key_id: 'key', + secret_access_key: 'secret' + ) + end + + let(:s3_client) do + stub_config_for_droplets(s3_cfg.path) + StorageCliClient.new( + directory_key: 'cc-droplets', + root_dir: nil, + resource_type: 'droplets' + ) + end + + after { s3_cfg.close! } + + it 'returns false for S3' do + expect(s3_client.supports_lazy_signing?).to be false + end + end + end + + describe '#sign_internal_url' do + let!(:dav_cfg) do + write_config_file( + provider: 'dav', + endpoint: 'https://blobstore.internal:4443/admin/cc-droplets', + public_endpoint: 'https://blobstore.example.com/admin/cc-droplets', + user: 'testuser', + password: 'testpass' + ) + end + + let(:dav_client) do + stub_config_for_droplets(dav_cfg.path) + StorageCliClient.new( + directory_key: 'cc-droplets', + root_dir: nil, + resource_type: 'droplets' + ) + end + + after { dav_cfg.close! } + + it 'calls storage-cli sign-internal command and returns signed URL' do + expect(dav_client).to receive(:run_cli).with('sign-internal', 'dr/o/dr/op/droplet-guid', 'get', + '7200s').and_return(['https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789', + instance_double(Process::Status, exitstatus: 0)]) + + signed_url = dav_client.sign_internal_url('dr/op/droplet-guid', verb: 'get', expires_in_seconds: 7200) + + expect(signed_url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789') + end + + it 'converts verb to lowercase' do + expect(dav_client).to receive(:run_cli).with('sign-internal', 'dr/o/dr/op/droplet-guid', 'get', + '3600s').and_return(['url', instance_double(Process::Status, exitstatus: 0)]) + + dav_client.sign_internal_url('dr/op/droplet-guid', verb: :GET, expires_in_seconds: 3600) + end + end + + describe '#sign_public_url' do + let!(:dav_cfg) do + write_config_file( + provider: 'dav', + endpoint: 'https://blobstore.internal:4443/admin/cc-droplets', + public_endpoint: 'https://blobstore.example.com/admin/cc-droplets', + user: 'testuser', + password: 'testpass' + ) + end + + let(:dav_client) do + stub_config_for_droplets(dav_cfg.path) + StorageCliClient.new( + directory_key: 'cc-droplets', + root_dir: nil, + resource_type: 'droplets' + ) + end + + after { dav_cfg.close! } + + it 'calls storage-cli sign-public command and returns signed URL' do + expect(dav_client).to receive(:run_cli).with('sign-public', 'pa/c/pa/ck/package-guid', 'get', + '1800s').and_return(['https://blobstore.example.com/read/cc-packages/pa/ck/package-guid?md5=public456&expires=999', + instance_double(Process::Status, exitstatus: 0)]) + + signed_url = dav_client.sign_public_url('pa/ck/package-guid', verb: 'get', expires_in_seconds: 1800) + + expect(signed_url).to eq('https://blobstore.example.com/read/cc-packages/pa/ck/package-guid?md5=public456&expires=999') + end + + it 'converts verb to lowercase' do + expect(dav_client).to receive(:run_cli).with('sign-public', 'pa/c/pa/ck/package-guid', 'put', + '3600s').and_return(['url', instance_double(Process::Status, exitstatus: 0)]) + + dav_client.sign_public_url('pa/ck/package-guid', verb: :PUT, expires_in_seconds: 3600) + end + end + describe '#run_cli' do before do allow(client).to receive(:additional_flags).and_return([]) From 592a065acf919dbbc181351b7f6c5098c06526c7 Mon Sep 17 00:00:00 2001 From: johha Date: Wed, 17 Jun 2026 15:07:42 +0200 Subject: [PATCH 3/3] Decouple StorageCliBlob from StorageCliClient Pass use_internal_url: through .blob() so the caller picks which URL to sign. StorageCliBlob becomes a pure value object holding a single signed_url; the dav-vs-other switch lives in StorageCliClient#sign_url. Also fix ErrorHandlingClient#blob to forward kwargs (was silently dropping them). --- .../blobstore/error_handling_client.rb | 4 +- .../blobstore/fog/fog_client.rb | 2 +- .../blobstore/local/local_client.rb | 2 +- lib/cloud_controller/blobstore/null_client.rb | 2 +- .../blobstore/retryable_client.rb | 7 +- .../blobstore/storage_cli/storage_cli_blob.rb | 13 +- .../storage_cli/storage_cli_client.rb | 37 ++-- .../url_generator/internal_url_generator.rb | 8 +- .../blobstore/webdav/dav_client.rb | 2 +- .../blobstore/retryable_client_spec.rb | 2 +- .../storage_cli/storage_cli_blob_spec.rb | 129 +------------ .../storage_cli/storage_cli_client_spec.rb | 174 ++---------------- .../internal_url_generator_spec.rb | 8 +- 13 files changed, 54 insertions(+), 336 deletions(-) diff --git a/lib/cloud_controller/blobstore/error_handling_client.rb b/lib/cloud_controller/blobstore/error_handling_client.rb index b11cedc3afa..23bfdab4496 100644 --- a/lib/cloud_controller/blobstore/error_handling_client.rb +++ b/lib/cloud_controller/blobstore/error_handling_client.rb @@ -23,8 +23,8 @@ def exists?(*) error_handling { wrapped_client.exists?(*) } end - def blob(*) - error_handling { wrapped_client.blob(*) } + def blob(*, **) + error_handling { wrapped_client.blob(*, **) } end def files_for(*) diff --git a/lib/cloud_controller/blobstore/fog/fog_client.rb b/lib/cloud_controller/blobstore/fog/fog_client.rb index 217093f8130..d6ea7033fa6 100644 --- a/lib/cloud_controller/blobstore/fog/fog_client.rb +++ b/lib/cloud_controller/blobstore/fog/fog_client.rb @@ -113,7 +113,7 @@ def delete_blob(blob) delete_file(blob.file) if blob.file end - def blob(key) + def blob(key, **) f = file(key) FogBlob.new(f, @cdn) if f end diff --git a/lib/cloud_controller/blobstore/local/local_client.rb b/lib/cloud_controller/blobstore/local/local_client.rb index 028e944a80c..cb331c27300 100644 --- a/lib/cloud_controller/blobstore/local/local_client.rb +++ b/lib/cloud_controller/blobstore/local/local_client.rb @@ -78,7 +78,7 @@ def delete(key) cleanup_empty_parent_directories(path) end - def blob(key) + def blob(key, **) path = file_path(key) return unless File.exist?(path) diff --git a/lib/cloud_controller/blobstore/null_client.rb b/lib/cloud_controller/blobstore/null_client.rb index 074eb183bff..51e0b7b2f00 100644 --- a/lib/cloud_controller/blobstore/null_client.rb +++ b/lib/cloud_controller/blobstore/null_client.rb @@ -31,7 +31,7 @@ def download_uri(_key) def ensure_bucket_exists; end - def blob(_key) + def blob(_key, **) Blob.new end diff --git a/lib/cloud_controller/blobstore/retryable_client.rb b/lib/cloud_controller/blobstore/retryable_client.rb index 89ee3297e4b..c0e1488fd49 100644 --- a/lib/cloud_controller/blobstore/retryable_client.rb +++ b/lib/cloud_controller/blobstore/retryable_client.rb @@ -110,13 +110,14 @@ def delete_blob(blob) end end - def blob(key) + def blob(key, use_internal_url: false) with_retries(__method__.to_s, { args: { - key: + key:, + use_internal_url: } }) do - blob = @wrapped_client.blob(key) + blob = @wrapped_client.blob(key, use_internal_url: use_internal_url) RetryableBlob.new(blob: blob, errors: @retryable_errors, logger: @logger, num_retries: @num_retries) if blob end end diff --git a/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb b/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb index 8c4c98563c2..f5e0e9e05e4 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb @@ -3,26 +3,17 @@ module Blobstore class StorageCliBlob < Blob attr_reader :key - def initialize(key, properties: nil, signed_url: nil, storage_cli_client: nil, expires_in_seconds: 3600) + def initialize(key, properties: nil, signed_url: nil) @key = key - @signed_url = signed_url if signed_url - @storage_cli_client = storage_cli_client - @expires_in_seconds = expires_in_seconds - # Set properties to an empty hash if nil to avoid nil errors + @signed_url = signed_url @properties = properties || {} end def internal_download_url - # For DAV with lazy signing support, generate URL on-demand - return @storage_cli_client.sign_internal_url(@key, verb: 'get', expires_in_seconds: @expires_in_seconds) if @storage_cli_client&.supports_lazy_signing? - signed_url end def public_download_url - # For DAV with lazy signing support, generate URL on-demand - return @storage_cli_client.sign_public_url(@key, verb: 'get', expires_in_seconds: @expires_in_seconds) if @storage_cli_client&.supports_lazy_signing? - signed_url end diff --git a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb index 43766c263d3..8b2b5ef3850 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb @@ -162,33 +162,12 @@ def delete_blob(blob) delete(blob.key) end - def blob(key) + def blob(key, use_internal_url: false) properties = properties(key) return nil if properties.nil? || properties.empty? - # For DAV with lazy signing support, pass client reference for on-demand signing - # For other providers, generate signed URL eagerly - if supports_lazy_signing? - StorageCliBlob.new(key, properties: properties, storage_cli_client: self, expires_in_seconds: 3600) - else - signed_url = sign_url(partitioned_key(key), verb: 'get', expires_in_seconds: 3600) - StorageCliBlob.new(key, properties:, signed_url:) - end - end - - def supports_lazy_signing? - # Only DAV with external signer needs lazy signing for internal vs public endpoints - @storage_type == 'dav' - end - - def sign_internal_url(key, verb:, expires_in_seconds:) - stdout, _status = run_cli('sign-internal', partitioned_key(key), verb.to_s.downcase, "#{expires_in_seconds}s") - stdout.strip - end - - def sign_public_url(key, verb:, expires_in_seconds:) - stdout, _status = run_cli('sign-public', partitioned_key(key), verb.to_s.downcase, "#{expires_in_seconds}s") - stdout.strip + signed_url = sign_url(partitioned_key(key), verb: 'get', expires_in_seconds: 3600, use_internal_url: use_internal_url) + StorageCliBlob.new(key, properties: properties, signed_url: signed_url) end def files_for(prefix, _ignored_directory_prefixes=[]) @@ -228,8 +207,14 @@ def run_cli(command, *args, allow_exit_code_three: false) [stdout, status] end - def sign_url(key, verb:, expires_in_seconds:) - stdout, _status = run_cli('sign', key, verb.to_s.downcase, "#{expires_in_seconds}s") + def sign_url(key, verb:, expires_in_seconds:, use_internal_url: false) + command = + if @storage_type == 'dav' + use_internal_url ? 'sign-internal' : 'sign-public' + else + 'sign' + end + stdout, _status = run_cli(command, key, verb.to_s.downcase, "#{expires_in_seconds}s") stdout.strip end diff --git a/lib/cloud_controller/blobstore/url_generator/internal_url_generator.rb b/lib/cloud_controller/blobstore/url_generator/internal_url_generator.rb index c9d81f0e49a..92ee55e907d 100644 --- a/lib/cloud_controller/blobstore/url_generator/internal_url_generator.rb +++ b/lib/cloud_controller/blobstore/url_generator/internal_url_generator.rb @@ -14,7 +14,7 @@ def initialize(blobstore_options, package_blobstore, buildpack_cache_blobstore, end def admin_buildpack_download_url(buildpack) - blob = @admin_buildpack_blobstore.blob(buildpack.key) + blob = @admin_buildpack_blobstore.blob(buildpack.key, use_internal_url: true) message = "Missing blob for #{buildpack.name}. Specify a different buildpack with the -b flag or contact your operator." raise CloudController::Errors::ApiError.new_from_details('StagingError', message) unless blob @@ -25,21 +25,21 @@ def admin_buildpack_download_url(buildpack) def droplet_download_url(droplet) return nil unless droplet - blob = @droplet_blobstore.blob(droplet.blobstore_key) + blob = @droplet_blobstore.blob(droplet.blobstore_key, use_internal_url: true) return nil unless blob url_for_blob(blob) end def buildpack_cache_download_url(app_guid, stack) - blob = @buildpack_cache_blobstore.blob("#{app_guid}/#{stack}") + blob = @buildpack_cache_blobstore.blob("#{app_guid}/#{stack}", use_internal_url: true) return nil unless blob url_for_blob(blob) end def package_download_url(package) - blob = @package_blobstore.blob(package.guid) + blob = @package_blobstore.blob(package.guid, use_internal_url: true) return nil unless blob url_for_blob(blob) diff --git a/lib/cloud_controller/blobstore/webdav/dav_client.rb b/lib/cloud_controller/blobstore/webdav/dav_client.rb index a5c9f85e664..421a68d45f1 100644 --- a/lib/cloud_controller/blobstore/webdav/dav_client.rb +++ b/lib/cloud_controller/blobstore/webdav/dav_client.rb @@ -132,7 +132,7 @@ def delete(key) raise_blobstore_error("Could not delete object, #{response.status}/#{response.content}") end - def blob(key) + def blob(key, **) response = with_error_handling { @client.head(url(key), header: @headers) } return DavBlob.new(httpmessage: response, key: partitioned_key(key), signer: @signer) if response.status == 200 diff --git a/spec/unit/lib/cloud_controller/blobstore/retryable_client_spec.rb b/spec/unit/lib/cloud_controller/blobstore/retryable_client_spec.rb index b7e3de44b00..ebfbf40ded5 100644 --- a/spec/unit/lib/cloud_controller/blobstore/retryable_client_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/retryable_client_spec.rb @@ -193,7 +193,7 @@ class RetryableError < StandardError client.blob(key) end.to raise_error RetryableError - expect(wrapped_client).to have_received(:blob).with(key).exactly(num_retries).times + expect(wrapped_client).to have_received(:blob).with(key, use_internal_url: false).exactly(num_retries).times end end diff --git a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb index 3fecb0549c3..0617270140a 100644 --- a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob_spec.rb @@ -18,134 +18,15 @@ module Blobstore end describe 'download_urls' do - context 'with pre-generated signed URL (eager signing for non-DAV providers)' do - it 'returns the internal download URL of the blob' do - expect(blob.internal_download_url).to eq(signed_url) - end - - it 'returns the public download URL of the blob' do - expect(blob.public_download_url).to eq(signed_url) - end - end - - context 'with lazy signing (for DAV provider)' do - let(:storage_cli_client) { double('StorageCliClient') } - - before do - allow(storage_cli_client).to receive(:supports_lazy_signing?).and_return(true) - end - - subject(:lazy_blob) do - StorageCliBlob.new( - 'dr/op/droplet-guid', - properties: properties, - storage_cli_client: storage_cli_client, - expires_in_seconds: 3600 - ) - end - - describe '#internal_download_url' do - it 'calls sign_internal_url on the storage_cli_client' do - expect(storage_cli_client).to receive(:sign_internal_url).with( - 'dr/op/droplet-guid', - verb: 'get', - expires_in_seconds: 3600 - ).and_return('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789') - - url = lazy_blob.internal_download_url - - expect(url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789') - end - - it 'generates URL on-demand each time it is called' do - call_count = 0 - allow(storage_cli_client).to receive(:sign_internal_url) do - call_count += 1 - "https://blobstore.internal/url-#{call_count}" - end - - url1 = lazy_blob.internal_download_url - url2 = lazy_blob.internal_download_url - - expect(url1).to eq('https://blobstore.internal/url-1') - expect(url2).to eq('https://blobstore.internal/url-2') - expect(call_count).to eq(2) - end - end - - describe '#public_download_url' do - it 'calls sign_public_url on the storage_cli_client' do - expect(storage_cli_client).to receive(:sign_public_url).with( - 'dr/op/droplet-guid', - verb: 'get', - expires_in_seconds: 3600 - ).and_return('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=public456&expires=999') - - url = lazy_blob.public_download_url - - expect(url).to eq('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=public456&expires=999') - end - - it 'generates URL on-demand each time it is called' do - call_count = 0 - allow(storage_cli_client).to receive(:sign_public_url) do - call_count += 1 - "https://blobstore.public/url-#{call_count}" - end - - url1 = lazy_blob.public_download_url - url2 = lazy_blob.public_download_url - - expect(url1).to eq('https://blobstore.public/url-1') - expect(url2).to eq('https://blobstore.public/url-2') - expect(call_count).to eq(2) - end - end - - it 'uses custom expires_in_seconds when provided' do - custom_blob = StorageCliBlob.new( - 'test-key', - properties: properties, - storage_cli_client: storage_cli_client, - expires_in_seconds: 7200 - ) - - expect(storage_cli_client).to receive(:sign_internal_url).with( - 'test-key', - verb: 'get', - expires_in_seconds: 7200 - ).and_return('url') - - custom_blob.internal_download_url - end + it 'returns the signed_url for internal_download_url' do + expect(blob.internal_download_url).to eq(signed_url) end - context 'when storage_cli_client does not support lazy signing' do - let(:storage_cli_client) { double('StorageCliClient') } - - before do - allow(storage_cli_client).to receive(:supports_lazy_signing?).and_return(false) - end - - subject(:non_lazy_blob) do - StorageCliBlob.new( - 'test-blob', - properties: properties, - signed_url: signed_url, - storage_cli_client: storage_cli_client - ) - end - - it 'falls back to using pre-generated signed_url for internal_download_url' do - expect(non_lazy_blob.internal_download_url).to eq(signed_url) - end - - it 'falls back to using pre-generated signed_url for public_download_url' do - expect(non_lazy_blob.public_download_url).to eq(signed_url) - end + it 'returns the signed_url for public_download_url' do + expect(blob.public_download_url).to eq(signed_url) end - context 'when signed_url is not provided and no storage_cli_client' do + context 'when signed_url is not provided' do subject(:blob_without_signed_url) { StorageCliBlob.new('test-blob', properties:) } it 'raises an error when accessing internal_download_url' do diff --git a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb index 67b8bd50ce9..09e4cf44a11 100644 --- a/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/storage_cli/storage_cli_client_spec.rb @@ -492,8 +492,8 @@ def build_client(resource_type) describe '#blob' do let(:properties_json) { '{"etag": "test-etag", "last_modified": "2024-10-01T00:00:00Z", "content_length": 1024}' } - context 'for non-DAV providers (eager signing)' do - it 'returns a StorageCliBlob with pre-generated signed URL' do + context 'for non-DAV providers' do + it 'returns a StorageCliBlob with a signed URL using the generic sign command (default use_internal_url: false)' do allow(client).to receive(:run_cli).with('properties', 'bommel/va/li/valid-blob').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) allow(client).to receive(:run_cli).with('sign', 'bommel/va/li/valid-blob', 'get', '3600s').and_return(['some-url', instance_double(Process::Status, exitstatus: 0)]) @@ -505,12 +505,19 @@ def build_client(resource_type) last_modified: '2024-10-01T00:00:00Z', content_length: 1024 }) - expect(blob.internal_download_url).to eq('some-url') expect(blob.public_download_url).to eq('some-url') end + + it 'uses the generic sign command regardless of use_internal_url' do + allow(client).to receive(:run_cli).with('properties', 'bommel/va/li/valid-blob').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) + allow(client).to receive(:run_cli).with('sign', 'bommel/va/li/valid-blob', 'get', '3600s').and_return(['some-url', instance_double(Process::Status, exitstatus: 0)]) + + blob = client.blob('valid-blob', use_internal_url: true) + expect(blob.internal_download_url).to eq('some-url') + end end - context 'for DAV provider (lazy signing)' do + context 'for DAV provider' do let!(:dav_cfg) do write_config_file( provider: 'dav', @@ -533,38 +540,25 @@ def build_client(resource_type) after { dav_cfg.close! } - it 'returns a StorageCliBlob with storage_cli_client reference for lazy signing' do - allow(dav_client).to receive(:run_cli).with('properties', 'dr/op/droplet-guid').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) - - blob = dav_client.blob('droplet-guid') - expect(blob).to be_a(StorageCliBlob) - expect(blob.key).to eq('droplet-guid') - expect(blob.instance_variable_get(:@storage_cli_client)).to eq(dav_client) - expect(blob.instance_variable_get(:@signed_url)).to be_nil - end - - it 'generates internal URL on-demand when internal_download_url is called' do + it 'invokes sign-internal when use_internal_url is true' do allow(dav_client).to receive(:run_cli).with('properties', 'dr/op/droplet-guid').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) allow(dav_client).to receive(:run_cli).with('sign-internal', 'dr/op/droplet-guid', 'get', '3600s').and_return(['https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=abc&expires=123', instance_double(Process::Status, exitstatus: 0)]) - blob = dav_client.blob('droplet-guid') - internal_url = blob.internal_download_url - - expect(internal_url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=abc&expires=123') + blob = dav_client.blob('droplet-guid', use_internal_url: true) + expect(blob).to be_a(StorageCliBlob) + expect(blob.internal_download_url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=abc&expires=123') end - it 'generates public URL on-demand when public_download_url is called' do + it 'invokes sign-public by default' do allow(dav_client).to receive(:run_cli).with('properties', 'dr/op/droplet-guid').and_return([properties_json, instance_double(Process::Status, exitstatus: 0)]) allow(dav_client).to receive(:run_cli).with('sign-public', 'dr/op/droplet-guid', 'get', '3600s').and_return(['https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=xyz&expires=456', instance_double(Process::Status, exitstatus: 0)]) blob = dav_client.blob('droplet-guid') - public_url = blob.public_download_url - - expect(public_url).to eq('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=xyz&expires=456') + expect(blob.public_download_url).to eq('https://blobstore.example.com/read/cc-droplets/dr/op/droplet-guid?md5=xyz&expires=456') end end @@ -579,140 +573,6 @@ def build_client(resource_type) end end - describe '#supports_lazy_signing?' do - context 'for DAV provider' do - let!(:dav_cfg) do - write_config_file( - provider: 'dav', - endpoint: 'https://blobstore.internal:4443', - user: 'testuser', - password: 'testpass' - ) - end - - let(:dav_client) do - stub_config_for_droplets(dav_cfg.path) - StorageCliClient.new( - directory_key: 'cc-droplets', - root_dir: nil, - resource_type: 'droplets' - ) - end - - after { dav_cfg.close! } - - it 'returns true' do - expect(dav_client.supports_lazy_signing?).to be true - end - end - - context 'for non-DAV providers' do - let!(:s3_cfg) do - write_config_file( - provider: 's3', - bucket_name: 'test-bucket', - access_key_id: 'key', - secret_access_key: 'secret' - ) - end - - let(:s3_client) do - stub_config_for_droplets(s3_cfg.path) - StorageCliClient.new( - directory_key: 'cc-droplets', - root_dir: nil, - resource_type: 'droplets' - ) - end - - after { s3_cfg.close! } - - it 'returns false for S3' do - expect(s3_client.supports_lazy_signing?).to be false - end - end - end - - describe '#sign_internal_url' do - let!(:dav_cfg) do - write_config_file( - provider: 'dav', - endpoint: 'https://blobstore.internal:4443/admin/cc-droplets', - public_endpoint: 'https://blobstore.example.com/admin/cc-droplets', - user: 'testuser', - password: 'testpass' - ) - end - - let(:dav_client) do - stub_config_for_droplets(dav_cfg.path) - StorageCliClient.new( - directory_key: 'cc-droplets', - root_dir: nil, - resource_type: 'droplets' - ) - end - - after { dav_cfg.close! } - - it 'calls storage-cli sign-internal command and returns signed URL' do - expect(dav_client).to receive(:run_cli).with('sign-internal', 'dr/o/dr/op/droplet-guid', 'get', - '7200s').and_return(['https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789', - instance_double(Process::Status, exitstatus: 0)]) - - signed_url = dav_client.sign_internal_url('dr/op/droplet-guid', verb: 'get', expires_in_seconds: 7200) - - expect(signed_url).to eq('https://blobstore.internal:4443/read/cc-droplets/dr/op/droplet-guid?md5=internal123&expires=789') - end - - it 'converts verb to lowercase' do - expect(dav_client).to receive(:run_cli).with('sign-internal', 'dr/o/dr/op/droplet-guid', 'get', - '3600s').and_return(['url', instance_double(Process::Status, exitstatus: 0)]) - - dav_client.sign_internal_url('dr/op/droplet-guid', verb: :GET, expires_in_seconds: 3600) - end - end - - describe '#sign_public_url' do - let!(:dav_cfg) do - write_config_file( - provider: 'dav', - endpoint: 'https://blobstore.internal:4443/admin/cc-droplets', - public_endpoint: 'https://blobstore.example.com/admin/cc-droplets', - user: 'testuser', - password: 'testpass' - ) - end - - let(:dav_client) do - stub_config_for_droplets(dav_cfg.path) - StorageCliClient.new( - directory_key: 'cc-droplets', - root_dir: nil, - resource_type: 'droplets' - ) - end - - after { dav_cfg.close! } - - it 'calls storage-cli sign-public command and returns signed URL' do - expect(dav_client).to receive(:run_cli).with('sign-public', 'pa/c/pa/ck/package-guid', 'get', - '1800s').and_return(['https://blobstore.example.com/read/cc-packages/pa/ck/package-guid?md5=public456&expires=999', - instance_double(Process::Status, exitstatus: 0)]) - - signed_url = dav_client.sign_public_url('pa/ck/package-guid', verb: 'get', expires_in_seconds: 1800) - - expect(signed_url).to eq('https://blobstore.example.com/read/cc-packages/pa/ck/package-guid?md5=public456&expires=999') - end - - it 'converts verb to lowercase' do - expect(dav_client).to receive(:run_cli).with('sign-public', 'pa/c/pa/ck/package-guid', 'put', - '3600s').and_return(['url', instance_double(Process::Status, exitstatus: 0)]) - - dav_client.sign_public_url('pa/ck/package-guid', verb: :PUT, expires_in_seconds: 3600) - end - end - describe '#run_cli' do before do allow(client).to receive(:additional_flags).and_return([]) diff --git a/spec/unit/lib/cloud_controller/blobstore/url_generator/internal_url_generator_spec.rb b/spec/unit/lib/cloud_controller/blobstore/url_generator/internal_url_generator_spec.rb index 4113555cf5c..12e70c16e16 100644 --- a/spec/unit/lib/cloud_controller/blobstore/url_generator/internal_url_generator_spec.rb +++ b/spec/unit/lib/cloud_controller/blobstore/url_generator/internal_url_generator_spec.rb @@ -43,7 +43,7 @@ module Blobstore it 'gives out signed url to remote blobstore for admin buildpack' do expect(url_generator.admin_buildpack_download_url(buildpack)).to eql(internal_url) - expect(admin_buildpack_blobstore).to have_received(:blob).with(buildpack.key) + expect(admin_buildpack_blobstore).to have_received(:blob).with(buildpack.key, use_internal_url: true) end context 'when the buildpack does not exist' do @@ -76,7 +76,7 @@ module Blobstore it 'gives out signed url to remote blobstore from the blob' do expect(url_generator.droplet_download_url(droplet)).to eql(internal_url) - expect(droplet_blobstore).to have_received(:blob).with(droplet.blobstore_key) + expect(droplet_blobstore).to have_received(:blob).with(droplet.blobstore_key, use_internal_url: true) end context 'when the droplet does not exist' do @@ -108,7 +108,7 @@ module Blobstore it 'gives out signed url to remote blobstore for buildpack cache' do expect(url_generator.buildpack_cache_download_url(app_model.guid, stack)).to eql(internal_url) - expect(buildpack_cache_blobstore).to have_received(:blob).with("#{app_model.guid}/#{stack}") + expect(buildpack_cache_blobstore).to have_received(:blob).with("#{app_model.guid}/#{stack}", use_internal_url: true) end context 'when the buildpack cache does not exist' do @@ -139,7 +139,7 @@ module Blobstore it 'gives out signed url to remote blobstore for package' do expect(url_generator.package_download_url(package)).to eql(internal_url) - expect(package_blobstore).to have_received(:blob).with(package.guid) + expect(package_blobstore).to have_received(:blob).with(package.guid, use_internal_url: true) end context 'and the package does not exist' do