From ad2aabb39bc8ba5d3e2b96f8b3f6c8bc172144b1 Mon Sep 17 00:00:00 2001 From: Jack Kleeman Date: Mon, 30 Sep 2019 18:09:50 +0100 Subject: [PATCH] Combine together ports of the same protocol 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. --- lib/backend/k8s/conversion/conversion.go | 36 ++++- lib/backend/k8s/conversion/conversion_test.go | 153 +++++++++++++----- 2 files changed, 142 insertions(+), 47 deletions(-) diff --git a/lib/backend/k8s/conversion/conversion.go b/lib/backend/k8s/conversion/conversion.go index 16af904af..6088007ee 100644 --- a/lib/backend/k8s/conversion/conversion.go +++ b/lib/backend/k8s/conversion/conversion.go @@ -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 { diff --git a/lib/backend/k8s/conversion/conversion_test.go b/lib/backend/k8s/conversion/conversion_test.go index ae69cceb9..843ea44ff 100644 --- a/lib/backend/k8s/conversion/conversion_test.go +++ b/lib/backend/k8s/conversion/conversion_test.go @@ -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() { @@ -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{ @@ -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")}, }, }, )) @@ -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")}, }, }, )) @@ -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})) }) }) @@ -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")) @@ -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})) }) })