Extend ACL API to support changing match critera and logging for an existing ACL #139
Conversation
There was a problem hiding this comment.
Thanks for the change. Could you explain why these APIs are needed? Can we achieve the same by deleting the old and adding new ACLs? The user can do both operations in a single transaction, which for my understanding would result in the same outcome in terms of both feature and performance. Did I misunderstand it?
|
Thanks for your comment. These APIs are implementing functionality currently supported by the ovn-nbctl database commands and used by ovn-kubernetes. ACLSetMatchEntity() would get called to update the gress policy when an address set is added to or deleted from a pod. ACLSetLoggingEntity() would get called when the logging is enabled/disabled or severity is changed. I agree that the end state would be equivalent if the user deleted the existing ACL and then re-added the ACL with the new information. This would require more work in ovn-k because we don't currently have all the necessary info to recreate the ACL in the place where it's needed. I haven't tried to work it out yet, but I'm assuming it's possible. Intuitively, it would seem to have less of a performance impact to update a field in a row than it would to delete the row and re-create it. But, I can't comment on the real performance impact. I also wonder whether deleting an ACL and then re-adding it (even if it's in the same transaction) would impact the data plane more if the ACL was deleted and readded vs. updated. E.g., is it possible for any packets to slip through while it's being deleted and readded? I also wonder why these set APIs are different from other the other set APIs that we have. |
In fact to call the "set" API you already have to provide most information of an ACL just so that it can be identified. Maybe the only thing you don't provide is for ACLSetMatchEntity() you don't need the log/meter part. If you want to keep the original log settings when updating an ACL's match condition, and assume you don't know what log settings should it be when you try to update the ACL, then one approach is to Get the ACL (which happens locally) first so you know all the existing information and then do a single transaction with "delete + add".
Intuitively yes, but in fact, OVN has to regenerate the flows for the changed ACL, which costs the same as if it is deleted and added back. If in the future this can be optimized in OVN of course we can look into ways to optimize it. (I hope with the new API framework introduced by #136 and related work this would be easily supported in general)
This shouldn't be worried as long as the operations are performed in a single transaction, which in turn ensures OVS changes happens atomically.
For other existing "set" APIs, their interfaces are more straightforward - they usually provides "name" as an identifier. For ACL, since we are not using "name" (which is a field added in a late version of the schema), the API would look really scary. And more importantly, if we continue this way there can be more and more "set" APIs added - in theory each field of each table requires a "set" API, which is a huge burden for maintenance. For this specific case, since I don't see real benefit of providing the new APIs instead of using "delete + add", for the reason I mentioned regarding the OVN implementation behind, I am hesitating about adding these APIs. But I am still open to opinions. Let me know if you still strongly feel it is needed. |
|
Sorry for the delay, but I wanted to run this by someone else from the ovn-kubernetes team before I responded. So, I discussed it with @trozet this morning. He is in agreement with me that it's preferable to use sets in these cases (and probably others). I believe that the setting of any attribute should be fair game if there's a use case for it. Though I don't think we need to arbitrarily add sets for everything. In many cases, they are not needed. Objects get created with everything they need and are never modified until they are deleted. In other cases, it makes more sense to have an add/delete API such as with ports being added and deleted from port groups as pods get created and deleted. Some of the reasoning/concerns (not necessarily in priority order):
I made a few more comments below.
The missing fields are:
There are solutions, if necessary.
I agree that #136 will help, but it may be a while until it's fully usable.
Right, because we don't support names or UUIDs for ACLs, it's necessary to provide the entity, direction, match, and priority to uniquely identify the ACL, so it's not as clean as some of the others. On a related note, @vgozer has requested that "name" be added to the ACL APIs in #137. I'd be interested to hear your opinion on it. As mentioned above, I don't think we necessarily need a set API for every field, but if someone has a use case and offers an implementation, I'd support them. Thanks, |
|
@anfredette thanks for the points. I agree with you "set" APIs are useful and harmless.
Do you think it is reasonable to support "name" in ACL APIs first and then add "set" APIs only using the "name"? Or do we need two sets of "set" APIs: the ones using "name" and the one using "match + priority + direction"? |
I think it would be best to just have one set of APIs. I’d like to confirm with everyone how ACL name will work. For example, is it required? I’m out today. I’ll follow up on Monday with comments on #139. Thanks! |
|
I've pushed a patch updating the client API code per discussion on Issue #137. I am requesting feedback before fully implementing the changes. Thanks, Andre |
client.go
Outdated
| // Set name for ACL | ||
| ACLSetName(aclUUID string, aclName string) (*OvnCommand, error) | ||
| // Set match criteria for ACL | ||
| ACLSetMatchEntity(entityType EntityType, entityUUID, aclUUID, newMatch string) (*OvnCommand, error) |
There was a problem hiding this comment.
I think entity type and name/id are not needed for the "set" APIs if ACL uuid is used.
There was a problem hiding this comment.
We ensure that the 3-tuple <direction, match, priority> is unique for a given entity. If we allow the user to change one of these items, we need to make sure that the new 3-tuple does not conflict with another ACL on the same entity. The ACL record does not contain the entity on which it was created. So, if we don't pass in the entity, the code would need to potentially loop through every ACL on every entity to find the entity on which the ACL was created before it could confirm that the new 3-tuple was still unique for that entity.
There was a problem hiding this comment.
I would leave this check to the client instead of in go-ovn API. When using aclUUID it is natural to assume this task on client side. In fact even if entity is passed in, go-ovn API can't guarantee the uniqueness because the same ACL may be used by multiple entities, and this API can only pass in one of them.
So I'd make the "Set" API simple (and efficient). If needed, a Get (or ListBy) API (as discussed earlier) can be provided for querying if certain ACLs existed, before calling the "Set" API.
There was a problem hiding this comment.
I would leave this check to the client instead of in go-ovn API. When using aclUUID it is natural to assume this task on client side. In fact even if entity is passed in, go-ovn API can't guarantee the uniqueness because the same ACL may be used by multiple entities, and this API can only pass in one of them.
I don't think the go-ovn API currently allows a given ACL to be associated with more than one entity. I think it can be done via the ovn-nbctl database API, but I don't think we do that in ovn-kubernetes. So, it seems possible that at some point, it could happen that an ACL is associated with more than one entity.
So I'd make the "Set" API simple (and efficient). If needed, a Get (or ListBy) API (as discussed earlier) can be provided for querying if certain ACLs existed, before calling the "Set" API.
That's fine.
client.go
Outdated
| @@ -73,19 +73,21 @@ type Client interface { | |||
| LSLBList(ls string) ([]*LoadBalancer, error) | |||
|
|
|||
| // Add ACL to entity (PORT_GROUP or LOGICAL_SWITCH) | |||
| ACLAddEntity(entityType EntityType, entity, direct, match, action string, priority int, external_ids map[string]string, logflag bool, meter string, severity string) (*OvnCommand, error) | |||
| ACLAddEntity(entityType EntityType, entityUUID, aclName, direct, match, action string, priority int, external_ids map[string]string, logflag bool, meter, severity string) (*OvnCommand, error) | |||
There was a problem hiding this comment.
Why entityUUID is preferred than entityName? I know this has been a big discussion on slack. I replied there as well.
There was a problem hiding this comment.
I'll respond in slack so we can keep the discussion in one place and then comment back here when there is a decision.
anfredette
force-pushed
the
pg-acls-set
branch
4 times, most recently
from
759a0c9 to
b98b241
Compare
|
This PR is ready for review. |
- Support setting ACL name in ACLAddEntity() - Add ACL set support for ACL name, match and logging for an existing ACL - Use ACL UUID to identify ACL in ACLDelEntity() Addresses Issues eBay#137 and eBay#138 Signed-off-by: Andre Fredette <[email protected]>
|
I just added one more negative test case. Now it's really ready for review :-) |
|
Thanks @anfredette |
Addresses Issues #137 and #138
Signed-off-by: Andre Fredette [email protected]