From 11206be1f6a0214f92cfc29093e83b97acec955a Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Fri, 11 Aug 2023 11:11:34 -0400 Subject: [PATCH 1/2] Reject any events which do not have a timestamp The event timestamp is critical for uniquely identifying events and ensuring there are no duplicates. We cannot properly handle events without a timestamp so we need to not queue these for the event handler. --- .../container_manager/event_catcher_mixin.rb | 7 ++++++- .../container_manager/event_catcher_mixin_spec.rb | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb b/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb index 15816812b7..daca27a7c3 100644 --- a/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb +++ b/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb @@ -48,6 +48,8 @@ def monitor_events def queue_event(event) event_data = extract_event_data(event) + return if event_data.nil? + _log.info "#{log_prefix} Queuing event [#{event_data}]" event_hash = ManageIQ::Providers::Kubernetes::ContainerManager::EventParser.event_to_hash(event_data, @cfg[:ems_id]) EmsEvent.add_queue('add', @cfg[:ems_id], event_hash) @@ -55,6 +57,7 @@ def queue_event(event) def filtered?(event) event_data = extract_event_data(event) + return true if event_data.nil? supported_reasons = ENABLED_EVENTS[event_data[:kind]] || [] !supported_reasons.include?(event_data[:reason]) || filtered_events.include?(event_data[:event_type]) @@ -73,11 +76,13 @@ def extract_event_data(event) :event_uid => event.object.metadata.uid, } + # If there is no timestamp we cannot properly handle the event + return if event_data[:timestamp].nil? + unless event.object.involvedObject.fieldPath.nil? event_data[:fieldpath] = event.object.involvedObject.fieldPath end - event_type_prefix = event_data[:kind].upcase # Handle event data for specific entities diff --git a/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb b/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb index af62de3e1f..6bbdb23b3f 100644 --- a/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb +++ b/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb @@ -256,6 +256,12 @@ def log_prefix )) end + let(:missing_timestamp_event) do + array_recursive_ostruct(:object => kubernetes_event.merge( + 'lastTimestamp' => nil + )) + end + it 'without matching node returns nil uid' do expect(test_class.new(ems).extract_event_data(bad_uid_event)[:uid]).to eq(nil) expect(test_class.new(ems).extract_event_data(missing_uid_event)[:uid]).to eq(nil) @@ -270,6 +276,10 @@ def log_prefix expect(test_class.new(ems).extract_event_data(bad_uid_event)).to eq(expected_data) expect(test_class.new(ems).extract_event_data(missing_uid_event)).to eq(expected_data) end + + it 'with missing timestamp' do + expect(test_class.new(ems).extract_event_data(missing_timestamp_event)).to be_nil + end end end end @@ -287,6 +297,7 @@ def log_prefix 'metadata' => { 'uid' => 'SomeRandomUid', }, + 'lastTimestamp' => '2016-07-25T11:45:34Z', } end @@ -306,6 +317,7 @@ def log_prefix 'metadata' => { 'uid' => 'SomeRandomUid', }, + 'lastTimestamp' => '2016-07-25T11:45:34Z', } end From f6631b6af4b877832945254da986929593462612 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Fri, 11 Aug 2023 12:18:58 -0400 Subject: [PATCH 2/2] Log when skipping invalid events --- .../container_manager/event_catcher_mixin.rb | 16 +++-- .../event_catcher_mixin_spec.rb | 69 +++++++++++++------ 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb b/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb index daca27a7c3..f1e8b41591 100644 --- a/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb +++ b/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb @@ -48,7 +48,10 @@ def monitor_events def queue_event(event) event_data = extract_event_data(event) - return if event_data.nil? + if !event_valid?(event_data) + _log.info "#{log_prefix} Skipping invalid event [#{event_data[:event_type]}]" + return + end _log.info "#{log_prefix} Queuing event [#{event_data}]" event_hash = ManageIQ::Providers::Kubernetes::ContainerManager::EventParser.event_to_hash(event_data, @cfg[:ems_id]) @@ -57,7 +60,6 @@ def queue_event(event) def filtered?(event) event_data = extract_event_data(event) - return true if event_data.nil? supported_reasons = ENABLED_EVENTS[event_data[:kind]] || [] !supported_reasons.include?(event_data[:reason]) || filtered_events.include?(event_data[:event_type]) @@ -76,9 +78,6 @@ def extract_event_data(event) :event_uid => event.object.metadata.uid, } - # If there is no timestamp we cannot properly handle the event - return if event_data[:timestamp].nil? - unless event.object.involvedObject.fieldPath.nil? event_data[:fieldpath] = event.object.involvedObject.fieldPath end @@ -111,4 +110,11 @@ def extract_event_data(event) event_data end + + def event_valid?(event_data) + # If there is no timestamp we cannot properly handle the event + return false if event_data[:timestamp].nil? + + true + end end diff --git a/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb b/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb index 6bbdb23b3f..b86e338e76 100644 --- a/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb +++ b/spec/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin_spec.rb @@ -17,19 +17,54 @@ def log_prefix let(:ems) { FactoryBot.create(:ems_kubernetes) } describe '#queue_event' do + let(:subject) do + test_class.new(ems).tap { |catcher| catcher.instance_variable_set(:@cfg, :ems_id => ems.id) } + end + + let(:kubernetes_event) do + { + 'kind' => 'Event', + 'apiVersion' => 'v1', + 'reason' => 'Scheduled', + 'involvedObject' => { + 'kind' => 'Pod', + }, + 'metadata' => { + 'uid' => 'SomeRandomUid', + }, + 'lastTimestamp' => '2016-07-25T11:45:34Z' + } + end + + let(:event) { array_recursive_ostruct(:object => kubernetes_event) } + it 'sends the received event to queue after parsing' do - catcher = test_class.new(ems) - expect(catcher).to receive(:extract_event_data).once.with(:raw_event).and_return(:extracted_event) - expect(catcher).to receive(:log_prefix) - expect(catcher).to receive_message_chain("_log.info") - catcher.instance_variable_set(:@cfg, :ems_id => ems.id) - expect( - ManageIQ::Providers::Kubernetes::ContainerManager::EventParser - ).to receive( - :event_to_hash - ).with(:extracted_event, ems.id).and_return(:hashed_event) - expect(EmsEvent).to receive(:add_queue).with('add', ems.id, :hashed_event) - catcher.queue_event(:raw_event) + expect(EmsEvent).to receive(:add_queue).with('add', ems.id, hash_including(:event_type => "POD_SCHEDULED", :timestamp => '2016-07-25T11:45:34Z')) + + subject.queue_event(event) + end + + context "with an invalid event" do + let(:kubernetes_event) do + { + 'kind' => 'Event', + 'apiVersion' => 'v1', + 'reason' => 'Scheduled', + 'involvedObject' => { + 'kind' => 'Pod', + }, + 'metadata' => { + 'uid' => 'SomeRandomUid', + }, + 'lastTimestamp' => nil + } + end + + it "doesn't queue the event" do + expect(EmsEvent).not_to receive(:add_queue) + + subject.queue_event(event) + end end end @@ -256,12 +291,6 @@ def log_prefix )) end - let(:missing_timestamp_event) do - array_recursive_ostruct(:object => kubernetes_event.merge( - 'lastTimestamp' => nil - )) - end - it 'without matching node returns nil uid' do expect(test_class.new(ems).extract_event_data(bad_uid_event)[:uid]).to eq(nil) expect(test_class.new(ems).extract_event_data(missing_uid_event)[:uid]).to eq(nil) @@ -276,10 +305,6 @@ def log_prefix expect(test_class.new(ems).extract_event_data(bad_uid_event)).to eq(expected_data) expect(test_class.new(ems).extract_event_data(missing_uid_event)).to eq(expected_data) end - - it 'with missing timestamp' do - expect(test_class.new(ems).extract_event_data(missing_timestamp_event)).to be_nil - end end end end