remove IPAddrPrefixDefault addrlen, use netip.Prefix and options to know/set the ip family

[nplanelcisco]

and handle multicast correctly

[fujita]

I had planned to remove addrlen when consolidating the separate structs for v4 and v6, but since you’ve started working on it, let's discuss the DecodeFromBytes and Serialize functions interface first.

With this change, since MarshallingOption becomes required to parse IPv6 addresses correctly, is the functional options pattern still appropriate? Better to use a normal argument?

I think thatFamily type is better than ipv6 bool type. Then we don't need to do the following stuff every time serializing a prefix.

	f := RF_IPv4_VPN
	isMulticast := l.Prefix.Addr().IsMulticast()
	isIPv6 := l.Prefix.Addr().Is6()
	switch {
	case !isMulticast && !isIPv6:
		f = RF_IPv4_VPN
	case isMulticast && !isIPv6:
		f = RF_IPv4_VPN_MC
	case !isMulticast && isIPv6:
		f = RF_IPv6_VPN
	case isMulticast && isIPv6:
		f = RF_IPv6_VPN_MC
	}

[fujita]

Furthermore, checking the AddPath map every time during serialization is not efficient.
When serializing an MP_REACH message containing multiple prefixes, it will only include a single family, so the map can be checked once at the beginning and the result reused for each prefix serialization.

[nplanelcisco]

I had planned to remove addrlen when consolidating the separate structs for v4 and v6, but since you’ve started working on it, let's discuss the DecodeFromBytes and Serialize functions interface first.

With this change, since MarshallingOption becomes required to parse IPv6 addresses correctly, is the functional options pattern still appropriate? Better to use a normal argument?

I think thatFamily type is better than ipv6 bool type. Then we don't need to do the following stuff every time serializing a prefix.

	f := RF_IPv4_VPN
	isMulticast := l.Prefix.Addr().IsMulticast()
	isIPv6 := l.Prefix.Addr().Is6()
	switch {
	case !isMulticast && !isIPv6:
		f = RF_IPv4_VPN
	case isMulticast && !isIPv6:
		f = RF_IPv4_VPN_MC
	case !isMulticast && isIPv6:
		f = RF_IPv6_VPN
	case isMulticast && isIPv6:
		f = RF_IPv6_VPN_MC
	}

Sure,

Marshaling option is only used in the DecodeFromBytes path and will be passed along the way from labeled vpn prefix to IPAddrPrefix, so keeping the isv6 in the MarshalingOption would make sense here. we can put the switch case in an helper GetFamily(prefix netip.Prefix) Family.
I don't think changing the DecodeFromBytes API to add family parameter could be justified here as it's like optional, so MarshallingOption make sense in this way

On the Serialize side l.Prefix already contain all the family needed info and avoid us to store twice the same info in the IPAddrPrefix (if we add Family)

[nplanelcisco]

Furthermore, checking the AddPath map every time during serialization is not efficient. When serializing an MP_REACH message containing multiple prefixes, it will only include a single family, so the map can be checked once at the beginning and the result reused for each prefix serialization.

Agreed, I tried to have minimal impact here on changes, and think it would be another PR as the marshaling option, should be accessed other way, like O(1) by merging MarshalingOptions in a single object, so no need to iterate on the options list afterwards

[nplanelcisco]

the problem I don't like with the isAddPath lookup (in DecodeFromBytes) is the prefix is not aware on what to expect
in case of multicast ipv6 (or v4) I would need to lookup twice unicast and multicast family, but this is based only to the fact that both unicast and multicast families entries are always in sync as the check is basically a logical or

[nplanelcisco]

overall I think we need to talk on our next sync, via sharing some code, the conversation/ideas would probably be more effective

[fujita]

overall I think we need to talk on our next sync, via sharing some code, the conversation/ideas would probably be more effective

Ok, I try to implement some ideas.

btw, the second commit to use Addr to see if multicast or not doesn't work for EOR path object. The fundamental problem is that EOR path should not have nlri. We should not use uninitialized nlri in any place but put uninitialized nlri to EOR path to get family when we convert the path to gRPC path. I'll fix Path.GetFamily() not to use nlri.AFI/SAFI(). Then we don't need AFI() and SAFI() in AddrPrefixInterface in the first place.