creating ACL matching on address set name causes syntax error in lflow

[trozet]

When creating an ACL for an address group like:
create acl priority=1001 direction=to-lport "match="ip4.src == {$a11661077897478047653} && outport == @a15416135329930652905"" action=allow-related

ovn-nbctl accepts the command, but ovn-controller complains:
2020-02-13T20:00:22.083Z|00059|lflow|WARN|error parsing match "!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0 && (ip4.src == {$a11661077897478047653} && outport == @a15416135329930652905)": Syntax error at `$a11661077897478047653' expecting address set name.

More logs here:
https://gist.githubusercontent.com/trozet/a9fb6c52be80a43aec9cb8aa1f36129f/raw/ae239271655ce030d45da69d8086f0b5ddb35d49/log.txt

[trozet]

@numansiddique @dcbw

[trozet]

To rule out a race condition, I tried adding a check in ovn-k8s before we create the ACL to ensure the address set exists in SB before we create the ACL. Didn't seem to make a difference.

[numansiddique]

@trozet I think you'll not see this error when the port group or address set is not empty.

[russellb]

Is the error message different between an address set that does not exist and one that is empty? If it’s the same the error msg seems misleading. Maybe that’s something to improve.

[trozet]

thanks @numansiddique I see that the address is set a few moments after the ACL is created. Let me try fixing that in ovn-k8s, and see if the problem goes away.

Agree with @russellb that message doesn't make any sense. Also, if the addresss set is configured with an ip later, does the lflow then get installed correctly? I'll try to check that too.

[trozet]

After investigating it looks like this warning comes on the delete side of things. It happens when the address set is deleted before the port group with the acl that references it. I submitted above fix to fix the order in ovn-k8s and it works. Not sure if this is still a bug in OVN. Should OVN reject deleting and address set if another entity references it?

[putnopvut]

The OVS database has the concept of references to other database elements, and in some cases, it will refuse to delete row A if it's referenced by row B.

ACLs are a bit weird though. Rather than containing direct references to other database elements, the "match" portion of an ACL is treated as an arbitrary string. An ACL match may contain literals, logical switch ports, port groups, or address sets in any combination. Parsing of the ACL match is not performed until ovn-controller handles the logical flow generated by ovn-northd.

Because of the nature of ACL match, I don't think it's possible to restrict references at the ovsdb level. In other words, with the way database schema are defined, you couldn't instruct it that the ACL match may optionally contain references to certain types of database objects. The best you could do is to ensure that specialized ovn-nbctl operations might take some extra precautions to ensure that objects being deleted are not referenced by ACLs. However, this would be very expensive since it would require re-parsing the ACL matches each time an address set, logical switch port, port group, etc. is deleted. By "specialized" ovn-nbctl operations, I refer to operations like ovn-nbctl lsp-add as opposed to the general database command ovn-nbctl create logical_switch_port

As it stands, I think this error is more of a cosmetic nuisance than it is indicative of an actual problem. The error is rate-limited, so it's not going to spam the logs in most cases. And with the submitted patch, the error will go away entirely. I think the error needs to stay for the cases where people legitimately make the mistake of referencing a non-existent address set, though.

[trozet]

Thanks for the explanation @putnopvut I have not seen the error since and from your explanation it seems like we can just close this bug.