ACL: missing 'name' field support

[vgozer]

nameless ACLs matching evens in logs look like this: 2021-03-02T19:29:05.415Z|06852|acl_log(ovn_pinctrl0)|INFO|name="<unnamed>", verdict=drop, severity=debug: ... which is not good ... trivial to add 'name' field support, but this will change the API signature for ACLAddEntity (extra parameter). Can keep the signature and pass the name in external_ids, but it's ugly... adding a new one (ACLAddEntityNamed) seems like an overkill. Opinions?

[anfredette]

I don't have a problem extending the ACLAddEntity API. This API is fairly new, so it's probably not widely used. I currently have a draft PR open on ovn-kubernetes that uses it, but I need to add some additional APIs to go-ovn before it can be merged anyway.

ovn-kubernets uses ACL name a lot, so I've been thinking about how to handle it. We could change ovn-kubernetes to not use ACL name, stash the ACL name in an external-id, or add the support. If we do the external-id option, we'd run the risk of using duplicate names.

It's not entirely trivial, but wouldn't be very hard. We'd need to extend the check for existing ACLs to also consider name, and it would be an "OR" check. So, if an ACL exists with the same (direction, match and priority) OR (same name), we'd need to reject the ADD. I think we'd also want to avoid looping through the existing ACLs twice, so I think we might want to replace the getACLUUIDByRow() call with a function that does the "OR" check.

[hzhou8]

Adding columns in OVSDB schema is expected over time, but creates problems for go-ovn lib. If we want to keep the backward compatibility, we may have to add new APIs, probably with suffix V2/V3/..., which is not ideal.

I hope this can be solved eventually by API auto-generation. One approach is to generate APIs for different versions of schema to different packages, and let client decide which version to use. I am not sure if there is a better way, say, only generate the APIs for the required version of schema. The C IDL is generated this way at compile time of the client code, but for GO dependency mechanism, not sure if it is possible. cc @amorenoz @vtolstov (we have been discussing this for the native bindings API effort)

For this case for now, I think updating ACLAddEntity signature directly may be ok as mentioned by @anfredette , since it is quite new, if no other objections.

[vgozer]

@hzhou8 @anfredette ACLAddEntity(..., name, ...) would work for me, in the interim. can it be coupled with ACLDelEntity(..., name, ...)?

[anfredette]

I've looked into this a bit this morning. I was originally thinking the name field could be used effectively as an index as is done for some other objects. However, if I'm reading the OVN NB schema correctly, name is an optional field for ACL, and there is no requirement that it be unique either within the ACL table or within a particular Logical Switch or Port Group. @hzhou8, @vgozer, do you agree?

[hzhou8]

I've looked into this a bit this morning. I was originally thinking the name field could be used effectively as an index as is done for some other objects. However, if I'm reading the OVN NB schema correctly, name is an optional field for ACL,

@anfredette Yes, in fact the "name" field of some other tables, such as LogicalSwitch, are also non-unique. I agree this is tricky, like what we did for LogicalSwitch. If duplicated names are added (not possible from go-ovn API, but possible by other means), the behavior can be undermined. We can of course error out in that situation in go-ovn.

Alternatively, we could create APIs that operates with UUID, like: ACLSetName(uuid, name) ACLSetMatch(uuid, match) ACLSetLog(uuid, log, severity) ...

Before calling these APIs, use ACLList to get the ACL object which contains UUID. What do you think?

and there is no requirement that it be unique either within the ACL table or within a particular Logical Switch or Port Group. @hzhou8, @vgozer, do you agree?

In Logical Switch and Port Group the ACL is referenced by uuid.

[vgozer]

@anfredette @hzhou8 name field being non-unique may be there for a good reason - ACLs can be grouped for logging purposes, e.g., we want to log events after some policy name for related rules vs after the individual rule. Artificially requiring 'name' to be a unique key in lower-level APIs doesn't seem right. @hzhou8 suggestion looks right to me:

ACLSetName(uuid, name) ACLSetMatch(uuid, match) ACLSetLog(uuid, log, severity)

for SetLog(), I'd use uuid[] list instead of scalar uuid. Also, if the intention is to use 'name' only for logging, ACLSetName() may be redundant. ACLSetLog() can set 'name' column for the given uuids, right? Also, ACLList() today returns a list of ACL objects pointers for the given entity, so, the client has to iterate on this list to get uuids for the fields of interest. How about another method, ACLListBy(entityType, entityName, searchTerm) that returns a list of UUIDs? searchTerm is a list of field names of interest (or a map of field names as keys/boolean value to include/exclude from the results set). Could be an overkill for cases where the number of ACLs are hundreds vs thousands, though.

[anfredette]

@anfredette @hzhou8 name field being non-unique may be there for a good reason - ACLs can be grouped for logging purposes, e.g., we want to log events after some policy name for related rules vs after the individual rule. Artificially requiring 'name' to be a unique key in lower-level APIs doesn't seem right.

I agree. I'm not comfortable arbitrarily enforcing uniqueness on the ACL name field.

@hzhou8 suggestion looks right to me:

ACLSetName(uuid, name) ACLSetMatch(uuid, match) ACLSetLog(uuid, log, severity)

