Skip to content

ACL fixes #465

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.

Sign up for GitHub

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

Jump to bottom
Merged
AltraMayor merged 5 commits into from
Apr 22, 2021
Merged

ACL fixes #465

AltraMayor merged 5 commits into from
Apr 22, 2021

Conversation

@cjdoucette
Copy link
Collaborator

@cjdoucette cjdoucette commented Apr 14, 2021

No description provided.

@cjdoucette cjdoucette added this to the First deployment milestone Apr 14, 2021
AltraMayor
AltraMayor requested changes Apr 14, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

lls: fix ICMP/ICMPv6 ACL registration

AltraMayor
AltraMayor requested changes Apr 14, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

acl/net: be careful about ntuple filters and ACLs

AltraMayor
AltraMayor requested changes Apr 14, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

acl/net: coalesce ACL/ntuple into one abstraction

AltraMayor
AltraMayor reviewed Apr 15, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

Patch acl: update rule registration interface is ready for merge.

AltraMayor
AltraMayor requested changes Apr 15, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

acl/net: coalesce ACL/ntuple into one abstraction

  1. With the exception of lib/net.c and the GK and GT blocks, we should be able to stop including the header gatekeeper_acl.h everywhere else.

  2. The LLS block is not dealing with ICMP packets that may arrive now that it registers for ntuple filters. Given the current patch is already large, it would be better to just add a TODO comment in this patch, and address this issue in a following patch.

@cjdoucette
Copy link
Collaborator Author

cjdoucette commented Apr 15, 2021

I removed gatekeeper_acl.h where I could, but note that the GGU still uses it for acl_enabled().

@cjdoucette cjdoucette force-pushed the acl_fixes branch 4 times, most recently from fa11ff2 to e4c5e17 Compare April 15, 2021 18:47
@cjdoucette
Copy link
Collaborator Author

cjdoucette commented Apr 15, 2021

Ready for another review (but is not yet tested).

@AltraMayor AltraMayor added bug Production requirement Either the issue is solved, or Gatekeeper doesn't work in production labels Apr 15, 2021
AltraMayor
AltraMayor requested changes Apr 15, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

acl/net: be careful about ntuple filters and ACLs

AltraMayor
AltraMayor requested changes Apr 15, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

acl/net: coalesce ACL/ntuple into one abstraction

AltraMayor
AltraMayor requested changes Apr 15, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

acl/net: coalesce ACL/ntuple into one abstraction

AltraMayor
AltraMayor requested changes Apr 15, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

acl/net: coalesce ACL/ntuple into one abstraction

@cjdoucette cjdoucette force-pushed the acl_fixes branch 2 times, most recently from fe63570 to 135db9b Compare April 20, 2021 21:29
@cjdoucette
Copy link
Collaborator Author

cjdoucette commented Apr 20, 2021

Ready for another review. Tested on Amazon.

AltraMayor
AltraMayor requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

net: define RX methods for blocks

AltraMayor
AltraMayor requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

net: combine ntuple filters/ACL into packet filters

We should be able to remove gatekeeper_acl.h from the GGU this time around.

It's worth verifying that only lib/net.c and the GK and GT blocks are adding the header gatekeeper_acl.h.

AltraMayor
AltraMayor requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

lls: use packet filter API

@cjdoucette cjdoucette force-pushed the acl_fixes branch 2 times, most recently from 38e67b6 to 6ad2d86 Compare April 21, 2021 16:11
AltraMayor
AltraMayor requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

net: define RX methods for blocks

@AltraMayor
Copy link
Owner

AltraMayor commented Apr 21, 2021

Shouldn't we remove line #include "gatekeeper_acl.h" from include/gatekeeper_lls.h?

@AltraMayor
Copy link
Owner

AltraMayor commented Apr 21, 2021

My fault, it has already been removed.

AltraMayor
AltraMayor requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

net: combine ntuple filters/ACL into packet filters

@AltraMayor
Copy link
Owner

AltraMayor commented Apr 21, 2021

Please fix the last comment, and run a test on Amazon.

@cjdoucette
net: define RX methods for blocks
1503451 
In at least some current NICs (Intel 82599) the IPv6 destination IP
field cannot be matched by ntuple filters (AltraMayor#466). Therefore, it is
possible for ntuple filters to appear to be available but can not
be used, e.g., in an IPv6-only deployment.

This patch makes the code more careful about this distinction, and
defines RX methods so that blocks can decide whether they need to
query NICs or mailboxes, for example, to receive packets.
@cjdoucette
net: combine ntuple filters/ACL into packet filters
f1b363e 
Since ntuple filters can be mostly equivalent to ACL rules
when hardware does not support ntuple filters, this patch
coalesces their creation into a single API for packet filters,
including one function for IPv4 and one for IPv6, so that
blocks don't have to worry about what hardware supports.

Closes AltraMayor#63.
@cjdoucette
lls: use packet filter API
0655a97
@cjdoucette
acl: update rule registration interface
2afe37a
@cjdoucette
lls: make Gatekeeper includes consistent
174aca3
@cjdoucette
Copy link
Collaborator Author

cjdoucette commented Apr 21, 2021

Fixed and tested. Ready for another review.

@AltraMayor AltraMayor merged commit 793e969 into AltraMayor:master Apr 22, 2021
@cjdoucette cjdoucette deleted the acl_fixes branch May 12, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AltraMayor AltraMayor AltraMayor requested changes

Assignees

No one assigned

Labels

bug Production requirement Either the issue is solved, or Gatekeeper doesn't work in production

Projects

None yet

Milestone

First deployment

Development

Successfully merging this pull request may close these issues.

2 participants

@cjdoucette @AltraMayor