Add support of VPP events to Stream API

[ondrej-fabry]

Opening this issue to track progress of design and implementation of event support in new Stream API which is one of the remaining features required to make this new API complete.

@edwarnicke has already opened a PR #42 with initial implementation:

type Connection interface {
  // ...
  SubscribeNotification(ctx context.Context, event Message, bufferSize int) (chan Message, error)
}

Ed included some comments on design decisions.

However, before merging this as is, I would like to go over some concerns and possible improvements.
Here is summary of what should be taken into consideration when designing this.

1. Generated RPC services should provide simple way to watching events without required boilerplate.

• subsribing/unsubscring to event would need to match value of EnableDisable field
• starting multiple watchers for same event would need to keep reference count to work properly for all

3. Instead of using a go channel for events we should look into re-using the Stream itself as it already has RecvMsg() (Message, error) method for receiving messages.

• stream would be added to subscribers of specific event type to get the event message (because context is 0)
• this could easily handle multiple event types using RecvMsg() with type switch

5. Consider if it makes sense to allow users to consume multiple events on same go channel/Stream.

Proposal for generated code

Currently the generated method for enable events has nothing to do with the actual event message:

// Current implementation of generated RPC method that enables events
func (c *serviceClient) WantWireguardPeerEvents(ctx context.Context, in *WantWireguardPeerEvents) (*WantWireguardPeerEventsReply, error) {
	out := new(WantWireguardPeerEventsReply)
	err := c.conn.Invoke(ctx, in, out)
	if err != nil {
		return nil, err
	}
	return out, api.RetvalToVPPApiError(out.Retval)
}

Instead, RPC method that enables events could work in a similar way that dump requests do:

// Proposed implementation of generated RPC method that handles events
func (c *serviceClient) WantWireguardPeerEvents(ctx context.Context, in *WantWireguardPeerEvents) (RPCService_WireguardPeersEventsClient, error) {
	stream, err := c.conn.NewStream(ctx)
	if err != nil {
		return nil, err
	}
	x := &serviceClient_WireguardPeersEventsClient{stream}
    // this should check the value of `EnableDisable` field or ??
	if err := x.Stream.SendMsg(in); err != nil {
		return nil, err
	}
	msg, err := x.Stream.RecvMsg()
	if err != nil {
		return nil, err
	}
	out, ok := msg.(*WantWireguardPeerEventsReply)
	if !ok {
		return nil, fmt.Errorf("unexpected message: %T %v", msg, msg)
	}
	if err := api.RetvalToVPPApiError(out.Retval); err != nil {
		return nil, err
	}
	// until here it is pretty much same, just creates stream instead of calling Invoke
	// here we subscribe to specific event that is handled by this RPC methoid
	c.conn.SubscribeEvent(ctx, (*WireguardPeerEvent)(nil), stream)
    
	go func() {
		<-ctx.Done()
		c.conn.UnsubscribeEvent(ctx, (*WireguardPeerEvent)(nil), stream)
	}()
	return x, nil
}

type RPCService_WireguardPeersEventsClient interface {
	Recv() (*WireguardPeerEvent, error)
	// Perhaps Stop() or Unsubscribe() method ??
	api.Stream
}

type serviceClient_WireguardPeersEventsClient struct {
	api.Stream
}

// this would work in
func (c *serviceClient_WireguardPeersEventsClient) Recv() (*WireguardPeerEvent, error) {
	msg, err := c.Stream.RecvMsg()
	if err != nil {
		return nil, err
	}
	switch m := msg.(type) {
	case *WireguardPeerEvent:
		return m, nil
	default:
		return nil, fmt.Errorf("unexpected message: %T %v", m, m)
	}
}

however this example above does not check EnableDisable field and perhaps should not even accept request message WantWireguardPeerEvents from user, but handle it in way that enables events at beginning and disables them when ending.

[edwarnicke]

Generally I like trying to move in this direction. I have a few things in my own mind that I have yet to sort out wrt this approach:

1. This changes the existing Want* api in a breaking way. This is a double edged sword in that it signals folks to fix their usage, but we should be clear up front about the change.
2. EnableDisable across many users. This message sends the Want* Message every time. This could very easily allow independent users in code in the same process to stomp each other, with one Disabling what another wants enabled.
3. How do we handle legacy channel senders of Want* messages wrt 2 here?

I think at the end of the day we need to make sure that Connection is handling the 'Wanted' state of events correctly, only Disabling with VPP in the event there is no longer any consumption of the events going on in the process.

[ondrej-fabry]

1. This changes the existing Want* api in a breaking way.  This is a double edged sword in that it signals folks to _fix_ their usage, but we should be clear up front about the change.

This would only break users of generated RPC services which is kind of in experimental mode since it uses Stream API. However I agree that this will be mentioned in the first section of changelog under Breaking Changes.

2. EnableDisable across many users.  This message sends the Want* Message every time.  This could very easily allow independent users in code in the same process to stomp each other, with one Disabling what another wants enabled.

Yes, this is like the

3. How do we handle legacy channel senders of Want* messages wrt 2 here?

