From df7b2e4f85e977e0f603b65146a59fa2381b5d52 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Tue, 24 May 2022 18:11:50 -0500 Subject: [PATCH 1/8] updated validation method and added unit tests --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 7 ++- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 52 +++++++++++++++++++ npm/util/util.go | 8 +-- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index a6a3f473ba..e2d93a0044 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -555,9 +555,8 @@ func validateIPSetMemberIP(ip string) bool { // always guaranteed to have ip, not guaranteed to have port + protocol ipDetails := strings.Split(ip, ",") if util.IsIPV4(ipDetails[0]) { - return true + err := ValidateIPBlock(ipDetails[0]) + return err == nil } - - err := ValidateIPBlock(ip) - return err == nil + return false } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 51d35a86fc..e43d9c1c6c 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -1509,3 +1509,55 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { require.Equal(t, expectedNumEntries, numEntries, "numEntries mismatch for set %s", set.Name) } } + +func TestValidateIPSetMemberIP(t *testing.T) { + tests := []struct { + name string + ipblock string + want bool + }{ + { + name: "cidr", + ipblock: "172.17.0.0/16", + want: true, + }, + { + name: "except ipblock", + ipblock: "172.17.1.0/24 nomatch", + want: true, + }, + { + name: "incorrect ip format", + ipblock: "1234", + want: false, + }, + { + name: "incorrect ip range", + ipblock: "256.1.2.3", + want: false, + }, + { + name: "empty cidr", + ipblock: "", + want: false, + }, + { + name: "ipv6", + ipblock: "2345:0425:2CA1:0000:0000:0567:5673:23b5/24", + want: false, + }, + { + name: "tcp", + ipblock: "192.168.0.0/24,tcp:25227", + want: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got := validateIPSetMemberIP(tt.ipblock) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/npm/util/util.go b/npm/util/util.go index ce95c31acc..6e1c09ba53 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -355,10 +355,6 @@ func SliceToString(list []string) string { } func IsIPV4(ip string) bool { - if net.ParseIP(ip).To4() != nil { - return true - } - - _, _, err := net.ParseCIDR(ip) - return err == nil + ipOnly := strings.Split(ip, "/") + return net.ParseIP(ipOnly[0]).To4() != nil } From c181aebb2f31ffd81fba83656026e4bba79e9fed Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Wed, 25 May 2022 13:20:56 -0500 Subject: [PATCH 2/8] moved unit test --- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 103 +++++++++--------- 1 file changed, 51 insertions(+), 52 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index e43d9c1c6c..ea8486f897 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -1402,6 +1402,57 @@ func TestValidateIPBlock(t *testing.T) { }) } } +func TestValidateIPSetMemberIP(t *testing.T) { + tests := []struct { + name string + ipblock string + want bool + }{ + { + name: "cidr", + ipblock: "172.17.0.0/16", + want: true, + }, + { + name: "except ipblock", + ipblock: "172.17.1.0/24 nomatch", + want: true, + }, + { + name: "incorrect ip format", + ipblock: "1234", + want: false, + }, + { + name: "incorrect ip range", + ipblock: "256.1.2.3", + want: false, + }, + { + name: "empty cidr", + ipblock: "", + want: false, + }, + { + name: "ipv6", + ipblock: "2345:0425:2CA1:0000:0000:0567:5673:23b5/24", + want: false, + }, + { + name: "tcp", + ipblock: "192.168.0.0/24,tcp:25227", + want: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got := validateIPSetMemberIP(tt.ipblock) + require.Equal(t, tt.want, got) + }) + } +} func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { // 1. assert cache contents @@ -1509,55 +1560,3 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { require.Equal(t, expectedNumEntries, numEntries, "numEntries mismatch for set %s", set.Name) } } - -func TestValidateIPSetMemberIP(t *testing.T) { - tests := []struct { - name string - ipblock string - want bool - }{ - { - name: "cidr", - ipblock: "172.17.0.0/16", - want: true, - }, - { - name: "except ipblock", - ipblock: "172.17.1.0/24 nomatch", - want: true, - }, - { - name: "incorrect ip format", - ipblock: "1234", - want: false, - }, - { - name: "incorrect ip range", - ipblock: "256.1.2.3", - want: false, - }, - { - name: "empty cidr", - ipblock: "", - want: false, - }, - { - name: "ipv6", - ipblock: "2345:0425:2CA1:0000:0000:0567:5673:23b5/24", - want: false, - }, - { - name: "tcp", - ipblock: "192.168.0.0/24,tcp:25227", - want: true, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - got := validateIPSetMemberIP(tt.ipblock) - require.Equal(t, tt.want, got) - }) - } -} From 5e2d6713c2a1b89bc1bc5c3bf04ea88e0b63f868 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Thu, 26 May 2022 14:04:13 -0500 Subject: [PATCH 3/8] updated isIPV4 to use netip --- npm/util/util.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/npm/util/util.go b/npm/util/util.go index 6e1c09ba53..ae43c6eb7f 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -5,7 +5,7 @@ package util import ( "fmt" "hash/fnv" - "net" + "net/netip" "os" "regexp" "runtime" @@ -356,5 +356,6 @@ func SliceToString(list []string) string { func IsIPV4(ip string) bool { ipOnly := strings.Split(ip, "/") - return net.ParseIP(ipOnly[0]).To4() != nil + address, err := netip.ParseAddr(ipOnly[0]) + return (err != nil || address.Is4()) } From 63b91d31f5fa02a669898cf4ca575ee3aa85b4e6 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Thu, 26 May 2022 15:36:11 -0500 Subject: [PATCH 4/8] fixed bool issue --- npm/util/util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/util/util.go b/npm/util/util.go index ae43c6eb7f..21379eea3d 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -357,5 +357,5 @@ func SliceToString(list []string) string { func IsIPV4(ip string) bool { ipOnly := strings.Split(ip, "/") address, err := netip.ParseAddr(ipOnly[0]) - return (err != nil || address.Is4()) + return (err == nil && address.Is4()) } From a97b126b0eb29d9dc2146aa7e15e264d50ac416d Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Fri, 27 May 2022 16:19:58 -0500 Subject: [PATCH 5/8] fixed ipblock issue --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 10 ++++++++-- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 10 ++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index e2d93a0044..800fe3961f 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -554,9 +554,15 @@ func validateIPSetMemberIP(ip string) bool { // 192.168.0.0/24 nomatch // always guaranteed to have ip, not guaranteed to have port + protocol ipDetails := strings.Split(ip, ",") + isIPBlock := strings.Contains(ipDetails[0], "/") + if util.IsIPV4(ipDetails[0]) { - err := ValidateIPBlock(ipDetails[0]) - return err == nil + + if isIPBlock { + err := ValidateIPBlock(ipDetails[0]) + return err == nil + } + return true } return false } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index ea8486f897..fb960ac72a 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -1443,6 +1443,16 @@ func TestValidateIPSetMemberIP(t *testing.T) { ipblock: "192.168.0.0/24,tcp:25227", want: true, }, + { + name: "valid ip no cidr", + ipblock: "10.0.0.0", + want: true, + }, + { + name: "invalid cidr", + ipblock: "10.0.0.1/33", + want: false, + }, } for _, tt := range tests { From 4f5bd464385368003d70597a97767cd5aa4250cd Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Thu, 9 Jun 2022 14:34:28 -0500 Subject: [PATCH 6/8] refactored code --- npm/pkg/dataplane/ipsets/ipset.go | 15 ----- npm/pkg/dataplane/ipsets/ipsetmanager.go | 12 +--- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 65 ++++++------------- npm/util/util.go | 8 +++ 4 files changed, 30 insertions(+), 70 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipset.go b/npm/pkg/dataplane/ipsets/ipset.go index 9b860bf225..a17c3f7902 100644 --- a/npm/pkg/dataplane/ipsets/ipset.go +++ b/npm/pkg/dataplane/ipsets/ipset.go @@ -3,12 +3,9 @@ package ipsets import ( "errors" "fmt" - "net" - "strings" "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/util" - npmerrors "github.com/Azure/azure-container-networking/npm/util/errors" ) type IPSetMetadata struct { @@ -397,18 +394,6 @@ func (set *IPSet) canSetBeSelectorIPSet() bool { set.Type == NestedLabelOfPod) } -// TODO: This is an adhoc approach for linux, but need to refactor data structure for better management. -func ValidateIPBlock(ipblock string) error { - // TODO: This is fragile code with strong dependency with " "(space). - // onlyCidr has only cidr without "space" and "nomatch" in case except ipblock to validate cidr format. - onlyCidr := strings.Split(ipblock, " ")[0] - _, _, err := net.ParseCIDR(onlyCidr) - if err != nil { - return npmerrors.SimpleErrorWrapper("failed to parse CIDR", err) - } - return nil -} - func GetMembersOfTranslatedSets(members []string) []*IPSetMetadata { memberList := make([]*IPSetMetadata, len(members)) i := 0 diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 800fe3961f..6ebfaf0178 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -554,15 +554,7 @@ func validateIPSetMemberIP(ip string) bool { // 192.168.0.0/24 nomatch // always guaranteed to have ip, not guaranteed to have port + protocol ipDetails := strings.Split(ip, ",") - isIPBlock := strings.Contains(ipDetails[0], "/") + ipField := strings.Split(ipDetails[0], " ") - if util.IsIPV4(ipDetails[0]) { - - if isIPBlock { - err := ValidateIPBlock(ipDetails[0]) - return err == nil - } - return true - } - return false + return util.IsIPV4(ipField[0]) } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index fb960ac72a..921c077d0e 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -1357,51 +1357,6 @@ func TestMain(m *testing.M) { os.Exit(exitCode) } -func TestValidateIPBlock(t *testing.T) { - tests := []struct { - name string - ipblock string - wantErr bool - }{ - { - name: "cidr", - ipblock: "172.17.0.0/16", - wantErr: false, - }, - { - name: "except ipblock", - ipblock: "172.17.1.0/24 nomatch", - wantErr: false, - }, - { - name: "incorrect ip format", - ipblock: "1234", - wantErr: true, - }, - { - name: "incorrect ip range", - ipblock: "256.1.2.3", - wantErr: true, - }, - { - name: "empty cidr", - ipblock: "", - wantErr: true, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - err := ValidateIPBlock(tt.ipblock) - if tt.wantErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } - }) - } -} func TestValidateIPSetMemberIP(t *testing.T) { tests := []struct { name string @@ -1453,6 +1408,26 @@ func TestValidateIPSetMemberIP(t *testing.T) { ipblock: "10.0.0.1/33", want: false, }, + { + name: "valid ip nomatch", + ipblock: "192.168.0.1 nomatch", + want: true, + }, + { + name: "valid ip tcp", + ipblock: "192.168.0.1,tcp:25227", + want: true, + }, + { + name: "ipv6 tcp", + ipblock: "2345:0425:2CA1:0000:0000:0567:5673:23b5/24,tcp:25227", + want: false, + }, + { + name: "ipv6 nomatch", + ipblock: "2345:0425:2CA1:0000:0000:0567:5673:23b5 nomatch", + want: false, + }, } for _, tt := range tests { diff --git a/npm/util/util.go b/npm/util/util.go index 21379eea3d..90efbd4447 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -5,6 +5,7 @@ package util import ( "fmt" "hash/fnv" + "net" "net/netip" "os" "regexp" @@ -355,7 +356,14 @@ func SliceToString(list []string) string { } func IsIPV4(ip string) bool { + isIPBlock := strings.Contains(ip, "/") ipOnly := strings.Split(ip, "/") address, err := netip.ParseAddr(ipOnly[0]) + + if address.Is4() && isIPBlock { + _, _, err = net.ParseCIDR(ip) + return (err == nil && address.Is4()) + } + return (err == nil && address.Is4()) } From a488e4725003f7afe47d3075f3360d638d150619 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Fri, 10 Jun 2022 14:44:31 -0500 Subject: [PATCH 7/8] updated error handling --- npm/util/util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/npm/util/util.go b/npm/util/util.go index 90efbd4447..13c52b9e9b 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -361,8 +361,8 @@ func IsIPV4(ip string) bool { address, err := netip.ParseAddr(ipOnly[0]) if address.Is4() && isIPBlock { - _, _, err = net.ParseCIDR(ip) - return (err == nil && address.Is4()) + _, _, err2 := net.ParseCIDR(ip) + return (err == nil && err2 == nil && address.Is4()) } return (err == nil && address.Is4()) From b76f4eb652d302d29ef7aa90764b4b5613b53ea3 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Fri, 10 Jun 2022 17:16:10 -0500 Subject: [PATCH 8/8] updated error handling --- npm/util/util.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/npm/util/util.go b/npm/util/util.go index 13c52b9e9b..0df8a649aa 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -359,11 +359,14 @@ func IsIPV4(ip string) bool { isIPBlock := strings.Contains(ip, "/") ipOnly := strings.Split(ip, "/") address, err := netip.ParseAddr(ipOnly[0]) + if err != nil { + return false + } if address.Is4() && isIPBlock { - _, _, err2 := net.ParseCIDR(ip) - return (err == nil && err2 == nil && address.Is4()) + _, _, err := net.ParseCIDR(ip) + return err == nil } - return (err == nil && address.Is4()) + return address.Is4() }