Merge pull request #2404 from danielfor/nats-race-condition-183572544

Nats race condition 183572544

src/bosh-nats-sync/lib/nats_sync/users_sync.rb

@@ -14,25 +14,53 @@ def initialize(nats_config_file_path, bosh_config, nats_server_executable, nats_
 
     def execute_users_sync
       NATSSync.logger.info 'Executing NATS Users Synchronization'
-      vms_uuids = query_all_running_vms
-      current_file_hash = nats_file_hash
-      write_nats_config_file(vms_uuids, read_subject_file(@bosh_config['director_subject_file']),
-                             read_subject_file(@bosh_config['hm_subject_file']))
-      new_file_hash = nats_file_hash
-      UsersSync.reload_nats_server_config(@nats_server_executable, @nats_server_pid_file) unless current_file_hash == new_file_hash
+      vms_uuids = []
+      overwriteable_config_file = true
+      begin
+        vms_uuids = query_all_running_vms
+      rescue RuntimeError => e
+        NATSSync.logger.error "Could not query all running vms: #{e.message}"
+        overwriteable_config_file = user_file_overwritable?
+        if overwriteable_config_file
+          NATSSync.logger.info 'NATS config file is empty, writing basic users config file.'
+        else
+          NATSSync.logger.info 'NATS config file is not empty, doing nothing.'
+        end
+      end
+
+      if overwriteable_config_file
+        current_file_hash = nats_file_hash
+        write_nats_config_file(vms_uuids, read_subject_file(@bosh_config['director_subject_file']),
+                               read_subject_file(@bosh_config['hm_subject_file']))
+        new_file_hash = nats_file_hash
+        unless current_file_hash == new_file_hash
+          UsersSync.reload_nats_server_config(@nats_server_executable,
+                                              @nats_server_pid_file)
+        end
+      end
       NATSSync.logger.info 'Finishing NATS Users Synchronization'
-      vms_uuids
     end
 
     def self.reload_nats_server_config(nats_server_executable, nats_server_pid_file)
       output, status = Open3.capture2e("#{nats_server_executable} --signal reload=#{nats_server_pid_file}")
+
+      # rubocop:disable Style/GuardClause
+      # rubocop:disable Layout/LineLength
       unless status.success?
         raise("Cannot execute: #{nats_server_executable} --signal reload=#{nats_server_pid_file}, Status Code: #{status} \nError: #{output}")
       end
+      # rubocop:enable Style/GuardClause
+      # rubocop:enable Layout/LineLength
     end
 
     private
 
+    def user_file_overwritable?
+      JSON.parse(File.read(@nats_config_file_path)).empty?
+    rescue RuntimeError, JSON::ParserError
+      true
+    end
+
     def read_subject_file(file_path)
       return nil unless File.exist?(file_path)
       return nil if File.empty?(file_path)
@@ -45,10 +73,12 @@ def nats_file_hash
     end
 
     def call_bosh_api(endpoint)
+      auth_header = create_authentication_header
+      NATSSync.logger.debug 'auth_header is empty, next REST call could fail' if auth_header.nil? || auth_header.empty?
       response = RestClient::Request.execute(
         url: @bosh_config['url'] + endpoint,
         method: :get,
-        headers: { 'Authorization' => create_authentication_header },
+        headers: { 'Authorization' => auth_header },
         verify_ssl: false,
       )
       NATSSync.logger.debug(response.inspect)

src/bosh-nats-sync/spec/nats_sync/users_sync_spec.rb

@@ -6,8 +6,10 @@ module NATSSync
   describe UsersSync do
     before do
       allow(NATSSync).to receive(:logger).and_return(logger)
-      allow(logger).to receive :info
-      allow(Open3).to receive(:capture2e).and_return(["Success", capture_status])
+      allow(logger).to receive(:debug)
+      allow(logger).to receive(:error)
+      allow(logger).to receive(:info)
+      allow(Open3).to receive(:capture2e).and_return(['Success', capture_status])
     end
 
     subject { UsersSync.new(nats_config_file_path, bosh_config, nats_executable, nats_server_pid_file) }
@@ -142,14 +144,70 @@ module NATSSync
 
     describe '#execute_nats_sync' do
       before do
-        stub_request(:get, url + '/info')
+        stub_request(:get, "#{url}/info")
           .to_return(status: 200, body: info_json)
-        stub_request(:get, url + '/deployments')
+        stub_request(:get, "#{url}/deployments")
           .with(headers: { 'Authorization' => 'Bearer xyz' })
           .to_return(status: 200, body: deployments_json)
         allow(auth_provider).to receive(:new).and_return(auth_provider_double)
         allow(auth_provider_double).to receive(:auth_header).and_return('Bearer xyz')
-        allow(logger).to receive(:debug)
+        File.open(nats_config_file_path, 'w') do |f|
+          f.write('{}')
+        end
+      end
+
+      describe 'when UAA is not deployed and the BOSH API is not available' do
+        before do
+          stub_request(:get, "#{url}/deployments")
+            .with(headers: { 'Authorization' => 'Bearer xyz' })
+            .to_return(status: 401, body: 'Unauthorized')
+        end
+
+        describe 'and the authentication file is empty' do
+          it 'should write the basic bosh configuration' do
+            expect(JSON.parse(File.read(nats_config_file_path)).empty?).to be true
+            subject.execute_users_sync
+            file = File.read(nats_config_file_path)
+            data_hash = JSON.parse(file)
+            expect(data_hash['authorization']['users'])
+              .to include(include('user' => director_subject))
+            expect(data_hash['authorization']['users'])
+              .to include(include('user' => hm_subject))
+            expect(data_hash['authorization']['users'].length).to eq(2)
+          end
+        end
+
+        describe 'and the authentication file is corrupted' do
+          before do
+            File.open(nats_config_file_path, 'w') do |f|
+              f.write('{invalidchar')
+            end
+          end
+          it 'should write the basic bosh configuration' do
+            subject.execute_users_sync
+            file = File.read(nats_config_file_path)
+            data_hash = JSON.parse(file)
+            expect(data_hash['authorization']['users'])
+              .to include(include('user' => director_subject))
+            expect(data_hash['authorization']['users'])
+              .to include(include('user' => hm_subject))
+            expect(data_hash['authorization']['users'].length).to eq(2)
+          end
+        end
+
+        describe 'and the authentication file is not empty' do
+          before do
+            File.open(nats_config_file_path, 'w') do |f|
+              f.write('{"authorization": {"users": [{"user": "foo"}]}}')
+            end
+          end
+          it 'should not overwrite the authentication file' do
+            subject.execute_users_sync
+            file = File.read(nats_config_file_path)
+            data_hash = JSON.parse(file)
+            expect(data_hash).to eq({ 'authorization' => { 'users' => [{ 'user' => 'foo' }] } })
+          end
+        end
       end
 
       describe 'when there are no deployments with running vms in Bosh' do
@@ -169,7 +227,7 @@ module NATSSync
 
       describe 'when there are deployments with running vms in Bosh' do
         before do
-          stub_request(:get, url + '/deployments/deployment-1/vms')
+          stub_request(:get, "#{url}/deployments/deployment-1/vms")
             .with(headers: { 'Authorization' => 'Bearer xyz' })
             .to_return(status: 200, body: vms_json)
         end
@@ -264,7 +322,7 @@ module NATSSync
         describe 'when reloading the NATs server fails' do
           let(:capture_status) { instance_double(Process::Status, success?: false) }
           before do
-            allow(Open3).to receive(:capture2e).and_return(["Failed to reload NATs server", capture_status])
+            allow(Open3).to receive(:capture2e).and_return(['Failed to reload NATs server', capture_status])
           end
 
           it 'should raise an error' do