From 9c0868147c5594cb3f9b388bd70908f4271461ee Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Fri, 30 Apr 2021 14:05:31 -0700 Subject: [PATCH 1/4] Filter pod which uses IPv6 and incoorect IPv4 form --- npm/podController.go | 19 +++++++++++++++- npm/podController_test.go | 47 +++++++++++++++++++++++++++++++-------- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/npm/podController.go b/npm/podController.go index f931c0ac8f..de711efe1f 100644 --- a/npm/podController.go +++ b/npm/podController.go @@ -4,7 +4,9 @@ package npm import ( "fmt" + "net" "reflect" + "strings" "time" "github.com/Azure/azure-container-networking/npm/ipsm" @@ -599,8 +601,23 @@ func isCompletePod(podObj *corev1.Pod) bool { return false } +// hasValidPodIP returns true when pod has a valid IPv4 form since currently NPM does not support IPv6. +// (TODO): will need to update this function when NPM wants to support IPv4 with dual stack configuration in K8s func hasValidPodIP(podObj *corev1.Pod) bool { - return len(podObj.Status.PodIP) > 0 + + // First filter IPv6 address. It also filters IPv4-mapped IPv6 ("::ffff:192.0.2.1") form, but not sure this form can happen in k8s + if strings.Contains(podObj.Status.PodIP, ":") { + utilruntime.HandleError(fmt.Errorf("IPv6 %s may be assigned to Pod. NPM does not support IPv6 yet", podObj.Status.PodIP)) + return false + } + + // Check a correct IPv4 form. + ip := net.ParseIP(podObj.Status.PodIP) + if ip == nil { + return false + } + + return true } func isHostNetworkPod(podObj *corev1.Pod) bool { diff --git a/npm/podController_test.go b/npm/podController_test.go index 8543959896..9171789083 100644 --- a/npm/podController_test.go +++ b/npm/podController_test.go @@ -269,12 +269,31 @@ func TestAddHostNetworkPod(t *testing.T) { {0, 1, 0}, } checkPodTestResult("TestAddHostNetworkPod", f, testCases) - if _, exists := f.npMgr.PodMap[podKey]; exists { t.Error("TestAddHostNetworkPod failed @ cached pod obj exists check") } } +func TestAddPodWithIPv6(t *testing.T) { + labels := map[string]string{ + "app": "test-pod", + } + + podObj := createPod("test-pod", "test-namespace", "0", "fc00:f853:ccd:e793::6", labels, NonHostNetwork, corev1.PodRunning) + f := newFixture(t) + f.podLister = append(f.podLister, podObj) + f.kubeobjects = append(f.kubeobjects, podObj) + stopCh := make(chan struct{}) + defer close(stopCh) + f.newPodController(stopCh) + + addPod(t, f, podObj) + testCases := []expectedValues{ + {0, 1, 0}, + } + checkPodTestResult("TestAddPodWithIPv6", f, testCases) +} + func TestDeletePod(t *testing.T) { labels := map[string]string{ "app": "test-pod", @@ -456,13 +475,23 @@ func TestPodStatusUpdatePod(t *testing.T) { } func TestHasValidPodIP(t *testing.T) { - podObj := &corev1.Pod{ - Status: corev1.PodStatus{ - Phase: "Running", - PodIP: "1.2.3.4", - }, - } - if ok := hasValidPodIP(podObj); !ok { - t.Errorf("TestisValidPod failed @ isValidPod") + testCases := map[string]bool{ + "1.2.3.4": true, + "172.19.0.6": true, + "": false, + "1234": false, + "256.1.2.3": false, + "fc00:f853:ccd:e793::6": false, + "::ffff:192.0.2.1": false, + ":::::": false, + } + + podObj := &corev1.Pod{} + for ip, want := range testCases { + podObj.Status.PodIP = ip + + if got := hasValidPodIP(podObj); got != want { + t.Errorf("TestHasValidPodIP failed @ isValidPod got %v, want %v", got, want) + } } } From ef058297871631ed1fd54e500729472200ac215a Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Mon, 3 May 2021 09:06:45 -0700 Subject: [PATCH 2/4] correct comments and log message --- npm/podController.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/npm/podController.go b/npm/podController.go index de711efe1f..7a8ebc2cff 100644 --- a/npm/podController.go +++ b/npm/podController.go @@ -602,12 +602,12 @@ func isCompletePod(podObj *corev1.Pod) bool { } // hasValidPodIP returns true when pod has a valid IPv4 form since currently NPM does not support IPv6. -// (TODO): will need to update this function when NPM wants to support IPv4 with dual stack configuration in K8s +// (TODO): will need to update this function when NPM wants to support IPV6 and dual stack configuration in K8s func hasValidPodIP(podObj *corev1.Pod) bool { // First filter IPv6 address. It also filters IPv4-mapped IPv6 ("::ffff:192.0.2.1") form, but not sure this form can happen in k8s if strings.Contains(podObj.Status.PodIP, ":") { - utilruntime.HandleError(fmt.Errorf("IPv6 %s may be assigned to Pod. NPM does not support IPv6 yet", podObj.Status.PodIP)) + utilruntime.HandleError(fmt.Errorf("IPv6 %s may be assigned to Pod, but NPM does not support IPv6 yet", podObj.Status.PodIP)) return false } From faa468bd7577d94b2794ccf790f84ef6cc4fed34 Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Wed, 28 Jul 2021 11:31:00 -0700 Subject: [PATCH 3/4] Make codes short and add comments for todo list related to this PR --- npm/ipsm/ipsm.go | 5 ++++- npm/podController.go | 7 +------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index da7412fee5..eca10d3d2b 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -349,7 +349,10 @@ func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podKey string) error { spec: resultSpec, } - // todo: check err handling besides error code, corrupt state possible here + // TODO(jungukcho): revisit all error code handling codes in ipsm.go and iptm.go. + // For example, in below code, ipsm caches the item even though invalid IPv4 or IPv6 address are inserted in case returned error code is 1. + // Further it causes controllers to process the rest of operations. + // May use error handler code in npm/util/errors/errors.go when revisiting error handling codes. if errCode, err := ipsMgr.Run(entry); err != nil && errCode != 1 { metrics.SendErrorLogAndMetric(util.IpsmID, "Error: failed to create ipset rules. %+v", entry) return err diff --git a/npm/podController.go b/npm/podController.go index 7a8ebc2cff..5b3831c475 100644 --- a/npm/podController.go +++ b/npm/podController.go @@ -604,7 +604,6 @@ func isCompletePod(podObj *corev1.Pod) bool { // hasValidPodIP returns true when pod has a valid IPv4 form since currently NPM does not support IPv6. // (TODO): will need to update this function when NPM wants to support IPV6 and dual stack configuration in K8s func hasValidPodIP(podObj *corev1.Pod) bool { - // First filter IPv6 address. It also filters IPv4-mapped IPv6 ("::ffff:192.0.2.1") form, but not sure this form can happen in k8s if strings.Contains(podObj.Status.PodIP, ":") { utilruntime.HandleError(fmt.Errorf("IPv6 %s may be assigned to Pod, but NPM does not support IPv6 yet", podObj.Status.PodIP)) @@ -613,11 +612,7 @@ func hasValidPodIP(podObj *corev1.Pod) bool { // Check a correct IPv4 form. ip := net.ParseIP(podObj.Status.PodIP) - if ip == nil { - return false - } - - return true + return ip != nil } func isHostNetworkPod(podObj *corev1.Pod) bool { From b7466af159aa93858972ce9e94b6f842f99d479f Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Wed, 28 Jul 2021 11:45:35 -0700 Subject: [PATCH 4/4] Better comments and resolve line length linter(lll) error --- npm/ipsm/ipsm.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index eca10d3d2b..97d5d97aea 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -350,7 +350,8 @@ func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podKey string) error { } // TODO(jungukcho): revisit all error code handling codes in ipsm.go and iptm.go. - // For example, in below code, ipsm caches the item even though invalid IPv4 or IPv6 address are inserted in case returned error code is 1. + // For example, in below code, ipsm caches the item + // even though invalid IPv4 or IPv6 address are inserted if returned error code is 1. // Further it causes controllers to process the rest of operations. // May use error handler code in npm/util/errors/errors.go when revisiting error handling codes. if errCode, err := ipsMgr.Run(entry); err != nil && errCode != 1 {