diff --git a/pkg/proxy/ipvs/ipset.go b/pkg/proxy/ipvs/ipset.go index d61992125a..16637455b7 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 { + return entry.Validate(&set.IPSet) +} + func (set *IPSet) isEmpty() bool { return len(set.activeEntries.UnsortedList()) == 0 } @@ -123,7 +127,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/proxy/ipvs/ipset_test.go b/pkg/proxy/ipvs/ipset_test.go index d6a0dc7a92..4610aceb9e 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 e86d1019a7..fc8eef57de 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -957,6 +957,9 @@ func (proxier *Proxier) OnEndpointsSynced() { proxier.syncProxyRules() } +// 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 func (proxier *Proxier) syncProxyRules() { @@ -1111,6 +1114,10 @@ func (proxier *Proxier) syncProxyRules() { IP2: epIP, SetType: utilipset.HashIPPortIP, } + if valid := proxier.loopbackSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.loopbackSet.Name)) + continue + } proxier.loopbackSet.activeEntries.Insert(entry.String()) } @@ -1125,9 +1132,11 @@ 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 { - 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 + } proxier.clusterIPSet.activeEntries.Insert(entry.String()) } // ipvs call @@ -1195,6 +1204,10 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPort, } // We have to SNAT packets to external IPs. + if valid := proxier.externalIPSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.externalIPSet.Name)) + continue + } proxier.externalIPSet.activeEntries.Insert(entry.String()) // ipvs call @@ -1234,12 +1247,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 := proxier.lbMasqSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbMasqSet.Name)) + 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 := proxier.lbIngressSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbIngressSet.Name)) + continue + } proxier.lbIngressSet.activeEntries.Insert(entry.String()) allowFromNode := false @@ -1253,6 +1274,10 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPortNet, } // enumerate all white list source cidr + if valid := proxier.lbWhiteListCIDRSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbWhiteListCIDRSet.Name)) + continue + } proxier.lbWhiteListCIDRSet.activeEntries.Insert(entry.String()) // ignore error because it has been validated @@ -1273,6 +1298,10 @@ func (proxier *Proxier) syncProxyRules() { SetType: utilipset.HashIPPortIP, } // enumerate all white list source ip + if valid := proxier.lbWhiteListIPSet.validateEntry(entry); !valid { + glog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, proxier.lbWhiteListIPSet.Name)) + continue + } proxier.lbWhiteListIPSet.activeEntries.Insert(entry.String()) } } @@ -1332,15 +1361,23 @@ func (proxier *Proxier) syncProxyRules() { Protocol: protocol, SetType: utilipset.BitmapPort, } + var nodePortSet *IPSet switch protocol { case "tcp": - proxier.nodePortSetTCP.activeEntries.Insert(entry.String()) + nodePortSet = proxier.nodePortSetTCP case "udp": - 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/BUILD b/pkg/util/ipset/BUILD index 9baf26f580..df629ef0f6 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( diff --git a/pkg/util/ipset/ipset.go b/pkg/util/ipset/ipset.go index 7ae2805009..c5ecdc45d3 100644 --- a/pkg/util/ipset/ipset.go +++ b/pkg/util/ipset/ipset.go @@ -19,10 +19,12 @@ package ipset import ( "bytes" "fmt" + "net" "regexp" "strconv" "strings" + "github.com/golang/glog" utilexec "k8s.io/utils/exec" ) @@ -34,10 +36,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 @@ -85,6 +87,39 @@ 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. +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 + } + } + // check set type + if valid := validateIPSetType(set.SetType); !valid { + return false + } + // check port range for bitmap type set + if set.SetType == BitmapPort { + if valid := validatePortRange(set.PortRange); !valid { + return false + } + } + // check hash size value of ipset + if set.HashSize <= 0 { + + return false + } + // check max elem value of ipset + if set.MaxElem <= 0 { + glog.Errorf("Invalid maxelem value %d, should be >0", set.MaxElem) + return false + } + + return true } // Entry represents a ipset entry. @@ -102,10 +137,92 @@ 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 { + 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 + if len(e.Protocol) == 0 { + e.Protocol = ProtocolTCP + } + + if net.ParseIP(e.IP) == nil { + 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 + } + case HashIPPortIP: + // set default protocol to tcp if empty + if len(e.Protocol) == 0 { + e.Protocol = ProtocolTCP + } + + if net.ParseIP(e.IP) == nil { + 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 + } + + // 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) + return false + } + case HashIPPortNet: + // set default protocol to tcp if empty + if len(e.Protocol) == 0 { + e.Protocol = ProtocolTCP + } + + if net.ParseIP(e.IP) == nil { + 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 + } + + // 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: + // 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) + return false + } + begin, end, err := parsePortRange(set.PortRange) + if err != nil { + 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 { + 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 +} + +// String returns the string format for ipset entry. func (e *Entry) String() string { switch e.SetType { case HashIPPort: @@ -141,28 +258,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 := set.Validate() + if !valid { + return fmt.Errorf("error creating ipset since it's invalid") + } return runner.createSet(set, ignoreExistErr) } @@ -178,12 +297,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 { @@ -198,8 +311,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") } @@ -309,19 +422,48 @@ func getIPSetVersionString(exec utilexec.Interface) (string, error) { return match[0], nil } +// 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 { strs := strings.Split(portRange, "-") if len(strs) != 2 { + glog.Errorf("port range should be in the format of `a-b`") return false } for i := range strs { - if _, err := strconv.Atoi(strs[i]); err != nil { + num, err := strconv.Atoi(strs[i]) + if err != nil { + glog.Errorf("Failed to parse %s, error: %v", strs[i], err) + return false + } + if num < 0 { + glog.Errorf("port number %d should be >=0", num) return false } } return true } +// checks if the given ipset type is valid. +func validateIPSetType(set Type) bool { + for _, valid := range ValidIPSetTypes { + if set == valid { + return true + } + } + glog.Errorf("Currently supported ipset types are: %v, %s is not supported", ValidIPSetTypes, set) + return false +} + +// checks if given hash family is supported in ipset +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 +} + // IsNotFoundError returns true if the error indicates "not found". It parses // the error string looking for known values, which is imperfect but works in // practice. @@ -341,4 +483,49 @@ func IsNotFoundError(err error) bool { return false } +// 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 +} + +// 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 71d4ce26d5..eaab924016 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") } @@ -308,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) @@ -481,3 +486,817 @@ 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) + } + } +} + +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/testing/fake.go b/pkg/util/ipset/testing/fake.go index 2a58bdd399..79f8f38554 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 } diff --git a/pkg/util/ipset/testing/fake_test.go b/pkg/util/ipset/testing/fake_test.go index 2128395cf8..f1f5d1e341 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) diff --git a/pkg/util/ipset/types.go b/pkg/util/ipset/types.go index d2406c0087..cacfd6f8d8 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 -}