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 b16f9f19777..f5e0e9e05e4 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_blob.rb @@ -5,8 +5,7 @@ class StorageCliBlob < Blob def initialize(key, properties: nil, signed_url: nil) @key = key - @signed_url = signed_url if signed_url - # Set properties to an empty hash if nil to avoid nil errors + @signed_url = signed_url @properties = properties || {} 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 2d6c1009de2..8b2b5ef3850 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb @@ -17,15 +17,15 @@ 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 = { '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) @@ -37,10 +37,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 @@ -166,12 +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? - signed_url = sign_url(partitioned_key(key), verb: 'get', expires_in_seconds: 3600) - StorageCliBlob.new(key, properties:, signed_url:) + 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=[]) @@ -211,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 1595f3caa7a..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,11 +18,11 @@ module Blobstore end describe 'download_urls' do - it 'returns the internal download URL of the blob' do + it 'returns the signed_url for internal_download_url' do expect(blob.internal_download_url).to eq(signed_url) end - it 'returns the public download URL of the blob' do + it 'returns the signed_url for public_download_url' do expect(blob.public_download_url).to eq(signed_url) end 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 322b5bc4069..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 @@ -4,125 +4,215 @@ module CloudController module Blobstore RSpec.describe StorageCliClient do - describe 'client init' do - 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' - ) - 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') + # Helper methods + def write_config_file(hash) + file = Tempfile.new(['storage-cli', '.json']) + file.write(hash.to_json) + file.flush + file + end - droplets_cfg.close! - 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 '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 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' ) - 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('s3') + 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 Google legacy provider to gcs storage-cli type' do + droplets_cfg = write_config_file( + provider: 'Google', + bucket_name: 'test-bucket', + json_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('gcs') + 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 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' ) - 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('alioss') + 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 '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' + ) + 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') + 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 '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' ) - 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: UnknownProvider/) + 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 azurebs (native storage-cli type)' do + droplets_cfg = write_config_file( + provider: 'azurebs', + account_key: 'bommelkey', + account_name: 'bommel', + container_name: 'bommelcontainer', + environment: 'BommelCloud' + ) + begin + stub_config_for_droplets(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 + + 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') @@ -132,21 +222,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: 'azurebs', - 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| @@ -235,22 +316,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: 'azurebs', - 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 @@ -273,18 +342,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: '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) + stub_config_for_droplets(droplets_cfg.path) StorageCliClient.new( directory_key: 'dummy-key', @@ -293,6 +353,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 @@ -311,14 +375,13 @@ def build_client(resource_type) describe 'client operations' do let!(:tmp_cfg) do - f = Tempfile.new(['storage_cli_config', '.json']) - f.write({ provider: 'azurebs', - 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 @@ -342,13 +405,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) } @@ -368,7 +424,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 @@ -436,20 +492,74 @@ 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 - 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)]) + 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)]) + + 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.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') - 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') + blob = client.blob('valid-blob', use_internal_url: true) + expect(blob.internal_download_url).to eq('some-url') + end + end + + context 'for DAV provider' 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 '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', 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 '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') + expect(blob.public_download_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 @@ -464,9 +574,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') @@ -475,7 +589,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') @@ -484,7 +598,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('') 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