Skip to content

Commit b9c52b0

Browse files
vakalapavakalapa
authored
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
1 parent e384fda commit b9c52b0

File tree

7 files changed

+256
-14
lines changed

7 files changed

+256
-14
lines changed

‎npm/pkg/controlplane/controllers/v2/podController.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/Azure/azure-container-networking/npm/pkg/dataplane"
1414
"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
1515
"github.com/Azure/azure-container-networking/npm/util"
16+
npmerrors "github.com/Azure/azure-container-networking/npm/util/errors"
1617
"github.com/pkg/errors"
1718
corev1 "k8s.io/api/core/v1"
1819
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -385,9 +386,16 @@ func (c *PodController) syncPod(key string) error {
385386
}
386387

387388
func (c *PodController) syncAddedPod(podObj *corev1.Pod) error {
388-
klog.Infof("POD CREATING: [%s%s/%s/%s%+v%s]", string(podObj.GetUID()), podObj.Namespace,
389+
klog.Infof("POD CREATING: [%s/%s/%s/%s/%+v/%s]", string(podObj.GetUID()), podObj.Namespace,
389390
podObj.Name, podObj.Spec.NodeName, podObj.Labels, podObj.Status.PodIP)
390391

392+
if !util.IsIPV4(podObj.Status.PodIP) {
393+
msg := fmt.Sprintf("[syncAddedPod] Error: ADD POD [%s/%s/%s/%+v/%s] failed as the PodIP is not valid ipv4 address", podObj.Namespace,
394+
podObj.Name, podObj.Spec.NodeName, podObj.Labels, podObj.Status.PodIP)
395+
metrics.SendErrorLogAndMetric(util.PodID, msg)
396+
return npmerrors.Errorf(npmerrors.AddPod, true, msg)
397+
}
398+
391399
var err error
392400
podKey, _ := cache.MetaNamespaceKeyFunc(podObj)
393401

‎npm/pkg/dataplane/dataplane.go‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -331,11 +331,7 @@ func (dp *DataPlane) createIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
331331
// ipblock can have either cidr (CIDR in IPBlock) or "cidr + " " (space) + nomatch" (Except in IPBlock)
332332
// (TODO) need to revise it for windows
333333
for _, ipblock := range set.Members {
334-
err := ipsets.ValidateIPBlock(ipblock)
335-
if err != nil {
336-
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to parseCIDR in addIPSetReferences with err: %s", err.Error()))
337-
}
338-
err = dp.ipsetMgr.AddToSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
334+
err := dp.ipsetMgr.AddToSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
339335
if err != nil {
340336
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to AddToSet in addIPSetReferences with err: %s", err.Error()))
341337
}
@@ -378,11 +374,7 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
378374
// ipblock can have either cidr (CIDR in IPBlock) or "cidr + " " (space) + nomatch" (Except in IPBlock)
379375
// (TODO) need to revise it for windows
380376
for _, ipblock := range set.Members {
381-
err := ipsets.ValidateIPBlock(ipblock)
382-
if err != nil {
383-
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to parseCIDR in deleteIPSetReferences with err: %s", err.Error()))
384-
}
385-
err = dp.ipsetMgr.RemoveFromSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
377+
err := dp.ipsetMgr.RemoveFromSets([]*ipsets.IPSetMetadata{set.Metadata}, ipblock, "")
386378
if err != nil {
387379
return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to RemoveFromSet in deleteIPSetReferences with err: %s", err.Error()))
388380
}

‎npm/pkg/dataplane/dataplane_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ func TestAddToSet(t *testing.T) {
162162
v6PodMetadata := NewPodMetadata("testns/a", "2001:db8:0:0:0:0:2:1", nodeName)
163163
// Test IPV6 addess it should error out
164164
err = dp.AddToSets(setsTocreate, v6PodMetadata)
165-
require.NoError(t, err)
165+
require.Error(t, err)
166166

167167
for _, v := range setsTocreate {
168168
dp.DeleteIPSet(v, util.SoftDelete)
@@ -178,7 +178,7 @@ func TestAddToSet(t *testing.T) {
178178
require.NoError(t, err)
179179

180180
err = dp.RemoveFromSets(setsTocreate, v6PodMetadata)
181-
require.NoError(t, err)
181+
require.Error(t, err)
182182

183183
for _, v := range setsTocreate {
184184
dp.DeleteIPSet(v, util.SoftDelete)

‎npm/pkg/dataplane/ipsets/ipsetmanager.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package ipsets
22

33
import (
44
"fmt"
5+
"strings"
56
"sync"
67

78
"github.com/Azure/azure-container-networking/common"
@@ -210,6 +211,13 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin
210211
if len(addToSets) == 0 {
211212
return nil
212213
}
214+
215+
if !validateIPSetMemberIP(ip) {
216+
msg := fmt.Sprintf("error: failed to add to sets: invalid ip %s", ip)
217+
metrics.SendErrorLogAndMetric(util.IpsmID, msg)
218+
return npmerrors.Errorf(npmerrors.AppendIPSet, true, msg)
219+
}
220+
213221
// TODO check if the IP is IPV4 family in controller
214222
iMgr.Lock()
215223
defer iMgr.Unlock()
@@ -242,6 +250,13 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po
242250
if len(removeFromSets) == 0 {
243251
return nil
244252
}
253+
254+
if !validateIPSetMemberIP(ip) {
255+
msg := fmt.Sprintf("error: failed to add to sets: invalid ip %s", ip)
256+
metrics.SendErrorLogAndMetric(util.IpsmID, msg)
257+
return npmerrors.Errorf(npmerrors.AppendIPSet, true, msg)
258+
}
259+
245260
iMgr.Lock()
246261
defer iMgr.Unlock()
247262

@@ -316,6 +331,10 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat
316331
// 3. add all members to the list
317332
for _, memberMetadata := range setMetadatas {
318333
memberName := memberMetadata.GetPrefixName()
334+
if memberName == "" {
335+
metrics.SendErrorLogAndMetric(util.IpsmID, "[AddToLists] warning: adding empty member name to list %s", list.Name)
336+
continue
337+
}
319338
// the member shouldn't be the list itself, but this is satisfied since we already asserted that the member is a HashSet
320339
if list.hasMember(memberName) {
321340
continue
@@ -361,6 +380,10 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
361380
modified := false
362381
for _, setMetadata := range setMetadatas {
363382
memberName := setMetadata.GetPrefixName()
383+
if memberName == "" {
384+
metrics.SendErrorLogAndMetric(util.IpsmID, "[RemoveFromList] warning: removing empty member name from list %s", list.Name)
385+
continue
386+
}
364387
member, exists := iMgr.setMap[memberName]
365388
if !exists {
366389
continue
@@ -531,3 +554,22 @@ func (iMgr *IPSetManager) clearDirtyCache() {
531554
iMgr.toAddOrUpdateCache = make(map[string]struct{})
532555
iMgr.toDeleteCache = make(map[string]struct{})
533556
}
557+
558+
// validateIPSetMemberIP helps valid if a member added to an HashSet has valid IP or CIDR
559+
func validateIPSetMemberIP(ip string) bool {
560+
// possible formats
561+
// 192.168.0.1
562+
// 192.168.0.1,tcp:25227
563+
// 192.168.0.1 nomatch
564+
// 192.168.0.0/24
565+
// 192.168.0.0/24,tcp:25227
566+
// 192.168.0.0/24 nomatch
567+
// always guaranteed to have ip, not guaranteed to have port + protocol
568+
ipDetails := strings.Split(ip, ",")
569+
if util.IsIPV4(ipDetails[0]) {
570+
return true
571+
}
572+
573+
err := ValidateIPBlock(ip)
574+
return err == nil
575+
}

‎npm/pkg/dataplane/ipsets/ipsetmanager_test.go‎

Lines changed: 190 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ var (
6767

6868
namespaceSet = NewIPSetMetadata("test-set1", Namespace)
6969
keyLabelOfPodSet = NewIPSetMetadata("test-set2", KeyLabelOfPod)
70+
portSet = NewIPSetMetadata("test-set3", NamedPorts)
7071
list = NewIPSetMetadata("test-list1", KeyLabelOfNamespace)
7172
)
7273

@@ -485,6 +486,40 @@ func TestAddToSets(t *testing.T) {
485486
},
486487
wantErr: false,
487488
},
489+
{
490+
name: "Apply On Need: cidr add to new set",
491+
args: args{
492+
cfg: applyOnNeedCfg,
493+
toCreateMetadatas: nil,
494+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
495+
member: "10.0.0.0/8",
496+
},
497+
expectedInfo: expectedInfo{
498+
mainCache: []setMembers{
499+
{metadata: namespaceSet, members: []member{{"10.0.0.0/8", isHashMember}}},
500+
},
501+
toAddUpdateCache: nil,
502+
toDeleteCache: nil,
503+
setsForKernel: nil,
504+
},
505+
wantErr: false,
506+
},
507+
{
508+
name: "Apply On Need: bad cidr add to new set",
509+
args: args{
510+
cfg: applyOnNeedCfg,
511+
toCreateMetadatas: nil,
512+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
513+
member: "310.0.0.0/8",
514+
},
515+
expectedInfo: expectedInfo{
516+
mainCache: []setMembers{},
517+
toAddUpdateCache: nil,
518+
toDeleteCache: nil,
519+
setsForKernel: nil,
520+
},
521+
wantErr: true,
522+
},
488523
{
489524
name: "add IPv6",
490525
args: args{
@@ -495,14 +530,96 @@ func TestAddToSets(t *testing.T) {
495530
},
496531
expectedInfo: expectedInfo{
497532
mainCache: []setMembers{
498-
{metadata: namespaceSet, members: []member{{ipv6, isHashMember}}},
533+
{metadata: namespaceSet, members: []member{}},
534+
},
535+
toAddUpdateCache: nil,
536+
toDeleteCache: nil,
537+
setsForKernel: nil,
538+
},
539+
wantErr: true,
540+
},
541+
{
542+
name: "add cidr",
543+
args: args{
544+
cfg: applyAlwaysCfg,
545+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
546+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
547+
member: "10.0.0.0/8",
548+
},
549+
expectedInfo: expectedInfo{
550+
mainCache: []setMembers{
551+
{metadata: namespaceSet, members: []member{{"10.0.0.0/8", isHashMember}}},
499552
},
500553
toAddUpdateCache: []*IPSetMetadata{namespaceSet},
501554
toDeleteCache: nil,
502555
setsForKernel: []*IPSetMetadata{namespaceSet},
503556
},
504557
wantErr: false,
505558
},
559+
{
560+
name: "add bad cidr",
561+
args: args{
562+
cfg: applyAlwaysCfg,
563+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
564+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
565+
member: "310.0.0.0/8",
566+
},
567+
expectedInfo: expectedInfo{
568+
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
569+
toAddUpdateCache: nil,
570+
toDeleteCache: nil,
571+
setsForKernel: nil,
572+
},
573+
wantErr: true,
574+
},
575+
{
576+
name: "add bad cidr 2",
577+
args: args{
578+
cfg: applyAlwaysCfg,
579+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
580+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
581+
member: "x.x.x.x/8",
582+
},
583+
expectedInfo: expectedInfo{
584+
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
585+
toAddUpdateCache: nil,
586+
toDeleteCache: nil,
587+
setsForKernel: nil,
588+
},
589+
wantErr: true,
590+
},
591+
{
592+
name: "add bad ip 1",
593+
args: args{
594+
cfg: applyAlwaysCfg,
595+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
596+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
597+
member: "x.x.x.x",
598+
},
599+
expectedInfo: expectedInfo{
600+
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
601+
toAddUpdateCache: nil,
602+
toDeleteCache: nil,
603+
setsForKernel: nil,
604+
},
605+
wantErr: true,
606+
},
607+
{
608+
name: "add bad ip port 1",
609+
args: args{
610+
cfg: applyAlwaysCfg,
611+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
612+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
613+
member: "x.x.x.x,80",
614+
},
615+
expectedInfo: expectedInfo{
616+
mainCache: []setMembers{{metadata: namespaceSet, members: []member{}}},
617+
toAddUpdateCache: nil,
618+
toDeleteCache: nil,
619+
setsForKernel: nil,
620+
},
621+
wantErr: true,
622+
},
506623
{
507624
name: "add existing IP to set (same pod key)",
508625
args: args{
@@ -590,6 +707,78 @@ func TestAddToSets(t *testing.T) {
590707
},
591708
wantErr: true,
592709
},
710+
{
711+
name: "add empty ip",
712+
args: args{
713+
cfg: applyAlwaysCfg,
714+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
715+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
716+
member: "",
717+
},
718+
expectedInfo: expectedInfo{
719+
mainCache: []setMembers{
720+
{metadata: namespaceSet, members: []member{}},
721+
},
722+
toAddUpdateCache: nil,
723+
toDeleteCache: nil,
724+
setsForKernel: nil,
725+
},
726+
wantErr: true,
727+
},
728+
{
729+
name: "add empty ip with port",
730+
args: args{
731+
cfg: applyAlwaysCfg,
732+
toCreateMetadatas: []*IPSetMetadata{namespaceSet},
733+
toAddMetadatas: []*IPSetMetadata{namespaceSet},
734+
member: ",80",
735+
},
736+
expectedInfo: expectedInfo{
737+
mainCache: []setMembers{
738+
{metadata: namespaceSet, members: []member{}},
739+
},
740+
toAddUpdateCache: nil,
741+
toDeleteCache: nil,
742+
setsForKernel: nil,
743+
},
744+
wantErr: true,
745+
},
746+
{
747+
name: "add ipv4 with port",
748+
args: args{
749+
cfg: applyAlwaysCfg,
750+
toCreateMetadatas: []*IPSetMetadata{portSet},
751+
toAddMetadatas: []*IPSetMetadata{portSet},
752+
member: "1.1.1.1,80",
753+
},
754+
expectedInfo: expectedInfo{
755+
mainCache: []setMembers{
756+
{metadata: portSet, members: []member{{"1.1.1.1,80", isHashMember}}},
757+
},
758+
toAddUpdateCache: []*IPSetMetadata{portSet},
759+
toDeleteCache: nil,
760+
setsForKernel: []*IPSetMetadata{portSet},
761+
},
762+
wantErr: false,
763+
},
764+
{
765+
name: "add cidr with nomatch",
766+
args: args{
767+
cfg: applyAlwaysCfg,
768+
toCreateMetadatas: []*IPSetMetadata{portSet},
769+
toAddMetadatas: []*IPSetMetadata{portSet},
770+
member: "10.10.2.0/28 nomatch",
771+
},
772+
expectedInfo: expectedInfo{
773+
mainCache: []setMembers{
774+
{metadata: portSet, members: []member{{"10.10.2.0/28 nomatch", isHashMember}}},
775+
},
776+
toAddUpdateCache: []*IPSetMetadata{portSet},
777+
toDeleteCache: nil,
778+
setsForKernel: []*IPSetMetadata{portSet},
779+
},
780+
wantErr: false,
781+
},
593782
}
594783
for _, tt := range tests {
595784
tt := tt

‎npm/util/errors/errors.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ const (
6060
AddNetPolReference = "AddNetPolReference"
6161
DeleteNetPolReference = "DeleteNetPolReference"
6262
RunFileCreator = "RunCommandWithFile"
63+
AddPod = "AddPod"
6364
)
6465

6566
// Error codes for ipsetmanager

0 commit comments

Comments
 (0)