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

anfredette · 2021-01-07T16:52:41Z

See #124 for details

Signed-off-by: Andre Fredette [email protected]

hzhou8

LGTM

softwarejl · 2021-01-07T22:10:17Z

LGTM

hzhou8

Sorry that I just realized that the change was opposite to what we have discussion on slack.

@anfredette, you mentioned: "Does it make sense that we include external_ids in the match criteria? I don't beleive that the external_ids have any impact on the ACL functionality, so technically, ACLs with different sets of external_ids work the same.
For comparison, the ovn-nbctl database command allows duplicate acls, while the ovn-nbctl acl-add command prevents duplicates, though it doesn't support external-id's."

My response was: "IMHO, since the APIs are mostly following ovn-nbctl format, it would be good to fix the inconsistency behavior of regarding external_ids match and ACL deletion in the way how ovn-nbctl works."

In other words, I suggested ignoring the external_ids while checking for duplicates in ACLAdd or matching in ACLDel. I thought that was what we agreed. However, this change is doing the opposite. It kept the original behavior which is different from the ovn-nbctl, and fixed the "nil" case to make it strictly matched. I think this would make the use of the API less convenient. I'd prefer the ovn-nbctl way. I am sorry that it was not communicated clear enough. What do you think?

hzhou8 · 2021-01-11T06:20:41Z

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 · 2021-01-12T00:20:08Z

@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 · 2021-01-12T00:28:27Z

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.

anfredette · 2021-01-13T01:45:44Z

acl.go

@@ -78,8 +79,14 @@ func (odbi *ovndb) getACLUUIDByRow(lsw, table string, row OVNRow) (string, error
 											goto unmatched
 										}
 									case "external_ids":
