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[
+            azure_storage_access_key
+            azure_storage_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/storage_cli_client_spec.rb

@@ -5,31 +5,65 @@ module CloudController
   module Blobstore
     RSpec.describe StorageCliClient do
       describe 'registry build and lookup' do
-        it 'builds the correct client' do
+        it 'builds the correct client when JSON has provider AzureRM' do
           droplets_cfg = Tempfile.new(['droplets', '.json'])
-          droplets_cfg.write({ connection_config: { provider: 'AzureRM' } }.to_json)
+          droplets_cfg.write({ provider: 'AzureRM',
+                               azure_storage_access_key: 'bommelkey',
+                               azure_storage_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_from_registry = StorageCliClient.build(connection_config: { provider: 'AzureRM' }, directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets')
+          client_from_registry = StorageCliClient.build(
+            directory_key: 'dummy-key',
+            root_dir: 'dummy-root',
+            resource_type: 'droplets'
+          )
           expect(client_from_registry).to be_a(AzureStorageCliClient)
 
           droplets_cfg.close!
         end
 
         it 'raises an error for an unregistered provider' do
+          droplets_cfg = Tempfile.new(['droplets', '.json'])
+          droplets_cfg.write(
+            { provider: 'UnknownProvider',
+              azure_storage_access_key: 'bommelkey',
+              azure_storage_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)
+
           expect do
-            StorageCliClient.build(connection_config: { provider: 'UnknownProvider' }, directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets')
+            StorageCliClient.build(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets')
           end.to raise_error(RuntimeError, 'No storage CLI client registered for provider UnknownProvider')
+
+          droplets_cfg.close!
         end
 
-        it 'raises an error if provider is missing' do
+        it 'raises an error when provider is missing from the JSON' do
+          droplets_cfg = Tempfile.new(['droplets', '.json'])
+          droplets_cfg.write({}.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)
+
           expect do
-            StorageCliClient.build(connection_config: {}, directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets')
-          end.to raise_error(RuntimeError, 'Missing connection_config[:provider]')
+            StorageCliClient.build(directory_key: 'dummy-key', root_dir: 'dummy-root', resource_type: 'droplets')
+          end.to raise_error(CloudController::Blobstore::BlobstoreError, /No provider specified in config file/)
+
+          droplets_cfg.close!
         end
       end
 
@@ -42,9 +76,12 @@ module Blobstore
         let(:resource_pool_cfg) { Tempfile.new(['resource_pool', '.json']) }
 
         before do
-          # Valid JSON (YAML can parse JSON)
           [droplets_cfg, buildpacks_cfg, packages_cfg, resource_pool_cfg].each do |f|
-            f.write({ connection_config: { provider: 'AzureRM' } }.to_json)
+            f.write({ provider: 'AzureRM',
+                      azure_storage_access_key: 'bommelkey',
+                      azure_storage_account_name: 'bommel',
+                      container_name: 'bommelcontainer',
+                      environment: 'BommelCloud' }.to_json)
             f.flush
           end
 
@@ -59,7 +96,6 @@ module Blobstore
             end
           end
 
-          # Quiet logger noise in specs
           allow(Steno).to receive(:logger).and_return(double(info: nil, error: nil))
         end
 
@@ -69,7 +105,6 @@ module Blobstore
 
         def build_client(resource_type)
           StorageCliClient.build(
-            connection_config: { provider: 'AzureRM' },
             directory_key: 'dir-key',
             root_dir: 'root',
             resource_type: resource_type
@@ -119,11 +154,39 @@ def build_client(resource_type)
           end.to raise_error(CloudController::Blobstore::BlobstoreError, /not found or not readable/)
         end
 
+        it 'raises when provider is missing from config file' do
+          File.write(packages_cfg.path, {
+            azure_storage_access_key: 'bommelkey',
+            azure_storage_account_name: 'bommel',
+            container_name: 'bommelcontainer',
+            environment: 'BommelCloud'
+          }.to_json)
+
+          expect do
+            build_client('packages')
+          end.to raise_error(
+            CloudController::Blobstore::BlobstoreError,
+            /No provider specified/
+          )
+        end
+
         it 'raises when YAML load fails' do
-          File.write(packages_cfg.path, '{ this is: [not, valid }')
+          File.write(packages_cfg.path, { provider: 'AzureRM',
+                                          azure_storage_access_key: 'bommelkey',
+                                          azure_storage_account_name: 'bommel',
+                                          container_name: 'bommelcontainer',
+                                          environment: 'BommelCloud' }.to_json)
+
+          expect(VCAP::CloudController::YAMLConfig).to receive(:safe_load_file).
+            with(packages_cfg.path).
+            and_raise(StandardError.new('parse blew up'))
+
           expect do
             build_client('packages')
-          end.to raise_error(CloudController::Blobstore::BlobstoreError, /Failed to load storage-cli config/)
+          end.to raise_error(
+            CloudController::Blobstore::BlobstoreError,
+            /Failed to load storage-cli config/
+          )
         end
       end
 
@@ -132,21 +195,23 @@ def build_client(resource_type)
         let(:droplets_cfg)  { Tempfile.new(['droplets', '.json']) }
 
         before do
-          droplets_cfg.write({ connection_config: { provider: 'AzureRM' } }.to_json)
+          droplets_cfg.write({ provider: 'AzureRM',
+                               azure_storage_access_key: 'bommelkey',
+                               azure_storage_account_name: 'bommel',
+                               container_name: 'bommelcontainer',
+                               environment: 'BommelCloud' }.to_json)
           droplets_cfg.flush
 
           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)
 
-          # Avoid logger noise
           allow(Steno).to receive(:logger).and_return(double(info: nil, error: nil))
         end
 
         after { droplets_cfg.close! }
 
         let(:client) do
           StorageCliClient.build(
-            connection_config: { provider: 'AzureRM' },
             directory_key: 'dir',
             root_dir: 'root',
             resource_type: 'droplets'