netlink: do not ignore vip as system owner

[lsang6WIND]

When a VRRP instance is waiting for an IP address on the interface to start, if the vip is added on the interface, keepalived ignores it and remains in FAULT state, even if the VRRP instance is the system owner (priority 255 and vip = system IP). However, when adding an IP address different than the vip, the instance can change the state.

This makes nonsense for an instance is configured as the system owner transit to FAULT state since the system has not yet the vip. When the vip is added to the system, keepalived ignores it and this instance remains in FAULT state, whereas an IP address different than the vip can change the state of this instance.

Thus, do not ignore if a vip is added to the system when a VRRP instance is configured with 255 priority.

Signed-off-by: Loïc Sang loic.sang@6wind.com

[lsang6WIND]

Hello,

As discussed in:
#2219

It is possible to integrate this commit to make VRRP work as a simple router?

[pqarmitage]

I notice that the first two versions of this patch didn't work (corrected in this latest version), and this gives me some concern when patches are being submitted with pull requests that evidently haven't been fully tested. I do appreciate that the problem has now been fixed, and I appreciate that.

One comment I would make on the style of the patch is:

	if (vrrp->base_priority == VRRP_PRIO_OWNER)
		continue;
	else
		return true;

This should always be written as:

	if (vrrp->base_priority == VRRP_PRIO_OWNER)
		continue;
	return true;

since the else is not only unnecessary but also gives the impression that this branch of the code can continue after the true block of the if statement has been executed.

I agree that your patch is correct and will need to be merged, but I have a significant concern that relates to the misconfiguration issues I have previously mentioned.

Suppose that, rather than a single VIP address, the configuration has the following:

  virtual_ipaddress {
    192.168.1.1/24
    192.168.1.2/24
    192.168.1.3/24
  }

If the VRRP instance is configured to be the address owner, then if any of those addresses are not configured on the system, the VRRP instance should not go to master state. The current code, although due to the functionality not being fully correct, will ensure that the VRRP instance will not go to master if there is no other IP address configured on the interface. This provides some protection, albeit somewhat imperfect, of the VIPs not being configured on the system. If just one of the VIPs is added, then with your patch the VRRP instance will be able to transition to master despite the other VIPs not being configured.

It may well be that there are users who rely on the current functionality to stop the address owner becoming master if the VIPs are not configured (if there are no other addresses that will be configured on the interface) and we need to make sure we don't break this for them.

The question is, "what is the right thing to do if VIPs of an address-owning VRRP instance are missing?" I think there are three possible answers:
i) Remain in fault state until all the VIPs have been added
ii) Add the VIPs, on the basis that the VRRP configuration says that they should exist
iii) If any of the VIPs exists, add all the remaining VIPs
iv) Go to MASTER state if there is an IP address on the VRRP instance's interface (this isn't really a proper solution but it is how it works currently).

iii) has the benefit that consistency is maintained; if any of the VIPs exist, they all will and we will be master. ii) similarly ensures consistency but has less flexibility. i) causes a problem if some of the VIPs exist but not all, since this instance will be in FAULT state, and another system will be in master state, so there will be some duplicate IP addresses configure. Similarly iv) can cause a problem of duplicate IP addresses. However, we need to maintain iv) since it is how keepalived currently operates.

I think we will need to implement all solutions, with iv) being the default since that is how keepalived works now. My view is that once this has been implemented, then it is safe to merge your patch.

If you would like to implement the above options, then I would be very grateful if you would implement it and submit a pull request. Otherwise I will do it when I have time, but that won't be for a few weeks.

[lsang6WIND]

Hello,

Thanks for your answer, I was not sure that you will respond to me as my patch is so minimalist.

"I notice that the first two versions of this patch didn't work (corrected in this latest version), and this gives me some concern when patches are being submitted with pull requests that evidently haven't been fully tested. I do appreciate that the problem has now been fixed, and I appreciate that."

Sorry for the multiple pull requests, as I am working on a maintenance branch, I manually copied the patch to the master with some errors.

"I think we will need to implement all solutions,..."

I will do my best for that but, how to implement all solutions? by adding an option when starting keepalived?

[pqarmitage]

Yes, there will need to be configuration options. Generally I would add an option for the VRRP instance, and also a global_defs option that changes the default setting.

[lsang6WIND]

hello,

"Yes, there will need to be configuration options. Generally I would add an option for the VRRP instance, and also a global_defs option that changes the default setting."

OK, I get it , we can configure the behavior by VRRP instance or by global_defs (the instance config can override global_defs).

• IV) is the current implementation of keepalived.
• III) is close to my current patch, we have to allow the transition only for any VIPs added.
• I) in terms of code, it is an enhancement of III).
• II) what do you mean by "on the basis that the VRRP configuration says that they should exist" ?

[lsang6WIND]

Hello,

I have implemented the I) and III) options.
It works for IPv4, I have tested. However, for IPv6, I have a problem when I add or delete addresses on the physical interface tracked by vrrp.

I will continue on II) and will fix the problem for IPv6.