fix review comments

pull/6/head
m1093782566 2017-12-22 09:46:18 +08:00
parent 4df6662d56
commit 477b0f0636
6 changed files with 122 additions and 77 deletions

View File

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

View File

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

View File

@ -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())

View File

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

View File

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

View File

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