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

Implement IP deny/allow ACL rules #2639

Closed
wants to merge 2 commits into from

Conversation

zetaab
Copy link
Contributor

@zetaab zetaab commented Feb 17, 2024

What type of PR is this?
feat: add Support for Allow/Deny IP Subnets

What this PR does / why we need it:

Usually there needs to be way to deny / allow subnets for single routes / entire gateway.

Which issue(s) this PR fixes:
Fixes #2462

Notes to reviewers

disclaimer: This is my first bigger PR to this repository

Questions

  • Gateway level ACL definitions works. However, if route do have any ACL rules the route will override the Gateway ACL settings. Like mentioned in Support Allow/Deny IP Subnets #2462 (comment) how this should work? Is this behavior ok?
  • This applies rules only to http, there is RBAC policy for network level as well (but those cannot be applied to route level). It might be needed if envoy gateway is used for tcp stuff? That is perhaps task to another PR?
  • Not sure does it work out of the box with ipv6, I do not have k8s with ipv6 to verify it.

I have verified this PR manually by following test cases:

route: allow ip - ok (allowed ip works and others not)
route: deny ip - ok (denied ip does not work and others will work)
route: allow big cidr, deny single ip(s) from allowed cidr - ok (big cidr works except denied ips)
route: deny big cidr, allow single ip(s) from denied cidr - not working, but it should not either? (whole big cidr does not work, no matter is there something in allow)

gateway: allow ip - ok (allowed ip works and others not)
gateway: deny ip - ok (denied ip does not work and others will work)
gateway: allow big cidr, deny single ip(s) from allowed cidr - ok (big cidr works except denied ips)
gateway: deny big cidr, allow single ip(s) from denied cidr - not working, but it should not either? (whole big cidr does not work, no matter is there something in allow)

@zetaab zetaab requested a review from a team as a code owner February 17, 2024 14:35
Copy link

codecov bot commented Feb 17, 2024

Codecov Report

Attention: 92 lines in your changes are missing coverage. Please review.

Comparison is base (a5125bf) 63.41% compared to head (8d67476) 63.33%.

Files Patch % Lines
internal/ir/zz_generated.deepcopy.go 0.00% 32 Missing and 1 partial ⚠️
internal/xds/translator/acl.go 76.76% 22 Missing and 11 partials ⚠️
internal/gatewayapi/securitypolicy.go 18.75% 24 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2639      +/-   ##
==========================================
- Coverage   63.41%   63.33%   -0.08%     
==========================================
  Files         119      120       +1     
  Lines       19227    19436     +209     
==========================================
+ Hits        12193    12310     +117     
- Misses       6222     6300      +78     
- Partials      812      826      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
}

// IPSpec defines the configuration for IP.
type IPSpec struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a common struct from upstream(gateway api or kubernetes)? maybe use a.b.c.d/24 for better UX?

@zirain
Copy link
Contributor

zirain commented Feb 19, 2024

Please submit the API design first, otherwise there may be a lot of waste.

@zetaab
Copy link
Contributor Author

zetaab commented Feb 19, 2024

separate PR opened about apis first #2652

@zetaab zetaab closed this Feb 27, 2024
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.

Support Allow/Deny IP Subnets
2 participants