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

feat: Implement bandwidth limit on egress using tc #2

Open
Revolyssup opened this issue Aug 15, 2023 · 12 comments · May be fixed by #4
Open

feat: Implement bandwidth limit on egress using tc #2

Revolyssup opened this issue Aug 15, 2023 · 12 comments · May be fixed by #4

Comments

@Revolyssup
Copy link
Collaborator

Revolyssup commented Aug 15, 2023

Proposal:
TC(Traffic Control) supports multiple types of filters that can be attached to different classes inside q-disks(RTM). Since neither libbpfgo nor ebpf-go libraries support tc functionality directly due to multiple reasons listed here

We can use a separate library like go-tc to attach ebpf filters on an interface(ingress/egress) to implement bandwidth limiting on any given interface.

If the proposal sounds good, then I would like to create a draft PR on this where we can carry forward the discussion.


Some references

  1. https://liuhangbin.netlify.app/post/ebpf-and-xdp/
@Revolyssup
Copy link
Collaborator Author

Revolyssup commented Aug 15, 2023

@DelusionalOptimist Please suggest if you have any better approach.

@DelusionalOptimist
Copy link
Owner

@Revolyssup Have we considered the TC hooking capabilities that libbpfgo provides? Are they not sufficient for the use case?
Here is an example - https://github.com/aquasecurity/libbpfgo/tree/main/selftest/tc

@Revolyssup
Copy link
Collaborator Author

@DelusionalOptimist LGTM, I might have missed this. Lets go with this.

@DelusionalOptimist
Copy link
Owner

Let's go! 🚀

@Revolyssup
Copy link
Collaborator Author

Revolyssup commented Aug 17, 2023

@DelusionalOptimist Just to confirm few things, here is the proposal:

  • Since meaning of source and destination will be different for Ingress and Egress, rules can have two fields ingress and egress. Both consisting of their own configuration. Most of the configuration will be the same like protocol, src ip, dest ip, dest por, src port.
  • Each field ingress and egress will be an array similar to what rules is right now.
  • We can use XDP for ingress and tc for Egress.
  • And last thing, for the first iteration we can skip having classes within classes in Qdisks and just use the root qdisk since the current use case is pretty straightforward.
  • So to summarise, we will be XDP_DROPing the packet on ingress rules and TC_SHOTing the packet on egress rule.
    What do you say?

For later (to be carried on in separate issues after tc Egress support. Only being noted here to capture overall direction):

  • Each field ingress and egress can be an array where the rules are evaluated in the order of priority where each rule can be given a priority like a nice value. The greater the value, the later it will be executed.
  • This can be helpful when later an optional action field is added that can be deny(default) and allow. This can help to chain together bunch of rules with necessary actions.
  • We can have sibling field action where more complicated packet manipulation actions could be defined, later referenced by ID/name in rules. This last one could be a no-no depending if we want dropit to do just one thing(act as a firewall dropping packets) or if we want to later on extend it to go beyond plain packet dropping.

@Revolyssup Revolyssup changed the title feat: Implement bandwidth limit on ingress-egress using tc feat: Implement bandwidth limit on egress using tc Aug 17, 2023
@DelusionalOptimist
Copy link
Owner

@Revolyssup based on your proposed configuration, I can picture it somewhat like below. Let me know if I'm right.

rules:
  - id: "restrict webserver"
    ingress:
      - id: "only allow port 443"
        sourceIP: '*'
        destinationPort: 443
        <...>
        # future
        action: allow
        priority: -20
    egress:
      - id: "block access to github.com"
        destIP: '20.207.73.82'
        destinationPort: '*'
        <...>
        # future
        priority: 20
  - id: "restrict firefox"
    egress:
      - id: "block access to ads.google.com"
        destIP: '142.250.194.206'
        destinationPort: '*'
        <...>
        # future
        priority: -20

This sounds good and also pretty close to Kubernetes' NetworkPolicy. I think this way we can use each individual "rule block" to granularly define process level networking.

A couple of things worth mentioning:

  1. Structuring the rules internally keeping in mind limitations of BPF Maps and how they can be iterated.
  2. For reference - https://github.com/xdp-project/xdp-tutorial/tree/master/advanced01-xdp-tc-interact

We can have sibling field action where more complicated packet manipulation actions could be defined, later referenced by ID/name in rules. This last one could be a no-no depending if we want dropit to do just one thing(act as a firewall dropping packets) or if we want to later on extend it to go beyond plain packet dropping.

I think we can extend further to basic things like redirecting packet based on some information and so on but let's think about it later.

@Revolyssup
Copy link
Collaborator Author

@DelusionalOptimist In first iteration, will we need the interaction between xdp and tc or can they work independently on ingress and egress respectively?

@DelusionalOptimist
Copy link
Owner

@Revolyssup My bad. I meant to cite that resource for future reference. I don't think we would need any interaction b/w the two for our current use case.

@Revolyssup
Copy link
Collaborator Author

@DelusionalOptimist What if we kept it simple and just add a type field to existing rules ? Assuming functionality added might have a different approach for ingress and egress but will be the same. In cases when something can not be implemented by either one, we can produce warn logs indicating that. For eg: [warn] xyz capability not supported on ingress by sanitising/checking the configuration provided with the features that can be supported at config load time.

Let me know if this categorization makes sense to you?

@Revolyssup
Copy link
Collaborator Author

I think I should create a draft PR from here and carry discussion there so we have something concrete to make comments on.

@Revolyssup
Copy link
Collaborator Author

@DelusionalOptimist Do you suggest we have separate maps for ingress and egress filter_rules? To avoid contention as the rules are evaluated on each entry, we would be evaluating ingress rules on egress packets.

@DelusionalOptimist
Copy link
Owner

What if we kept it simple and just add a type field to existing rules ? Assuming functionality added might have a different approach for ingress and egress but will be the same. In cases when something can not be implemented by either one, we can produce warn logs indicating that. For eg: [warn] xyz capability not supported on ingress by sanitising/checking the configuration provided with the features that can be supported at config load time.

Quite frankly that was what I was thinking initially but I thought you wanted to keep it close to Network Policy. I'm fine either ways.

@Revolyssup Revolyssup linked a pull request Aug 22, 2023 that will close this issue
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 a pull request may close this issue.

2 participants