Skip to content

Commit

Permalink
Combine together ports of the same protocol
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jackkleeman
jackkleeman committed Sep 30, 2019
1 parent 988e024 commit ad2aabb
Show file tree
Hide file tree
Showing 2 changed files with 142 additions and 47 deletions.
36 changes: 33 additions & 3 deletions lib/backend/k8s/conversion/conversion.go
View file
Expand Up @@ -535,15 +535,45 @@ func (c Converter) k8sRuleToCalico(rPeers []networkingv1.NetworkPolicyPeer, rPor
ports = []*networkingv1.NetworkPolicyPort{nil}
}

// Combine destinations with sources to generate rules.
// TODO: This currently creates a lot of rules by making every combination of from / ports
// into a rule. We can combine these so that we don't need as many rules!
protocolPorts := map[string][]numorstring.Port{}

for _, port := range ports {
protocol, calicoPorts, err := c.k8sPortToCalicoFields(port)
if err != nil {
return nil, fmt.Errorf("failed to parse k8s port: %s", err)
}

// These are either both present or both nil
if protocol == nil && calicoPorts == nil {
// If nil, no ports were specified, or an empty port struct was provided, which we translate to allowing all.
// We want to use a nil protocol and a nil list of ports, which will allow any destination (for ingress).
// Given we're gonna allow all, we may as well break here and keep only this rule
protocolPorts = map[string][]numorstring.Port{"": nil}
break
}

pStr := protocol.String()
protocolPorts[pStr] = append(protocolPorts[pStr], calicoPorts...)
}

protocols := make([]string, 0, len(protocolPorts))
for k := range protocolPorts {
protocols = append(protocols, k)
}
// Ensure deterministic output
sort.Strings(protocols)

// Combine destinations with sources to generate rules. We generate one rule per protocol,
// with each rule containing all the allowed ports.
for _, protocolStr:= range protocols {
calicoPorts := protocolPorts[protocolStr]

var protocol *numorstring.Protocol
if protocolStr != "" {
p := numorstring.ProtocolFromString(protocolStr)
protocol = &p
}

for _, peer := range peers {
selector, nsSelector, nets, notNets := c.k8sPeerToCalicoFields(peer, ns)
if ingress {
Expand Down
153 changes: 109 additions & 44 deletions lib/backend/k8s/conversion/conversion_test.go
View file
Expand Up @@ -682,27 +682,112 @@ var _ = Describe("Test NetworkPolicy conversion", func() {
Selector: "projectcalico.org/orchestrator == 'k8s' && k == 'v' && k2 == 'v2'",
},
Destination: apiv3.EntityRule{
Ports: []numorstring.Port{numorstring.SinglePort(80)},
Ports: []numorstring.Port{numorstring.SinglePort(80), {MinPort: 0, MaxPort: 0, PortName: "foo"}},
},
},
))

// There should be no Egress rules
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress)).To(Equal(0))

// Check that Types field exists and has only 'ingress'
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Types)).To(Equal(1))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Types[0]).To(Equal(apiv3.PolicyTypeIngress))
})
It("should parse a k8s NetworkPolicy with no ports", func() {
np := networkingv1.NetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test.policy",
Namespace: "default",
},
Spec: networkingv1.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
"label": "value",
"label2": "value2",
},
},
Ingress: []networkingv1.NetworkPolicyIngressRule{
{
From: []networkingv1.NetworkPolicyPeer{
{
PodSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"k": "v",
"k2": "v2",
},
},
},
},
},
},
PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress},
},
}

// Parse the policy.
pol, err := c.K8sNetworkPolicyToCalico(&np)
Expect(err).NotTo(HaveOccurred())

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress).To(ConsistOf(
apiv3.Rule{
Action: "Allow",
Protocol: &protoTCP, // Defaulted to TCP.
Protocol: nil, // We only default to TCP when ports exist
Source: apiv3.EntityRule{
Selector: "projectcalico.org/orchestrator == 'k8s' && k == 'v' && k2 == 'v2'",
},
Destination: apiv3.EntityRule{
Ports: []numorstring.Port{numorstring.NamedPort("foo")},
},
Destination: apiv3.EntityRule{},
},
))
})

// There should be no Egress rules
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress)).To(Equal(0))
It("should parse a k8s NetworkPolicy with blank ports", func() {
port80 := intstr.FromInt(80)
np := networkingv1.NetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test.policy",
Namespace: "default",
},
Spec: networkingv1.NetworkPolicySpec{
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
"label": "value",
"label2": "value2",
},
},
Ingress: []networkingv1.NetworkPolicyIngressRule{
{
Ports: []networkingv1.NetworkPolicyPort{{Port: &port80}, {}},
From: []networkingv1.NetworkPolicyPeer{
{
PodSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"k": "v",
"k2": "v2",
},
},
},
},
},
},
PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress},
},
}

// Check that Types field exists and has only 'ingress'
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Types)).To(Equal(1))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Types[0]).To(Equal(apiv3.PolicyTypeIngress))
// Parse the policy.
pol, err := c.K8sNetworkPolicyToCalico(&np)
Expect(err).NotTo(HaveOccurred())

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress).To(ConsistOf(
apiv3.Rule{
Action: "Allow",
Protocol: nil, // We only default to TCP when ports exist
Source: apiv3.EntityRule{
Selector: "projectcalico.org/orchestrator == 'k8s' && k == 'v' && k2 == 'v2'",
},
Destination: apiv3.EntityRule{},
},
))
})

