Empty NeighborSet condition always evaluates to true

[thoro]

I'm not sure if this is intended behaviour, but it definitly counts as confusing.

For example, this policy always matches, in case the nodes NeighborSet is empty.

StatementName export_accept_nodes:
Conditions:
PrefixSet: any default-nodes
NeighborSet: any nodes
Actions:
MED: 180
Nexthop: self
accept

There's actually the comment on the Evaluate function of the policy.go

"// If NeighborList's length is zero, return true."

But I'm wondering - why?

I would assume:

Empty NeighborSet matches never if match = any
Empty NeighborSet matches always if match = invert

[thoro]

Is actually also affects NextHopCondition, but is correctly handled for PrefixCondition

[fujita]

Sounds like v1.X works in the above way. Then we cannot change the behavior (even if it's confusing) because the configuration file format for v1.X is supposed to work in the same way with v2.X.

[thoro]

I would rather think it's a bug that no one noticed before. But it really prompts a weird behaviour. Also, config file format was already changed as far as I know with node sets allowing only cidr-notation.

I'm populating the nodes via GRPC, if the second service isn't started yet, all routes are published to everyone -> not what you would want.

Work around would be to add a fake host into the nodeSet -> could put that as an info to the documentation, so that the behaviour is documented.

[fujita]

But it really prompts a weird behaviour. Also, config file format was already changed as far as I know with node sets allowing only cidr-notation.

The format was changed but the behavior wasn't. The point is that we cannot change the behavior.

Why you use empty NeighborCondition? If you don't have anything about neighbors, you don't need to create a NeighborCondition.

[thoro]

Please read the last comment, I'm prepopulating my rules with sets. The rules are static, the sets are not, and they might be empty at some point, or filled. With that bug one time the rules are applied to all hosts, and one time they are applied only to the hosts in the set.

I understand your concern about changing behaviour, but don't you think/feel that this behavior is wrong?

[fujita]

I understand your concern about changing behaviour, but don't you think/feel that this behavior is wrong?

If I started the project today, I might do differently. But changing the behavior is not an option.

btw, IMHO, modifying the existing Conditions isn't a good idea. I prefer creating a new Conditions.

[thoro]

ok, then let's agree on adding a big fat warning to the documentation, that the behaviour is documented.

[fujita]

Absolutely. Please add the description of the behavior.