-										if value != nil && !odbi.oMapContians(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap, value.(*libovsdb.OvsMap).GoMap) {
-											goto unmatched
+										if value == nil {


NOTE: I left the fix in so that getACLUUIDByRow() properly compares external_ids if they are passed int, but I'm not passing external_ids into the function, so they are not actually compared. I could have omitted this change and it would still work but I thought it was better to include it.

hzhou8 · 2021-01-13T03:07:22Z

acl.go

@@ -220,13 +233,6 @@ func (odbi *ovndb) aclDelImp(lsw, direct, match string, priority int, external_i
 		row["priority"] = priority
 	}

-	if external_ids != nil {


Sorry that I have a second thought on the ACLDel interface & implementation while reviewing this code. It seems the arg external_ids can be removed. However, I want to avoid interface change if possible. On the other hand, it seems confusing for users if the arg is provided by not used. I think it should be honored if not nil - the API user should have a natural expectation that the external_ids is used for looking up the candidate for deletion when they pass in a non-nil value. And it is not harmful to support it, because users can always put a nil value if they don't want it to be part of the comparison. What do you think?

I agree that we can't remove external_ids from ACLDel() because we need to preserve backward compatibility, and I also agree that this could be confusing to the user. However, I think it could also be confusing to support external_ids as an ACLDel() match criterion. The way ACLAdd works now, there can only be one ACL for a given entity (logical switch or port group), direction, match, and priority. I don’t see any added value in optionally supporting external_ids as a match criterion, and if we do support it, and they provide the wrong set of external_ids, the ACL won’t get deleted and we’ll return an error.

Vasiliy mentioned on Slack the possibility of a new API that “search only via uniq field and another via matched fields. So technically you can delete for example all ACLS with specific fields in external ids.“

If I understand correctly, I think this could be implemented via a new API called, for example, “ACLDelBulk()”. ACLDelBulk() would delete all ACLs that matched the provided criteria. I can imagine a use case where we have a set of ACLs that implement a policy. We could tag them all via external_ids with the policy name, and then delete them all with one call if the policy is changed. I don’t think we have a use case for that right now, but I’m open to implementing that functionality separately if needed.

My proposal is the following:

Add a comment on the API in my next PR (ACL for port groups) saying that external_ids are no longer used, but are being retained for backward compatibility. The user can safely use nil for the field. Note, I already have a comment in my next PR saying that the ACLDel() API is being deprecated in favor of ACLDelEntity().

Remove external_ids my new ACLDelEntity() API.

Remove external_ids from aclDelImp() so if the user looks into it, they will see pretty quickly and clearly that it’s not used.

What do you think?

and if we do support it, and they provide the wrong set of external_ids, the ACL won’t get deleted and we’ll return an error.

I think in this case if they provide a wrong set of external_ids, then the expectation to the API should be fail instead of deleting by mistake.

For the idea of ACLDelBulk(), I agree, but can be discussed separately.

vtolstov · 2021-01-13T19:27:54Z

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 · 2021-01-13T20:05:38Z

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 ***@***.***> 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 <#125 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHY7X3E2KGLGEKBITRPNWTSZXX4VANCNFSM4VZHTPNA> .

vtolstov · 2021-01-13T20:34:04Z

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 · 2021-01-13T20:35:32Z

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 · 2021-01-13T21:38:40Z

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 · 2021-01-13T21:55:30Z

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 · 2021-01-13T22:10:04Z

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 · 2021-01-13T22:40:44Z

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 · 2021-01-13T22:57:31Z

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)

… correctly eBay#124 external_ids should be ignored while checking for duplicates in ACLAdd The parameters that define a unique ACL for a given entity (logical switch or port group) are direction, match, and priority. The code currently also attempts to also use external_ids, but it should not. Signed-off-by: Andre Fredette <[email protected]>

anfredette · 2021-01-13T23:50:38Z

The changes have been pushed.

hzhou8 · 2021-01-14T00:17:30Z

LGTM

hzhou8 · 2021-01-14T00:17:49Z

@vtolstov @softwarejl would you take a look again?

vtolstov · 2021-01-14T07:09:31Z

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

anfredette mentioned this pull request Jan 7, 2021

Added support for ACLs on port groups. #122

Merged

hzhou8 approved these changes Jan 7, 2021

View reviewed changes

softwarejl approved these changes Jan 7, 2021

View reviewed changes

hzhou8 suggested changes Jan 11, 2021

View reviewed changes

anfredette force-pushed the issue-124 branch from 30caecc to 3b324f0 Compare January 13, 2021 01:39

anfredette commented Jan 13, 2021

View reviewed changes

hzhou8 suggested changes Jan 13, 2021

View reviewed changes

anfredette force-pushed the issue-124 branch from 3b324f0 to 0e9a960 Compare January 13, 2021 23:47

hzhou8 approved these changes Jan 14, 2021

View reviewed changes

hzhou8 merged commit d2f82d8 into eBay:master Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anfredette commented Jan 7, 2021

hzhou8 left a comment

softwarejl commented Jan 7, 2021

hzhou8 left a comment

hzhou8 commented Jan 11, 2021

anfredette commented Jan 12, 2021

hzhou8 commented Jan 12, 2021

anfredette Jan 13, 2021

hzhou8 Jan 13, 2021

anfredette Jan 13, 2021

hzhou8 Jan 13, 2021

vtolstov commented Jan 13, 2021

anfredette commented Jan 13, 2021 via email

vtolstov commented Jan 13, 2021

hzhou8 commented Jan 13, 2021 •

edited

Loading

anfredette commented Jan 13, 2021

hzhou8 commented Jan 13, 2021

anfredette commented Jan 13, 2021

anfredette commented Jan 13, 2021

hzhou8 commented Jan 13, 2021 •

edited

Loading

anfredette commented Jan 13, 2021

hzhou8 commented Jan 14, 2021

hzhou8 commented Jan 14, 2021

vtolstov commented Jan 14, 2021

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

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

Conversation

anfredette commented Jan 7, 2021

hzhou8 left a comment

Choose a reason for hiding this comment

softwarejl commented Jan 7, 2021

hzhou8 left a comment

Choose a reason for hiding this comment

hzhou8 commented Jan 11, 2021

anfredette commented Jan 12, 2021

hzhou8 commented Jan 12, 2021

anfredette Jan 13, 2021

Choose a reason for hiding this comment

hzhou8 Jan 13, 2021

Choose a reason for hiding this comment

anfredette Jan 13, 2021

Choose a reason for hiding this comment

hzhou8 Jan 13, 2021

Choose a reason for hiding this comment

vtolstov commented Jan 13, 2021

anfredette commented Jan 13, 2021 via email

vtolstov commented Jan 13, 2021

hzhou8 commented Jan 13, 2021 • edited Loading

anfredette commented Jan 13, 2021

hzhou8 commented Jan 13, 2021

anfredette commented Jan 13, 2021

anfredette commented Jan 13, 2021

hzhou8 commented Jan 13, 2021 • edited Loading

anfredette commented Jan 13, 2021

hzhou8 commented Jan 14, 2021

hzhou8 commented Jan 14, 2021

vtolstov commented Jan 14, 2021

hzhou8 commented Jan 13, 2021 •

edited

Loading

hzhou8 commented Jan 13, 2021 •

edited

Loading