Merge pull request #57268 from m1093782566/validate-ipset

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

validate ipset set and entry - ipvs proxier validate entry before adding it and validate set before creating it

**What this PR does / why we need it**:

validate ipset util.

**Which issue(s) this PR fixes**:
Fixes #57269

**Special notes for your reviewer**:

I splitted this PR into some small pieces for the sake of code review.

**Release note**:

```release-note
NONE
```

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)

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)
 			}

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 {
+		if proxier.masqueradeAll || len(proxier.clusterCIDR) > 0 {
-			proxier.clusterIPSet.activeEntries.Insert(entry.String())
+			if valid := proxier.clusterIPSet.validateEntry(entry); !valid {
-		} else if len(proxier.clusterCIDR) > 0 {
+				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

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(

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 adds a new entry to the named set.  It will ignore error when the entry already exists if ignoreExistErr=true.
-	AddEntry(entry string, set string, ignoreExistErr bool) error
+	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 len(set.PortRange) == 0 {
-	if !IsValidIPSetType(set.SetType) {
+		set.PortRange = DefaultPortRange
-		return fmt.Errorf("Currently supported ipset types are: %v, %s is not supported", ValidIPSetTypes, set.SetType)
 	}
 
+	// 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 {
+func (runner *runner) AddEntry(entry string, set *IPSet, ignoreExistErr bool) error {
-	args := []string{"add", set, entry}
+	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{})

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)
+		}
+	}
+}

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 {
+func (f *FakeIPSet) AddEntry(entry string, set *ipset.IPSet, ignoreExistErr bool) error {
-	if f.Entries[set].Has(entry) {
+	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
 }
 

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.1,tcp:8080", set, true)
-	fake.AddEntry("192.168.1.2,tcp:8081", set.Name, 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)

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
-}