Avoid global state in notification handling

[johananl]

While working on e2e tests for i2gw I've encountered the fact that we're using a global variable to store notifications. This isn't a problem as long as i2gw doesn't use concurrency internally, however in the e2e tests I did use concurrency to drastically reduce the time it takes to execute the test suite which made me stumble upon this.

Right now, we handle "notifications" as follows:

1. We create a global NotificationAggregator as a global variable:

ingress2gateway/pkg/i2gw/notifications/notifications.go

Line 52 in 2c81a62

var NotificationAggr NotificationAggregator

2. We initialize it here:

ingress2gateway/pkg/i2gw/notifications/notifications.go

Lines 29 to 31 in 2c81a62

	func init() {
	NotificationAggr = NotificationAggregator{Notifications: map[string][]Notification{}}
	}

3. We then "instrument" provider code with notify() functions which call the DispatchNotification() method on the NotificationAggregator. Example:

ingress2gateway/pkg/i2gw/providers/istio/notification.go

Lines 24 to 27 in 2c81a62

	func notify(mType notifications.MessageType, message string, callingObject ...client.Object) {
	newNotification := notifications.NewNotification(mType, message, callingObject...)
	notifications.NotificationAggr.DispatchNotification(newNotification, string(ProviderName))
	}

ingress2gateway/pkg/i2gw/providers/istio/converter.go

Line 157 in 2c81a62

notify(notifications.ErrorNotification, fmt.Sprintf("port is nil, path %v", serverFieldPath), gw)

4. Eventually we call the CreateNotificationTables() method which returns a map of per-provider notifications formatted as a text table:

ingress2gateway/pkg/i2gw/notifications/notifications.go

Line 63 in 2c81a62

func (na *NotificationAggregator) CreateNotificationTables() map[string]string {

5. The notification tables get printed to stderr:

ingress2gateway/cmd/print.go

Lines 104 to 106 in 2c81a62

	for _, table := range notificationTablesMap {
	fmt.Fprintln(os.Stderr, table)
	}

AFAICT there is no strict requirement to store notification state in a global variable. In fact, I'm not even sure we need to store notification state at all.

In the meantime I've found a solution for the concurrency issue affecting the tests, however we might want to reconsider the notifications design at some point.

Potential solutions I can think of:

1. Dependency injection. Treat notifications similar to logging and inject a "notifier" type as a dependency to all functions which need to produce notifications, thus moving the global state to a local "notifier" variable which can be initialized once in some main function.
2. No state. Log notifications ad hoc using slog without storing them anywhere. AFAICT, the only reason we store notifications is so that we can format them into a table, which IMO isn't a very strong argument in favor of the global var design.

Lastly, even if we end up not changing the design at all, we might want to at least have a way to clear the notification state from the global var safely. Otherwise notifications stay in the global state after being printed and accumulate forever. Again -- this isn't a problem when running the binary since it exits quickly but in test code (or if i2gw is ever used as a library) it certainly causes problems.