diff --git a/npm/pkg/controlplane/controllers/v2/podController.go b/npm/pkg/controlplane/controllers/v2/podController.go index 2762078bd3..1d13fdf4d6 100644 --- a/npm/pkg/controlplane/controllers/v2/podController.go +++ b/npm/pkg/controlplane/controllers/v2/podController.go @@ -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) diff --git a/npm/pkg/dataplane/dataplane.go b/npm/pkg/dataplane/dataplane.go index 46edfea323..f78b91d5e8 100644 --- a/npm/pkg/dataplane/dataplane.go +++ b/npm/pkg/dataplane/dataplane.go @@ -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())) } diff --git a/npm/pkg/dataplane/dataplane_test.go b/npm/pkg/dataplane/dataplane_test.go index 9f9f48c219..008e580594 100644 --- a/npm/pkg/dataplane/dataplane_test.go +++ b/npm/pkg/dataplane/dataplane_test.go @@ -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) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 0703638718..29bab5b12e 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -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 +} diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 3af773af77..3128ad67d6 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -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 diff --git a/npm/util/errors/errors.go b/npm/util/errors/errors.go index c34803c4b8..baf47d261e 100644 --- a/npm/util/errors/errors.go +++ b/npm/util/errors/errors.go @@ -60,6 +60,7 @@ const ( AddNetPolReference = "AddNetPolReference" DeleteNetPolReference = "DeleteNetPolReference" RunFileCreator = "RunCommandWithFile" + AddPod = "AddPod" ) // Error codes for ipsetmanager diff --git a/npm/util/util.go b/npm/util/util.go index ac60c7a398..ce95c31acc 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -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 +}