Fix issue #124: acl add/delete code isn't handling external_ids correctly

[anfredette]

See https://github.com/eBay/go-ovn/issues/124 for details

Signed-off-by: Andre Fredette afredette@redhat.com

[softwarejl]

LGTM

[hzhou8]

BTW, thanks for putting the link to the reported issue in the commit message, but I think it would be better to still describe the problem briefly and the solution in the commit message. Thank you.

[anfredette]

@hzhou8, Looking back at the discussion, I should have clarified this earlier. If I'm understanding you correctly, what you are describing is what I was suggesting in my initial message, but it sounded to me like you and @vtolstov were disagreeing with me and wanted to keep external-ids as a match condition but fix it. As I mentioned earlier, the ovn-nbctl commands are inconsistent depending on whether the database or acl-add/del commands are used, so doing it "the ovn-nbctl way" is ambiguous.

Your last comment is clear: "ignore external_ids while checking for duplicates in ACLAdd or matching in ACLDel".

I think this is the right way to do it.

Do you agree that we can also remove external_ids from the new ACLDelEntity() since it's not being used for anything?

Thanks, Andre

[hzhou8]

ok, sorry for the miscommunication.

As I mentioned earlier, the ovn-nbctl commands are inconsistent depending on whether the database or acl-add/del commands are used, so doing it "the ovn-nbctl way" is ambiguous.

I was focusing on the acl-add/del commands instead of raw database commands, because database commands is generic to all tables, only restricted by schema. I meant to have consistent behavior with ovn-nbctl acl-add/del commands. (I think it is unfair to say that the database commands and the acl-add/del are inconsistent because they belong to different command groups and serve different purposes.)

Do you agree that we can also remove external_ids from the new ACLDelEntity() since it's not being used for anything? Agree.

[vtolstov]

i'm prefer to have ability to remove acl via external_ids because i have uuid of rule in external_ids that joins my cms database rule with ovn rule.

[anfredette]

I just looked at the delete code again more closely. Delete does not require all the fields to be provided. As it is, aclDelImp will delete the first ACL it finds that matches the criteria provided. I assume it works for your case because the UUID is unique among ACLs on a given logical switch. However, if the user provides a field that is not unique for the entity, a random ACL that matches the criteria provided gets deleted. Is that what we want?

I also think I may now understand why you implemented the matching for external_ids as a subset with oMapContians(). I.e., the current code will match an ACL if the provided external_ids are a subset of the external_ids on an ACL. This does what you want on a delete, but caused problems during the add when we were matching on external_ids to find duplicates. Again, is this what we want?

I can make it work either way.

On Wed, Jan 13, 2021 at 2:28 PM Vasiliy Tolstov notifications@github.com wrote:

i'm prefer to have ability to remove acl via external_ids because i have uuid of rule in external_ids that joins my cms database rule with ovn rule.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eBay/go-ovn/pull/125#issuecomment-759669863, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHY7X3E2KGLGEKBITRPNWTSZXX4VANCNFSM4VZHTPNA .

[vtolstov]

I don't have deep knowledge about external_ids in ovn, but as i understand it used from cms that store unique id of item to be able to select/delete/update needed stuff. external_ids is not for the end-user. @hzhou8 you have more knowledge about ovn and maybe openstack neutron stuff (as i know main user of external_ids is neutron from openstack)

[hzhou8]

However, if the user provides a field that is not unique for the entity, a random ACL that matches the criteria provided gets deleted. Is that what we want?

I don't think a random ACL deletion is what we want. It should either delete all matched ACLs, or, delete only one matched ACL and return error if there is more than one found. With this patch, it ensures that ACLs added by the ACLAdd API will be unique and thus ACLDel would only have one ACL found. (of course it doesn't prevent duplicated ACLs added through other tools directly to DB)

I also think I may now understand why you implemented the matching for external_ids as a subset with oMapContians(). I.e., the current code will match an ACL if the provided external_ids are a subset of the external_ids on an ACL. This does what you want on a delete, but caused problems during the add when we were matching on external_ids to find duplicates. Again, is this what we want?

One thing for sure is that we should never rely on external-ids for finding duplicated ACLs in ACLAdd, which is fixed by this PR. However, for deletion, if we still want to handle the corner cases when duplicated ACLs may exist (added through other tools) and want to rely on ACLDel() API to delete ACLs with external_ids checked, e.g. delete a matched ACL if external_ids:owner=\<owner-id>, then the user may still want to have the external_ids parameter, and want the oMapContains() behavior. (This is similar to ovn-nbctl find ... external_ids:x=y. To find the ACL you don't need to supply all k-v pairs).

So, in my opinion I'd prefer keeping the external_ids parameter behavior in ACLDel(), because at least it is not harmful: users can always pass nil if they don't need to match on it, but if they pass a none nil value, they are definitely expecting something there. However, I don't have a very strong opinion here. I am ok either way, deprecate it (unused), or keep it. Just make sure these use cases are thought through.

[anfredette]

I don't think a random ACL deletion is what we want. It should either delete all matched ACLs, or, delete only one matched ACL and return error if there is more than one found. With this patch, it ensures that ACLs added by the ACLAdd API will be unique and thus ACLDel would only have one ACL found. (of course it doesn't prevent duplicated ACLs added through other tools directly to DB)

I'm thinking that the best approach is to delete all ACLs that match the provided criteria. If no ACLs are found, then we return an error. Are you both okay with that?

[hzhou8]

I don't think a random ACL deletion is what we want. It should either delete all matched ACLs, or, delete only one matched ACL and return error if there is more than one found. With this patch, it ensures that ACLs added by the ACLAdd API will be unique and thus ACLDel would only have one ACL found. (of course it doesn't prevent duplicated ACLs added through other tools directly to DB)

I'm thinking that the best approach is to delete all ACLs that match the provided criteria. If no ACLs are found, then we return an error. Are you both okay with that?

Yes, but let's address it in a separate PR.

[anfredette]

I don't think a random ACL deletion is what we want. It should either delete all matched ACLs, or, delete only one matched ACL and return error if there is more than one found. With this patch, it ensures that ACLs added by the ACLAdd API will be unique and thus ACLDel would only have one ACL found. (of course it doesn't prevent duplicated ACLs added through other tools directly to DB)

I'm thinking that the best approach is to delete all ACLs that match the provided criteria. If no ACLs are found, then we return an error. Are you both okay with that?

Yes, but let's address it in a separate PR.

Is it okay with you if I revert the ACLDel behavior to how it currently works in this PR?

[anfredette]

I don't think a random ACL deletion is what we want. It should either delete all matched ACLs, or, delete only one matched ACL and return error if there is more than one found. With this patch, it ensures that ACLs added by the ACLAdd API will be unique and thus ACLDel would only have one ACL found. (of course it doesn't prevent duplicated ACLs added through other tools directly to DB)

I'm thinking that the best approach is to delete all ACLs that match the provided criteria. If no ACLs are found, then we return an error. Are you both okay with that?

Yes, but let's address it in a separate PR.

Is it okay with you if I revert the ACLDel behavior to how it currently works in this PR?

I think I could also pretty easily return an error if more than one ACL is matched.

[hzhou8]

Is it okay with you if I revert the ACLDel behavior to how it currently works in this PR?

Yes, I am ok with that.

I think I could also pretty easily return an error if more than one ACL is matched.

I'd prefer a separate PR. (so that the current one can be merged sooner, independently)

[anfredette]

The changes have been pushed.

[hzhou8]

LGTM

[hzhou8]

@vtolstov @softwarejl would you take a look again?

[vtolstov]

LGTM, @hzhou8 i think that it's ok for now