Skip to content

fix: [NPM] check for valid ipv4 podip while adding to sets #1291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 21, 2022
Merged

fix: [NPM] check for valid ipv4 podip while adding to sets #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fe78e79
fix: [NPM] check for valid ipv4 podip while adding to sets
vakalapa Mar 17, 2022
69b1d9f
checking for valid ip in controller
vakalapa Mar 17, 2022
5ff4610
fixing uts
vakalapa Mar 17, 2022
fe4a577
fixing uts
vakalapa Mar 17, 2022
2200dfd
adding cidr block check as member
vakalapa Mar 17, 2022
02d988b
addressing comments
vakalapa Mar 17, 2022
6d014fa
addressing comments
vakalapa Mar 18, 2022
5efb631
Fixing an additional member type validation
vakalapa Mar 18, 2022
00c0e6c
changing the check to include current cidr check
vakalapa Mar 18, 2022
2a5238d
changing the check to include current cidr check
vakalapa Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion npm/pkg/controlplane/controllers/v2/podController.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

Expand Down
12 changes: 2 additions & 10 deletions npm/pkg/dataplane/dataplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
}
Expand Down Expand Up @@ -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()))
}
Expand Down
4 changes: 2 additions & 2 deletions npm/pkg/dataplane/dataplane_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
42 changes: 42 additions & 0 deletions npm/pkg/dataplane/ipsets/ipsetmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ipsets

import (
"fmt"
"strings"
"sync"

"github.com/Azure/azure-container-networking/common"
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
191 changes: 190 additions & 1 deletion npm/pkg/dataplane/ipsets/ipsetmanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)

Expand Down Expand Up @@ -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{
Expand All @@ -495,14 +530,96 @@ 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,
setsForKernel: []*IPSetMetadata{namespaceSet},
},
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{
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions npm/util/errors/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const (
AddNetPolReference = "AddNetPolReference"
DeleteNetPolReference = "DeleteNetPolReference"
RunFileCreator = "RunCommandWithFile"
AddPod = "AddPod"
)

// Error codes for ipsetmanager
Expand Down
Loading