It("should drop rules with invalid ports in a k8s NetworkPolicy", func() {
Expand Down Expand Up @@ -765,8 +850,8 @@ var _ = Describe("Test NetworkPolicy conversion", func() {

protoTCP := numorstring.ProtocolFromString("TCP")

// Only the two valid rules should exist. The third should have been dropped.
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress)).To(Equal(2))
// Only the two valid ports should exist. The third should have been dropped.
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress)).To(Equal(1))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress).To(ConsistOf(
apiv3.Rule{
Expand All @@ -776,17 +861,7 @@ var _ = Describe("Test NetworkPolicy conversion", func() {
Selector: "projectcalico.org/orchestrator == 'k8s' && k == 'v' && k2 == 'v2'",
},
Destination: apiv3.EntityRule{
Ports: []numorstring.Port{numorstring.SinglePort(80)},
},
},
apiv3.Rule{
Action: "Allow",
Protocol: &protoTCP, // Defaulted to TCP.
Source: apiv3.EntityRule{
Selector: "projectcalico.org/orchestrator == 'k8s' && k == 'v' && k2 == 'v2'",
},
Destination: apiv3.EntityRule{
Ports: []numorstring.Port{numorstring.NamedPort("foo")},
Ports: []numorstring.Port{numorstring.SinglePort(80), numorstring.NamedPort("foo")},
},
},
))
Expand Down Expand Up @@ -999,17 +1074,7 @@ var _ = Describe("Test NetworkPolicy conversion", func() {
Selector: "projectcalico.org/orchestrator == 'k8s' && ! has(toast)",
},
Destination: apiv3.EntityRule{
Ports: []numorstring.Port{numorstring.SinglePort(80)},
},
},
apiv3.Rule{
Action: "Allow",
Protocol: &protoTCP, // Defaulted to TCP.
Source: apiv3.EntityRule{
Selector: "projectcalico.org/orchestrator == 'k8s' && ! has(toast)",
},
Destination: apiv3.EntityRule{
Ports: []numorstring.Port{numorstring.NamedPort("foo")},
Ports: []numorstring.Port{numorstring.SinglePort(80), numorstring.NamedPort("foo")},
},
},
))
Expand Down Expand Up @@ -1100,16 +1165,16 @@ var _ = Describe("Test NetworkPolicy conversion", func() {
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Types[0]).To(Equal(apiv3.PolicyTypeIngress))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[0].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k == 'v'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[0].Destination.Ports).To(Equal([]numorstring.Port{ninety}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[0].Destination.Ports).To(Equal([]numorstring.Port{eighty}))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[1].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k2 == 'v2'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[1].Destination.Ports).To(Equal([]numorstring.Port{ninety}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[1].Destination.Ports).To(Equal([]numorstring.Port{eighty}))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[2].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k == 'v'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[2].Destination.Ports).To(Equal([]numorstring.Port{eighty}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[2].Destination.Ports).To(Equal([]numorstring.Port{ninety}))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[3].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k2 == 'v2'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[3].Destination.Ports).To(Equal([]numorstring.Port{eighty}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[3].Destination.Ports).To(Equal([]numorstring.Port{ninety}))
})
})

Expand Down Expand Up @@ -1626,13 +1691,13 @@ var _ = Describe("Test NetworkPolicy conversion", func() {
Expect(int(*pol.Value.(*apiv3.NetworkPolicy).Spec.Order)).To(Equal(1000))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s'"))
Expect(len(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress)).To(Equal(2))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[0].Destination.Ports).To(Equal([]numorstring.Port{ninetyName}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[0].Destination.Ports).To(Equal([]numorstring.Port{eightyName}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[0].Destination.Nets[0]).To(Equal("192.168.0.0/16"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[0].Destination.NotNets[0]).To(Equal("192.168.3.0/24"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[0].Destination.NotNets[1]).To(Equal("192.168.4.0/24"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[0].Destination.NotNets[2]).To(Equal("192.168.5.0/24"))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[1].Destination.Ports).To(Equal([]numorstring.Port{eightyName}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[1].Destination.Ports).To(Equal([]numorstring.Port{ninetyName}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[1].Destination.Nets[0]).To(Equal("192.168.0.0/16"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[1].Destination.NotNets[0]).To(Equal("192.168.3.0/24"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Egress[1].Destination.NotNets[1]).To(Equal("192.168.4.0/24"))
Expand Down Expand Up @@ -1962,16 +2027,16 @@ var _ = Describe("Test NetworkPolicy conversion (k8s <= 1.7, no policyTypes)", f
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Types[0]).To(Equal(apiv3.PolicyTypeIngress))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[0].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k == 'v'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[0].Destination.Ports).To(Equal([]numorstring.Port{ninety}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[0].Destination.Ports).To(Equal([]numorstring.Port{eighty}))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[1].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k2 == 'v2'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[1].Destination.Ports).To(Equal([]numorstring.Port{ninety}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[1].Destination.Ports).To(Equal([]numorstring.Port{eighty}))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[2].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k == 'v'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[2].Destination.Ports).To(Equal([]numorstring.Port{eighty}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[2].Destination.Ports).To(Equal([]numorstring.Port{ninety}))

Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[3].Source.Selector).To(Equal("projectcalico.org/orchestrator == 'k8s' && k2 == 'v2'"))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[3].Destination.Ports).To(Equal([]numorstring.Port{eighty}))
Expect(pol.Value.(*apiv3.NetworkPolicy).Spec.Ingress[3].Destination.Ports).To(Equal([]numorstring.Port{ninety}))
})
})

Expand Down

0 comments on commit ad2aabb

Please sign in to comment.