I do not think we should invest too much time into making Channel users happy. We can add some warning where suitable, but I the usage of generated RPC service together with usage of Channel is not intended to be mixed together. And if that is a case, user should take precautions.

I think at the end of the day we need to make sure that Connection is handling the 'Wanted' state of events correctly, only Disabling with VPP in the event there is no longer any consumption of the events going on in the process.

This will be the most difficult part to handle reliably in all situations.

HOW to enable/disable events

To handle the enabling and disabling of events in VPP we first need to either:

• detect which action (enable or disable) user wants to do
• or take control over the action and handle it for user.

Both of these scenarios have their shortcomings though. The VPP API, as usual, is not consistent at all here. See list of event messages and their RPC requests that control them:

    EVENTS MESSAGES:
        [
        	  "bfd_udp_session_event",
        	  "sw_interface_event",
        	  "ip6_ra_event",
        	  "ip_neighbor_event",
        	  "ip_neighbor_event_v2",
        	  "l2_macs_event",
        	  "l2_arp_term_event",
        	  "dhcp6_reply_event",
        	  "dhcp_compl_event",
        	  "dhcp6_pd_reply_event",
        	  "igmp_event",
        	  "nat_ha_resync_completed_event",
        	  "nat44_ei_ha_resync_completed_event",
        	  "vrrp_vr_event",
        	  "wireguard_peer_event"
        	]
    WANTS REQUESTS:
        {
        	  "dhcp_client_config": [
        	    "is_add (bool)",
        	    "client (vl_api_dhcp_client_t)"
        	  ],
        	  "nat44_ei_ha_resync": [
        	    "want_resync_event (u8)",
        	    "pid (u32)"
        	  ],
        	  "nat_ha_resync": [
        	    "want_resync_event (u8)",
        	    "pid (u32)"
        	  ],
        	  "want_bfd_events": [
        	    "enable_disable (bool)",
        	    "pid (u32)"
        	  ],
        	  "want_dhcp6_pd_reply_events": [
        	    "enable_disable (bool)",
        	    "pid (u32)"
        	  ],
        	  "want_dhcp6_reply_events": [
        	    "enable_disable (u8)",
        	    "pid (u32)"
        	  ],
        	  "want_igmp_events": [
        	    "enable (u32)",
        	    "pid (u32)"
        	  ],
        	  "want_interface_events": [
        	    "enable_disable (u32)",
        	    "pid (u32)"
        	  ],
        	  "want_ip6_ra_events": [
        	    "enable (bool)",
        	    "pid (u32)"
        	  ],
        	  "want_ip_neighbor_events": [
        	    "enable (bool)",
        	    "pid (u32)",
        	    "ip (vl_api_address_t)",
        	    "sw_if_index (vl_api_interface_index_t)"
        	  ],
        	  "want_ip_neighbor_events_v2": [
        	    "enable (bool)",
        	    "pid (u32)",
        	    "ip (vl_api_address_t)",
        	    "sw_if_index (vl_api_interface_index_t)"
        	  ],
        	  "want_l2_arp_term_events": [
        	    "enable (bool)",
        	    "pid (u32)"
        	  ],
        	  "want_l2_macs_events": [
        	    "learn_limit (u32)",
        	    "scan_delay (u8)",
        	    "max_macs_in_event (u8)",
        	    "enable_disable (bool)",
        	    "pid (u32)"
        	  ],
        	  "want_vrrp_vr_events": [
        	    "enable_disable (bool)",
        	    "pid (u32)"
        	  ],
        	  "want_wireguard_peer_events": [
        	    "sw_if_index (vl_api_interface_index_t)",
        	    "peer_index (u32)",
        	    "enable_disable (u32)",
        	    "pid (u32)"
        	  ]

There are several different names/types for the boolean field.

• enable (u32/bool)
• enable_disable (u8/u32/bool)
• want_XXX_event (u8)
• and special cases where this field is nested - dhcp_client_config has it under client.want_dhcp_event (dhcp_client type)

On the last community meeting we agreed that I will open discussion on vpp-dev about this so the VPP API is cleaned up and ideally some rules are specified for this including step in the CI pipeline to encorce this for future development. Until then, I guess we could handle the cases mentioned above by hard-coding or checking for possible variations.

WHEN to enable/disable events

The events in the VPP can be enabled/disabled globally and they are received with context=0 so the destination is the connection (or process) itself. This means to allow multiple watchers for single event we will need to keep reference counter for watchers where we send WantXXX request with Enable=true when counter gets from 0->1 and send WantXXX request with Enable=false when it reaches 0 again.

The problems I see with this are:

1. keeping reference counter in the connection itself and handling it for all WantXXX requests sent including Channel/Stream and generated RPC client (Stream too) it going a bit against the reasons behind the Stream API where the goal is too avoid handling VPP API semantics - also it might confuse users if their request is not really sent when they clearly send it because of some reference counter not reaching 0 yet.
2. the special cases of the VPP events are possibly more tricky since the requests seem to have filter/target for which they want to enable events - e.g. DHCP events are enabled per client (have not tested this though, just assuming because the field is part of client config) and others like IP neighbor events and Wireguard peer events.. this would get quickly very complex to handle if we needed to keep reference counter for everything...