From 9e9e264964b73ebeb3871bc951ee9700a1cc65ce Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 18 Dec 2017 14:51:34 +0800 Subject: [PATCH 1/7] refactor ipset interface AddEntry() --- pkg/proxy/ipvs/ipset.go | 2 +- pkg/util/ipset/ipset.go | 11 ++++++----- pkg/util/ipset/ipset_test.go | 10 +++++++--- pkg/util/ipset/testing/fake.go | 6 +++--- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/pkg/proxy/ipvs/ipset.go b/pkg/proxy/ipvs/ipset.go index d61992125ae..4cba7ff4f53 100644 --- a/pkg/proxy/ipvs/ipset.go +++ b/pkg/proxy/ipvs/ipset.go @@ -123,7 +123,7 @@ func (set *IPSet) syncIPSetEntries() { } // Create active entries for _, entry := range set.activeEntries.Difference(currentIPSetEntries).List() { - if err := set.handle.AddEntry(entry, set.Name, true); err != nil { + if err := set.handle.AddEntry(entry, &set.IPSet, true); err != nil { glog.Errorf("Failed to add entry: %v to ip set: %s, error: %v", entry, set.Name, err) } else { glog.V(3).Infof("Successfully add entry: %v to ip set: %s", entry, set.Name) diff --git a/pkg/util/ipset/ipset.go b/pkg/util/ipset/ipset.go index 7ae28050097..608bba920ce 100644 --- a/pkg/util/ipset/ipset.go +++ b/pkg/util/ipset/ipset.go @@ -34,10 +34,10 @@ type Interface interface { DestroySet(set string) error // DestroyAllSets deletes all sets. DestroyAllSets() error - // CreateSet creates a new set, it will ignore error when the set already exists if ignoreExistErr=true. + // CreateSet creates a new set. It will ignore error when the set already exists if ignoreExistErr=true. CreateSet(set *IPSet, ignoreExistErr bool) error - // AddEntry adds a new entry to the named set. - AddEntry(entry string, set string, ignoreExistErr bool) error + // AddEntry adds a new entry to the named set. It will ignore error when the entry already exists if ignoreExistErr=true. + AddEntry(entry string, set *IPSet, ignoreExistErr bool) error // DelEntry deletes one entry from the named set DelEntry(entry string, set string) error // Test test if an entry exists in the named set @@ -48,6 +48,7 @@ type Interface interface { ListSets() ([]string, error) // GetVersion returns the "X.Y" version string for ipset. GetVersion() (string, error) + // TODO: add comment message for ipset } // IPSetCmd represents the ipset util. We use ipset command for ipset execute. @@ -198,8 +199,8 @@ func (runner *runner) createSet(set *IPSet, ignoreExistErr bool) error { // AddEntry adds a new entry to the named set. // If the -exist option is specified, ipset ignores the error otherwise raised when // the same set (setname and create parameters are identical) already exists. -func (runner *runner) AddEntry(entry string, set string, ignoreExistErr bool) error { - args := []string{"add", set, entry} +func (runner *runner) AddEntry(entry string, set *IPSet, ignoreExistErr bool) error { + args := []string{"add", set.Name, entry} if ignoreExistErr { args = append(args, "-exist") } diff --git a/pkg/util/ipset/ipset_test.go b/pkg/util/ipset/ipset_test.go index 71d4ce26d58..e740a1ab55e 100644 --- a/pkg/util/ipset/ipset_test.go +++ b/pkg/util/ipset/ipset_test.go @@ -233,6 +233,10 @@ func TestAddEntry(t *testing.T) { SetType: HashIPPort, } + testSet := &IPSet{ + Name: "FOOBAR", + } + fcmd := fakeexec.FakeCmd{ CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ // Success @@ -254,7 +258,7 @@ func TestAddEntry(t *testing.T) { } runner := New(&fexec) // Create with ignoreExistErr = false, expect success - err := runner.AddEntry(testEntry.String(), "FOOBAR", false) + err := runner.AddEntry(testEntry.String(), testSet, false) if err != nil { t.Errorf("expected success, got %v", err) } @@ -265,7 +269,7 @@ func TestAddEntry(t *testing.T) { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[0]) } // Create with ignoreExistErr = true, expect success - err = runner.AddEntry(testEntry.String(), "FOOBAR", true) + err = runner.AddEntry(testEntry.String(), testSet, true) if err != nil { t.Errorf("expected success, got %v", err) } @@ -276,7 +280,7 @@ func TestAddEntry(t *testing.T) { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) } // Create with ignoreExistErr = false, expect failure - err = runner.AddEntry(testEntry.String(), "FOOBAR", false) + err = runner.AddEntry(testEntry.String(), testSet, false) if err == nil { t.Errorf("expected failure, got nil") } diff --git a/pkg/util/ipset/testing/fake.go b/pkg/util/ipset/testing/fake.go index 2a58bdd399d..79f8f38554d 100644 --- a/pkg/util/ipset/testing/fake.go +++ b/pkg/util/ipset/testing/fake.go @@ -93,15 +93,15 @@ func (f *FakeIPSet) CreateSet(set *ipset.IPSet, ignoreExistErr bool) error { } // AddEntry is part of interface. -func (f *FakeIPSet) AddEntry(entry string, set string, ignoreExistErr bool) error { - if f.Entries[set].Has(entry) { +func (f *FakeIPSet) AddEntry(entry string, set *ipset.IPSet, ignoreExistErr bool) error { + if f.Entries[set.Name].Has(entry) { if !ignoreExistErr { // already exists return fmt.Errorf("Element cannot be added to the set: it's already added") } return nil } - f.Entries[set].Insert(entry) + f.Entries[set.Name].Insert(entry) return nil } From 4e0b4fca943c29fe21989cf2b75b7fdeed867e48 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 18 Dec 2017 14:57:37 +0800 Subject: [PATCH 2/7] validate set in ipset --- pkg/util/ipset/ipset.go | 64 +++++++++++-- pkg/util/ipset/ipset_test.go | 169 +++++++++++++++++++++++++++++++++++ 2 files changed, 228 insertions(+), 5 deletions(-) diff --git a/pkg/util/ipset/ipset.go b/pkg/util/ipset/ipset.go index 608bba920ce..e2971e7d24b 100644 --- a/pkg/util/ipset/ipset.go +++ b/pkg/util/ipset/ipset.go @@ -88,6 +88,36 @@ type IPSet struct { PortRange string } +// Validate checks if a given ipset is valid or not. +func (set *IPSet) Validate() (bool, error) { + // Check if protocol is valid for `HashIPPort`, `HashIPPortIP` and `HashIPPortNet` type set. + if set.SetType == HashIPPort || set.SetType == HashIPPortIP || set.SetType == HashIPPortNet { + if valid := validateHashFamily(set.HashFamily); !valid { + return false, fmt.Errorf("currently supported ip set hash families are: [%s, %s], %s is not supported", ProtocolFamilyIPV4, ProtocolFamilyIPV6, set.HashFamily) + } + } + // check set type + if valid := validateIPSetType(set.SetType); !valid { + return false, fmt.Errorf("currently supported ipset types are: %v, %s is not supported", ValidIPSetTypes, set.SetType) + } + // check port range for bitmap type set + if set.SetType == BitmapPort { + if valid, err := validatePortRange(set.PortRange); !valid { + return false, err + } + } + // check hash size value of ipset + if set.HashSize <= 0 { + return false, fmt.Errorf("invalid hashsize value, should be >0") + } + // check max elem value of ipset + if set.MaxElem <= 0 { + return false, fmt.Errorf("invalid maxelem value, should be >0") + } + + return true, nil +} + // Entry represents a ipset entry. type Entry struct { // IP is the entry's IP. The IP address protocol corresponds to the HashFamily of IPSet. @@ -310,17 +340,41 @@ func getIPSetVersionString(exec utilexec.Interface) (string, error) { return match[0], nil } -func validatePortRange(portRange string) bool { +// checks if port range is valid. The begin port number is not necessarily less than +// end port number - ipset util can accept it. It means both 1-100 and 100-1 are valid. +func validatePortRange(portRange string) (bool, error) { strs := strings.Split(portRange, "-") if len(strs) != 2 { - return false + return false, fmt.Errorf("port range should be in the format of `a-b`") } for i := range strs { - if _, err := strconv.Atoi(strs[i]); err != nil { - return false + num, err := strconv.Atoi(strs[i]) + if err != nil { + return false, err + } + if num < 0 { + return false, fmt.Errorf("port number %d should be >=0", num) } } - return true + return true, nil +} + +// checks if the given ipset type is valid. +func validateIPSetType(set Type) bool { + for _, valid := range ValidIPSetTypes { + if set == valid { + return true + } + } + return false +} + +// checks if given hash family is supported in ipset +func validateHashFamily(family string) bool { + if family == ProtocolFamilyIPV4 || family == ProtocolFamilyIPV6 { + return true + } + return false } // IsNotFoundError returns true if the error indicates "not found". It parses diff --git a/pkg/util/ipset/ipset_test.go b/pkg/util/ipset/ipset_test.go index e740a1ab55e..59a5712185f 100644 --- a/pkg/util/ipset/ipset_test.go +++ b/pkg/util/ipset/ipset_test.go @@ -485,3 +485,172 @@ baz` t.Errorf("expected sets: %v, got: %v", expected, list) } } + +func Test_validIPSetType(t *testing.T) { + testCases := []struct { + setType Type + valid bool + }{ + { // case[0] + setType: Type("foo"), + valid: false, + }, + { // case[1] + setType: HashIPPortNet, + valid: true, + }, + { // case[2] + setType: HashIPPort, + valid: true, + }, + { // case[3] + setType: HashIPPortIP, + valid: true, + }, + { // case[4] + setType: BitmapPort, + valid: true, + }, + { // case[5] + setType: Type(""), + valid: false, + }, + } + for i := range testCases { + valid := validateIPSetType(testCases[i].setType) + if valid != testCases[i].valid { + t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v]", i, testCases[i].valid, valid) + } + } +} + +func Test_validatePortRange(t *testing.T) { + testCases := []struct { + portRange string + valid bool + desc string + }{ + { // case[0] + portRange: "a-b", + valid: false, + desc: "invalid port number", + }, + { // case[1] + portRange: "1-2", + valid: true, + desc: "valid", + }, + { // case[2] + portRange: "90-1", + valid: true, + desc: "ipset util can accept the input of begin port number can be less than end port number", + }, + { // case[3] + portRange: DefaultPortRange, + valid: true, + desc: "default port range is valid, of course", + }, + { // case[4] + portRange: "12", + valid: false, + desc: "a single number is invalid", + }, + { // case[5] + portRange: "1-", + valid: false, + desc: "should specify end port", + }, + { // case[6] + portRange: "-100", + valid: false, + desc: "should specify begin port", + }, + { // case[7] + portRange: "1:100", + valid: false, + desc: "delimiter should be -", + }, + { // case[8] + portRange: "1~100", + valid: false, + desc: "delimiter should be -", + }, + { // case[9] + portRange: "1,100", + valid: false, + desc: "delimiter should be -", + }, + { // case[10] + portRange: "100-100", + valid: true, + desc: "begin port number can be equal to end port number", + }, + { // case[11] + portRange: "", + valid: false, + desc: "empty string is invalid", + }, + { // case[12] + portRange: "-1-12", + valid: false, + desc: "port number can not be negative value", + }, + { // case[13] + portRange: "-1--8", + valid: false, + desc: "port number can not be negative value", + }, + } + for i := range testCases { + valid, _ := validatePortRange(testCases[i].portRange) + if valid != testCases[i].valid { + t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].desc) + } + } +} + +func Test_validateFamily(t *testing.T) { + testCases := []struct { + family string + valid bool + }{ + { // case[0] + family: "foo", + valid: false, + }, + { // case[1] + family: ProtocolFamilyIPV4, + valid: true, + }, + { // case[2] + family: ProtocolFamilyIPV6, + valid: true, + }, + { // case[3] + family: "ipv4", + valid: false, + }, + { // case[4] + family: "ipv6", + valid: false, + }, + { // case[5] + family: "tcp", + valid: false, + }, + { // case[6] + family: "udp", + valid: false, + }, + { // case[7] + family: "", + valid: false, + }, + } + for i := range testCases { + valid := validateHashFamily(testCases[i].family) + if valid != testCases[i].valid { + t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v]", i, testCases[i].valid, valid) + } + } +} From e768924a62699ec0870211817628bc6866c64769 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 18 Dec 2017 15:01:32 +0800 Subject: [PATCH 3/7] validate entry in ipset --- pkg/proxy/ipvs/ipset.go | 4 + pkg/util/ipset/ipset.go | 156 ++++++++- pkg/util/ipset/ipset_test.go | 646 +++++++++++++++++++++++++++++++++++ pkg/util/ipset/types.go | 10 - 4 files changed, 792 insertions(+), 24 deletions(-) diff --git a/pkg/proxy/ipvs/ipset.go b/pkg/proxy/ipvs/ipset.go index 4cba7ff4f53..bc639b92f59 100644 --- a/pkg/proxy/ipvs/ipset.go +++ b/pkg/proxy/ipvs/ipset.go @@ -89,6 +89,10 @@ func NewIPSet(handle utilipset.Interface, name string, setType utilipset.Type, i return set } +func (set *IPSet) validateEntry(entry *utilipset.Entry) (bool, error) { + return entry.Validate(&set.IPSet) +} + func (set *IPSet) isEmpty() bool { return len(set.activeEntries.UnsortedList()) == 0 } diff --git a/pkg/util/ipset/ipset.go b/pkg/util/ipset/ipset.go index e2971e7d24b..7442135c9e7 100644 --- a/pkg/util/ipset/ipset.go +++ b/pkg/util/ipset/ipset.go @@ -19,6 +19,7 @@ package ipset import ( "bytes" "fmt" + "net" "regexp" "strconv" "strings" @@ -86,6 +87,7 @@ type IPSet struct { MaxElem int // PortRange specifies the port range of bitmap:port type ipset. PortRange string + // TODO: add comment message for ipset } // Validate checks if a given ipset is valid or not. @@ -133,10 +135,96 @@ type Entry struct { Net string // IP2 is the entry's second IP. IP2 may not be empty for `hash:ip,port,ip` type ip set. IP2 string - // SetType specifies the type of ip set where the entry exists. + // SetType is the type of ipset where the entry exists. SetType Type } +// Validate checks if a given ipset entry is valid or not. The set parameter is the ipset that entry belongs to. +func (e *Entry) Validate(set *IPSet) (bool, error) { + switch e.SetType { + case HashIPPort: + // set default protocol to tcp if empty + if len(e.Protocol) == 0 { + e.Protocol = ProtocolTCP + } + + if net.ParseIP(e.IP) == nil { + return false, fmt.Errorf("error parsing entry's ip address %v for %s type set", e.IP, HashIPPort) + } + + if valid := validateProtocol(e.Protocol); !valid { + return false, fmt.Errorf("invalid entry's protocol: %s, supported protocols are [%s, %s]", e.Protocol, ProtocolTCP, ProtocolUDP) + } + + if e.Port < 0 { + return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + } + case HashIPPortIP: + // set default protocol to tcp if empty + if len(e.Protocol) == 0 { + e.Protocol = ProtocolTCP + } + + if net.ParseIP(e.IP) == nil { + return false, fmt.Errorf("error parsing entry's ip address %v for %s type set", e.IP, HashIPPortIP) + } + + if valid := validateProtocol(e.Protocol); !valid { + return false, fmt.Errorf("invalid entry's protocol, supported protocols are [%s, %s]", ProtocolTCP, ProtocolUDP) + } + + if e.Port < 0 { + return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + } + + // IP2 can not be empty for `hash:ip,port,ip` type ip set + if net.ParseIP(e.IP2) == nil { + return false, fmt.Errorf("error parsing entry's second ip address %v for %s type set", e.IP2, HashIPPortIP) + } + case HashIPPortNet: + // set default protocol to tcp if empty + if len(e.Protocol) == 0 { + e.Protocol = ProtocolTCP + } + + if net.ParseIP(e.IP) == nil { + return false, fmt.Errorf("error parsing entry's ip address %v for %s type set", e.IP, HashIPPortIP) + } + + if valid := validateProtocol(e.Protocol); !valid { + return false, fmt.Errorf("invalid entry's protocol, supported protocols are [%s, %s]", ProtocolTCP, ProtocolUDP) + } + + if e.Port < 0 { + return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + } + + // Net can not be empty for `hash:ip,port,net` type ip set + if _, ipNet, _ := net.ParseCIDR(e.Net); ipNet == nil { + return false, fmt.Errorf("error parsing entry's ip net %v for %s type set", e.Net, HashIPPortNet) + } + case BitmapPort: + if e.Port < 0 { + return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + } + + // check if port number satisfies its ipset's requirement of port range + if set == nil { + return false, fmt.Errorf("can not reference ip set where the entry exists") + } + begin, end, err := parsePortRange(set.PortRange) + if err != nil { + return false, err + } + if e.Port < begin || e.Port > end { + return false, fmt.Errorf("entry's port number %d is not in the port range %s of its ipset", e.Port, set.PortRange) + } + } + + return true, nil +} + +// String returns the string format for ipset entry. func (e *Entry) String() string { switch e.SetType { case HashIPPort: @@ -172,28 +260,30 @@ func New(exec utilexec.Interface) Interface { // CreateSet creates a new set, it will ignore error when the set already exists if ignoreExistErr=true. func (runner *runner) CreateSet(set *IPSet, ignoreExistErr bool) error { - // Using default values. + // Setting default values if not present if set.HashSize == 0 { set.HashSize = 1024 } if set.MaxElem == 0 { set.MaxElem = 65536 } + // Default protocol is IPv4 if set.HashFamily == "" { set.HashFamily = ProtocolFamilyIPV4 } - if len(set.HashFamily) != 0 && set.HashFamily != ProtocolFamilyIPV4 && set.HashFamily != ProtocolFamilyIPV6 { - return fmt.Errorf("Currently supported protocol families are: %s and %s, %s is not supported", ProtocolFamilyIPV4, ProtocolFamilyIPV6, set.HashFamily) - } // Default ipset type is "hash:ip,port" if len(set.SetType) == 0 { set.SetType = HashIPPort } - // Check if setType is supported - if !IsValidIPSetType(set.SetType) { - return fmt.Errorf("Currently supported ipset types are: %v, %s is not supported", ValidIPSetTypes, set.SetType) + if len(set.PortRange) == 0 { + set.PortRange = DefaultPortRange } + // Validate ipset before creating + valid, err := set.Validate() + if err != nil || !valid { + return fmt.Errorf("ipset is invalid because of %v", err) + } return runner.createSet(set, ignoreExistErr) } @@ -209,12 +299,6 @@ func (runner *runner) createSet(set *IPSet, ignoreExistErr bool) error { ) } if set.SetType == BitmapPort { - if len(set.PortRange) == 0 { - set.PortRange = DefaultPortRange - } - if !validatePortRange(set.PortRange) { - return fmt.Errorf("invalid port range for %s type ip set: %s, expect: a-b", BitmapPort, set.PortRange) - } args = append(args, "range", set.PortRange) } if ignoreExistErr { @@ -396,4 +480,48 @@ func IsNotFoundError(err error) bool { return false } +// checks if given hash family is supported in ipset +func validateProtocol(protocol string) bool { + if protocol == ProtocolTCP || protocol == ProtocolUDP { + return true + } + return false +} + +// parsePortRange parse the begin and end port from a raw string(format: a-b). beginPort <= endPort +// in the return value. +func parsePortRange(portRange string) (beginPort int, endPort int, err error) { + if len(portRange) == 0 { + portRange = DefaultPortRange + } + + strs := strings.Split(portRange, "-") + if len(strs) != 2 { + // port number -1 indicates invalid + return -1, -1, fmt.Errorf("port range should be in the format of `a-b`") + } + for i := range strs { + num, err := strconv.Atoi(strs[i]) + if err != nil { + // port number -1 indicates invalid + return -1, -1, err + } + if num < 0 { + // port number -1 indicates invalid + return -1, -1, fmt.Errorf("port number %d should be >=0", num) + } + if i == 0 { + beginPort = num + continue + } + endPort = num + // switch when first port number > second port number + if beginPort > endPort { + endPort = beginPort + beginPort = num + } + } + return beginPort, endPort, nil +} + var _ = Interface(&runner{}) diff --git a/pkg/util/ipset/ipset_test.go b/pkg/util/ipset/ipset_test.go index 59a5712185f..f556982d147 100644 --- a/pkg/util/ipset/ipset_test.go +++ b/pkg/util/ipset/ipset_test.go @@ -312,6 +312,7 @@ func TestDelEntry(t *testing.T) { }, } runner := New(&fexec) + err := runner.DelEntry(testEntry.String(), "FOOBAR") if err != nil { t.Errorf("expected success, got %v", err) @@ -654,3 +655,648 @@ func Test_validateFamily(t *testing.T) { } } } + +func Test_validateProtocol(t *testing.T) { + testCases := []struct { + protocol string + valid bool + desc string + }{ + { // case[0] + protocol: "foo", + valid: false, + }, + { // case[1] + protocol: ProtocolTCP, + valid: true, + }, + { // case[2] + protocol: ProtocolUDP, + valid: true, + }, + { // case[3] + protocol: "ipv4", + valid: false, + }, + { // case[4] + protocol: "ipv6", + valid: false, + }, + { // case[5] + protocol: "TCP", + valid: false, + desc: "should be low case", + }, + { // case[6] + protocol: "UDP", + valid: false, + desc: "should be low case", + }, + { // case[7] + protocol: "", + valid: false, + }, + } + for i := range testCases { + valid := validateProtocol(testCases[i].protocol) + if valid != testCases[i].valid { + t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].desc) + } + } +} + +func TestValidateIPSet(t *testing.T) { + testCases := []struct { + ipset *IPSet + valid bool + desc string + }{ + { // case[0] + ipset: &IPSet{ + Name: "test", + SetType: HashIPPort, + HashFamily: ProtocolFamilyIPV4, + HashSize: 1024, + MaxElem: 1024, + }, + valid: true, + }, + { // case[1] + ipset: &IPSet{ + Name: "SET", + SetType: BitmapPort, + HashFamily: ProtocolFamilyIPV6, + HashSize: 65535, + MaxElem: 2048, + PortRange: DefaultPortRange, + }, + valid: true, + }, + { // case[2] + ipset: &IPSet{ + Name: "foo", + SetType: BitmapPort, + HashFamily: ProtocolFamilyIPV6, + HashSize: 65535, + MaxElem: 2048, + }, + valid: false, + desc: "should specify right port range for bitmap type set", + }, + { // case[3] + ipset: &IPSet{ + Name: "bar", + SetType: BitmapPort, + HashFamily: ProtocolFamilyIPV6, + HashSize: 0, + MaxElem: 2048, + }, + valid: false, + desc: "wrong hash size number", + }, + { // case[4] + ipset: &IPSet{ + Name: "baz", + SetType: BitmapPort, + HashFamily: ProtocolFamilyIPV6, + HashSize: 1024, + MaxElem: -1, + }, + valid: false, + desc: "wrong hash max elem number", + }, + { // case[5] + ipset: &IPSet{ + Name: "baz", + SetType: HashIPPortNet, + HashFamily: "ip", + HashSize: 1024, + MaxElem: 1024, + }, + valid: false, + desc: "wrong protocol", + }, + { // case[6] + ipset: &IPSet{ + Name: "foo-bar", + SetType: "xxx", + HashFamily: ProtocolFamilyIPV4, + HashSize: 1024, + MaxElem: 1024, + }, + valid: false, + desc: "wrong set type", + }, + } + for i := range testCases { + valid, _ := testCases[i].ipset.Validate() + if valid != testCases[i].valid { + t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].desc) + } + } +} + +func Test_parsePortRange(t *testing.T) { + testCases := []struct { + portRange string + expectErr bool + beginPort int + endPort int + desc string + }{ + { // case[0] + portRange: "1-100", + expectErr: false, + beginPort: 1, + endPort: 100, + }, + { // case[1] + portRange: "0-0", + expectErr: false, + beginPort: 0, + endPort: 0, + }, + { // case[2] + portRange: "100-10", + expectErr: false, + beginPort: 10, + endPort: 100, + }, + { // case[3] + portRange: "1024", + expectErr: true, + desc: "single port number is not allowed", + }, + { // case[4] + portRange: DefaultPortRange, + expectErr: false, + beginPort: 0, + endPort: 65535, + }, + { // case[5] + portRange: "1-", + expectErr: true, + desc: "should specify end port", + }, + { // case[6] + portRange: "-100", + expectErr: true, + desc: "should specify begin port", + }, + { // case[7] + portRange: "1:100", + expectErr: true, + desc: "delimiter should be -", + }, + { // case[8] + portRange: "1~100", + expectErr: true, + desc: "delimiter should be -", + }, + { // case[9] + portRange: "1,100", + expectErr: true, + desc: "delimiter should be -", + }, + { // case[10] + portRange: "100-100", + expectErr: false, + desc: "begin port number can be equal to end port number", + beginPort: 100, + endPort: 100, + }, + { // case[11] + portRange: "", + expectErr: false, + desc: "empty string indicates default port range", + beginPort: 0, + endPort: 65535, + }, + { // case[12] + portRange: "-1-12", + expectErr: true, + desc: "port number can not be negative value", + }, + { // case[13] + portRange: "-1--8", + expectErr: true, + desc: "port number can not be negative value", + }, + } + for i := range testCases { + begin, end, err := parsePortRange(testCases[i].portRange) + if err != nil { + if !testCases[i].expectErr { + t.Errorf("case [%d]: unexpected err: %v, desc: %s", i, err, testCases[i].desc) + } + continue + } + if begin != testCases[i].beginPort || end != testCases[i].endPort { + t.Errorf("case [%d]: unexpected mismatch [beginPort, endPort] pair, expect [%d, %d], got [%d, %d], desc: %s", i, testCases[i].beginPort, testCases[i].endPort, begin, end, testCases[i].desc) + } + } +} + +// This is a coarse test, but it offers some modicum of confidence as the code is evolved. +func TestValidateEntry(t *testing.T) { + testCases := []struct { + entry *Entry + set *IPSet + valid bool + desc string + }{ + { // case[0] + entry: &Entry{ + SetType: BitmapPort, + }, + set: &IPSet{ + PortRange: DefaultPortRange, + }, + valid: true, + desc: "port number can be empty, default is 0. And port number is in the range of its ipset's port range", + }, + { // case[1] + entry: &Entry{ + SetType: BitmapPort, + Port: 0, + }, + set: &IPSet{ + PortRange: DefaultPortRange, + }, + valid: true, + desc: "port number can be 0. And port number is in the range of its ipset's port range", + }, + { // case[2] + entry: &Entry{ + SetType: BitmapPort, + Port: -1, + }, + valid: false, + desc: "port number can not be negative value", + }, + { // case[3] + entry: &Entry{ + SetType: BitmapPort, + Port: 1080, + }, + set: &IPSet{ + Name: "baz", + PortRange: DefaultPortRange, + }, + desc: "port number is in the range of its ipset's port range", + valid: true, + }, + { // case[4] + entry: &Entry{ + SetType: BitmapPort, + Port: 1080, + }, + set: &IPSet{ + Name: "foo", + PortRange: "0-1079", + }, + desc: "port number is NOT in the range of its ipset's port range", + valid: false, + }, + { // case[5] + entry: &Entry{ + SetType: HashIPPort, + IP: "1.2.3.4", + Protocol: ProtocolTCP, + Port: 8080, + }, + set: &IPSet{ + Name: "bar", + }, + valid: true, + }, + { // case[6] + entry: &Entry{ + SetType: HashIPPort, + IP: "1.2.3.4", + Protocol: ProtocolUDP, + Port: 0, + }, + set: &IPSet{ + Name: "bar", + }, + valid: true, + }, + { // case[7] + entry: &Entry{ + SetType: HashIPPort, + IP: "FE80:0000:0000:0000:0202:B3FF:FE1E:8329", + Protocol: ProtocolTCP, + Port: 1111, + }, + set: &IPSet{ + Name: "ipv6", + }, + valid: true, + }, + { // case[8] + entry: &Entry{ + SetType: HashIPPort, + IP: "", + Protocol: ProtocolTCP, + Port: 1234, + }, + set: &IPSet{ + Name: "empty-ip", + }, + valid: false, + }, + { // case[9] + entry: &Entry{ + SetType: HashIPPort, + IP: "1-2-3-4", + Protocol: ProtocolTCP, + Port: 8900, + }, + set: &IPSet{ + Name: "bad-ip", + }, + valid: false, + }, + { // case[10] + entry: &Entry{ + SetType: HashIPPort, + IP: "10.20.30.40", + Protocol: "", + Port: 8090, + }, + set: &IPSet{ + Name: "empty-protocol", + }, + valid: true, + }, + { // case[11] + entry: &Entry{ + SetType: HashIPPort, + IP: "10.20.30.40", + Protocol: "ICMP", + Port: 8090, + }, + set: &IPSet{ + Name: "unsupported-protocol", + }, + valid: false, + }, + { // case[11] + entry: &Entry{ + SetType: HashIPPort, + IP: "10.20.30.40", + Protocol: "ICMP", + Port: -1, + }, + set: &IPSet{ + // TODO: set name string with white space? + Name: "negative-port-number", + }, + valid: false, + }, + { // case[12] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: ProtocolUDP, + Port: 53, + IP2: "10.20.30.40", + }, + set: &IPSet{ + Name: "LOOP-BACK", + }, + valid: true, + }, + { // case[13] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: ProtocolUDP, + Port: 53, + IP2: "", + }, + set: &IPSet{ + Name: "empty IP2", + }, + valid: false, + }, + { // case[14] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: ProtocolUDP, + Port: 53, + IP2: "foo", + }, + set: &IPSet{ + Name: "invalid IP2", + }, + valid: false, + }, + { // case[15] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: ProtocolTCP, + Port: 0, + IP2: "1.2.3.4", + }, + set: &IPSet{ + Name: "zero port", + }, + valid: true, + }, + { // case[16] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10::40", + Protocol: ProtocolTCP, + Port: 10000, + IP2: "1::4", + }, + set: &IPSet{ + Name: "IPV6", + // TODO: check set's hash family + }, + valid: true, + }, + { // case[17] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "", + Protocol: ProtocolTCP, + Port: 1234, + IP2: "1.2.3.4", + }, + set: &IPSet{ + Name: "empty-ip", + }, + valid: false, + }, + { // case[18] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "1-2-3-4", + Protocol: ProtocolTCP, + Port: 8900, + IP2: "10.20.30.41", + }, + set: &IPSet{ + Name: "bad-ip", + }, + valid: false, + }, + { // case[19] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: "SCTP ", + Port: 8090, + IP2: "10.20.30.41", + }, + set: &IPSet{ + Name: "unsupported-protocol", + }, + valid: false, + }, + { // case[20] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: "ICMP", + Port: -1, + IP2: "100.200.30.41", + }, + set: &IPSet{ + Name: "negative-port-number", + }, + valid: false, + }, + { // case[21] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "10.20.30.40", + Protocol: ProtocolTCP, + Port: 53, + // TODO: CIDR /32 may not be valid + Net: "10.20.30.0/24", + }, + set: &IPSet{ + Name: "abc", + }, + valid: true, + }, + { // case[22] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "11.21.31.41", + Protocol: ProtocolUDP, + Port: 1122, + Net: "", + }, + set: &IPSet{ + Name: "empty Net", + }, + valid: false, + }, + { // case[23] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "10.20.30.40", + Protocol: ProtocolUDP, + Port: 8080, + Net: "x-y-z-w", + }, + set: &IPSet{ + Name: "invalid Net", + }, + valid: false, + }, + { // case[24] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "10.20.30.40", + Protocol: ProtocolTCP, + Port: 0, + Net: "10.1.0.0/16", + }, + set: &IPSet{ + Name: "zero port", + }, + valid: true, + }, + { // case[25] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "10::40", + Protocol: ProtocolTCP, + Port: 80, + Net: "2001:db8::/32", + }, + set: &IPSet{ + Name: "IPV6", + // TODO: check set's hash family + }, + valid: true, + }, + { // case[26] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "", + Protocol: ProtocolTCP, + Port: 1234, + Net: "1.2.3.4/22", + }, + set: &IPSet{ + Name: "empty-ip", + }, + valid: false, + }, + { // case[27] + entry: &Entry{ + SetType: HashIPPortNet, + IP: "1-2-3-4", + Protocol: ProtocolTCP, + Port: 8900, + Net: "10.20.30.41/31", + }, + set: &IPSet{ + Name: "bad-ip", + }, + valid: false, + }, + { // case[28] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: "FOO", + Port: 8090, + IP2: "10.20.30.0/10", + }, + set: &IPSet{ + Name: "unsupported-protocol", + }, + valid: false, + }, + { // case[29] + entry: &Entry{ + SetType: HashIPPortIP, + IP: "10.20.30.40", + Protocol: ProtocolUDP, + Port: -1, + IP2: "100.200.30.0/12", + }, + set: &IPSet{ + Name: "negative-port-number", + }, + valid: false, + }, + } + for i := range testCases { + valid, _ := testCases[i].entry.Validate(testCases[i].set) + if valid != testCases[i].valid { + t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].entry) + } + } +} diff --git a/pkg/util/ipset/types.go b/pkg/util/ipset/types.go index d2406c00878..cacfd6f8d8e 100644 --- a/pkg/util/ipset/types.go +++ b/pkg/util/ipset/types.go @@ -58,13 +58,3 @@ var ValidIPSetTypes = []Type{ BitmapPort, HashIPPortNet, } - -// IsValidIPSetType checks if the given ipset type is valid. -func IsValidIPSetType(set Type) bool { - for _, valid := range ValidIPSetTypes { - if set == valid { - return true - } - } - return false -} From 4df6662d561c87b49ba3450fa43ea1637567578f Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Mon, 18 Dec 2017 15:21:28 +0800 Subject: [PATCH 4/7] validate ipset entry before adding in ipvs proxier --- pkg/proxy/ipvs/proxier.go | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index cc92745f2de..1b324e2af71 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -970,6 +970,9 @@ func (proxier *Proxier) OnEndpointsSynced() { proxier.syncProxyRules() } +// EntryInvalidErr indiates if an ipset entry is invalid or not +const EntryInvalidErr = "entry is invalid" + // This is where all of the ipvs calls happen. // assumes proxier.mu is held func (proxier *Proxier) syncProxyRules() { @@ -1124,6 +1127,10 @@ func (proxier *Proxier) syncProxyRules() { IP2: epIP, SetType: utilipset.HashIPPortIP, } + if valid, err := proxier.loopbackSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.loopbackSet.Name, EntryInvalidErr, err) + continue + } proxier.loopbackSet.activeEntries.Insert(entry.String()) } @@ -1139,8 +1146,16 @@ func (proxier *Proxier) syncProxyRules() { // proxier.kubeServiceAccessSet.activeEntries.Insert(entry.String()) // Install masquerade rules if 'masqueradeAll' or 'clusterCIDR' is specified. if proxier.masqueradeAll { + if valid, err := proxier.clusterIPSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.clusterIPSet.Name, EntryInvalidErr, err) + continue + } proxier.clusterIPSet.activeEntries.Insert(entry.String()) } else if len(proxier.clusterCIDR) > 0 { + if valid, err := proxier.clusterIPSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.clusterIPSet.Name, EntryInvalidErr, err) + continue + } proxier.clusterIPSet.activeEntries.Insert(entry.String()) } // ipvs call @@ -1208,6 +1223,10 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPort, } // We have to SNAT packets to external IPs. + if valid, err := proxier.externalIPSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.externalIPSet.Name, EntryInvalidErr, err) + continue + } proxier.externalIPSet.activeEntries.Insert(entry.String()) // ipvs call @@ -1247,12 +1266,20 @@ func (proxier *Proxier) syncProxyRules() { // If we are proxying globally, we need to masquerade in case we cross nodes. // If we are proxying only locally, we can retain the source IP. if !svcInfo.onlyNodeLocalEndpoints { + if valid, err := proxier.lbMasqSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbMasqSet.Name, EntryInvalidErr, err) + continue + } proxier.lbMasqSet.activeEntries.Insert(entry.String()) } if len(svcInfo.loadBalancerSourceRanges) != 0 { // The service firewall rules are created based on ServiceSpec.loadBalancerSourceRanges field. // This currently works for loadbalancers that preserves source ips. // For loadbalancers which direct traffic to service NodePort, the firewall rules will not apply. + if valid, err := proxier.lbIngressSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbIngressSet.Name, EntryInvalidErr, err) + continue + } proxier.lbIngressSet.activeEntries.Insert(entry.String()) allowFromNode := false @@ -1266,6 +1293,10 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPortNet, } // enumerate all white list source cidr + if valid, err := proxier.lbWhiteListCIDRSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbWhiteListCIDRSet.Name, EntryInvalidErr, err) + continue + } proxier.lbWhiteListCIDRSet.activeEntries.Insert(entry.String()) // ignore error because it has been validated @@ -1286,6 +1317,10 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPortIP, } // enumerate all white list source ip + if valid, err := proxier.lbWhiteListIPSet.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbWhiteListIPSet.Name, EntryInvalidErr, err) + continue + } proxier.lbWhiteListIPSet.activeEntries.Insert(entry.String()) } } @@ -1347,8 +1382,16 @@ func (proxier *Proxier) syncProxyRules() { } switch protocol { case "tcp": + if valid, err := proxier.nodePortSetTCP.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.nodePortSetTCP.Name, EntryInvalidErr, err) + continue + } proxier.nodePortSetTCP.activeEntries.Insert(entry.String()) case "udp": + if valid, err := proxier.nodePortSetUDP.validateEntry(entry); !valid { + glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.nodePortSetUDP.Name, EntryInvalidErr, err) + continue + } proxier.nodePortSetUDP.activeEntries.Insert(entry.String()) default: // It should never hit From 477b0f06362ed033758c1f8291a8049df1e741c8 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Fri, 22 Dec 2017 09:46:18 +0800 Subject: [PATCH 5/7] fix review comments --- pkg/proxy/ipvs/ipset.go | 2 +- pkg/proxy/ipvs/ipset_test.go | 56 ++++++++++++------ pkg/proxy/ipvs/proxier.go | 42 +++++++------- pkg/util/ipset/ipset.go | 89 ++++++++++++++++++----------- pkg/util/ipset/ipset_test.go | 6 +- pkg/util/ipset/testing/fake_test.go | 4 +- 6 files changed, 122 insertions(+), 77 deletions(-) diff --git a/pkg/proxy/ipvs/ipset.go b/pkg/proxy/ipvs/ipset.go index bc639b92f59..16637455b7b 100644 --- a/pkg/proxy/ipvs/ipset.go +++ b/pkg/proxy/ipvs/ipset.go @@ -89,7 +89,7 @@ func NewIPSet(handle utilipset.Interface, name string, setType utilipset.Type, i return set } -func (set *IPSet) validateEntry(entry *utilipset.Entry) (bool, error) { +func (set *IPSet) validateEntry(entry *utilipset.Entry) bool { return entry.Validate(&set.IPSet) } diff --git a/pkg/proxy/ipvs/ipset_test.go b/pkg/proxy/ipvs/ipset_test.go index d6a0dc7a926..4610aceb9e7 100644 --- a/pkg/proxy/ipvs/ipset_test.go +++ b/pkg/proxy/ipvs/ipset_test.go @@ -55,7 +55,7 @@ const testIPSetVersion = "v6.19" func TestSyncIPSetEntries(t *testing.T) { testCases := []struct { - setName string + set *utilipset.IPSet setType utilipset.Type ipv6 bool activeEntries []string @@ -63,7 +63,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries []string }{ { // case 0 - setName: "foo", + set: &utilipset.IPSet{ + Name: "foo", + }, setType: utilipset.HashIPPort, ipv6: false, activeEntries: []string{"172.17.0.4,tcp:80"}, @@ -71,7 +73,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"172.17.0.4,tcp:80"}, }, { // case 1 - setName: "abz", + set: &utilipset.IPSet{ + Name: "abz", + }, setType: utilipset.HashIPPort, ipv6: true, activeEntries: []string{"FE80::0202:B3FF:FE1E:8329,tcp:80"}, @@ -79,7 +83,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"FE80::0202:B3FF:FE1E:8329,tcp:80"}, }, { // case 2 - setName: "bca", + set: &utilipset.IPSet{ + Name: "bca", + }, setType: utilipset.HashIPPort, ipv6: false, activeEntries: []string{"172.17.0.4,tcp:80", "172.17.0.5,tcp:80"}, @@ -87,7 +93,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"172.17.0.4,tcp:80", "172.17.0.5,tcp:80"}, }, { // case 3 - setName: "bar", + set: &utilipset.IPSet{ + Name: "bar", + }, setType: utilipset.HashIPPortIP, ipv6: false, activeEntries: []string{"172.17.0.4,tcp:80:172.17.0.4"}, @@ -95,7 +103,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"172.17.0.4,tcp:80:172.17.0.4"}, }, { // case 4 - setName: "baz", + set: &utilipset.IPSet{ + Name: "baz", + }, setType: utilipset.HashIPPortIP, ipv6: true, activeEntries: []string{"FE80:0000:0000:0000:0202:B3FF:FE1E:8329,tcp:8080:FE80:0000:0000:0000:0202:B3FF:FE1E:8329"}, @@ -103,7 +113,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"FE80:0000:0000:0000:0202:B3FF:FE1E:8329,tcp:8080:FE80:0000:0000:0000:0202:B3FF:FE1E:8329"}, }, { // case 5 - setName: "NOPE", + set: &utilipset.IPSet{ + Name: "NOPE", + }, setType: utilipset.HashIPPortIP, ipv6: false, activeEntries: []string{"172.17.0.4,tcp:80,172.17.0.9", "172.17.0.5,tcp:80,172.17.0.10"}, @@ -111,7 +123,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"172.17.0.4,tcp:80,172.17.0.9", "172.17.0.5,tcp:80,172.17.0.10"}, }, { // case 6 - setName: "ABC-DEF", + set: &utilipset.IPSet{ + Name: "ABC-DEF", + }, setType: utilipset.HashIPPortNet, ipv6: false, activeEntries: []string{"172.17.0.4,tcp:80,172.17.0.0/16", "172.17.0.5,tcp:80,172.17.0.0/16"}, @@ -119,7 +133,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"172.17.0.4,tcp:80,172.17.0.0/16", "172.17.0.5,tcp:80,172.17.0.0/16"}, }, { // case 7 - setName: "zar", + set: &utilipset.IPSet{ + Name: "zar", + }, setType: utilipset.HashIPPortNet, ipv6: true, activeEntries: []string{"FE80::8329,tcp:8800,2001:db8::/32"}, @@ -127,7 +143,9 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: []string{"FE80::8329,tcp:8800,2001:db8::/32"}, }, { // case 8 - setName: "bbb", + set: &utilipset.IPSet{ + Name: "bbb", + }, setType: utilipset.HashIPPortNet, ipv6: true, activeEntries: nil, @@ -135,21 +153,27 @@ func TestSyncIPSetEntries(t *testing.T) { expectedEntries: nil, }, { // case 9 - setName: "AAA", + set: &utilipset.IPSet{ + Name: "AAA", + }, setType: utilipset.BitmapPort, activeEntries: nil, currentEntries: []string{"80"}, expectedEntries: nil, }, { // case 10 - setName: "c-c-c", + set: &utilipset.IPSet{ + Name: "c-c-c", + }, setType: utilipset.BitmapPort, activeEntries: []string{"8080", "9090"}, currentEntries: []string{"80"}, expectedEntries: []string{"8080", "9090"}, }, { // case 11 - setName: "NODE-PORT", + set: &utilipset.IPSet{ + Name: "NODE-PORT", + }, setType: utilipset.BitmapPort, activeEntries: []string{"8080"}, currentEntries: []string{"80", "9090", "8081", "8082"}, @@ -158,19 +182,19 @@ func TestSyncIPSetEntries(t *testing.T) { } for i := range testCases { - set := NewIPSet(fakeipset.NewFake(testIPSetVersion), testCases[i].setName, testCases[i].setType, testCases[i].ipv6) + set := NewIPSet(fakeipset.NewFake(testIPSetVersion), testCases[i].set.Name, testCases[i].setType, testCases[i].ipv6) if err := set.handle.CreateSet(&set.IPSet, true); err != nil { t.Errorf("Unexpected error: %v", err) } for _, entry := range testCases[i].expectedEntries { - set.handle.AddEntry(entry, testCases[i].setName, true) + set.handle.AddEntry(entry, testCases[i].set, true) } set.activeEntries.Insert(testCases[i].activeEntries...) set.syncIPSetEntries() for _, entry := range testCases[i].expectedEntries { - found, err := set.handle.TestEntry(entry, testCases[i].setName) + found, err := set.handle.TestEntry(entry, testCases[i].set.Name) if err != nil { t.Errorf("Unexpected error: %v", err) } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 1b324e2af71..99a76eb63e2 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -971,7 +971,7 @@ func (proxier *Proxier) OnEndpointsSynced() { } // EntryInvalidErr indiates if an ipset entry is invalid or not -const EntryInvalidErr = "entry is invalid" +const EntryInvalidErr = "error adding entry %s to ipset %s since entry is invalid" // This is where all of the ipvs calls happen. // assumes proxier.mu is held @@ -1127,8 +1127,8 @@ func (proxier *Proxier) syncProxyRules() { IP2: epIP, SetType: utilipset.HashIPPortIP, } - if valid, err := proxier.loopbackSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.loopbackSet.Name, EntryInvalidErr, err) + if valid := proxier.loopbackSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.loopbackSet.Name)) continue } proxier.loopbackSet.activeEntries.Insert(entry.String()) @@ -1146,14 +1146,14 @@ func (proxier *Proxier) syncProxyRules() { // proxier.kubeServiceAccessSet.activeEntries.Insert(entry.String()) // Install masquerade rules if 'masqueradeAll' or 'clusterCIDR' is specified. if proxier.masqueradeAll { - if valid, err := proxier.clusterIPSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.clusterIPSet.Name, EntryInvalidErr, err) + if valid := proxier.clusterIPSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.clusterIPSet.Name)) continue } proxier.clusterIPSet.activeEntries.Insert(entry.String()) } else if len(proxier.clusterCIDR) > 0 { - if valid, err := proxier.clusterIPSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.clusterIPSet.Name, EntryInvalidErr, err) + if valid := proxier.clusterIPSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.clusterIPSet.Name)) continue } proxier.clusterIPSet.activeEntries.Insert(entry.String()) @@ -1223,8 +1223,8 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPort, } // We have to SNAT packets to external IPs. - if valid, err := proxier.externalIPSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.externalIPSet.Name, EntryInvalidErr, err) + if valid := proxier.externalIPSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.externalIPSet.Name)) continue } proxier.externalIPSet.activeEntries.Insert(entry.String()) @@ -1266,8 +1266,8 @@ func (proxier *Proxier) syncProxyRules() { // If we are proxying globally, we need to masquerade in case we cross nodes. // If we are proxying only locally, we can retain the source IP. if !svcInfo.onlyNodeLocalEndpoints { - if valid, err := proxier.lbMasqSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbMasqSet.Name, EntryInvalidErr, err) + if valid := proxier.lbMasqSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbMasqSet.Name)) continue } proxier.lbMasqSet.activeEntries.Insert(entry.String()) @@ -1276,8 +1276,8 @@ func (proxier *Proxier) syncProxyRules() { // The service firewall rules are created based on ServiceSpec.loadBalancerSourceRanges field. // This currently works for loadbalancers that preserves source ips. // For loadbalancers which direct traffic to service NodePort, the firewall rules will not apply. - if valid, err := proxier.lbIngressSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbIngressSet.Name, EntryInvalidErr, err) + if valid := proxier.lbIngressSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbIngressSet.Name)) continue } proxier.lbIngressSet.activeEntries.Insert(entry.String()) @@ -1293,8 +1293,8 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPortNet, } // enumerate all white list source cidr - if valid, err := proxier.lbWhiteListCIDRSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbWhiteListCIDRSet.Name, EntryInvalidErr, err) + if valid := proxier.lbWhiteListCIDRSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbWhiteListCIDRSet.Name)) continue } proxier.lbWhiteListCIDRSet.activeEntries.Insert(entry.String()) @@ -1317,8 +1317,8 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPortIP, } // enumerate all white list source ip - if valid, err := proxier.lbWhiteListIPSet.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.lbWhiteListIPSet.Name, EntryInvalidErr, err) + if valid := proxier.lbWhiteListIPSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbWhiteListIPSet.Name)) continue } proxier.lbWhiteListIPSet.activeEntries.Insert(entry.String()) @@ -1382,14 +1382,14 @@ func (proxier *Proxier) syncProxyRules() { } switch protocol { case "tcp": - if valid, err := proxier.nodePortSetTCP.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.nodePortSetTCP.Name, EntryInvalidErr, err) + if valid := proxier.nodePortSetTCP.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.nodePortSetTCP.Name)) continue } proxier.nodePortSetTCP.activeEntries.Insert(entry.String()) case "udp": - if valid, err := proxier.nodePortSetUDP.validateEntry(entry); !valid { - glog.Errorf("Failed to add entry %v to set %s, error: %s, %v", entry, proxier.nodePortSetUDP.Name, EntryInvalidErr, err) + if valid := proxier.nodePortSetUDP.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.nodePortSetUDP.Name)) continue } proxier.nodePortSetUDP.activeEntries.Insert(entry.String()) diff --git a/pkg/util/ipset/ipset.go b/pkg/util/ipset/ipset.go index 7442135c9e7..a3e4b937fa0 100644 --- a/pkg/util/ipset/ipset.go +++ b/pkg/util/ipset/ipset.go @@ -24,6 +24,7 @@ import ( "strconv" "strings" + "github.com/golang/glog" utilexec "k8s.io/utils/exec" ) @@ -91,33 +92,35 @@ type IPSet struct { } // Validate checks if a given ipset is valid or not. -func (set *IPSet) Validate() (bool, error) { +func (set *IPSet) Validate() bool { // Check if protocol is valid for `HashIPPort`, `HashIPPortIP` and `HashIPPortNet` type set. if set.SetType == HashIPPort || set.SetType == HashIPPortIP || set.SetType == HashIPPortNet { if valid := validateHashFamily(set.HashFamily); !valid { - return false, fmt.Errorf("currently supported ip set hash families are: [%s, %s], %s is not supported", ProtocolFamilyIPV4, ProtocolFamilyIPV6, set.HashFamily) + return false } } // check set type if valid := validateIPSetType(set.SetType); !valid { - return false, fmt.Errorf("currently supported ipset types are: %v, %s is not supported", ValidIPSetTypes, set.SetType) + return false } // check port range for bitmap type set if set.SetType == BitmapPort { - if valid, err := validatePortRange(set.PortRange); !valid { - return false, err + if valid := validatePortRange(set.PortRange); !valid { + return false } } // check hash size value of ipset if set.HashSize <= 0 { - return false, fmt.Errorf("invalid hashsize value, should be >0") + + return false } // check max elem value of ipset if set.MaxElem <= 0 { - return false, fmt.Errorf("invalid maxelem value, should be >0") + glog.Errorf("Invalid maxelem value %d, should be >0", set.MaxElem) + return false } - return true, nil + return true } // Entry represents a ipset entry. @@ -140,7 +143,7 @@ type Entry struct { } // Validate checks if a given ipset entry is valid or not. The set parameter is the ipset that entry belongs to. -func (e *Entry) Validate(set *IPSet) (bool, error) { +func (e *Entry) Validate(set *IPSet) bool { switch e.SetType { case HashIPPort: // set default protocol to tcp if empty @@ -149,15 +152,17 @@ func (e *Entry) Validate(set *IPSet) (bool, error) { } if net.ParseIP(e.IP) == nil { - return false, fmt.Errorf("error parsing entry's ip address %v for %s type set", e.IP, HashIPPort) + glog.Errorf("Error parsing entry %v ip address %v for ipset %v", e, e.IP, set) + return false } if valid := validateProtocol(e.Protocol); !valid { - return false, fmt.Errorf("invalid entry's protocol: %s, supported protocols are [%s, %s]", e.Protocol, ProtocolTCP, ProtocolUDP) + return false } if e.Port < 0 { - return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) + return false } case HashIPPortIP: // set default protocol to tcp if empty @@ -166,20 +171,23 @@ func (e *Entry) Validate(set *IPSet) (bool, error) { } if net.ParseIP(e.IP) == nil { - return false, fmt.Errorf("error parsing entry's ip address %v for %s type set", e.IP, HashIPPortIP) + glog.Errorf("Error parsing entry %v ip address %v for ipset %v", e, e.IP, set) + return false } if valid := validateProtocol(e.Protocol); !valid { - return false, fmt.Errorf("invalid entry's protocol, supported protocols are [%s, %s]", ProtocolTCP, ProtocolUDP) + return false } if e.Port < 0 { - return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + glog.Errorf("Entry %v port number %d should be >=0 for ipset %v ", e, e.Port, set) + return false } // IP2 can not be empty for `hash:ip,port,ip` type ip set if net.ParseIP(e.IP2) == nil { - return false, fmt.Errorf("error parsing entry's second ip address %v for %s type set", e.IP2, HashIPPortIP) + glog.Errorf("Error parsing entry %v second ip address %v for ipset %v", e, e.IP2, set) + return false } case HashIPPortNet: // set default protocol to tcp if empty @@ -188,40 +196,47 @@ func (e *Entry) Validate(set *IPSet) (bool, error) { } if net.ParseIP(e.IP) == nil { - return false, fmt.Errorf("error parsing entry's ip address %v for %s type set", e.IP, HashIPPortIP) + glog.Errorf("Error parsing entry %v ip address %v for ipset %v", e, e.IP, set) + return false } if valid := validateProtocol(e.Protocol); !valid { - return false, fmt.Errorf("invalid entry's protocol, supported protocols are [%s, %s]", ProtocolTCP, ProtocolUDP) + return false } if e.Port < 0 { - return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) + return false } // Net can not be empty for `hash:ip,port,net` type ip set if _, ipNet, _ := net.ParseCIDR(e.Net); ipNet == nil { - return false, fmt.Errorf("error parsing entry's ip net %v for %s type set", e.Net, HashIPPortNet) + glog.Errorf("Error parsing entry %v ip net %v for ipset %v", e, e.Net, set) + return false } case BitmapPort: if e.Port < 0 { - return false, fmt.Errorf("port number %d should be >=0 ", e.Port) + glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) + return false } // check if port number satisfies its ipset's requirement of port range if set == nil { - return false, fmt.Errorf("can not reference ip set where the entry exists") + glog.Errorf("Unable to reference ip set where the entry %v exists", e) + return false } begin, end, err := parsePortRange(set.PortRange) if err != nil { - return false, err + glog.Errorf("Failed to parse set %v port range %s for ipset %v, error: %v", set, set.PortRange, set, err) + return false } if e.Port < begin || e.Port > end { - return false, fmt.Errorf("entry's port number %d is not in the port range %s of its ipset", e.Port, set.PortRange) + glog.Errorf("Entry %v port number %d is not in the port range %s of its ipset %v", e, e.Port, set.PortRange, set) + return false } } - return true, nil + return true } // String returns the string format for ipset entry. @@ -280,9 +295,9 @@ func (runner *runner) CreateSet(set *IPSet, ignoreExistErr bool) error { } // Validate ipset before creating - valid, err := set.Validate() - if err != nil || !valid { - return fmt.Errorf("ipset is invalid because of %v", err) + valid := set.Validate() + if !valid { + return fmt.Errorf("Error creating ipset since it's invalid") } return runner.createSet(set, ignoreExistErr) } @@ -426,21 +441,24 @@ func getIPSetVersionString(exec utilexec.Interface) (string, error) { // checks if port range is valid. The begin port number is not necessarily less than // end port number - ipset util can accept it. It means both 1-100 and 100-1 are valid. -func validatePortRange(portRange string) (bool, error) { +func validatePortRange(portRange string) bool { strs := strings.Split(portRange, "-") if len(strs) != 2 { - return false, fmt.Errorf("port range should be in the format of `a-b`") + glog.Errorf("port range should be in the format of `a-b`") + return false } for i := range strs { num, err := strconv.Atoi(strs[i]) if err != nil { - return false, err + glog.Errorf("Failed to parse %s, error: %v", strs[i], err) + return false } if num < 0 { - return false, fmt.Errorf("port number %d should be >=0", num) + glog.Errorf("port number %d should be >=0", num) + return false } } - return true, nil + return true } // checks if the given ipset type is valid. @@ -450,6 +468,7 @@ func validateIPSetType(set Type) bool { return true } } + glog.Errorf("Currently supported ipset types are: %v, %s is not supported", ValidIPSetTypes, set) return false } @@ -458,6 +477,7 @@ func validateHashFamily(family string) bool { if family == ProtocolFamilyIPV4 || family == ProtocolFamilyIPV6 { return true } + glog.Errorf("Currently supported ip set hash families are: [%s, %s], %s is not supported", ProtocolFamilyIPV4, ProtocolFamilyIPV6, family) return false } @@ -480,11 +500,12 @@ func IsNotFoundError(err error) bool { return false } -// checks if given hash family is supported in ipset +// checks if given protocol is supported in entry func validateProtocol(protocol string) bool { if protocol == ProtocolTCP || protocol == ProtocolUDP { return true } + glog.Errorf("Invalid entry's protocol: %s, supported protocols are [%s, %s]", protocol, ProtocolTCP, ProtocolUDP) return false } diff --git a/pkg/util/ipset/ipset_test.go b/pkg/util/ipset/ipset_test.go index f556982d147..eaab924016c 100644 --- a/pkg/util/ipset/ipset_test.go +++ b/pkg/util/ipset/ipset_test.go @@ -603,7 +603,7 @@ func Test_validatePortRange(t *testing.T) { }, } for i := range testCases { - valid, _ := validatePortRange(testCases[i].portRange) + valid := validatePortRange(testCases[i].portRange) if valid != testCases[i].valid { t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].desc) } @@ -789,7 +789,7 @@ func TestValidateIPSet(t *testing.T) { }, } for i := range testCases { - valid, _ := testCases[i].ipset.Validate() + valid := testCases[i].ipset.Validate() if valid != testCases[i].valid { t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].desc) } @@ -1294,7 +1294,7 @@ func TestValidateEntry(t *testing.T) { }, } for i := range testCases { - valid, _ := testCases[i].entry.Validate(testCases[i].set) + valid := testCases[i].entry.Validate(testCases[i].set) if valid != testCases[i].valid { t.Errorf("case [%d]: unexpected mismatch, expect valid[%v], got valid[%v], desc: %s", i, testCases[i].valid, valid, testCases[i].entry) } diff --git a/pkg/util/ipset/testing/fake_test.go b/pkg/util/ipset/testing/fake_test.go index 2128395cf8e..f1f5d1e3417 100644 --- a/pkg/util/ipset/testing/fake_test.go +++ b/pkg/util/ipset/testing/fake_test.go @@ -45,8 +45,8 @@ func TestSetEntry(t *testing.T) { } // add two entries - fake.AddEntry("192.168.1.1,tcp:8080", set.Name, true) - fake.AddEntry("192.168.1.2,tcp:8081", set.Name, true) + fake.AddEntry("192.168.1.1,tcp:8080", set, true) + fake.AddEntry("192.168.1.2,tcp:8081", set, true) entries, err := fake.ListEntries(set.Name) if err != nil { t.Errorf("Unexpected error: %v", err) From 3574aba7bd41939463a12e8e69438bd145b41e07 Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Fri, 22 Dec 2017 11:20:12 +0800 Subject: [PATCH 6/7] update bazel BUILD --- pkg/util/ipset/BUILD | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/util/ipset/BUILD b/pkg/util/ipset/BUILD index 9baf26f5809..df629ef0f65 100644 --- a/pkg/util/ipset/BUILD +++ b/pkg/util/ipset/BUILD @@ -8,7 +8,10 @@ go_library( ], importpath = "k8s.io/kubernetes/pkg/util/ipset", visibility = ["//visibility:public"], - deps = ["//vendor/k8s.io/utils/exec:go_default_library"], + deps = [ + "//vendor/github.com/golang/glog:go_default_library", + "//vendor/k8s.io/utils/exec:go_default_library", + ], ) go_test( From 10a899f31eed0f42bf23aa126351d7888e9c527a Mon Sep 17 00:00:00 2001 From: m1093782566 Date: Thu, 11 Jan 2018 15:42:24 +0800 Subject: [PATCH 7/7] clean up code --- pkg/proxy/ipvs/proxier.go | 32 +++++++++++++------------------- pkg/util/ipset/ipset.go | 27 +++++---------------------- 2 files changed, 18 insertions(+), 41 deletions(-) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 99a76eb63e2..791872384b3 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -970,8 +970,8 @@ func (proxier *Proxier) OnEndpointsSynced() { proxier.syncProxyRules() } -// EntryInvalidErr indiates if an ipset entry is invalid or not -const EntryInvalidErr = "error adding entry %s to ipset %s since entry is invalid" +// EntryInvalidErr indicates if an ipset entry is invalid or not +const EntryInvalidErr = "error adding entry %s to ipset %s" // This is where all of the ipvs calls happen. // assumes proxier.mu is held @@ -1145,13 +1145,7 @@ func (proxier *Proxier) syncProxyRules() { // add service Cluster IP:Port to kubeServiceAccess ip set for the purpose of solving hairpin. // proxier.kubeServiceAccessSet.activeEntries.Insert(entry.String()) // Install masquerade rules if 'masqueradeAll' or 'clusterCIDR' is specified. - if proxier.masqueradeAll { - if valid := proxier.clusterIPSet.validateEntry(entry); !valid { - glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.clusterIPSet.Name)) - continue - } - proxier.clusterIPSet.activeEntries.Insert(entry.String()) - } else if len(proxier.clusterCIDR) > 0 { + if proxier.masqueradeAll || len(proxier.clusterCIDR) > 0 { if valid := proxier.clusterIPSet.validateEntry(entry); !valid { glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.clusterIPSet.Name)) continue @@ -1380,23 +1374,23 @@ func (proxier *Proxier) syncProxyRules() { Protocol: protocol, SetType: utilipset.BitmapPort, } + var nodePortSet *IPSet switch protocol { case "tcp": - if valid := proxier.nodePortSetTCP.validateEntry(entry); !valid { - glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.nodePortSetTCP.Name)) - continue - } - proxier.nodePortSetTCP.activeEntries.Insert(entry.String()) + nodePortSet = proxier.nodePortSetTCP case "udp": - if valid := proxier.nodePortSetUDP.validateEntry(entry); !valid { - glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.nodePortSetUDP.Name)) - continue - } - proxier.nodePortSetUDP.activeEntries.Insert(entry.String()) + nodePortSet = proxier.nodePortSetUDP default: // It should never hit glog.Errorf("Unsupported protocol type: %s", protocol) } + if nodePortSet != nil { + if valid := nodePortSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, nodePortSet.Name)) + continue + } + nodePortSet.activeEntries.Insert(entry.String()) + } } // Build ipvs kernel routes for each node ip address diff --git a/pkg/util/ipset/ipset.go b/pkg/util/ipset/ipset.go index a3e4b937fa0..c5ecdc45d33 100644 --- a/pkg/util/ipset/ipset.go +++ b/pkg/util/ipset/ipset.go @@ -50,7 +50,6 @@ type Interface interface { ListSets() ([]string, error) // GetVersion returns the "X.Y" version string for ipset. GetVersion() (string, error) - // TODO: add comment message for ipset } // IPSetCmd represents the ipset util. We use ipset command for ipset execute. @@ -144,6 +143,10 @@ type Entry struct { // Validate checks if a given ipset entry is valid or not. The set parameter is the ipset that entry belongs to. func (e *Entry) Validate(set *IPSet) bool { + if e.Port < 0 { + glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) + return false + } switch e.SetType { case HashIPPort: // set default protocol to tcp if empty @@ -159,11 +162,6 @@ func (e *Entry) Validate(set *IPSet) bool { if valid := validateProtocol(e.Protocol); !valid { return false } - - if e.Port < 0 { - glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) - return false - } case HashIPPortIP: // set default protocol to tcp if empty if len(e.Protocol) == 0 { @@ -179,11 +177,6 @@ func (e *Entry) Validate(set *IPSet) bool { return false } - if e.Port < 0 { - glog.Errorf("Entry %v port number %d should be >=0 for ipset %v ", e, e.Port, set) - return false - } - // IP2 can not be empty for `hash:ip,port,ip` type ip set if net.ParseIP(e.IP2) == nil { glog.Errorf("Error parsing entry %v second ip address %v for ipset %v", e, e.IP2, set) @@ -204,22 +197,12 @@ func (e *Entry) Validate(set *IPSet) bool { return false } - if e.Port < 0 { - glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) - return false - } - // Net can not be empty for `hash:ip,port,net` type ip set if _, ipNet, _ := net.ParseCIDR(e.Net); ipNet == nil { glog.Errorf("Error parsing entry %v ip net %v for ipset %v", e, e.Net, set) return false } case BitmapPort: - if e.Port < 0 { - glog.Errorf("Entry %v port number %d should be >=0 for ipset %v", e, e.Port, set) - return false - } - // check if port number satisfies its ipset's requirement of port range if set == nil { glog.Errorf("Unable to reference ip set where the entry %v exists", e) @@ -297,7 +280,7 @@ func (runner *runner) CreateSet(set *IPSet, ignoreExistErr bool) error { // Validate ipset before creating valid := set.Validate() if !valid { - return fmt.Errorf("Error creating ipset since it's invalid") + return fmt.Errorf("error creating ipset since it's invalid") } return runner.createSet(set, ignoreExistErr) }