Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for ACLs on port groups. #122

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Conversation

anfredette
Copy link
Contributor

@anfredette anfredette commented Dec 23, 2020

Added support for ACLs on port groups.

  • Using ovn-nbctl acl-add/del as a model, an entityType parameter was added to
    the API which could be either PORT_GROUP or LOICAL_SWITCH.
  • In order to maintain backward compatibility of the existing APIs, a new set
    of APIs called ACLAddEntity/ACLDelEntity/ACLListEntity were added.
  • Note: both versions of the APIs call the same underlying
    implementation functions.

Also made a minor optimization suggested by vtolstov on pr-117.

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

@hzhou8
Copy link
Contributor

hzhou8 commented Jan 3, 2021

Thanks @anfredette . LGTM!

A minor comment is that it would be better to split the fixes such as for the problem when external_ids == nil in separate commits. Let me know if you'd like to do that.

@anfredette
Copy link
Contributor Author

anfredette commented Jan 4, 2021 via email

@anfredette anfredette force-pushed the pg-acls3 branch 3 times, most recently from 86f34b8 to 62e7a78 Compare January 7, 2021 18:57
@anfredette
Copy link
Contributor Author

I've updated this PR as follows:

  1. Created Issue acl add/delete code isn't handling external_ids correctly #124 and split out fix as PR Fix issue #124: acl add/delete code isn't handling external_ids correctly #125.
  2. Rebased this PR on PR acl add/delete code isn't handling external_ids correctly #124.
  3. Optimized getACLUUIDByRow()
    • when we have the row UUID, we don't have to loop through the table to find it.

Copy link
Contributor

@softwarejl softwarejl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

acl.go Outdated Show resolved Hide resolved
@anfredette
Copy link
Contributor Author

Per Han's request above, and discussion on Slack, I've changed the port group ACL APIs to use the PG name instead of the PG UUID.

NOTE: I've kept this change as a separate commit to facilitate reviews. After I get the thumbs up, I will squash the last two commits.

Copy link
Contributor

@hzhou8 hzhou8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that I have to put this on hold again because of the external-ids commit. Please see my comment in #125 for the same commit.

- Using ovn-nbctl acl-add/del as a model, an entityType parameter was added to
  the API which could be either PORT_GROUP or LOICAL_SWITCH.
- In order to maintain backward compatibility of the existing APIs, a new set
  of APIs called ACLAddEntity/ACLDelEntity/ACLListEntity were added.
- Note: both versions of the APIs call the same underlying
  implementation functions.

Also made a minor optimization suggested by vtolstov on pr-117.

Signed-off-by: Andre Fredette <[email protected]>
@anfredette
Copy link
Contributor Author

This PR has been reworked on top of PR-125.

@hzhou8 hzhou8 merged commit b96f6c1 into eBay:master Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants