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

Combine together ports of the same protocol #1135

Merged
merged 1 commit into from Oct 4, 2019

Conversation

@jackkleeman
Copy link
Contributor

jackkleeman commented Sep 30, 2019

Description

This is a new feature that isn't particularly client facing, intended to reduce the number of iptables rules in cases where we have multiple ports in a k8s network policy. This will hopefully improve application times and reduce errors in cases where people have a lot of ports. This code is already fairly well tested but I have added two new tests to cover particular edge cases that concerned me, where there is no ports provided, and where an empty port struct is provided. This affects the calico kube controllers.

When converting k8s network policies to calico policies, we currently create one calico rule per port x peer combination. Instead, we should probably create one rule per protocol x peer combination, and combine together ports of the same protocol.

As normal, no ports leads to allowing any port and any protocol. The existing behaviour is that any empty port struct, even in a list with other ports, will lead to the creation of a rule that allows all ports. Currently we also create other rules for other ports, but this seems pointless, so I have decided to only produce the allow all rule in this case.

Todos

  • Tests
  • Documentation - not sure this is needed
  • Release note

Release Note

Kubernetes NetworkPolicyIngressRules with multiple ports are kept as a single Calico rule per peer where possible, instead of one rule per port/peer combination.
When converting k8s network policies to calico policies, we currently
create one calico rule per port x peer combination. Instead, we should
probably create one rule per protocol x peer combination, and combine
together ports of the same protocol.

As normal, no ports leads to allowing any port and any protocol. The
existing behaviour is that any empty port struct, even in a list with
other ports, will lead to the creation of a rule that allows all ports.
Currently we also create other rules for other ports, but this seems
pointless.
@jackkleeman jackkleeman force-pushed the monzo:convert-ports branch from 303e00b to 3912f3d Sep 30, 2019
@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Oct 1, 2019

@jackkleeman thanks for the PR! This is pretty cool. I've given it a quick look and it seems like the right stuff. Planning on doing a more detailed look sometime this week I hope.

Copy link
Member

caseydavenport left a comment

This looks great. Thanks for contributing this!

@caseydavenport caseydavenport merged commit 6ccf338 into projectcalico:master Oct 4, 2019
2 checks passed
2 checks passed
license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details
@jackkleeman jackkleeman deleted the monzo:convert-ports branch Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.