adapt ccng to read config from json, add config checks, adapt test

Author: kathap

lib/cloud_controller/blobstore/client_provider.rb

@@ -72,14 +72,13 @@ def provide_webdav(options, directory_key, root_dir)
         end
 
         def provide_storage_cli(options, directory_key, root_dir, resource_type)
-          raise BlobstoreError.new('connection_config for storage-cli is not provided') unless options[:connection_config]
-
-          client = StorageCliClient.build(connection_config: options.fetch(:connection_config),
-                                          directory_key: directory_key,
-                                          resource_type: resource_type,
-                                          root_dir: root_dir,
-                                          min_size: options[:minimum_size],
-                                          max_size: options[:maximum_size])
+          client = StorageCliClient.build(
+            directory_key: directory_key,
+            resource_type: resource_type,
+            root_dir: root_dir,
+            min_size: options[:minimum_size],
+            max_size: options[:maximum_size]
+          )
 
           logger = Steno.logger('cc.blobstore.storage_cli_client')
           errors = [StandardError]

lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb

@@ -16,29 +16,82 @@ class << self
         attr_reader :registry
 
         def register(provider, klass)
-          registry[provider] = klass
+          registry[provider.to_s] = klass
         end
 
-        def build(connection_config:, directory_key:, root_dir:, resource_type: nil, min_size: nil, max_size: nil)
-          provider = connection_config[:provider]
-          raise 'Missing connection_config[:provider]' if provider.nil?
+        def build(directory_key:, root_dir:, resource_type: nil, min_size: nil, max_size: nil)
           raise 'Missing resource_type' if resource_type.nil?
 
-          impl_class = registry[provider]
+          cfg = fetch_and_validate_config!(resource_type)
+          provider = cfg['provider']
+
+          key        = provider.to_s
+          impl_class = registry[key] || registry[key.downcase] || registry[key.upcase]
           raise "No storage CLI client registered for provider #{provider}" unless impl_class
 
-          impl_class.new(connection_config:, directory_key:, root_dir:, resource_type:, min_size:, max_size:)
+          impl_class.new(provider:, directory_key:, root_dir:, resource_type:, min_size:, max_size:)
+        end
+
+        def fetch_and_validate_config!(resource_type)
+          path = config_path_for!(resource_type)
+
+          begin
+            json = Oj.load(File.read(path))
+          rescue StandardError => e
+            raise BlobstoreError.new("Failed to parse storage-cli config JSON at #{path}: #{e.message}")
+          end
+
+          validate_required_keys!(json, path)
+          json
+        end
+
+        def config_path_for!(resource_type)
+          key =
+            case resource_type.to_s
+            when 'droplets', 'buildpack_cache' then :storage_cli_config_file_droplets
+            when 'buildpacks'                  then :storage_cli_config_file_buildpacks
+            when 'packages'                    then :storage_cli_config_file_packages
+            when 'resource_pool'               then :storage_cli_config_file_resource_pool
+            else
+              raise BlobstoreError.new("Unknown resource_type: #{resource_type}")
+            end
+
+          path = VCAP::CloudController::Config.config.get(key)
+          raise BlobstoreError.new("storage-cli config file not found or not readable at: #{path.inspect}") unless path && File.file?(path) && File.readable?(path)
+
+          path
+        end
+
+        def validate_required_keys!(json, path)
+          validate_provider!(json, path)
+          required = %w[
+            account_key
+            account_name
+            container_name
+            environment
+          ]
+          missing = required.reject { |k| json.key?(k) && !json[k].to_s.strip.empty? }
+          return if missing.empty?
+
+          raise BlobstoreError.new("Missing required keys in config file #{path}: #{missing.join(', ')} (json: #{json})")
+        end
+
+        def validate_provider!(json, path)
+          provider = json['provider']
+          return unless provider.nil? || provider.to_s.strip.empty?
+
+          raise BlobstoreError.new("No provider specified in config file: #{path.inspect} json: #{json}")
         end
       end
 
-      def initialize(connection_config:, directory_key:, resource_type:, root_dir:, min_size: nil, max_size: nil)
+      def initialize(provider:, directory_key:, resource_type:, root_dir:, min_size: nil, max_size: nil)
         @cli_path = cli_path
         @directory_key = directory_key
         @resource_type = resource_type.to_s
         @root_dir = root_dir
         @min_size = min_size || 0
         @max_size = max_size
-        @fork = connection_config.fetch(:fork, false)
+        @provider = provider
 
         file_path = case @resource_type
                     when 'droplets', 'buildpack_cache'
@@ -114,28 +167,16 @@ def cp_to_blobstore(source_path, destination_key)
       end
 
       def cp_file_between_keys(source_key, destination_key)
-        if @fork
-          run_cli('copy', partitioned_key(source_key), partitioned_key(destination_key))
-        else
-          # Azure CLI doesn't support server-side copy yet, so fallback to local copy
-          Tempfile.create('blob-copy') do |tmp|
-            download_from_blobstore(source_key, tmp.path)
-            cp_to_blobstore(tmp.path, destination_key)
-          end
-        end
+        run_cli('copy', partitioned_key(source_key), partitioned_key(destination_key))
       end
 
       def delete_all(_=nil)
         # page_size is currently not considered. Azure SDK / API has a limit of 5000
