[swss/orchagent] : Correct subnet check for the same prefix using function isAddressInSubnet()

[sudhanshukumar22]

[swss/orchagent]: Correct subnet check for the same prefix using function isAddressInSubnet()

What I did
Correct subnet check for the same prefix using function isAddressInSubnet()
Why I did it
The check for (prefixIt.getSubnet() == ip_prefix) is incorrect as it does not consider the mask for the 2nd parameter(ip_prefix). Ideally, for subnet check, both 1st parameter(prefixIt) and 2nd parameter(ip_prefix) must be appended by the mask. This is done in the function isAddressInSubnet(), where the mask present in the 1st parameter will be appended to both 1st and 2nd parameter, before comparison.

How I verified it
Tested using routes 30.1.0.0/24 and 30.1.0.1/32
Tested bgp docker restart
Details if related

Signed-off-by: sudhanshu.kumar@broadcom.com

[VladimirKuk]

I believe this is not the correct place to fix it since it is not the same check as intended.

Originally, prefixes were compared, not if both addresses are in the subnet.

So, if (i.e.) you look at orchagent/routeorch.cpp:878
/* fullmask subnet route is same as ip2me route /
else if (ip_prefix.isFullMask() && m_intfsOrch->isPrefixSubnet(ip_prefix, alsv[0]))
{
/ The prefix is full mask (/32 or /128) and it is an interface subnet route, so IntfOrch has already
* created an IP2ME route for it and we skip programming such route here as it already exists.
* However, to keep APPL_DB and APPL_STATE_DB consistent we have to publish it. */
publishRouteState(ctx);
it = consumer.m_toSync.erase(it);
}
it was intended to verify whether IP2ME route was configured, i.e. only /32 or /128, now even /24 prefix will suffice to pass this check.

[sudhanshukumar22]

Thanks for the review.
E.g. if we create Vlan100, 100.1.1.1/24 will be stored in m_intfsOrch. Now, when we create a host route Loopback1 with ip as 100.1.1.1/32. ip_prefix has the route 100.1.1.1/32.
In existing code, we have the following check.
if (prefixIt.getSubnet() == ip_prefix)
Here, prefixIt.getSubnet() will take ip address from the m_intfsOrch (with mask reset) as 100.1.1.0. wheras ip_prefix is 100.1.1.1. So, both will never match. But our goal is to discard the entry in such a scenario. (i.e. don't send to orchagent from programming, simply update the route state). So, the code is incorrect.

However, I think you are saying to write a new function for this part of code, (instead or modifying existing function which is also called at other places). Hence, I will write a new function, which will be called at this place alone.

[VladimirKuk]

Yes, I wasn't clear on this, but I did meant to use new function.
One note though.
I think the correct implementation should consider both prefix lengths and take the longest of them.