Notifications should be logs

[johananl]

Right now we have the concept of "notifications". We define a set of types and methods under pkg/i2gw/notifications/notifications.go, then we use these types in package-specific notify() functions which we call whenever we want to alert the user to special situations during ingress->GWAPI conversion. Finally we present the notifications to the user formatted as a table which gets printed to stderr.

I see the following problems with the current design:

1. We rely on global state.
2. We duplicate our logging infrastructure.

Let me elaborate on each problem.

Global State

In order to format notifications as a table, we must print all notifications for a provider together. To do that, we need to store all notifications as state while the binary is running and construct the table at the end.

Right now, this state is stored in a global variable:

ingress2gateway/pkg/i2gw/notifications/notifications.go

Lines 29 to 31 in 2802595

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

As long as i2gw is run as a binary there is no problem. However, when trying to convert resources concurrently (which we do, for example, in our e2e tests) the global variable becomes a source of problems. For example, right now there is no mechanism for clearing stored notifications, so the global state is an ever-growing map of notifications which e.g. makes notifications from provider A to be printed for provider B.

In addition, although we do use a mutex for the global map, under certain conditions we still have races (which I've encountered while working on the e2e tests).

Lastly, AFAICT the only reason we need to store a global map is so that we can format notifications as a table. After all, if instead we printed notifications ad hoc as they occurred we wouldn't need to store them at all. From the user's perspective there is no difference between stored state and ad-hoc printing: In both cases notifications show up on stderr while GWAPI resources are shown on stdout. However, a table output requires state storage under the hood while ad hoc printing doesn't.

Duplicate Logging Infrastructure

In addition to notifications, we also have logs. In fact, we often have the two together with the same data. Example:

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

Lines 203 to 214 in 2802595

	if serverTLS.GetHttpsRedirect() {
	notify(notifications.WarningNotification, fmt.Sprintf("ignoring field: %v", tlsFieldPath.Child("HttpsRedirect")), gw)
	klog.Infof("ignoring field: %v", tlsFieldPath.Child("HttpsRedirect"))
	}
	if serverTLS.GetServerCertificate() != "" {
	notify(notifications.WarningNotification, fmt.Sprintf("ignoring field: %v", tlsFieldPath.Child("ServerCertificate")), gw)
	klog.Infof("ignoring field: %v", tlsFieldPath.Child("ServerCertificate"))
	}
	if serverTLS.GetPrivateKey() != "" {
	notify(notifications.WarningNotification, fmt.Sprintf("ignoring field: %v", tlsFieldPath.Child("PrivateKey")), gw)
	klog.Infof("ignoring field: %v", tlsFieldPath.Child("PrivateKey"))
	}

If we look at the types we define for notifications, we notice that this package is essentially a logging package. We implement log levels:

ingress2gateway/pkg/i2gw/notifications/notifications.go

Lines 33 to 37 in 2802595

	const (
	InfoNotification MessageType = "INFO"
	WarningNotification MessageType = "WARNING"
	ErrorNotification MessageType = "ERROR"
	)

We have a basic logging function (which stores the event as state rather than print immediately):

ingress2gateway/pkg/i2gw/notifications/notifications.go

Lines 54 to 59 in 2802595

	// DispatchNotification is used to send a notification to the NotificationAggregator
	func (na *NotificationAggregator) DispatchNotification(notification Notification, ProviderName string) {
	na.mutex.Lock()
	na.Notifications[ProviderName] = append(na.Notifications[ProviderName], notification)
	na.mutex.Unlock()
	}

And we have a "handler" which reads state, applies formatting and emits events for printing:

ingress2gateway/pkg/i2gw/notifications/notifications.go

Lines 61 to 88 in 2802595

	// CreateNotificationTables takes all generated notifications and returns a map[string]string
	// that displays the notifications in a tabular format based on provider
	func (na *NotificationAggregator) CreateNotificationTables() map[string]string {
	notificationTablesMap := make(map[string]string)
	na.mutex.Lock()
	defer na.mutex.Unlock()
	for provider, msgs := range na.Notifications {
	providerTable := strings.Builder{}
	t := tablewriter.NewWriter(&providerTable)
	t.SetHeader([]string{"Message Type", "Notification", "Calling Object"})
	t.SetColWidth(200)
	t.SetRowLine(true)
	for _, n := range msgs {
	row := []string{string(n.Type), n.Message, convertObjectsToStr(n.CallingObjects)}
	t.Append(row)
	}
	providerTable.WriteString(fmt.Sprintf("Notifications from %v:\n", strings.ToUpper(provider)))
	t.Render()
	notificationTablesMap[provider] = providerTable.String()
	}
	return notificationTablesMap
	}

In effect, we created our own custom, stateful logging package, which we then use to instrument the entire code base.

Proposed Solution

I want propose that notifications are logs. If we accept that, we can:

• Leverage existing Go patterns and libraries for logging.
• Get rid of global state which IMO creates many problems for almost no benefit.
• Deduplicate our code and make it easier to test.
• Make the code less fragile.
• Allow consuming the i2gw code as a library with no extra cost (think controllers which migrate ingresses automatically for example).

Specifically, I want to suggest we leverage log/slog. We can have two handlers: A pretty handler which formats events for human consumption and a JSON handler for processing by machines.

I'll soon open a draft PR with a proposed solution we can evaluate.

Related Issues:

#325
#314
#276

[johananl]

/kind feature

[johananl]

/assign