-        pass unless @fork
-
         # Currently, storage-cli does not support bulk deletion.
         run_cli('delete-recursive', @root_dir)
       end
 
       def delete_all_in_path(path)
-        pass unless @fork
-
         # Currently, storage-cli does not support bulk deletion.
         run_cli('delete-recursive', partitioned_key(path))
       end
@@ -149,29 +190,19 @@ def delete_blob(blob)
       end
 
       def blob(key)
-        if @fork
-          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:)
-        elsif exists?(key)
-          # Azure CLI does not support getting blob properties directly, so fallback to local check
-          signed_url = sign_url(partitioned_key(key), verb: 'get', expires_in_seconds: 3600)
-          StorageCliBlob.new(key, signed_url:)
-        end
+        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:)
       end
 
       def files_for(prefix, _ignored_directory_prefixes=[])
-        return nil unless @fork
-
         files, _status = run_cli('list', prefix)
         files.split("\n").map(&:strip).reject(&:empty?).map { |file| StorageCliBlob.new(file) }
       end
 
       def ensure_bucket_exists
-        return unless @fork
-
         run_cli('ensure-bucket-exists')
       end
 

spec/unit/lib/cloud_controller/blobstore/client_provider_spec.rb

@@ -129,24 +129,37 @@ module Blobstore
       context 'when storage-cli is requested' do
         let(:blobstore_type) { 'storage-cli' }
         let(:directory_key) { 'some-bucket' }
-        let(:resource_type) { 'some-resource-type' }
+        let(:resource_type) { 'droplets' }
         let(:root_dir) { 'some-root-dir' }
         let(:storage_cli_client_mock) { class_double(CloudController::Blobstore::StorageCliClient) }
+        let(:tmpdir)         { Dir.mktmpdir('storage_cli_spec') }
+        let(:config_path)    { File.join(tmpdir, 'storage_cli_config_droplets.json') }
 
         before do
-          options.merge!(connection_config: {}, minimum_size: 100, maximum_size: 1000)
+          File.write(config_path, '{"provider": "AzureRM",
+                  "account_name": "some-account-name",
+                  "account_key": "some-access-key",
+                  "container_name": "directory_key",
+                  "environment": "AzureCloud" }')
+          allow(VCAP::CloudController::Config.config).to receive(:get).with(:storage_cli_config_file_droplets).and_return(config_path)
+          options.merge!(provider: 'AzureRM', minimum_size: 100, maximum_size: 1000)
         end
 
         it 'provides a storage-cli client' do
           allow(StorageCliClient).to receive(:build).and_return(storage_cli_client_mock)
           ClientProvider.provide(options:, directory_key:, root_dir:, resource_type:)
-          expect(StorageCliClient).to have_received(:build).with(connection_config: {}, directory_key: directory_key, resource_type: resource_type, root_dir: root_dir,
+          expect(StorageCliClient).to have_received(:build).with(directory_key: directory_key, resource_type: resource_type, root_dir: root_dir,
                                                                  min_size: 100, max_size: 1000)
         end
 
-        it 'raises an error if connection_config is not provided' do
-          options.delete(:connection_config)
-          expect { ClientProvider.provide(options:, directory_key:, root_dir:) }.to raise_error(BlobstoreError, 'connection_config for storage-cli is not provided')
+        it 'raises an error if provider is not provided' do
+          config_path = VCAP::CloudController::Config.config.get(:storage_cli_config_file_droplets)
+          File.write(config_path,
+                     '{"provider": "", "account_name": "some-account-name", "account_key": "some-access-key", "container_name": "directory_key", "environment": "AzureCloud" }')
+          expect { ClientProvider.provide(options:, directory_key:, root_dir:, resource_type:) }.to raise_error(BlobstoreError) { |e|
+            expect(e.message).to include('No provider specified in config file:')
+            expect(e.message).to include(File.basename(config_path))
+          }
         end
       end
     end

spec/unit/lib/cloud_controller/blobstore/storage_cli/azure_storage_cli_client_spec.rb

@@ -32,23 +32,14 @@ module Blobstore
             tmp_cfg.path
           end
         end
+        allow(Steno).to receive(:logger).and_return(double(info: nil, error: nil))
       end
 
       after { tmp_cfg.close! }
 
-      subject(:client) { AzureStorageCliClient.new(connection_config: connection_config, directory_key: directory_key, resource_type: resource_type, root_dir: 'bommel') }
+      subject(:client) { AzureStorageCliClient.new(provider: 'AzureRM', directory_key: directory_key, resource_type: resource_type, root_dir: 'bommel') }
       let(:directory_key) { 'my-bucket' }
       let(:resource_type) { 'resource_pool' }
-      let(:connection_config) do
-        {
-          azure_storage_access_key: 'some-access-key',
-          azure_storage_account_name: 'some-account-name',
-          container_name: directory_key,
-          environment: 'AzureCloud',
-          provider: 'AzureRM',
-          fork: true
-        }
-      end
       let(:downloaded_file) do
         Tempfile.open('') do |tmpfile|
           tmpfile.write('downloaded file content')