Geo: Ignore invalid attributes when updating Geo node status

If replication has not caught up properly on the secondary,
it was possible for a primary Geo node to attempt an update
of the GeoNodeStatus entry with a `nil` ID, resulting
in an Error 500.

We handle this in two ways:

1. On the primary, ignore `id` and other unknown attributes.
2. On the secondary, never send `id`.

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/8966

ee/app/services/geo/node_status_post_service.rb

@@ -5,7 +5,7 @@ module Geo
     include Gitlab::Geo::LogHelpers
 
     def execute(status)
-      response = Gitlab::HTTP.post(primary_status_url, body: status.attributes, allow_local_requests: true, headers: headers, timeout: timeout)
+      response = Gitlab::HTTP.post(primary_status_url, body: payload(status), allow_local_requests: true, headers: headers, timeout: timeout)
 
       unless response.success?
         handle_failure_for(response)
@@ -26,6 +26,10 @@ module Geo
 
     private
 
+    def payload(status)
+      status.attributes.except('id') # rubocop: disable CodeReuse/ActiveRecord
+    end
+
     def handle_failure_for(response)
       message = "Could not connect to Geo primary node - HTTP Status Code: #{response.code} #{response.message}"
       payload = response.parsed_response

ee/changelogs/unreleased/sh-fix-geo-node-status-null-insert.yml

+---
+title: 'Geo: Ignore invalid attributes when updating Geo node status'
+merge_request: 8957
+author:
+type: fixed

ee/lib/api/geo.rb

@@ -5,6 +5,14 @@ require 'base64'
 module API
   class Geo < Grape::API
     resource :geo do
+      helpers do
+        def sanitized_node_status_params
+          allowed_attributes = GeoNodeStatus.attribute_names - ['id']
+          valid_attributes = params.keys & allowed_attributes
+          params.slice(*valid_attributes)
+        end
+      end
+
       # Verify the GitLab Geo transfer request is valid
       # All transfers use the Authorization header to pass a JWT
       # payload.
@@ -40,7 +48,7 @@ module API
 
         db_status = GeoNode.find(params[:geo_node_id]).find_or_build_status
 
-        unless db_status.update(params.merge(last_successful_status_check_at: Time.now.utc))
+        unless db_status.update(sanitized_node_status_params.merge(last_successful_status_check_at: Time.now.utc))
           render_validation_error!(db_status)
         end
       end

ee/spec/requests/api/geo_spec.rb

@@ -223,10 +223,9 @@ describe API::Geo do
         geo_node_id: secondary_node.id,
         status_message: nil,
         db_replication_lag_seconds: 0,
-        repositories_count: 10,
+        projects_count: 10,
         repositories_synced_count: 1,
         repositories_failed_count: 2,
-        wikis_count: 10,
         wikis_synced_count: 2,
         wikis_failed_count: 3,
         lfs_objects_count: 100,
@@ -281,7 +280,21 @@ describe API::Geo do
         expect { request }.to change { GeoNodeStatus.count }.by(1)
 
         expect(response).to have_gitlab_http_status(201)
-        expect(secondary_node.reload.status.repositories_count).to eq(10)
+        expect(secondary_node.reload.status.projects_count).to eq(10)
+      end
+
+      it 'ignores invalid attributes upon update' do
+        GeoNodeStatus.create(data)
+        data.merge!(
+          {
+            'id' => nil,
+            'test' => 'something'
+          }
+        )
+
+        post api('/geo/status'), params: data, headers: geo_base_request.headers
+
+        expect(response).to have_gitlab_http_status(201)
       end
 
       it_behaves_like 'with terms enforced'

ee/spec/services/geo/node_status_post_service_spec.rb

@@ -26,7 +26,7 @@ describe Geo::NodeStatusPostService, :geo do
       allow(Gitlab::HTTP).to receive(:post).and_raise(OpenSSL::SSL::SSLError.new(message))
       expect(subject).to receive(:log_error).with('Failed to post status data to primary', kind_of(OpenSSL::SSL::SSLError))
 
-      expect(subject.execute(secondary)).to be_falsey
+      expect(subject.execute(secondary.find_or_build_status)).to be_falsey
     end
 
     it 'handles connection refused' do
@@ -34,7 +34,7 @@ describe Geo::NodeStatusPostService, :geo do
 
       expect(subject).to receive(:log_error).with('Failed to post status data to primary', kind_of(Errno::ECONNREFUSED))
 
-      expect(subject.execute(secondary)).to be_falsey
+      expect(subject.execute(secondary.find_or_build_status)).to be_falsey
     end
 
     it 'returns meaningful error message when primary uses incorrect db key' do
@@ -45,7 +45,7 @@ describe Geo::NodeStatusPostService, :geo do
         kind_of(OpenSSL::Cipher::CipherError)
       )
 
-      expect(subject.execute(secondary)).to be_falsey
+      expect(subject.execute(secondary.find_or_build_status)).to be_falsey
     end
 
     it 'gracefully handles case when primary is deleted' do
@@ -55,7 +55,24 @@ describe Geo::NodeStatusPostService, :geo do
         'Failed to look up Geo primary node in the database'
       )
 
-      expect(subject.execute(secondary)).to be_falsey
+      expect(subject.execute(secondary.find_or_build_status)).to be_falsey
+    end
+
+    it 'does not include id in the payload' do
+      stub_current_geo_node(primary)
+
+      expect(Gitlab::HTTP).to receive(:post)
+                                .with(
+                                  primary.status_url,
+                                  hash_including(body: hash_not_including('id')))
+                                .and_return(double(success?: true))
+
+      subject.execute(GeoNodeStatus.new({
+        geo_node_id: secondary.id,
+        status_message: nil,
+        db_replication_lag_seconds: 0,
+        repositories_count: 10
+      }))
     end
 
     it 'sends geo_node_id in the request' do