fix: [NPM] check for valid ipv4 podip while adding to sets (#1291)
* fix: [NPM] check for valid ipv4 podip while adding to sets * checking for valid ip in controller * fixing uts * fixing uts * adding cidr block check as member * addressing comments * addressing comments * Fixing an additional member type validation * changing the check to include current cidr check * changing the check to include current cidr check
This commit is contained in:
Vamsi Kalapala
GitHub
Родитель
e384fdaa4b
Коммит
b9c52b09bb
|
|
@ -13,6 +13,7 @@ import (
|
|||
"github.com/Azure/azure-container-networking/npm/pkg/dataplane"
|
||||
"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
|
||||
"github.com/Azure/azure-container-networking/npm/util"
|
||||
npmerrors "github.com/Azure/azure-container-networking/npm/util/errors"
|
||||
"github.com/pkg/errors"
|
||||
corev1 "k8s.io/api/core/v1"
|
||||
apierrors "k8s.io/apimachinery/pkg/api/errors"
|
||||
|
|
@ -385,9 +386,16 @@ func (c *PodController) syncPod(key string) error {
|
|||
}
|
||||
|
||||
func (c *PodController) syncAddedPod(podObj *corev1.Pod) error {
|
||||
klog.Infof("POD CREATING: [%s%s/%s/%s%+v%s]", string(podObj.GetUID()), podObj.Namespace,
|
||||
klog.Infof("POD CREATING: [%s/%s/%s/%s/%+v/%s]", string(podObj.GetUID()), podObj.Namespace,
|
||||
podObj.Name, podObj.Spec.NodeName, podObj.Labels, podObj.Status.PodIP)
|
||||
|
||||
if !util.IsIPV4(podObj.Status.PodIP) {
|
||||
msg := fmt.Sprintf("[syncAddedPod] Error: ADD POD [%s/%s/%s/%+v/%s] failed as the PodIP is not valid ipv4 address", podObj.Namespace,
|
||||
podObj.Name, podObj.Spec.NodeName, podObj.Labels, podObj.Status.PodIP)
|
||||
metrics.SendErrorLogAndMetric(util.PodID, msg)
|
||||
return npmerrors.Errorf(npmerrors.AddPod, true, msg)
|
||||
}
|
||||
|
||||
var err error
|
||||
podKey, _ := cache.MetaNamespaceKeyFunc(podObj)
|
||||
|
||||
|
|
|
|||
|
|
@ -331,11 +331,7 @@ func (dp *DataPlane) createIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
|
|||
// ipblock can have either cidr (CIDR in IPBlock) or "cidr + " " (space) + nomatch" (Except in IPBlock)
|
||||
// (TODO) need to revise it for windows
|
||||
for _, ipblock := range set.Members {
|
||||
err := ipsets.ValidateIPBlock(ipblock)
|
||||
if err != nil {
|
||||
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to parseCIDR in addIPSetReferences with err: %s", err.Error()))
|
||||
}
|
||||
err = dp.ipsetMgr.AddToSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
|
||||
err := dp.ipsetMgr.AddToSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
|
||||
if err != nil {
|
||||
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to AddToSet in addIPSetReferences with err: %s", err.Error()))
|
||||
}
|
||||
|
|
@ -378,11 +374,7 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
|
|||
// ipblock can have either cidr (CIDR in IPBlock) or "cidr + " " (space) + nomatch" (Except in IPBlock)
|
||||
// (TODO) need to revise it for windows
|
||||
for _, ipblock := range set.Members {
|
||||
err := ipsets.ValidateIPBlock(ipblock)
|
||||
if err != nil {
|
||||
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to parseCIDR in deleteIPSetReferences with err: %s", err.Error()))
|
||||
}
|
||||
err = dp.ipsetMgr.RemoveFromSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
|
||||
err := dp.ipsetMgr.RemoveFromSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
|
||||
if err != nil {
|
||||
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to RemoveFromSet in deleteIPSetReferences with err: %s", err.Error()))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -162,7 +162,7 @@ func TestAddToSet(t *testing.T) {
|
|||
v6PodMetadata := NewPodMetadata("testns/a", "2001:db8:0:0:0:0:2:1", nodeName)
|
||||
// Test IPV6 addess it should error out
|
||||
err = dp.AddToSets(setsTocreate, v6PodMetadata)
|
||||
require.NoError(t, err)
|
||||
require.Error(t, err)
|
||||
|
||||
for _, v := range setsTocreate {
|
||||
dp.DeleteIPSet(v, util.SoftDelete)
|
||||
|
|
@ -178,7 +178,7 @@ func TestAddToSet(t *testing.T) {
|
|||
require.NoError(t, err)
|
||||
|
||||
err = dp.RemoveFromSets(setsTocreate, v6PodMetadata)
|
||||
require.NoError(t, err)
|
||||
require.Error(t, err)
|
||||
|
||||
for _, v := range setsTocreate {
|
||||
dp.DeleteIPSet(v, util.SoftDelete)
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ package ipsets
|
|||
|
||||
import (
|
||||
"fmt"
|
||||
"strings"
|
||||
"sync"
|
||||
|
||||
"github.com/Azure/azure-container-networking/common"
|
||||
|
|
@ -210,6 +211,13 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin
|
|||
if len(addToSets) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
if !validateIPSetMemberIP(ip) {
|
||||
msg := fmt.Sprintf("error: failed to add to sets: invalid ip %s", ip)
|
||||
metrics.SendErrorLogAndMetric(util.IpsmID, msg)
|
||||
return npmerrors.Errorf(npmerrors.AppendIPSet, true, msg)
|
||||
}
|
||||
|
||||
// TODO check if the IP is IPV4 family in controller
|
||||
iMgr.Lock()
|
||||
defer iMgr.Unlock()
|
||||
|
|
@ -242,6 +250,13 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po
|
|||
if len(removeFromSets) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
if !validateIPSetMemberIP(ip) {
|
||||
msg := fmt.Sprintf("error: failed to add to sets: invalid ip %s", ip)
|
||||
metrics.SendErrorLogAndMetric(util.IpsmID, msg)
|
||||
return npmerrors.Errorf(npmerrors.AppendIPSet, true, msg)
|
||||
}
|
||||
|
||||
iMgr.Lock()
|
||||
defer iMgr.Unlock()
|
||||
|
||||
|
|
@ -316,6 +331,10 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat
|
|||
// 3. add all members to the list
|
||||
for _, memberMetadata := range setMetadatas {
|
||||
memberName := memberMetadata.GetPrefixName()
|
||||
if memberName == "" {
|
||||
metrics.SendErrorLogAndMetric(util.IpsmID, "[AddToLists] warning: adding empty member name to list %s", list.Name)
|
||||
continue
|
||||
}
|
||||
// the member shouldn't be the list itself, but this is satisfied since we already asserted that the member is a HashSet
|
||||
if list.hasMember(memberName) {
|
||||
continue
|
||||
|
|
@ -361,6 +380,10 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
|
|||
modified := false
|
||||
for _, setMetadata := range setMetadatas {
|
||||
memberName := setMetadata.GetPrefixName()
|
||||
if memberName == "" {
|
||||
metrics.SendErrorLogAndMetric(util.IpsmID, "[RemoveFromList] warning: removing empty member name from list %s", list.Name)
|
||||
continue
|
||||
}
|
||||
member, exists := iMgr.setMap[memberName]
|
||||
if !exists {
|
||||
continue
|
||||
|
|
@ -531,3 +554,22 @@ func (iMgr *IPSetManager) clearDirtyCache() {
|
|||
iMgr.toAddOrUpdateCache = make(map[string]struct{})
|
||||
iMgr.toDeleteCache = make(map[string]struct{})
|
||||
}
|
||||
|
||||
// validateIPSetMemberIP helps valid if a member added to an HashSet has valid IP or CIDR
|
||||
func validateIPSetMemberIP(ip string) bool {
|
||||
// possible formats
|
||||
// 192.168.0.1
|
||||
// 192.168.0.1,tcp:25227
|
||||
// 192.168.0.1 nomatch
|
||||
// 192.168.0.0/24
|
||||
// 192.168.0.0/24,tcp:25227
|
||||
// 192.168.0.0/24 nomatch
|
||||
// always guaranteed to have ip, not guaranteed to have port + protocol
|
||||
ipDetails := strings.Split(ip, ",")
|
||||
if util.IsIPV4(ipDetails[0]) {
|
||||
return true
|
||||
}
|
||||
|
||||
err := ValidateIPBlock(ip)
|
||||
return err == nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -67,6 +67,7 @@ var (
|
|||
|
||||
namespaceSet = NewIPSetMetadata("test-set1", Namespace)
|
||||
keyLabelOfPodSet = NewIPSetMetadata("test-set2", KeyLabelOfPod)
|
||||
portSet = NewIPSetMetadata("test-set3", NamedPorts)
|
||||
list = NewIPSetMetadata("test-list1", KeyLabelOfNamespace)
|
||||
)
|
||||
|
||||
|
|
@ -485,6 +486,40 @@ func TestAddToSets(t *testing.T) {
|
|||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "Apply On Need: cidr add to new set",
|
||||
args: args{
|
||||
cfg: applyOnNeedCfg,
|
||||
toCreateMetadatas: nil,
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "10.0.0.0/8",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: namespaceSet, members: []member{{"10.0.0.0/8", isHashMember}}},
|
||||
},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "Apply On Need: bad cidr add to new set",
|
||||
args: args{
|
||||
cfg: applyOnNeedCfg,
|
||||
toCreateMetadatas: nil,
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "310.0.0.0/8",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add IPv6",
|
||||
args: args{
|
||||
|
|
@ -495,7 +530,25 @@ func TestAddToSets(t *testing.T) {
|
|||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: namespaceSet, members: []member{{ipv6, isHashMember}}},
|
||||
{metadata: namespaceSet, members: []member{}},
|
||||
},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add cidr",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "10.0.0.0/8",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: namespaceSet, members: []member{{"10.0.0.0/8", isHashMember}}},
|
||||
},
|
||||
toAddUpdateCache: []*IPSetMetadata{namespaceSet},
|
||||
toDeleteCache: nil,
|
||||
|
|
@ -503,6 +556,70 @@ func TestAddToSets(t *testing.T) {
|
|||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "add bad cidr",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "310.0.0.0/8",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add bad cidr 2",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "x.x.x.x/8",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add bad ip 1",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "x.x.x.x",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add bad ip port 1",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "x.x.x.x,80",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add existing IP to set (same pod key)",
|
||||
args: args{
|
||||
|
|
@ -590,6 +707,78 @@ func TestAddToSets(t *testing.T) {
|
|||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add empty ip",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: "",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: namespaceSet, members: []member{}},
|
||||
},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add empty ip with port",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
toAddMetadatas: []*IPSetMetadata{namespaceSet},
|
||||
member: ",80",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: namespaceSet, members: []member{}},
|
||||
},
|
||||
toAddUpdateCache: nil,
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: nil,
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add ipv4 with port",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{portSet},
|
||||
toAddMetadatas: []*IPSetMetadata{portSet},
|
||||
member: "1.1.1.1,80",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: portSet, members: []member{{"1.1.1.1,80", isHashMember}}},
|
||||
},
|
||||
toAddUpdateCache: []*IPSetMetadata{portSet},
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: []*IPSetMetadata{portSet},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "add cidr with nomatch",
|
||||
args: args{
|
||||
cfg: applyAlwaysCfg,
|
||||
toCreateMetadatas: []*IPSetMetadata{portSet},
|
||||
toAddMetadatas: []*IPSetMetadata{portSet},
|
||||
member: "10.10.2.0/28 nomatch",
|
||||
},
|
||||
expectedInfo: expectedInfo{
|
||||
mainCache: []setMembers{
|
||||
{metadata: portSet, members: []member{{"10.10.2.0/28 nomatch", isHashMember}}},
|
||||
},
|
||||
toAddUpdateCache: []*IPSetMetadata{portSet},
|
||||
toDeleteCache: nil,
|
||||
setsForKernel: []*IPSetMetadata{portSet},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
tt := tt
|
||||
|
|
|
|||
|
|
@ -60,6 +60,7 @@ const (
|
|||
AddNetPolReference = "AddNetPolReference"
|
||||
DeleteNetPolReference = "DeleteNetPolReference"
|
||||
RunFileCreator = "RunCommandWithFile"
|
||||
AddPod = "AddPod"
|
||||
)
|
||||
|
||||
// Error codes for ipsetmanager
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ package util
|
|||
import (
|
||||
"fmt"
|
||||
"hash/fnv"
|
||||
"net"
|
||||
"os"
|
||||
"regexp"
|
||||
"runtime"
|
||||
|
|
@ -352,3 +353,12 @@ func CompareSlices(list1, list2 []string) bool {
|
|||
func SliceToString(list []string) string {
|
||||
return strings.Join(list, SetPolicyDelimiter)
|
||||
}
|
||||
|
||||
func IsIPV4(ip string) bool {
|
||||
if net.ParseIP(ip).To4() != nil {
|
||||
return true
|
||||
}
|
||||
|
||||
_, _, err := net.ParseCIDR(ip)
|
||||
return err == nil
|
||||
}
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче