icinga2: Reset Event#ID before resending ack set event

[yhabteab]

Event#Sync() synchronises an event row with the database only if its ID is set to no other value than 0. Therefore, we must reset the event ID for the acknowledgement set event generated in the catch-up phase before resending it.

[oxzi]

I tried to retrace the steps how this can happened. Thanks for giving me some pointers, @yhabteab, as otherwise I have totally missed it.

• The journey of our new fakeEv starts in checkMissedChanges, from where it will be send to catchupEventCh by reference
  

  icinga-notifications/internal/icinga2/client_api.go

  Line 305 in 15392c9

  case catchupEventCh <- &catchupEventMsg{eventMsg: &eventMsg{fakeEv, attrs.LastStateChange.Time()}}:

• It ends up in the worker's catchup case..
  

  icinga-notifications/internal/icinga2/client.go

  Line 499 in 683ea69

  case catchupMsg, ok := <-catchupEventCh:

• ..and will be eventually passed to the CallbackFn
  

  icinga-notifications/internal/icinga2/client.go

  Line 538 in 683ea69

  client.CallbackFn(ev)

• In there, our adventurous event will be passed on to incident.ProcessEvent
  

  icinga-notifications/internal/icinga2/launcher.go

  Line 132 in 7b6ea92

  err := incident.ProcessEvent(subCtx, launcher.Db, launcher.Logs, launcher.RuntimeConfig, ev)

• As it is a new incident, it gets .Synced
  

  icinga-notifications/internal/incident/incidents.go

  Line 248 in 7b6ea92

  err = utils.RunInTx(ctx, db, func(tx *sqlx.Tx) error { return ev.Sync(ctx, tx, db, obj.ID) })

• And ends up in the database and gets its .ID set
  

  icinga-notifications/internal/event/event.go

  Lines 164 to 168 in 7b6ea92

  	eventRow := NewEventRow(e, objectId)
  	eventID, err := utils.InsertAndFetchId(ctx, tx, utils.BuildInsertStmtWithout(db, eventRow, "id"), eventRow)
  	if err == nil {
  	e.ID = eventID
  	}

• Back in checkMissedChanges, after sending the host service event, the fakeEv might be sent again
  

  icinga-notifications/internal/icinga2/client_api.go

  Lines 322 to 329 in 15392c9

  	case catchupEventCh <- &catchupEventMsg{eventMsg: &eventMsg{ev, attrs.LastStateChange.Time()}}:
  	stateChangeEvents++
  	if fakeEv.Type == event.TypeAcknowledgementSet {
  	fakeEv.ID = 0 // We don't want to reference a previously sent event, so reset its ID!
  	select {
  	// Retry the AckSet event so that the author of the ack is set as the incident
  	// manager if there was no existing incident before the above state change event.
  	case catchupEventCh <- &catchupEventMsg{eventMsg: &eventMsg{fakeEv, attrs.LastStateChange.Time()}}:

• Now, I am going to skip some steps as shown above until the event ends up in Sync
  

  icinga-notifications/internal/event/event.go

  Lines 160 to 162 in 7b6ea92

  	if e.ID != 0 {
  	return nil
  	}

• And here it goes silently unnoticed as it got an ID in the meantime. Yikes.

I am going to take another look tomorrow.

[yhabteab]

• As it is a new incident, it gets .Synced
  

  icinga-notifications/internal/incident/incidents.go

  Line 248 in 7b6ea92

  err = utils.RunInTx(ctx, db, func(tx *sqlx.Tx) error { return ev.Sync(ctx, tx, db, obj.ID) })

If it is synced here, there should be no problems. The real problem, however, is that if there is an existing incident, that fake event will make its way to the fellowing scopes multiple times.

icinga-notifications/internal/incident/incident.go

Line 147 in 5986128

if err = ev.Sync(ctx, tx, i.db, i.Object.ID); err != nil {

icinga-notifications/internal/incident/incident.go

Lines 163 to 164 in 5986128

	if err = i.AddEvent(ctx, tx, ev); err != nil {
	i.logger.Errorw("Can't insert incident event to the database", zap.Error(err))

When the incident finally notices that this ack set event is superfluous here ...

icinga-notifications/internal/incident/incident.go

Lines 199 to 200 in 5986128

	if err := i.processAcknowledgementEvent(ctx, tx, ev); err != nil {
	if errors.Is(err, errSuperfluousAckEvent) {

It will simply return without committing the transaction, leaving the now dangling ev.ID set above.

[julianbrost]

duplicate the whole event before sending it to any channel

By that, I meant before fakeEv is sent to any channel for the first time, i.e. before line 305. That way, the pointer wasn't yet passed anywhere else and duplicating it is safe for sure. So that would mean creating a second variable next to fakeEv and duplicating the event right after client.buildAcknowledgementEvent().