From 985f07788dd6f7aa43b205c01a87e2d1f0eb2130 Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 17 Nov 2020 20:14:32 -0800 Subject: [PATCH 01/14] Adding port_ format for named ports --- npm/pod.go | 6 ++++-- npm/pod_test.go | 15 +++++++++++++++ npm/translatePolicy.go | 24 ++++++++++++------------ npm/util/const.go | 3 +++ 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/npm/pod.go b/npm/pod.go index 64ad54c22e..7edf68259e 100644 --- a/npm/pod.go +++ b/npm/pod.go @@ -92,7 +92,8 @@ func (npMgr *NetworkPolicyManager) AddPod(podObj *corev1.Pod) error { case v1.ProtocolSCTP: protocol = util.IpsetSCTPFlag } - ipsMgr.AddToSet(port.Name, fmt.Sprintf("%s,%s%d", podIP, protocol, port.ContainerPort), util.IpsetIPPortHashFlag, podUid) + namedPortname := util.NamedPortIPSetPrefix + port.Name + ipsMgr.AddToSet(namedPortname, fmt.Sprintf("%s,%s%d", podIP, protocol, port.ContainerPort), util.IpsetIPPortHashFlag, podUid) } } } @@ -209,7 +210,8 @@ func (npMgr *NetworkPolicyManager) DeletePod(podObj *corev1.Pod) error { case v1.ProtocolSCTP: protocol = util.IpsetSCTPFlag } - ipsMgr.DeleteFromSet(port.Name, fmt.Sprintf("%s,%s%d", cachedPodIp, protocol, port.ContainerPort), podUid) + namedPortname := util.NamedPortIPSetPrefix + port.Name + ipsMgr.DeleteFromSet(namedPortname, fmt.Sprintf("%s,%s%d", cachedPodIp, protocol, port.ContainerPort), podUid) } } } diff --git a/npm/pod_test.go b/npm/pod_test.go index 057d996b09..0cee6511f5 100644 --- a/npm/pod_test.go +++ b/npm/pod_test.go @@ -70,12 +70,27 @@ func TestAddPod(t *testing.T) { Phase: "Running", PodIP: "1.2.3.4", }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + corev1.Container{ + Ports: []corev1.ContainerPort{ + corev1.ContainerPort{ + Name: "app:test-pod", + ContainerPort: 8080, + }, + }, + }, + }, + }, } npMgr.Lock() if err := npMgr.AddPod(podObj); err != nil { t.Errorf("TestAddPod failed @ AddPod") } + if !ipsMgr.Exists(util.GetHashedName("app:test-pod"), "1.2.3.4,8080", "") { + t.Errorf("TestAddPod failed @ AddPod, Checking Port named same as Label") + } npMgr.Unlock() } diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index 4e8eef1cc9..5bbe64b76b 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -232,7 +232,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS if portRuleExists && !fromRuleExists && !allowExternal { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, @@ -288,7 +288,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS if len(fromRule.IPBlock.Except) > 0 { for _, except := range fromRule.IPBlock.Except { // TODO move IP cidrs rule to allow based only - ipCidrs[i] = append(ipCidrs[i], except + util.IpsetNomatch) + ipCidrs[i] = append(ipCidrs[i], except+util.IpsetNomatch) } addedIngressFromEntry = true } @@ -298,7 +298,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, @@ -414,7 +414,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, @@ -508,7 +508,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, @@ -615,7 +615,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, @@ -870,7 +870,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe if portRuleExists && !toRuleExists && !allowExternal { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, @@ -936,7 +936,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, @@ -984,7 +984,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe util.GetHashedName(cidrIpsetName), util.IptablesDstFlag, ) - entry.Specs = append( + entry.Specs = append( entry.Specs, util.IptablesJumpFlag, util.IptablesAccept, @@ -1058,7 +1058,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, @@ -1152,7 +1152,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, @@ -1259,7 +1259,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe if portRuleExists { for _, portRule := range rule.Ports { if portRule.Port != nil && portRule.Port.IntValue() == 0 { - portName := portRule.Port.String() + portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, diff --git a/npm/util/const.go b/npm/util/const.go index e553b4e683..2118ff2d94 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -109,6 +109,9 @@ const ( IpsetMaxelemNum string = "4294967295" IpsetNomatch string = "nomatch" + + //Prefixes for ipsets + NamedPortIPSetPrefix string = "port_" ) //NPM telemetry constants. From 87b0dc325cc2c10eddefdfdb234b00ab7ec3ae7d Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 17 Nov 2020 20:23:09 -0800 Subject: [PATCH 02/14] Cleaning existing ipsets incase of a upgrade --- npm/npm.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/npm/npm.go b/npm/npm.go index df6530e409..c3f540c017 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/azure-container-networking/aitelemetry" "github.com/Azure/azure-container-networking/log" + "github.com/Azure/azure-container-networking/npm/ipsm" "github.com/Azure/azure-container-networking/npm/iptm" "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/util" @@ -188,6 +189,12 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in iptMgr := iptm.NewIptablesManager() iptMgr.UninitNpmChains() + log.Logf("Azure-NPM creating, cleaning existing IPSets") + destroyErr := ipsm.NewIpsetManager().Destroy() + if destroyErr != nil { + log.Logf("Azure-NPM error occurred while destroying existing IPSets err: %s", destroyErr.Error()) + } + var ( podInformer = informerFactory.Core().V1().Pods() nsInformer = informerFactory.Core().V1().Namespaces() From 0c692a2c4df09f6ba1d3865fdb77cc4a8a895e4e Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 17 Nov 2020 20:49:23 -0800 Subject: [PATCH 03/14] Changing the delimiter for port prefix --- npm/util/const.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/util/const.go b/npm/util/const.go index 2118ff2d94..b40885b894 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -111,7 +111,7 @@ const ( IpsetNomatch string = "nomatch" //Prefixes for ipsets - NamedPortIPSetPrefix string = "port_" + NamedPortIPSetPrefix string = "port:" ) //NPM telemetry constants. From 422add6d9029e57f057d163b531741e762cffa77 Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 17 Nov 2020 20:54:53 -0800 Subject: [PATCH 04/14] Some basic formatting --- npm/pod.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/npm/pod.go b/npm/pod.go index 7edf68259e..1c2a11560c 100644 --- a/npm/pod.go +++ b/npm/pod.go @@ -93,7 +93,13 @@ func (npMgr *NetworkPolicyManager) AddPod(podObj *corev1.Pod) error { protocol = util.IpsetSCTPFlag } namedPortname := util.NamedPortIPSetPrefix + port.Name - ipsMgr.AddToSet(namedPortname, fmt.Sprintf("%s,%s%d", podIP, protocol, port.ContainerPort), util.IpsetIPPortHashFlag, podUid) + ipsMgr.AddToSet( + namedPortname, + fmt.Sprintf("%s,%s%d", podIP, protocol, port.ContainerPort), + util.IpsetIPPortHashFlag, + podUid, + ) + } } } @@ -211,7 +217,11 @@ func (npMgr *NetworkPolicyManager) DeletePod(podObj *corev1.Pod) error { protocol = util.IpsetSCTPFlag } namedPortname := util.NamedPortIPSetPrefix + port.Name - ipsMgr.DeleteFromSet(namedPortname, fmt.Sprintf("%s,%s%d", cachedPodIp, protocol, port.ContainerPort), podUid) + ipsMgr.DeleteFromSet( + namedPortname, + fmt.Sprintf("%s,%s%d", cachedPodIp, protocol, port.ContainerPort), + podUid, + ) } } } From 192023ca4ed97bb014f432862bd477aebae34f95 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 08:13:25 -0800 Subject: [PATCH 05/14] Adding fixes to testcases --- npm/pod_test.go | 2 +- npm/translatePolicy_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/npm/pod_test.go b/npm/pod_test.go index 0cee6511f5..0ab84fbbe1 100644 --- a/npm/pod_test.go +++ b/npm/pod_test.go @@ -88,7 +88,7 @@ func TestAddPod(t *testing.T) { if err := npMgr.AddPod(podObj); err != nil { t.Errorf("TestAddPod failed @ AddPod") } - if !ipsMgr.Exists(util.GetHashedName("app:test-pod"), "1.2.3.4,8080", "") { + if !ipsMgr.Exists(util.GetHashedName("port:app:test-pod"), "1.2.3.4,8080", "") { t.Errorf("TestAddPod failed @ AddPod, Checking Port named same as Label") } npMgr.Unlock() diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 236de93e46..81b8ef405e 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -547,7 +547,7 @@ func TestGetDefaultDropEntries(t *testing.T) { func TestTranslateIngress(t *testing.T) { ns := "testnamespace" - name := "testnetworkpolicyname" + name := "testnetworkpolicyname" targetSelector := metav1.LabelSelector{ MatchLabels: map[string]string{ "context": "dev", @@ -3057,15 +3057,15 @@ func TestComplexPolicy(t *testing.T) { t.Errorf("expectedLists: %v", expectedLists) } - expectedIngressIPCidrs := [][]string { + expectedIngressIPCidrs := [][]string{ {"", "", "", "172.17.0.0/16", "172.17.1.0/24nomatch"}, } - expectedEgressIPCidrs := [][]string { + expectedEgressIPCidrs := [][]string{ {"", "10.0.0.0/24", "10.0.0.1/32nomatch"}, } - if !reflect.DeepEqual(ingressIPCidrs, expectedIngressIPCidrs) || !reflect.DeepEqual(ingressIPCidrsDiffOrder, expectedIngressIPCidrs){ + if !reflect.DeepEqual(ingressIPCidrs, expectedIngressIPCidrs) || !reflect.DeepEqual(ingressIPCidrsDiffOrder, expectedIngressIPCidrs) { t.Errorf("translatedPolicy failed @ k8s-example-policy ingress IP Cidrs comparison") t.Errorf("ingress IP Cidrs: %v", ingressIPCidrs) t.Errorf("expected ingress IP Cidrs: %v", expectedIngressIPCidrs) @@ -3807,7 +3807,7 @@ func TestNamedPorts(t *testing.T) { } expectedNamedPorts := []string{ - "serve-80", + "port:serve-80", } if !reflect.DeepEqual(namedPorts, expectedNamedPorts) { t.Errorf("translatedPolicy failed @ ALLOW-ALL-TCP-PORT-serve-80-TO-app:server-IN-ns-test-policy namedPorts comparison") From c16bc13a47d11ff78e9a3fe2b64ba74a16060eab Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 12:22:07 -0800 Subject: [PATCH 06/14] Addressing comments --- npm/pod_test.go | 5 ++++- npm/translatePolicy_test.go | 4 ++-- npm/util/const.go | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/npm/pod_test.go b/npm/pod_test.go index 0ab84fbbe1..f399b11d3d 100644 --- a/npm/pod_test.go +++ b/npm/pod_test.go @@ -88,9 +88,12 @@ func TestAddPod(t *testing.T) { if err := npMgr.AddPod(podObj); err != nil { t.Errorf("TestAddPod failed @ AddPod") } - if !ipsMgr.Exists(util.GetHashedName("port:app:test-pod"), "1.2.3.4,8080", "") { + if !ipsMgr.Exists(util.GetHashedName("namedport:app:test-pod"), "hash:ip,port", "") { t.Errorf("TestAddPod failed @ AddPod, Checking Port named same as Label") } + if !ipsMgr.Exists(util.GetHashedName("app:test-pod"), "nethash", "") { + t.Errorf("TestAddPod failed @ AddPod, Checking LabelKeyValue named same as Port") + } npMgr.Unlock() } diff --git a/npm/translatePolicy_test.go b/npm/translatePolicy_test.go index 81b8ef405e..66e09c398e 100644 --- a/npm/translatePolicy_test.go +++ b/npm/translatePolicy_test.go @@ -3807,7 +3807,7 @@ func TestNamedPorts(t *testing.T) { } expectedNamedPorts := []string{ - "port:serve-80", + "namedport:serve-80", } if !reflect.DeepEqual(namedPorts, expectedNamedPorts) { t.Errorf("translatedPolicy failed @ ALLOW-ALL-TCP-PORT-serve-80-TO-app:server-IN-ns-test-policy namedPorts comparison") @@ -3840,7 +3840,7 @@ func TestNamedPorts(t *testing.T) { util.IptablesModuleFlag, util.IptablesSetModuleFlag, util.IptablesMatchSetFlag, - util.GetHashedName("serve-80"), + util.GetHashedName("namedport:serve-80"), util.IptablesDstFlag + "," + util.IptablesDstFlag, util.IptablesJumpFlag, util.IptablesAccept, diff --git a/npm/util/const.go b/npm/util/const.go index b40885b894..0e0e9319ab 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -111,7 +111,7 @@ const ( IpsetNomatch string = "nomatch" //Prefixes for ipsets - NamedPortIPSetPrefix string = "port:" + NamedPortIPSetPrefix string = "namedport:" ) //NPM telemetry constants. From 380602870e50c238529c815cace635938cb1a803 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 14:15:17 -0800 Subject: [PATCH 07/14] Adding mitigation for empty string in named port --- npm/translatePolicy.go | 83 ++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 20 deletions(-) diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index 5bbe64b76b..ee684c272d 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -38,6 +38,19 @@ func craftPartialIptEntrySpecFromPort(portRule networkingv1.NetworkPolicyPort, s return partialSpec } +func getPortType(portRule networkingv1.NetworkPolicyPort) string { + if portRule.Port == nil { + return "invalid" + } else if portRule.Port.IntValue() == 0 && portRule.Port.String() == "" { + return "invalid" + } else if portRule.Port.IntValue() == 0 && portRule.Port.String() != "" { + return "namedport" + } else if portRule.Port.IntValue() != 0 { + return "validport" + } + return "invalid" +} + func craftPartialIptablesCommentFromPort(portRule networkingv1.NetworkPolicyPort, sPortOrDPortFlag string) string { partialComment := "" if portRule.Protocol != nil { @@ -231,7 +244,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS // Only Ports rules exist if portRuleExists && !fromRuleExists && !allowExternal { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -255,7 +269,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, Specs: craftPartialIptEntrySpecFromPort(portRule, util.IptablesDstPortFlag), @@ -273,6 +287,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } continue @@ -297,7 +313,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS } if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -329,7 +346,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) fromRuleEntries = append(fromRuleEntries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, Specs: append([]string(nil), targetSelectorIptEntrySpec...), @@ -358,6 +375,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) fromRuleEntries = append(fromRuleEntries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -413,7 +432,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS iptPartialNsComment := craftPartialIptablesCommentFromSelector("", fromRule.NamespaceSelector, true) if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -441,7 +461,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, Specs: append([]string(nil), targetSelectorIptEntrySpec...), @@ -466,6 +486,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -507,7 +529,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS iptPartialPodComment := craftPartialIptablesCommentFromSelector(ns, fromRule.PodSelector, false) if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -535,7 +558,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, Specs: append([]string(nil), targetSelectorIptEntrySpec...), @@ -560,6 +583,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -614,7 +639,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS iptPartialPodComment := craftPartialIptablesCommentFromSelector("", fromRule.PodSelector, false) if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -647,7 +673,7 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureIngressPortChain, Specs: append([]string(nil), iptPartialNsSpec...), @@ -677,6 +703,8 @@ func translateIngress(ns string, policyName string, targetSelector metav1.LabelS "-TO-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -869,7 +897,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe // Only Ports rules exist if portRuleExists && !toRuleExists && !allowExternal { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -893,7 +922,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, Specs: craftPartialIptEntrySpecFromPort(portRule, util.IptablesDstPortFlag), @@ -911,6 +940,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } continue @@ -935,7 +966,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe } if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -967,7 +999,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) toRuleEntries = append(toRuleEntries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, Specs: craftPartialIptEntrySpecFromPort(portRule, util.IptablesDstPortFlag), @@ -996,6 +1028,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) toRuleEntries = append(toRuleEntries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -1057,7 +1091,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe iptPartialNsComment := craftPartialIptablesCommentFromSelector("", toRule.NamespaceSelector, true) if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -1085,7 +1120,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, Specs: append([]string(nil), iptPartialNsSpec...), @@ -1110,6 +1145,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -1151,7 +1188,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe iptPartialPodComment := craftPartialIptablesCommentFromSelector(ns, toRule.PodSelector, false) if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -1179,7 +1217,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, Specs: append([]string(nil), iptPartialPodSpec...), @@ -1204,6 +1242,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-FROM-"+targetSelectorComment, ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { @@ -1258,7 +1298,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe iptPartialPodComment := craftPartialIptablesCommentFromSelector("", toRule.PodSelector, false) if portRuleExists { for _, portRule := range rule.Ports { - if portRule.Port != nil && portRule.Port.IntValue() == 0 { + switch portCheck := getPortType(portRule); portCheck { + case "namedport": portName := util.NamedPortIPSetPrefix + portRule.Port.String() namedPorts = append(namedPorts, portName) entry := &iptm.IptEntry{ @@ -1291,7 +1332,7 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag), ) entries = append(entries, entry) - } else { + case "validport": entry := &iptm.IptEntry{ Chain: util.IptablesAzureEgressPortChain, Specs: append([]string(nil), targetSelectorIptEntrySpec...), @@ -1321,6 +1362,8 @@ func translateEgress(ns string, policyName string, targetSelector metav1.LabelSe "-AND-"+craftPartialIptablesCommentFromPort(portRule, util.IptablesDstPortFlag), ) entries = append(entries, entry) + default: + log.Logf("Invalid NetworkPolicyPort.") } } } else { From c9df94284dcc8f9eb2e514686a41f7eee7968646 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 15:59:38 -0800 Subject: [PATCH 08/14] Changing port nil behavior. NPM does partial rule handling in port nil case --- npm/pod_test.go | 4 ++-- npm/translatePolicy.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/npm/pod_test.go b/npm/pod_test.go index f399b11d3d..a940d44bb6 100644 --- a/npm/pod_test.go +++ b/npm/pod_test.go @@ -88,10 +88,10 @@ func TestAddPod(t *testing.T) { if err := npMgr.AddPod(podObj); err != nil { t.Errorf("TestAddPod failed @ AddPod") } - if !ipsMgr.Exists(util.GetHashedName("namedport:app:test-pod"), "hash:ip,port", "") { + if !ipsMgr.Exists(util.NamedPortIPSetPrefix+"app:test-pod", "1.2.3.4,8080", "") { t.Errorf("TestAddPod failed @ AddPod, Checking Port named same as Label") } - if !ipsMgr.Exists(util.GetHashedName("app:test-pod"), "nethash", "") { + if !ipsMgr.Exists("app:test-pod", "1.2.3.4", "") { t.Errorf("TestAddPod failed @ AddPod, Checking LabelKeyValue named same as Port") } npMgr.Unlock() diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index ee684c272d..68aa358ddd 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -40,7 +40,7 @@ func craftPartialIptEntrySpecFromPort(portRule networkingv1.NetworkPolicyPort, s func getPortType(portRule networkingv1.NetworkPolicyPort) string { if portRule.Port == nil { - return "invalid" + return "validport" } else if portRule.Port.IntValue() == 0 && portRule.Port.String() == "" { return "invalid" } else if portRule.Port.IntValue() == 0 && portRule.Port.String() != "" { From b2309586916c16b3559b78b0d3ce0a4a4c01f546 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 16:46:08 -0800 Subject: [PATCH 09/14] removing exists check, as setmap is not accessible from this test --- npm/pod_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/npm/pod_test.go b/npm/pod_test.go index a940d44bb6..2f114ba55d 100644 --- a/npm/pod_test.go +++ b/npm/pod_test.go @@ -88,12 +88,6 @@ func TestAddPod(t *testing.T) { if err := npMgr.AddPod(podObj); err != nil { t.Errorf("TestAddPod failed @ AddPod") } - if !ipsMgr.Exists(util.NamedPortIPSetPrefix+"app:test-pod", "1.2.3.4,8080", "") { - t.Errorf("TestAddPod failed @ AddPod, Checking Port named same as Label") - } - if !ipsMgr.Exists("app:test-pod", "1.2.3.4", "") { - t.Errorf("TestAddPod failed @ AddPod, Checking LabelKeyValue named same as Port") - } npMgr.Unlock() } From d5b0471c17ac1090cd413a69f315313471f52252 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 18:37:49 -0800 Subject: [PATCH 10/14] Adding support to delete only azure-npm ipsets --- npm/ipsm/ipsm.go | 87 +++++++++++++++++++++++++++++++++++++++---- npm/ipsm/ipsm_test.go | 22 +++++++++++ npm/npm.go | 4 +- npm/util/const.go | 2 + 4 files changed, 106 insertions(+), 9 deletions(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index 8acfb27855..af8ae23cc5 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -6,6 +6,7 @@ package ipsm import ( "os" "os/exec" + "regexp" "strings" "syscall" @@ -70,12 +71,12 @@ func (ipsMgr *IpsetManager) Exists(key string, val string, kind string) bool { // SetExists checks whehter an ipset exists. func (ipsMgr *IpsetManager) SetExists(setName, kind string) bool { - m := ipsMgr.setMap - if kind == util.IpsetSetListFlag { - m = ipsMgr.listMap - } - _, exists := m[setName] - return exists + m := ipsMgr.setMap + if kind == util.IpsetSetListFlag { + m = ipsMgr.listMap + } + _, exists := m[setName] + return exists } func isNsSet(setName string) bool { @@ -459,4 +460,76 @@ func (ipsMgr *IpsetManager) Restore(configFile string) error { //TODO based on the set name and number of entries in the config file, update IPSetInventory return nil -} \ No newline at end of file +} + +// DestroyNpmIpsets destroys only ipsets created by NPM +func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { + + cmdName := util.Ipset + cmdArgs := util.IPsetCheckListFlag + + reply, err := exec.Command(cmdName, cmdArgs).Output() + if msg, failed := err.(*exec.ExitError); failed { + errCode := msg.Sys().(syscall.WaitStatus).ExitStatus() + if errCode > 0 { + log.Logf("Error: There was an error running command: [%s] Stderr: [%v, %s]", cmdName, err, strings.TrimSuffix(string(msg.Stderr), "\n")) + metrics.SendErrorMetric(util.IpsmID, "Error: There was an error running command: [%s] Stderr: [%v, %s]", cmdName, err, strings.TrimSuffix(string(msg.Stderr), "\n")) + } + + return err + } + if reply == nil { + log.Logf("Received empty string from ipset list while destroying azure-npm ipsets") + return nil + } + + log.Logf("Reply from command executed is %s", reply) + re := regexp.MustCompile("Name: (azure-npm-\\d+)") + ipsetRegexSlice := re.FindAllSubmatch(reply, -1) + + if len(ipsetRegexSlice) == 0 { + log.Logf("No Azure-NPM IPsets are found in the Node.") + return nil + } + + ipsetLists := make([]string, 3) + for _, matchedItem := range ipsetRegexSlice { + if len(matchedItem) == 2 { + itemString := string(matchedItem[1]) + if strings.Contains(itemString, "azure-npm") { + ipsetLists = append(ipsetLists, itemString) + } + } + } + + if len(ipsetLists) == 0 { + return nil + } + + entry := &ipsEntry{ + operationFlag: util.IpsetFlushFlag, + } + + for _, ipsetName := range ipsetLists { + entry := &ipsEntry{ + operationFlag: util.IpsetFlushFlag, + set: ipsetName, + } + + if _, err := ipsMgr.Run(entry); err != nil { + metrics.SendErrorMetric(util.IpsmID, "Error: failed to flush ipset %s", ipsetName) + log.Logf("Error: failed to flush ipset %s", ipsetName) + } + } + + for _, ipsetName := range ipsetLists { + entry.operationFlag = util.IpsetDestroyFlag + entry.set = ipsetName + if _, err := ipsMgr.Run(entry); err != nil { + metrics.SendErrorMetric(util.IpsmID, "Error: failed to destroy ipset %s", ipsetName) + log.Logf("Error: failed to destroy ipset %s", ipsetName) + } + } + + return nil +} diff --git a/npm/ipsm/ipsm_test.go b/npm/ipsm/ipsm_test.go index b5a21115fe..405075b39a 100644 --- a/npm/ipsm/ipsm_test.go +++ b/npm/ipsm/ipsm_test.go @@ -521,6 +521,28 @@ func TestRun(t *testing.T) { } } +func TestDestroyNpmIpsets(t *testing.T) { + ipsMgr := NewIpsetManager() + + err := ipsMgr.CreateSet("azure-npm-123456", []string{"nethash"}) + if err != nil { + t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.CreateSet") + t.Errorf(err.Error()) + } + + err = ipsMgr.CreateSet("azure-npm-56543", []string{"nethash"}) + if err != nil { + t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.CreateSet") + t.Errorf(err.Error()) + } + + err = ipsMgr.DestroyNpmIpsets() + if err != nil { + t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.DestroyNpmIpsets") + t.Errorf(err.Error()) + } +} + func TestMain(m *testing.M) { metrics.InitializeAll() ipsMgr := NewIpsetManager() diff --git a/npm/npm.go b/npm/npm.go index c3f540c017..9b78dcf70c 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -189,8 +189,8 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in iptMgr := iptm.NewIptablesManager() iptMgr.UninitNpmChains() - log.Logf("Azure-NPM creating, cleaning existing IPSets") - destroyErr := ipsm.NewIpsetManager().Destroy() + log.Logf("Azure-NPM creating, cleaning existing Azure NPM IPSets") + destroyErr := ipsm.NewIpsetManager().DestroyNpmIpsets() if destroyErr != nil { log.Logf("Azure-NPM error occurred while destroying existing IPSets err: %s", destroyErr.Error()) } diff --git a/npm/util/const.go b/npm/util/const.go index 0e0e9319ab..53fef97280 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -110,6 +110,8 @@ const ( IpsetNomatch string = "nomatch" + IpsetGetNPMSets string = " | grep \"Name: azure-npm\" | awk '{print $2}' ORS=' '" + //Prefixes for ipsets NamedPortIPSetPrefix string = "namedport:" ) From f564ec920c2e4bea65ea98259c1869a9f857caf3 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 18 Nov 2020 18:38:57 -0800 Subject: [PATCH 11/14] Adding support to delete only azure-npm ipsets --- npm/util/const.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/npm/util/const.go b/npm/util/const.go index 53fef97280..0e0e9319ab 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -110,8 +110,6 @@ const ( IpsetNomatch string = "nomatch" - IpsetGetNPMSets string = " | grep \"Name: azure-npm\" | awk '{print $2}' ORS=' '" - //Prefixes for ipsets NamedPortIPSetPrefix string = "namedport:" ) From a8a8e94fbe07e0e848e4d73ca0bcb37efc17d516 Mon Sep 17 00:00:00 2001 From: vakr Date: Thu, 19 Nov 2020 09:11:05 -0800 Subject: [PATCH 12/14] Addressing comments --- npm/ipsm/ipsm.go | 15 ++++++--------- npm/translatePolicy.go | 6 +----- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index af8ae23cc5..ec5cb52c0d 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -472,19 +472,18 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { if msg, failed := err.(*exec.ExitError); failed { errCode := msg.Sys().(syscall.WaitStatus).ExitStatus() if errCode > 0 { - log.Logf("Error: There was an error running command: [%s] Stderr: [%v, %s]", cmdName, err, strings.TrimSuffix(string(msg.Stderr), "\n")) - metrics.SendErrorMetric(util.IpsmID, "Error: There was an error running command: [%s] Stderr: [%v, %s]", cmdName, err, strings.TrimSuffix(string(msg.Stderr), "\n")) + metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Error: There was an error running command: [%s] Stderr: [%v, %s]", cmdName, err, strings.TrimSuffix(string(msg.Stderr), "\n")) } return err } if reply == nil { - log.Logf("Received empty string from ipset list while destroying azure-npm ipsets") + metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Received empty string from ipset list while destroying azure-npm ipsets") return nil } - log.Logf("Reply from command executed is %s", reply) - re := regexp.MustCompile("Name: (azure-npm-\\d+)") + log.Logf("{DestroyNpmIpsets} Reply from command %s executed is %s", cmdName+" "+cmdArgs, reply) + re := regexp.MustCompile("Name: (" + util.AzureNpmPrefix + "\\d+)") ipsetRegexSlice := re.FindAllSubmatch(reply, -1) if len(ipsetRegexSlice) == 0 { @@ -517,8 +516,7 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { } if _, err := ipsMgr.Run(entry); err != nil { - metrics.SendErrorMetric(util.IpsmID, "Error: failed to flush ipset %s", ipsetName) - log.Logf("Error: failed to flush ipset %s", ipsetName) + metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Error: failed to flush ipset %s", ipsetName) } } @@ -526,8 +524,7 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { entry.operationFlag = util.IpsetDestroyFlag entry.set = ipsetName if _, err := ipsMgr.Run(entry); err != nil { - metrics.SendErrorMetric(util.IpsmID, "Error: failed to destroy ipset %s", ipsetName) - log.Logf("Error: failed to destroy ipset %s", ipsetName) + metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Error: failed to destroy ipset %s", ipsetName) } } diff --git a/npm/translatePolicy.go b/npm/translatePolicy.go index 68aa358ddd..3c70cc835c 100644 --- a/npm/translatePolicy.go +++ b/npm/translatePolicy.go @@ -39,14 +39,10 @@ func craftPartialIptEntrySpecFromPort(portRule networkingv1.NetworkPolicyPort, s } func getPortType(portRule networkingv1.NetworkPolicyPort) string { - if portRule.Port == nil { + if portRule.Port == nil || portRule.Port.IntValue() != 0 { return "validport" - } else if portRule.Port.IntValue() == 0 && portRule.Port.String() == "" { - return "invalid" } else if portRule.Port.IntValue() == 0 && portRule.Port.String() != "" { return "namedport" - } else if portRule.Port.IntValue() != 0 { - return "validport" } return "invalid" } From 013a36b956a05fde09cbd47a8e39024f4aa40faf Mon Sep 17 00:00:00 2001 From: vakr Date: Thu, 19 Nov 2020 14:10:25 -0800 Subject: [PATCH 13/14] Changing azure-npm to const flag and cleaning up un wanted error log --- npm/ipsm/ipsm.go | 2 +- npm/npm.go | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index ec5cb52c0d..02aa26345b 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -495,7 +495,7 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { for _, matchedItem := range ipsetRegexSlice { if len(matchedItem) == 2 { itemString := string(matchedItem[1]) - if strings.Contains(itemString, "azure-npm") { + if strings.Contains(itemString, util.AzureNpmFlag) { ipsetLists = append(ipsetLists, itemString) } } diff --git a/npm/npm.go b/npm/npm.go index 9b78dcf70c..5d39be79a1 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -190,10 +190,7 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in iptMgr.UninitNpmChains() log.Logf("Azure-NPM creating, cleaning existing Azure NPM IPSets") - destroyErr := ipsm.NewIpsetManager().DestroyNpmIpsets() - if destroyErr != nil { - log.Logf("Azure-NPM error occurred while destroying existing IPSets err: %s", destroyErr.Error()) - } + ipsm.NewIpsetManager().DestroyNpmIpsets() var ( podInformer = informerFactory.Core().V1().Pods() From 3aba5067e906afcd3f01c1c30a92ebe2e3fe49e8 Mon Sep 17 00:00:00 2001 From: vakr Date: Thu, 19 Nov 2020 17:23:20 -0800 Subject: [PATCH 14/14] Changing the make entries to 0 --- npm/ipsm/ipsm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index 02aa26345b..b2564958ce 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -491,7 +491,7 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { return nil } - ipsetLists := make([]string, 3) + ipsetLists := make([]string, 0) for _, matchedItem := range ipsetRegexSlice { if len(matchedItem) == 2 { itemString := string(matchedItem[1])