I like this form, though I'd be inclined to include name in the ACLAddEntity since we don't have a need to change it after the ACL is created. That said, I'd be happy to include a set command if others need it.

Also, ExectuteR() can now be used to get the uuid when the ACL is created, so there's no need to do another lookup.

for SetLog(), I'd use uuid[] list instead of scalar uuid.

Also, if the intention is to use 'name' only for logging, ACLSetName() may be redundant. ACLSetLog() can set 'name' column for the given uuids, right?

I don't think logging is the only reason for the ACL name, so I wouldn't tie them together.

Also, ACLList() today returns a list of ACL objects pointers for the given entity, so, the client has to iterate on this list to get uuids for the fields of interest. How about another method, ACLListBy(entityType, entityName, searchTerm) that returns a list of UUIDs? searchTerm is a list of field names of interest (or a map of field names as keys/boolean value to include/exclude from the results set). Could be an overkill for cases where the number of ACLs are hundreds vs thousands, though.

We have a use case to get a list of ACLs by name, so I was going to propose that.

[hzhou8]

Hi @vgozer ,

for SetLog(), I'd use uuid[] list instead of scalar uuid. Also, if the intention is to use 'name' only for logging, ACLSetName() may be redundant. ACLSetLog() can set 'name' column for the given uuids, right?

uuid[] list would look strange in the API, since all our "set" APIs operate on a single object. If this is for efficiency when updating multiple objects in bulk, it can be achieved by calling the API multiple times with multiple Command object returned and then use a single transaction to execute all the Command objects.

For "name" column I agree with @anfredette that it is more flexible to have it supported in a separate API. Although "name" was added for ACL logging, but the users may want to use it in a different way, decoupling from logging.

Also, ACLList() today returns a list of ACL objects pointers for the given entity, so, the client has to iterate on this list to get uuids for the fields of interest. How about another method, ACLListBy(entityType, entityName, searchTerm) that returns a list of UUIDs? searchTerm is a list of field names of interest (or a map of field names as keys/boolean value to include/exclude from the results set). Could be an overkill for cases where the number of ACLs are hundreds vs thousands, though.

Maybe I didn't understand your proposal completely. It seems this new API you are proposing is different from the old one in two ways:

1) It only returns UUIDs instead of all fields. But how would the user get the fields then? Maybe we should also provide a "get" API that directly returns an ACL object by uuid? (same may be useful for other tables that doesn't have "name" as the unique identifier)

2) This is where I was confused. For seachTerm I guess you meant filter conditions so that only interested ACLs are returned. But "a map of field names as keys/boolean value to include/exclude from the results set" seems to suggest a way to specify "columns" included in the result set. However, in 1) it mentioned only returns UUIDs in the result. Could you clarify more on this?

If it is for filter condition, which sounds reasonable to me, I think it may be a map of map[string]interface{}. The key is field name, and the value is the filter value. A missing field means "any" for that field. Would this work?

[hzhou8]

@hzhou8 suggestion looks right to me:

ACLSetName(uuid, name) ACLSetMatch(uuid, match) ACLSetLog(uuid, log, severity)

I like this form, though I'd be inclined to include name in the ACLAddEntity since we don't have a need to change it after the ACL is created. That said, I'd be happy to include a set command if others need it.

@anfredette , I am ok with adding "name" to ACLAddEntity, too, as long as there is no objection from existing users of ACLAddEntity. (This makes sense in terms of efficiency because currently there is no way in go-ovn to do "add" and "set" in the same transaction)

Also, ExectuteR() can now be used to get the uuid when the ACL is created, so there's no need to do another lookup.

Yes, the uuid returned by ExecuteR() can be used by the "set" APIs directly, but the users may wants to update an ACL later, as your PR https://github.com/eBay/go-ovn/pull/139 suggested. In that case, "list" API (or better, a new API that filters the ACL(s) really needed) may still be needed to find the ACL object to update.

I don't think logging is the only reason for the ACL name, so I wouldn't tie them together.

Agree.

[vgozer]

@hzhou8

Maybe I didn't understand your proposal completely. It seems this new API you are proposing is different from the old one in two ways:

correction/clarification: yes, new method, Get() or Search() to not be confused with List() and searchTerm should've been a map[string]string (or interface{} as value), i.e., column:value - e.g., 'name' : 'PolicyBlockBadGuys'. This method would iterate on all cached acl rows and match fields with the provided map, returning uuids slice. Then we use the returned slice immediately for SetLog, e.g, SetLog(uuid[], logName, 'debug'). Agree, the benefits of this approach vs ACLList()/SetLog(scalar_uuid,..) is not apparent for reasonable (low hundreds?) number of ACLs per entity. For much larger numbers, we might have other problems masking this one :) so, let's put this idea to rest ...

[anfredette]

@hzhou8, @vgozer, I updated the ACL API based on this discussion. Well, I actually took it a little farther and used UUIDs for both entity and ACL. I've reviewed all of the ovn-kubernetes ACL code and believe this will meet all of the current requirements.

This code doesn't work yet. I'm holding off on implementing the supporting code until I get the thumbs up. Please let me know what you think. Thanks, Andre