diff --git a/.github/workflows/cyclonus-netpol-test.yaml b/.github/workflows/cyclonus-netpol-test.yaml index 7a310fd674..f62a630077 100644 --- a/.github/workflows/cyclonus-netpol-test.yaml +++ b/.github/workflows/cyclonus-netpol-test.yaml @@ -20,7 +20,7 @@ jobs: strategy: matrix: # run cyclonus tests in parallel for NPM with the given ConfigMaps - profile: [v1-default.yaml, v1-place-azure-chain-first.yaml] + profile: [v1-default.yaml, v1-place-azure-chain-first.yaml, v2-default.yaml] steps: - name: Checkout uses: actions/checkout@v2 diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 46fa8be5bd..424e74807f 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -80,7 +80,7 @@ func namedPortRuleInfo(portRule *networkingv1.NetworkPolicyPort) (namedPortIPSet return nil, protocol } - namedPortIPSet = ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+portRule.Port.String(), ipsets.NamedPorts) + namedPortIPSet = ipsets.NewTranslatedIPSet(portRule.Port.String(), ipsets.NamedPorts) return namedPortIPSet, protocol } @@ -90,7 +90,7 @@ func namedPortRule(portRule *networkingv1.NetworkPolicyPort) (*ipsets.Translated } namedPortIPSet, protocol := namedPortRuleInfo(portRule) - setInfo := policies.NewSetInfo(util.NamedPortIPSetPrefix+portRule.Port.String(), ipsets.NamedPorts, included, policies.DstDstMatch) + setInfo := policies.NewSetInfo(portRule.Port.String(), ipsets.NamedPorts, included, policies.DstDstMatch) return namedPortIPSet, setInfo, protocol } @@ -481,7 +481,7 @@ func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, egress []networkingv1.Ne } // #2. If egress is nil (in yaml file, it is specified with '[]'), it means "Deny all" - it does not allow sending traffic to others. - if egress != nil { + if egress == nil { // Except for allow all traffic case in #1, the rest of them should have default drop rules. dropACL := defaultDropACL(npmNetPol.NameSpace, npmNetPol.Name, policies.Egress) npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL) @@ -503,10 +503,7 @@ func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, egress []networkingv1.Ne // TranslatePolicy traslates networkpolicy object to NPMNetworkPolicy object // and return the NPMNetworkPolicy object. func TranslatePolicy(npObj *networkingv1.NetworkPolicy) *policies.NPMNetworkPolicy { - npmNetPol := &policies.NPMNetworkPolicy{ - Name: npObj.ObjectMeta.Name, - NameSpace: npObj.ObjectMeta.Namespace, - } + npmNetPol := policies.NewNPMNetworkPolicy(npObj.Name, npObj.Namespace) // podSelector in spec.PodSelector is common for ingress and egress. // Process this podSelector first. @@ -522,6 +519,7 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) *policies.NPMNetworkPoli egressPolicy(npmNetPol, npObj.Spec.Egress) } } - + klog.Infof("JUST-TRANSLATED-THIS-POLICY:\n%s", npmNetPol.String()) + klog.Infof("THIS-NPOBJ:\n%+v", npObj) return npmNetPol } diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index d40c441a9f..59188cc05d 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -192,7 +192,7 @@ func TestNamedPortRuleInfo(t *testing.T) { }, want: &namedPortOutput{ - translatedIPSet: ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + translatedIPSet: ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), protocol: "TCP", }, }, @@ -202,7 +202,7 @@ func TestNamedPortRuleInfo(t *testing.T) { Port: &namedPort, }, want: &namedPortOutput{ - translatedIPSet: ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + translatedIPSet: ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), protocol: "TCP", }, }, @@ -253,8 +253,8 @@ func TestNamedPortRule(t *testing.T) { Port: &namedPort, }, want: &namedPortRuleOutput{ - translatedIPSet: ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), - setInfo: policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), + translatedIPSet: ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), + setInfo: policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), protocol: "TCP", }, }, @@ -264,8 +264,8 @@ func TestNamedPortRule(t *testing.T) { Port: &namedPort, }, want: &namedPortRuleOutput{ - translatedIPSet: ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), - setInfo: policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), + translatedIPSet: ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), + setInfo: policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), protocol: "TCP", }, }, @@ -970,11 +970,11 @@ func TestPortRuleWithNamedPort(t *testing.T) { Port: &namedPort, }, ruleIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), }, acl: &policies.ACLPolicy{ DstList: []policies.SetInfo{ - policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, matchType), + policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, matchType), }, Protocol: "TCP", }, @@ -985,11 +985,11 @@ func TestPortRuleWithNamedPort(t *testing.T) { Port: &namedPort, }, ruleIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), }, acl: &policies.ACLPolicy{ DstList: []policies.SetInfo{ - policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, matchType), + policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, matchType), }, Protocol: "TCP", }, @@ -1159,7 +1159,7 @@ func TestPeerAndPortRule(t *testing.T) { Name: namedPortStr, NameSpace: "default", RuleIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), }, ACLs: []*policies.ACLPolicy{ { @@ -1168,7 +1168,7 @@ func TestPeerAndPortRule(t *testing.T) { Direction: policies.Ingress, SrcList: []policies.SetInfo{}, DstList: []policies.SetInfo{ - policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), + policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), }, Protocol: "TCP", }, @@ -1187,7 +1187,7 @@ func TestPeerAndPortRule(t *testing.T) { Name: namedPortStr, NameSpace: "default", RuleIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), }, ACLs: []*policies.ACLPolicy{ { @@ -1198,7 +1198,7 @@ func TestPeerAndPortRule(t *testing.T) { policies.NewSetInfo("test-in-ns-default-0IN", ipsets.CIDRBlocks, included, matchType), }, DstList: []policies.SetInfo{ - policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), + policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), }, Protocol: "TCP", }, @@ -1217,7 +1217,7 @@ func TestPeerAndPortRule(t *testing.T) { Name: namedPortStr, NameSpace: "default", RuleIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), }, ACLs: []*policies.ACLPolicy{ { @@ -1226,7 +1226,7 @@ func TestPeerAndPortRule(t *testing.T) { Direction: policies.Ingress, SrcList: []policies.SetInfo{}, DstList: []policies.SetInfo{ - policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), + policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), }, Protocol: "TCP", }, @@ -1245,7 +1245,7 @@ func TestPeerAndPortRule(t *testing.T) { Name: namedPortStr, NameSpace: "default", RuleIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts), + ipsets.NewTranslatedIPSet("serve-tcp", ipsets.NamedPorts), }, ACLs: []*policies.ACLPolicy{ { @@ -1254,7 +1254,7 @@ func TestPeerAndPortRule(t *testing.T) { Direction: policies.Ingress, SrcList: []policies.SetInfo{}, DstList: []policies.SetInfo{ - policies.NewSetInfo(util.NamedPortIPSetPrefix+"serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), + policies.NewSetInfo("serve-tcp", ipsets.NamedPorts, included, policies.DstDstMatch), }, Protocol: "TCP", }, diff --git a/npm/pkg/dataplane/dataplane.go b/npm/pkg/dataplane/dataplane.go index 2b97e206bb..5baf87eab7 100644 --- a/npm/pkg/dataplane/dataplane.go +++ b/npm/pkg/dataplane/dataplane.go @@ -48,6 +48,7 @@ type NPMEndpoint struct { Name string ID string IP string + // TODO: check it may use PolicyKey instead of Policy name // Map with Key as Network Policy name to to emulate set // and value as struct{} for minimal memory consumption NetPolReference map[string]struct{} @@ -207,16 +208,16 @@ func (dp *DataPlane) ApplyDataPlane() error { // AddPolicy takes in a translated NPMNetworkPolicy object and applies on dataplane func (dp *DataPlane) AddPolicy(policy *policies.NPMNetworkPolicy) error { - klog.Infof("[DataPlane] Add Policy called for %s", policy.Name) + klog.Infof("[DataPlane] Add Policy called for %s", policy.PolicyKey) // Create and add references for Selector IPSets first - err := dp.createIPSetsAndReferences(policy.PodSelectorIPSets, policy.Name, ipsets.SelectorType) + err := dp.createIPSetsAndReferences(policy.PodSelectorIPSets, policy.PolicyKey, ipsets.SelectorType) if err != nil { klog.Infof("[DataPlane] error while adding Selector IPSet references: %s", err.Error()) return fmt.Errorf("[DataPlane] error while adding Selector IPSet references: %w", err) } // Create and add references for Rule IPSets - err = dp.createIPSetsAndReferences(policy.RuleIPSets, policy.Name, ipsets.NetPolType) + err = dp.createIPSetsAndReferences(policy.RuleIPSets, policy.PolicyKey, ipsets.NetPolType) if err != nil { klog.Infof("[DataPlane] error while adding Rule IPSet references: %s", err.Error()) return fmt.Errorf("[DataPlane] error while adding Rule IPSet references: %w", err) @@ -239,29 +240,29 @@ func (dp *DataPlane) AddPolicy(policy *policies.NPMNetworkPolicy) error { return nil } -// RemovePolicy takes in network policy name and removes it from dataplane and cache -func (dp *DataPlane) RemovePolicy(policyName string) error { - klog.Infof("[DataPlane] Remove Policy called for %s", policyName) +// RemovePolicy takes in network policyKey (namespace/name of network policy) and removes it from dataplane and cache +func (dp *DataPlane) RemovePolicy(policyKey string) error { + klog.Infof("[DataPlane] Remove Policy called for %s", policyKey) // because policy Manager will remove from policy from cache // keep a local copy to remove references for ipsets - policy, ok := dp.policyMgr.GetPolicy(policyName) + policy, ok := dp.policyMgr.GetPolicy(policyKey) if !ok { - klog.Infof("[DataPlane] Policy %s is not found. Might been deleted already", policyName) + klog.Infof("[DataPlane] Policy %s is not found. Might been deleted already", policyKey) return nil } // Use the endpoint list saved in cache for this network policy to remove - err := dp.policyMgr.RemovePolicy(policy.Name, nil) + err := dp.policyMgr.RemovePolicy(policy.PolicyKey, nil) if err != nil { return fmt.Errorf("[DataPlane] error while removing policy: %w", err) } // Remove references for Rule IPSets first - err = dp.deleteIPSetsAndReferences(policy.RuleIPSets, policy.Name, ipsets.NetPolType) + err = dp.deleteIPSetsAndReferences(policy.RuleIPSets, policy.PolicyKey, ipsets.NetPolType) if err != nil { return err } // Remove references for Selector IPSets - err = dp.deleteIPSetsAndReferences(policy.PodSelectorIPSets, policy.Name, ipsets.SelectorType) + err = dp.deleteIPSetsAndReferences(policy.PodSelectorIPSets, policy.PolicyKey, ipsets.SelectorType) if err != nil { return err } @@ -277,10 +278,10 @@ func (dp *DataPlane) RemovePolicy(policyName string) error { // UpdatePolicy takes in updated policy object, calculates the delta and applies changes // onto dataplane accordingly func (dp *DataPlane) UpdatePolicy(policy *policies.NPMNetworkPolicy) error { - klog.Infof("[DataPlane] Update Policy called for %s", policy.Name) - ok := dp.policyMgr.PolicyExists(policy.Name) + klog.Infof("[DataPlane] Update Policy called for %s", policy.PolicyKey) + ok := dp.policyMgr.PolicyExists(policy.PolicyKey) if !ok { - klog.Infof("[DataPlane] Policy %s is not found. Might been deleted already", policy.Name) + klog.Infof("[DataPlane] Policy %s is not found. Might been deleted already", policy.PolicyKey) return dp.AddPolicy(policy) } @@ -288,7 +289,7 @@ func (dp *DataPlane) UpdatePolicy(policy *policies.NPMNetworkPolicy) error { // and remove/apply only the delta of IPSets and policies // Taking the easy route here, delete existing policy - err := dp.RemovePolicy(policy.Name) + err := dp.RemovePolicy(policy.PolicyKey) if err != nil { return fmt.Errorf("[DataPlane] error while updating policy: %w", err) } @@ -321,22 +322,14 @@ func (dp *DataPlane) createIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n // Check if any CIDR block IPSets needs to be applied setType := set.Metadata.Type if setType == ipsets.CIDRBlocks { - // cidrInfo can have either cidr (CIDR in IPBlock) or "cidr + " " (space) + nomatch" (Except in IPBlock) - for _, cidrInfo := range set.Members { - // TODO(jungukcho): This is an adhoc approach for linux, but need to refactor data structure for better management. - // onlyCidr has only cidr without "nomatch" to validate cidr format. - var onlyCidr string - if strings.Contains(cidrInfo, util.IpsetNomatch) { - onlyCidr = strings.Trim(onlyCidr, util.IpsetNomatch) - } else { - onlyCidr = cidrInfo - } - - _, _, err := net.ParseCIDR(onlyCidr) + // 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 := 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}, cidrInfo, "") + 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())) } @@ -377,12 +370,14 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n // Check if any CIDR block IPSets needs to be applied setType := set.Metadata.Type if setType == ipsets.CIDRBlocks { - for _, ip := range set.Members { - _, _, err := net.ParseCIDR(ip) + // 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 := 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}, ip, "") + 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())) } @@ -402,6 +397,18 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n return nil } +// TODO: This is an adhoc approach for linux, but need to refactor data structure for better management. +func validateIPBlock(ipblock string) error { + // TODO: This is fragile code with strong dependency with " "(space). + // onlyCidr has only cidr without "space" and "nomatch" in case except ipblock to validate cidr format. + onlyCidr := strings.Split(ipblock, " ")[0] + _, _, err := net.ParseCIDR(onlyCidr) + if err != nil { + return npmerrors.SimpleErrorWrapper("failed to parse CIDR", err) + } + return nil +} + func getMembersOfTranslatedSets(members []string) []*ipsets.IPSetMetadata { memberList := make([]*ipsets.IPSetMetadata, len(members)) i := 0 diff --git a/npm/pkg/dataplane/dataplane_test.go b/npm/pkg/dataplane/dataplane_test.go index 57395637b3..53db432c8c 100644 --- a/npm/pkg/dataplane/dataplane_test.go +++ b/npm/pkg/dataplane/dataplane_test.go @@ -25,7 +25,9 @@ var ( Metadata: ipsets.NewIPSetMetadata("setpodkey1", ipsets.KeyLabelOfPod), } testPolicyobj = policies.NPMNetworkPolicy{ - Name: "ns1/testpolicy", + Name: "testpolicy", + NameSpace: "ns1", + PolicyKey: "ns1/testpolicy", PodSelectorIPSets: []*ipsets.TranslatedIPSet{ { Metadata: ipsets.NewIPSetMetadata("setns1", ipsets.Namespace), @@ -245,7 +247,7 @@ func TestRemovePolicy(t *testing.T) { err = dp.AddPolicy(&testPolicyobj) require.NoError(t, err) - err = dp.RemovePolicy(testPolicyobj.Name) + err = dp.RemovePolicy(testPolicyobj.PolicyKey) require.NoError(t, err) } @@ -316,3 +318,49 @@ func getAffectedIPSets(networkPolicy *policies.NPMNetworkPolicy) []*ipsets.IPSet } return sets } + +func TestValidateIPBlock(t *testing.T) { + tests := []struct { + name string + ipblock string + wantErr bool + }{ + { + name: "cidr", + ipblock: "172.17.0.0/16", + wantErr: false, + }, + { + name: "except ipblock", + ipblock: "172.17.1.0/24 nomatch", + wantErr: false, + }, + { + name: "incorrect ip format", + ipblock: "1234", + wantErr: true, + }, + { + name: "incorrect ip range", + ipblock: "256.1.2.3", + wantErr: true, + }, + { + name: "empty cidr", + ipblock: "", + wantErr: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + err := validateIPBlock(tt.ipblock) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/npm/pkg/dataplane/ioutil/restore_linux.go b/npm/pkg/dataplane/ioutil/restore_linux.go index d66979366e..c92b396aeb 100644 --- a/npm/pkg/dataplane/ioutil/restore_linux.go +++ b/npm/pkg/dataplane/ioutil/restore_linux.go @@ -126,7 +126,7 @@ func (creator *FileCreator) ToString() string { func (creator *FileCreator) RunCommandWithFile(cmd string, args ...string) error { fileString := creator.ToString() - klog.Infof("beginning to run command for restorer:\nEND-CREATOR-FILE-FOR-COMMAND-%s\n%s\nEND-CREATOR-FILE-FOR-COMMAND-%s", cmd, fileString, cmd) // TODO remove + klog.Infof("beginning to run command for restorer:\nBEGIN-CREATOR-FILE-FOR-COMMAND-%s\n%s\nEND-CREATOR-FILE-FOR-COMMAND-%s", cmd, fileString, cmd) // TODO remove wasFileAltered, err := creator.runCommandOnceWithFile(fileString, cmd, args...) if err == nil { return nil @@ -177,11 +177,11 @@ func (creator *FileCreator) runCommandOnceWithFile(fileString, cmd string, args // run the command stdErrBytes, err := command.CombinedOutput() + creator.tryCount++ if err == nil { // success return false, nil } - creator.tryCount++ stdErr := string(stdErrBytes) klog.Errorf("on try number %d, failed to run command [%s] with error [%v] and stdErr [%s]. Used file:\n%s", creator.tryCount, commandString, err, stdErr, fileString) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go index 93a3b2c861..753ea51e8f 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go @@ -216,11 +216,11 @@ for delete: - skip the delete if it fails for any reason overall format for ipset restore file: - [flushes] (random order) - [destroys] (random order) [creates] (random order) [deletes and adds for sets already in the kernel] (in order of occurrence in save file, deletes first (in random order), then adds (in random order)) [adds for new sets] (random order for sets and members) + [flushes] (random order) + [destroys] (random order) example where every set in add/update cache should have ip 1.2.3.4 and 2.3.4.5: save file showing current kernel state: @@ -232,12 +232,6 @@ example where every set in add/update cache should have ip 1.2.3.4 and 2.3.4.5: add set-in-kernel-1 3.3.3.3 restore file: [flag meanings: -F (flush), -X (destroy), -N (create), -D (delete), -A (add)] - -F set-to-delete2 - -F set-to-delete3 - -F set-to-delete1 - -X set-to-delete2 - -X set-to-delete3 - -X set-to-delete1 -N new-set-2 -N set-in-kernel-2 -N set-in-kernel-1 @@ -255,6 +249,12 @@ example where every set in add/update cache should have ip 1.2.3.4 and 2.3.4.5: -A new-set-1 1.2.3.4 -A new-set-3 1.2.3.4 -A new-set-3 2.3.4.5 + -F set-to-delete2 + -F set-to-delete3 + -F set-to-delete1 + -X set-to-delete2 + -X set-to-delete3 + -X set-to-delete1 */ func (iMgr *IPSetManager) applyIPSets() error { @@ -290,37 +290,45 @@ func (iMgr *IPSetManager) ipsetSave() ([]byte, error) { func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte) *ioutil.FileCreator { creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) // TODO make the line failure pattern into a definition constant eventually - // flush all sets first so we don't try to delete an ipset referenced by a list we're deleting too - for prefixedName := range iMgr.toDeleteCache { - iMgr.flushSetInFile(creator, prefixedName) - } - for prefixedName := range iMgr.toDeleteCache { - iMgr.destroySetInFile(creator, prefixedName) - } - - // create all sets first so we don't try to add a member set to a list if it hasn't been created yet + // 1. create all sets first so we don't try to add a member set to a list if it hasn't been created yet for prefixedName := range iMgr.toAddOrUpdateCache { set := iMgr.setMap[prefixedName] - iMgr.createSetInFile(creator, set) + iMgr.createSetForApply(creator, set) } - // for dirty sets already in the kernel, update members (add members not in the kernel, and delete undesired members in the kernel) + // 2. for dirty sets already in the kernel, update members (add members not in the kernel, and delete undesired members in the kernel) iMgr.updateDirtyKernelSets(saveFile, creator) - // for the remaining dirty sets, add their members to the kernel + // 3. for the remaining dirty sets, add their members to the kernel for prefixedName := range iMgr.toAddOrUpdateCache { set := iMgr.setMap[prefixedName] sectionID := sectionID(addOrUpdateSectionPrefix, prefixedName) if set.Kind == HashSet { for ip := range set.IPPodKey { - iMgr.addMemberInFile(creator, set, sectionID, ip) + iMgr.addMemberForApply(creator, set, sectionID, ip) } } else { for _, member := range set.MemberIPSets { - iMgr.addMemberInFile(creator, set, sectionID, member.HashedName) + iMgr.addMemberForApply(creator, set, sectionID, member.HashedName) } } } + + /* + 4. flush and destroy sets in the original delete cache + + We must perform this step after member deletions because of the following scenario: + Suppose we want to destroy set A, which is referenced by list L. For set A to be in the toDeleteCache, + we must have deleted the reference in list L, so list L is in the toAddOrUpdateCache. In step 2, we will delete this reference, + but until then, set A is in use by a kernel component and can't be destroyed. + */ + // flush all sets first in case a set we're destroying is referenced by a list we're destroying + for prefixedName := range iMgr.toDeleteCache { + iMgr.flushSetForApply(creator, prefixedName) + } + for prefixedName := range iMgr.toDeleteCache { + iMgr.destroySetForApply(creator, prefixedName) + } return creator } @@ -416,11 +424,11 @@ func (iMgr *IPSetManager) updateDirtyKernelSets(saveFile []byte, creator *ioutil // delete undesired members from restore file sectionID := sectionID(addOrUpdateSectionPrefix, prefixedName) for member := range membersToDelete { - iMgr.deleteMemberInFile(creator, set, sectionID, member) + iMgr.deleteMemberForApply(creator, set, sectionID, member) } // add new members to restore file for member := range membersToAdd { - iMgr.addMemberInFile(creator, set, sectionID, member) + iMgr.addMemberForApply(creator, set, sectionID, member) } } } @@ -466,7 +474,7 @@ func hasPrefix(line []byte, prefix string) bool { return len(line) >= len(prefix) && string(line[:len(prefix)]) == prefix } -func (iMgr *IPSetManager) flushSetInFile(creator *ioutil.FileCreator, prefixedName string) { +func (iMgr *IPSetManager) flushSetForApply(creator *ioutil.FileCreator, prefixedName string) { errorHandlers := []*ioutil.LineErrorHandler{ { Definition: setDoesntExistDefinition, @@ -490,7 +498,7 @@ func (iMgr *IPSetManager) flushSetInFile(creator *ioutil.FileCreator, prefixedNa creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedName) // flush set } -func (iMgr *IPSetManager) destroySetInFile(creator *ioutil.FileCreator, prefixedName string) { +func (iMgr *IPSetManager) destroySetForApply(creator *ioutil.FileCreator, prefixedName string) { errorHandlers := []*ioutil.LineErrorHandler{ { Definition: setInUseByKernelDefinition, @@ -513,7 +521,7 @@ func (iMgr *IPSetManager) destroySetInFile(creator *ioutil.FileCreator, prefixed creator.AddLine(sectionID, errorHandlers, ipsetDestroyFlag, hashedName) // destroy set } -func (iMgr *IPSetManager) createSetInFile(creator *ioutil.FileCreator, set *IPSet) { +func (iMgr *IPSetManager) createSetForApply(creator *ioutil.FileCreator, set *IPSet) { methodFlag := ipsetNetHashFlag if set.Kind == ListSet { methodFlag = ipsetSetListFlag @@ -549,7 +557,7 @@ func (iMgr *IPSetManager) createSetInFile(creator *ioutil.FileCreator, set *IPSe creator.AddLine(sectionID, errorHandlers, specs...) // create set } -func (iMgr *IPSetManager) deleteMemberInFile(creator *ioutil.FileCreator, set *IPSet, sectionID, member string) { +func (iMgr *IPSetManager) deleteMemberForApply(creator *ioutil.FileCreator, set *IPSet, sectionID, member string) { errorHandlers := []*ioutil.LineErrorHandler{ { Definition: ioutil.AlwaysMatchDefinition, @@ -562,7 +570,7 @@ func (iMgr *IPSetManager) deleteMemberInFile(creator *ioutil.FileCreator, set *I creator.AddLine(sectionID, errorHandlers, ipsetDeleteFlag, set.HashedName, member) // delete member } -func (iMgr *IPSetManager) addMemberInFile(creator *ioutil.FileCreator, set *IPSet, sectionID, member string) { +func (iMgr *IPSetManager) addMemberForApply(creator *ioutil.FileCreator, set *IPSet, sectionID, member string) { var errorHandlers []*ioutil.LineErrorHandler if set.Kind == ListSet { errorHandlers = []*ioutil.LineErrorHandler{ diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go index 8fe803e55e..55f8bf2697 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go @@ -428,7 +428,7 @@ func TestCreateForAllSetTypes(t *testing.T) { creator := iMgr.fileCreatorForApply(len(calls), nil) actualLines := testAndSortRestoreFileString(t, creator.ToString()) - lines := []string{ + expectedLines := []string{ fmt.Sprintf("-N %s --exist nethash", TestNSSet.HashedName), fmt.Sprintf("-N %s --exist nethash", TestKeyPodSet.HashedName), fmt.Sprintf("-N %s --exist nethash", TestKVPodSet.HashedName), @@ -445,7 +445,7 @@ func TestCreateForAllSetTypes(t *testing.T) { fmt.Sprintf("-A %s %s", TestKVNSList.HashedName, TestKVPodSet.HashedName), "", } - sortedExpectedLines := testAndSortRestoreFileLines(t, lines) + sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines) dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines) wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") @@ -475,19 +475,19 @@ func TestDestroy(t *testing.T) { creator := iMgr.fileCreatorForApply(len(calls), nil) actualLines := testAndSortRestoreFileString(t, creator.ToString()) - lines := []string{ - fmt.Sprintf("-F %s", TestCIDRSet.HashedName), - fmt.Sprintf("-F %s", TestNestedLabelList.HashedName), - fmt.Sprintf("-X %s", TestCIDRSet.HashedName), - fmt.Sprintf("-X %s", TestNestedLabelList.HashedName), + expectedLines := []string{ fmt.Sprintf("-N %s --exist nethash", TestNSSet.HashedName), fmt.Sprintf("-N %s --exist nethash", TestKeyPodSet.HashedName), fmt.Sprintf("-N %s --exist setlist", TestKeyNSList.HashedName), fmt.Sprintf("-A %s 10.0.0.0", TestNSSet.HashedName), fmt.Sprintf("-A %s %s", TestKeyNSList.HashedName, TestNSSet.HashedName), + fmt.Sprintf("-F %s", TestCIDRSet.HashedName), + fmt.Sprintf("-F %s", TestNestedLabelList.HashedName), + fmt.Sprintf("-X %s", TestCIDRSet.HashedName), + fmt.Sprintf("-X %s", TestNestedLabelList.HashedName), "", } - sortedExpectedLines := testAndSortRestoreFileLines(t, lines) + sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines) dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines) wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") @@ -529,7 +529,7 @@ func TestUpdateWithIdenticalSaveFile(t *testing.T) { creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) - lines := []string{ + expectedLines := []string{ fmt.Sprintf("-N %s --exist nethash", TestNSSet.HashedName), fmt.Sprintf("-N %s --exist nethash", TestKeyPodSet.HashedName), fmt.Sprintf("-N %s --exist nethash", TestKVPodSet.HashedName), @@ -539,7 +539,7 @@ func TestUpdateWithIdenticalSaveFile(t *testing.T) { fmt.Sprintf("-N %s --exist setlist", TestNestedLabelList.HashedName), "", } - sortedExpectedLines := testAndSortRestoreFileLines(t, lines) + sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines) dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines) wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") @@ -591,9 +591,7 @@ func TestUpdateWithRealisticSaveFile(t *testing.T) { creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) // adding NSSet and KeyPodSet (should be keeping NSSet and deleting NamedportSet) - lines := []string{ - fmt.Sprintf("-F %s", TestNestedLabelList.HashedName), - fmt.Sprintf("-X %s", TestNestedLabelList.HashedName), + expectedLines := []string{ fmt.Sprintf("-N %s --exist nethash", TestNSSet.HashedName), fmt.Sprintf("-N %s --exist nethash", TestKeyPodSet.HashedName), fmt.Sprintf("-N %s --exist setlist", TestKeyNSList.HashedName), @@ -609,9 +607,11 @@ func TestUpdateWithRealisticSaveFile(t *testing.T) { fmt.Sprintf("-A %s 10.0.0.5", TestNSSet.HashedName), fmt.Sprintf("-D %s %s", TestKeyNSList.HashedName, TestNamedportSet.HashedName), fmt.Sprintf("-A %s %s", TestKeyNSList.HashedName, TestKeyPodSet.HashedName), + fmt.Sprintf("-F %s", TestNestedLabelList.HashedName), + fmt.Sprintf("-X %s", TestNestedLabelList.HashedName), "", } - sortedExpectedLines := testAndSortRestoreFileLines(t, lines) + sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines) dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines) wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") @@ -718,7 +718,7 @@ func TestFailureOnCreateForNewSet(t *testing.T) { // test logic: // - delete a set // - create three sets, each with two members. the second set to appear will fail to be created - errorLineNum := 4 + errorLineNum := 2 setToCreateAlreadyExistsCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set cannot be created: set with the same name already exists", errorLineNum), @@ -767,7 +767,7 @@ func TestFailureOnCreateForSetInKernel(t *testing.T) { // test logic: // - delete a set // - update three sets already in the kernel, each with a delete and add line. the second set to appear will fail to be created - errorLineNum := 4 + errorLineNum := 2 setToCreateAlreadyExistsCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set cannot be created: set with the same name already exists", errorLineNum), @@ -827,7 +827,7 @@ func TestFailureOnAddToListInKernel(t *testing.T) { // - delete a set // - update three lists already in the set, each with a delete and add line. the second list to appear will have the failed add // - create a set and add a member to it - errorLineNum := 10 + errorLineNum := 8 memberDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set to be added/deleted/tested as element does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel @@ -884,7 +884,7 @@ func TestFailureOnAddToNewList(t *testing.T) { // - delete a set // - update a set already in the kernel with a delete and add line // - create three lists in the set, each with an add line. the second list to appear will have the failed add - errorLineNum := 10 + errorLineNum := 8 memberDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set to be added/deleted/tested as element does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel @@ -939,7 +939,7 @@ func TestFailureOnFlush(t *testing.T) { // - delete two sets. the first to appear will fail to flush // - update a set by deleting a member // - create a set with a member - errorLineNum := 1 + errorLineNum := 5 setDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: The set with the given name does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel @@ -992,7 +992,7 @@ func TestFailureOnDestroy(t *testing.T) { // - delete two sets. the first to appear will fail to delete // - update a set by deleting a member // - create a set with a member - errorLineNum := 3 + errorLineNum := 7 inUseByKernelCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set cannot be destroyed: it is in use by a kernel component", errorLineNum), @@ -1072,72 +1072,45 @@ func testAndSortRestoreFileString(t *testing.T, multilineString string) []string return testAndSortRestoreFileLines(t, strings.Split(multilineString, "\n")) } -// make sure file goes in order of flushes, destroys, creates, then adds/deletes, +// make sure file goes in order of creates, adds/deletes, flushes, then destroys // then sort those sections and return the lines in an array func testAndSortRestoreFileLines(t *testing.T, lines []string) []string { require.True(t, lines[len(lines)-1] == "", "restore file must end with blank line") lines = lines[:len(lines)-1] // remove the blank line - flushIndices := [2]int{0, len(lines)} - destroyIndices := [2]int{-1, len(lines)} // -1 means the file ended with the previous operation - createIndices := [2]int{-1, len(lines)} - addDeleteIndices := [2]int{-1, len(lines)} - k := 0 - for k < len(lines) { - operation := lines[k][0:2] - if operation != "-F" { - flushIndices[1] = k - destroyIndices[0] = k - break - } - k++ - } - for k < len(lines) { - operation := lines[k][0:2] - require.False(t, operation == "-F", "should not get -F operation in the restore file after flush section") - if operation != "-X" { - destroyIndices[1] = k - createIndices[0] = k - break - } - k++ + // order of operation groups in restore file (can have groups with multiple possible operatoins) + operationGroups := [][]string{ + {"-N"}, // creates + {"-A", "-D"}, // adds/deletes + {"-F"}, // flushes + {"-X"}, // destroys } + result := make([]string, 0, len(lines)) + groupIndex := 0 + groupStartIndex := 0 + k := 0 for k < len(lines) { - operation := lines[k][0:2] - require.False(t, operation == "-F" || operation == "-X", "should not get %s operation in the restore file after destroy section") - if operation != "-N" { - createIndices[1] = k - addDeleteIndices[0] = k - break + for k < len(lines) { + // iterate until we reach an operation not in the current operation group + operation := lines[k][0:2] + expectedOperations := operationGroups[groupIndex] + if !isStringInSlice(operation, expectedOperations) { + require.True(t, groupIndex < len(operationGroups)-1, "ran out of operation groups. got operation %s", operation) + operationLines := lines[groupStartIndex:k] + sort.Strings(operationLines) + result = append(result, operationLines...) + groupStartIndex = k + groupIndex++ + break + } + k++ } - k++ - } - for k < len(lines) { - operation := lines[k][0:2] - require.True(t, operation == "-D" || operation == "-A", "should not get %s operation in the restore file after create section", operation) - k++ } - flushLines := lines[flushIndices[0]:flushIndices[1]] - var destroyLines []string - var createLines []string - var addDeleteLines []string - if destroyIndices[0] != -1 { - destroyLines = lines[destroyIndices[0]:destroyIndices[1]] - } - if createIndices[0] != -1 { - createLines = lines[createIndices[0]:createIndices[1]] - } - if addDeleteIndices[0] != -1 { - addDeleteLines = lines[addDeleteIndices[0]:addDeleteIndices[1]] - } - sort.Strings(flushLines) - sort.Strings(destroyLines) - sort.Strings(createLines) - sort.Strings(addDeleteLines) - result := flushLines - result = append(result, destroyLines...) - result = append(result, createLines...) - result = append(result, addDeleteLines...) + // add the remaining lines since the final operation group won't pass through the if statement in the loop above + operatrionLines := lines[groupStartIndex:] + sort.Strings(operatrionLines) + result = append(result, operatrionLines...) + result = append(result, "") // add the blank line return result } @@ -1162,15 +1135,19 @@ func memberNameOfSetImpacted(t *testing.T, lines []string, lineNum int) string { return member } -func requireStringInSlice(t *testing.T, item string, possibleValues []string) { +func isStringInSlice(item string, values []string) bool { success := false - for _, value := range possibleValues { + for _, value := range values { if item == value { success = true break } } - require.Truef(t, success, "item %s was not one of the possible values", item) + return success +} + +func requireStringInSlice(t *testing.T, item string, values []string) { + require.Truef(t, isStringInSlice(item, values), "item %s was not one of the possible values %+v", item, values) } // remove lines that start with the operation (include the dash in the operations) e.g. diff --git a/npm/pkg/dataplane/policies/policy.go b/npm/pkg/dataplane/policies/policy.go index 981abd7c42..030890af84 100644 --- a/npm/pkg/dataplane/policies/policy.go +++ b/npm/pkg/dataplane/policies/policy.go @@ -3,14 +3,19 @@ package policies import ( "fmt" "strconv" + "strings" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/util" + "k8s.io/klog" ) type NPMNetworkPolicy struct { Name string NameSpace string + // TODO remove Name and Namespace field + // PolicyKey is a unique combination of "namespace/name" of network policy + PolicyKey string // PodSelectorIPSets holds all the IPSets generated from Pod Selector PodSelectorIPSets []*ipsets.TranslatedIPSet // PodSelectorList holds target pod information to avoid duplicatoin in SrcList and DstList fields in ACLs @@ -24,6 +29,14 @@ type NPMNetworkPolicy struct { PodEndpoints map[string]string } +func NewNPMNetworkPolicy(netPolName, netPolNamespace string) *NPMNetworkPolicy { + return &NPMNetworkPolicy{ + Name: netPolName, + NameSpace: netPolNamespace, + PolicyKey: fmt.Sprintf("%s/%s", netPolNamespace, netPolName), + } +} + // ACLPolicy equivalent to a single iptable rule in linux // or a single HNS rule in windows type ACLPolicy struct { @@ -108,10 +121,10 @@ func (aclPolicy *ACLPolicy) hasEgress() bool { } func (aclPolicy *ACLPolicy) hasKnownProtocol() bool { - return aclPolicy.Protocol != "" && (aclPolicy.Protocol == TCP || + return aclPolicy.Protocol == TCP || aclPolicy.Protocol == UDP || aclPolicy.Protocol == SCTP || - aclPolicy.Protocol == AnyProtocol) + aclPolicy.Protocol == UnspecifiedProtocol } func (aclPolicy *ACLPolicy) hasKnownTarget() bool { @@ -119,7 +132,52 @@ func (aclPolicy *ACLPolicy) hasKnownTarget() bool { } func (aclPolicy *ACLPolicy) satisifiesPortAndProtocolConstraints() bool { - return (aclPolicy.Protocol != AnyProtocol) || (aclPolicy.DstPorts.Port == 0 && aclPolicy.DstPorts.EndPort == 0) + return (aclPolicy.Protocol != UnspecifiedProtocol) || (aclPolicy.DstPorts.Port == 0 && aclPolicy.DstPorts.EndPort == 0) +} + +func (netPol *NPMNetworkPolicy) String() string { + if netPol == nil { + klog.Infof("NPMNetworkPolicy is nil when trying to print string") + return "nil NPMNetworkPolicy" + } + itemStrings := make([]string, 0, len(netPol.ACLs)) + for _, item := range netPol.ACLs { + itemStrings = append(itemStrings, item.String()) + } + aclArrayString := strings.Join(itemStrings, "\n--\n") + + podSelectorIPSetString := translatedIPSetsToString(netPol.PodSelectorIPSets) + podSelectorListString := infoArrayToString(netPol.PodSelectorList) + format := `Name:%s Namespace:%s +PodSelectorIPSets: %s +PodSelectorList: %s +ACLs: +%s` + return fmt.Sprintf(format, netPol.Name, netPol.NameSpace, podSelectorIPSetString, podSelectorListString, aclArrayString) +} + +func (aclPolicy *ACLPolicy) String() string { + format := `Target:%s Direction:%s Protocol:%s Ports:%+v +SrcList: %s +DstList: %s` + return fmt.Sprintf(format, aclPolicy.Target, aclPolicy.Direction, aclPolicy.Protocol, aclPolicy.DstPorts, infoArrayToString(aclPolicy.SrcList), infoArrayToString(aclPolicy.DstList)) +} + +func infoArrayToString(items []SetInfo) string { + itemStrings := make([]string, 0, len(items)) + for _, item := range items { + itemStrings = append(itemStrings, fmt.Sprintf("{%s}", item.String())) + } + return fmt.Sprintf("[%s]", strings.Join(itemStrings, ",")) +} + +func translatedIPSetsToString(items []*ipsets.TranslatedIPSet) string { + itemStrings := make([]string, 0, len(items)) + for _, item := range items { + ipset := ipsets.NewIPSet(item.Metadata) + itemStrings = append(itemStrings, fmt.Sprintf("{%s}", ipset.String())) + } + return fmt.Sprintf("[%s]", strings.Join(itemStrings, ",")) } // SetInfo helps capture additional details in a matchSet. @@ -150,6 +208,10 @@ func NewSetInfo(name string, setType ipsets.SetType, included bool, matchType Ma } } +func (info SetInfo) String() string { + return fmt.Sprintf("Name:%s HashedName:%s MatchType:%v Included:%v", info.IPSet.GetPrefixName(), info.IPSet.GetHashedName(), info.MatchType, info.Included) +} + type Ports struct { Port int32 EndPort int32 @@ -188,19 +250,22 @@ const ( Dropped Verdict = "DROP" ) -// Protocol can be TCP, UDP, SCTP, or ANY since they are currently supported in networkpolicy. +// Protocol can be TCP, UDP, SCTP, or unspecified since they are currently supported in networkpolicy. +// Protocol value is case-sensitive (Capital now). +// TODO: Need to remove this dependency on case-sensitivity. // NPM is not fully tested with SCTP. type Protocol string const ( + // TCP Protocol - TCP Protocol = "tcp" + TCP Protocol = "TCP" // UDP Protocol - UDP Protocol = "udp" + UDP Protocol = "UDP" // SCTP Protocol - SCTP Protocol = "sctp" - // AnyProtocol can be used for all other protocols - AnyProtocol Protocol = "all" + SCTP Protocol = "SCTP" + // UnspecifiedProtocol leaves protocol unspecified. For a named port, this represents its protocol. Otherwise, this represents any protocol. + UnspecifiedProtocol Protocol = "unspecified" ) type MatchType int8 diff --git a/npm/pkg/dataplane/policies/policy_linux.go b/npm/pkg/dataplane/policies/policy_linux.go index 9683a65642..5a1902ae81 100644 --- a/npm/pkg/dataplane/policies/policy_linux.go +++ b/npm/pkg/dataplane/policies/policy_linux.go @@ -22,6 +22,6 @@ func (networkPolicy *NPMNetworkPolicy) ingressChainName() string { } func (networkPolicy *NPMNetworkPolicy) chainName(prefix string) string { - policyHash := util.Hash(networkPolicy.Name) // assuming the name is unique + policyHash := util.Hash(networkPolicy.PolicyKey) return joinWithDash(prefix, policyHash) } diff --git a/npm/pkg/dataplane/policies/policy_windows.go b/npm/pkg/dataplane/policies/policy_windows.go index c2cdfe085d..0dcec9d68e 100644 --- a/npm/pkg/dataplane/policies/policy_windows.go +++ b/npm/pkg/dataplane/policies/policy_windows.go @@ -19,7 +19,7 @@ var ( UDP: "17", SCTP: "132", // HNS thinks 256 as ANY protocol - AnyProtocol: "256", + UnspecifiedProtocol: "256", } ErrNamedPortsNotSupported = errors.New("Named Port translation is not supported in windows dataplane") @@ -78,9 +78,6 @@ func (acl *ACLPolicy) convertToAclSettings() (*NPMACLPolSettings, error) { if policySettings.Action == hcn.ActionTypeBlock { policySettings.Priority = uint16(blockRulePriotity) } - if acl.Protocol == "" { - acl.Protocol = AnyProtocol - } protoNum, ok := protocolNumMap[acl.Protocol] if !ok { return policySettings, ErrProtocolNotSupported diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index eea62a0518..86c5d0eb4b 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -83,25 +83,26 @@ func (pMgr *PolicyManager) Reconcile(stopChannel <-chan struct{}) { }() } -func (pMgr *PolicyManager) PolicyExists(name string) bool { - _, ok := pMgr.policyMap.cache[name] +func (pMgr *PolicyManager) PolicyExists(policyKey string) bool { + _, ok := pMgr.policyMap.cache[policyKey] return ok } -func (pMgr *PolicyManager) GetPolicy(name string) (*NPMNetworkPolicy, bool) { - policy, ok := pMgr.policyMap.cache[name] +func (pMgr *PolicyManager) GetPolicy(policyKey string) (*NPMNetworkPolicy, bool) { + policy, ok := pMgr.policyMap.cache[policyKey] return policy, ok } func (pMgr *PolicyManager) AddPolicy(policy *NPMNetworkPolicy, endpointList map[string]string) error { if len(policy.ACLs) == 0 { - klog.Infof("[DataPlane] No ACLs in policy %s to apply", policy.Name) + klog.Infof("[DataPlane] No ACLs in policy %s to apply", policy.PolicyKey) return nil } normalizePolicy(policy) - if err := checkForErrors(policy); err != nil { + if err := validatePolicy(policy); err != nil { return npmerrors.Errorf(npmerrors.AddPolicy, false, fmt.Sprintf("couldn't add malformed policy: %s", err.Error())) } + klog.Infof("PRINTING-CONTENTS-FOR-ADDING-POLICY:\n%s", policy.String()) // Call actual dataplane function to apply changes err := pMgr.addPolicy(policy, endpointList) @@ -109,18 +110,22 @@ func (pMgr *PolicyManager) AddPolicy(policy *NPMNetworkPolicy, endpointList map[ return npmerrors.Errorf(npmerrors.AddPolicy, false, fmt.Sprintf("failed to add policy: %v", err)) } - pMgr.policyMap.cache[policy.Name] = policy + pMgr.policyMap.cache[policy.PolicyKey] = policy return nil } -func (pMgr *PolicyManager) RemovePolicy(name string, endpointList map[string]string) error { - policy, ok := pMgr.GetPolicy(name) +func (pMgr *PolicyManager) RemovePolicy(policyKey string, endpointList map[string]string) error { + policy, ok := pMgr.GetPolicy(policyKey) + klog.Infof("PRINTING-CONTENTS-FOR-REMOVING-POLICY:\n%s", policy.String()) + if !ok { + klog.Infof("DEBUGME-POLICY-DOESN'T-EXIST-WHEN-DELETING") + klog.Infof("POLICY-CACHE: %+v", pMgr.policyMap.cache) return nil } if len(policy.ACLs) == 0 { - klog.Infof("[DataPlane] No ACLs in policy %s to remove", policy.Name) + klog.Infof("[DataPlane] No ACLs in policy %s to remove", policyKey) return nil } // Call actual dataplane function to apply changes @@ -129,7 +134,7 @@ func (pMgr *PolicyManager) RemovePolicy(name string, endpointList map[string]str return npmerrors.Errorf(npmerrors.RemovePolicy, false, fmt.Sprintf("failed to remove policy: %v", err)) } - delete(pMgr.policyMap.cache, name) + delete(pMgr.policyMap.cache, policyKey) if len(pMgr.policyMap.cache) == 0 { klog.Infof("rebooting policy manager since there are no policies remaining in the cache") if err := pMgr.reboot(); err != nil { @@ -143,7 +148,7 @@ func (pMgr *PolicyManager) RemovePolicy(name string, endpointList map[string]str func normalizePolicy(networkPolicy *NPMNetworkPolicy) { for _, aclPolicy := range networkPolicy.ACLs { if aclPolicy.Protocol == "" { - aclPolicy.Protocol = AnyProtocol + aclPolicy.Protocol = UnspecifiedProtocol } if aclPolicy.DstPorts.EndPort == 0 { @@ -153,16 +158,16 @@ func normalizePolicy(networkPolicy *NPMNetworkPolicy) { } // TODO do verification in controller? -func checkForErrors(networkPolicy *NPMNetworkPolicy) error { +func validatePolicy(networkPolicy *NPMNetworkPolicy) error { for _, aclPolicy := range networkPolicy.ACLs { if !aclPolicy.hasKnownTarget() { - return npmerrors.SimpleError(fmt.Sprintf("ACL policy %s has unknown target", aclPolicy.PolicyID)) + return npmerrors.SimpleError(fmt.Sprintf("ACL policy %s has unknown target [%s]", aclPolicy.PolicyID, aclPolicy.Target)) } if !aclPolicy.hasKnownDirection() { - return npmerrors.SimpleError(fmt.Sprintf("ACL policy %s has unknown direction", aclPolicy.PolicyID)) + return npmerrors.SimpleError(fmt.Sprintf("ACL policy %s has unknown direction [%s]", aclPolicy.PolicyID, aclPolicy.Direction)) } if !aclPolicy.hasKnownProtocol() { - return npmerrors.SimpleError(fmt.Sprintf("ACL policy %s has unknown protocol (set to All if desired)", aclPolicy.PolicyID)) + return npmerrors.SimpleError(fmt.Sprintf("ACL policy %s has unknown protocol [%s]", aclPolicy.PolicyID, aclPolicy.Protocol)) } if !aclPolicy.satisifiesPortAndProtocolConstraints() { return npmerrors.SimpleError(fmt.Sprintf( diff --git a/npm/pkg/dataplane/policies/policymanager_linux.go b/npm/pkg/dataplane/policies/policymanager_linux.go index 31a6d227b7..346cca8065 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux.go +++ b/npm/pkg/dataplane/policies/policymanager_linux.go @@ -51,7 +51,7 @@ func (pMgr *PolicyManager) removePolicy(networkPolicy *NPMNetworkPolicy, _ map[s } func restore(creator *ioutil.FileCreator) error { - err := creator.RunCommandWithFile(util.IptablesRestore, util.IptablesRestoreTableFlag, util.IptablesFilterTable, util.IptablesRestoreNoFlushFlag) + err := creator.RunCommandWithFile(util.IptablesRestore, util.IptablesWaitFlag, defaultlockWaitTimeInSeconds, util.IptablesRestoreTableFlag, util.IptablesFilterTable, util.IptablesRestoreNoFlushFlag) if err != nil { return npmerrors.SimpleErrorWrapper("failed to restore iptables file", err) } @@ -129,7 +129,7 @@ func (pMgr *PolicyManager) deleteJumpRule(policy *NPMNetworkPolicy, isIngress bo errCode, err := pMgr.runIPTablesCommand(util.IptablesDeletionFlag, specs...) if err != nil && errCode != couldntLoadTargetErrorCode { // TODO check rule doesn't exist error code instead because the chain should exist - errorString := fmt.Sprintf("failed to delete jump from %s chain to %s chain for policy %s with exit code %d", baseChainName, chainName, policy.Name, errCode) + errorString := fmt.Sprintf("failed to delete jump from %s chain to %s chain for policy %s with exit code %d", baseChainName, chainName, policy.PolicyKey, errCode) log.Errorf(errorString+": %w", err) return npmerrors.SimpleErrorWrapper(errorString, err) } @@ -183,7 +183,7 @@ func writeNetworkPolicyRules(creator *ioutil.FileCreator, networkPolicy *NPMNetw if aclPolicy.hasIngress() { chainName = networkPolicy.ingressChainName() if aclPolicy.Target == Allowed { - actionSpecs = []string{util.IptablesJumpFlag, util.IptablesAzureEgressChain} + actionSpecs = []string{util.IptablesJumpFlag, util.IptablesAzureIngressAllowMarkChain} } else { actionSpecs = setMarkSpecs(util.IptablesAzureIngressDropMarkHex) } @@ -204,7 +204,9 @@ func writeNetworkPolicyRules(creator *ioutil.FileCreator, networkPolicy *NPMNetw func iptablesRuleSpecs(aclPolicy *ACLPolicy) []string { specs := make([]string, 0) - specs = append(specs, util.IptablesProtFlag, string(aclPolicy.Protocol)) // NOTE: protocol must be ALL instead of nil + if aclPolicy.Protocol != UnspecifiedProtocol { + specs = append(specs, util.IptablesProtFlag, string(aclPolicy.Protocol)) + } specs = append(specs, dstPortSpecs(aclPolicy.DstPorts)...) specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.SrcList)...) specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.DstList)...) @@ -223,11 +225,16 @@ func dstPortSpecs(portRange Ports) []string { func matchSetSpecsForNetworkPolicy(networkPolicy *NPMNetworkPolicy, matchType MatchType) []string { // TODO update to use included boolean/new data structure from Junguk's PR - specs := make([]string, 0, maxLengthForMatchSetSpecs*len(networkPolicy.PodSelectorIPSets)) - for _, translatedIPSet := range networkPolicy.PodSelectorIPSets { - matchString := matchType.toIPTablesString() - hashedSetName := translatedIPSet.Metadata.GetHashedName() - specs = append(specs, util.IptablesModuleFlag, util.IptablesSetModuleFlag, util.IptablesMatchSetFlag, hashedSetName, matchString) + specs := make([]string, 0, maxLengthForMatchSetSpecs*len(networkPolicy.PodSelectorList)) + matchString := matchType.toIPTablesString() + for _, setInfo := range networkPolicy.PodSelectorList { + // TODO consolidate this code with that in matchSetSpecsFromSetInfo + specs = append(specs, util.IptablesModuleFlag, util.IptablesSetModuleFlag) + if !setInfo.Included { + specs = append(specs, util.IptablesNotFlag) + } + hashedSetName := setInfo.IPSet.GetHashedName() + specs = append(specs, util.IptablesMatchSetFlag, hashedSetName, matchString) } return specs } diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 6ce6d8f31e..f3f3db0c23 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -25,19 +25,19 @@ var ( testPolicy3EgressJump = fmt.Sprintf("-j %s", testPolicy3EgressChain) testACLRule1 = fmt.Sprintf( - "-j MARK --set-mark 0x4000 -p tcp --dport 222:333 -m set --match-set %s src -m set ! --match-set %s dst -m comment --comment comment1", + "-j MARK --set-mark 0x4000 -p TCP --dport 222:333 -m set --match-set %s src -m set ! --match-set %s dst -m comment --comment comment1", ipsets.TestCIDRSet.HashedName, ipsets.TestKeyPodSet.HashedName, ) - testACLRule2 = fmt.Sprintf("-j AZURE-NPM-EGRESS -p udp -m set --match-set %s src -m comment --comment comment2", ipsets.TestCIDRSet.HashedName) - testACLRule3 = fmt.Sprintf("-j MARK --set-mark 0x5000 -p udp --dport 144 -m set --match-set %s src -m comment --comment comment3", ipsets.TestCIDRSet.HashedName) - testACLRule4 = fmt.Sprintf("-j AZURE-NPM-ACCEPT -p all -m set --match-set %s src -m comment --comment comment4", ipsets.TestCIDRSet.HashedName) + testACLRule2 = fmt.Sprintf("-j AZURE-NPM-INGRESS-ALLOW-MARK -p UDP -m set --match-set %s src -m comment --comment comment2", ipsets.TestCIDRSet.HashedName) + testACLRule3 = fmt.Sprintf("-j MARK --set-mark 0x5000 -p UDP --dport 144 -m set --match-set %s src -m comment --comment comment3", ipsets.TestCIDRSet.HashedName) + testACLRule4 = fmt.Sprintf("-j AZURE-NPM-ACCEPT -m set --match-set %s src -m comment --comment comment4", ipsets.TestCIDRSet.HashedName) ) func TestChainNames(t *testing.T) { - expectedName := fmt.Sprintf("AZURE-NPM-INGRESS-%s", util.Hash(TestNetworkPolicies[0].Name)) + expectedName := fmt.Sprintf("AZURE-NPM-INGRESS-%s", util.Hash(TestNetworkPolicies[0].PolicyKey)) require.Equal(t, expectedName, TestNetworkPolicies[0].ingressChainName()) - expectedName = fmt.Sprintf("AZURE-NPM-EGRESS-%s", util.Hash(TestNetworkPolicies[0].Name)) + expectedName = fmt.Sprintf("AZURE-NPM-EGRESS-%s", util.Hash(TestNetworkPolicies[0].PolicyKey)) require.Equal(t, expectedName, TestNetworkPolicies[0].egressChainName()) } @@ -68,7 +68,8 @@ func TestAddPolicies(t *testing.T) { // policy 3 fmt.Sprintf("-A %s %s", testPolicy3EgressChain, testACLRule4), fmt.Sprintf("-I AZURE-NPM-EGRESS 2 %s", testPolicy3EgressJump), - "COMMIT\n", + "COMMIT", + "", } dptestutils.AssertEqualLines(t, expectedLines, actualLines) @@ -103,13 +104,14 @@ func TestRemovePolicies(t *testing.T) { fmt.Sprintf(":%s - -", testPolicy1EgressChain), fmt.Sprintf(":%s - -", testPolicy2IngressChain), fmt.Sprintf(":%s - -", testPolicy3EgressChain), - "COMMIT\n", + "COMMIT", + "", } dptestutils.AssertEqualLines(t, expectedLines, actualLines) err := pMgr.AddPolicy(TestNetworkPolicies[0], nil) // need the policy in the cache require.NoError(t, err) - err = pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil) + err = pMgr.RemovePolicy(TestNetworkPolicies[0].PolicyKey, nil) require.NoError(t, err) } @@ -125,7 +127,7 @@ func TestRemovePoliciesErrorOnRestore(t *testing.T) { pMgr := NewPolicyManager(ioshim) err := pMgr.AddPolicy(TestNetworkPolicies[0], nil) require.NoError(t, err) - err = pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil) + err = pMgr.RemovePolicy(TestNetworkPolicies[0].PolicyKey, nil) require.Error(t, err) } @@ -139,7 +141,7 @@ func TestRemovePoliciesErrorOnDeleteForIngress(t *testing.T) { pMgr := NewPolicyManager(ioshim) err := pMgr.AddPolicy(TestNetworkPolicies[0], nil) require.NoError(t, err) - err = pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil) + err = pMgr.RemovePolicy(TestNetworkPolicies[0].PolicyKey, nil) require.Error(t, err) } @@ -154,7 +156,7 @@ func TestRemovePoliciesErrorOnDeleteForEgress(t *testing.T) { pMgr := NewPolicyManager(ioshim) err := pMgr.AddPolicy(TestNetworkPolicies[0], nil) require.NoError(t, err) - err = pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil) + err = pMgr.RemovePolicy(TestNetworkPolicies[0].PolicyKey, nil) require.Error(t, err) } @@ -176,7 +178,7 @@ func TestUpdatingChainsToCleanup(t *testing.T) { assertStaleChainsContain(t, pMgr.staleChains) // successful removal, so mark the policy's chains as stale - require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil)) + require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[0].PolicyKey, nil)) assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // successful add, so keep the same stale chains @@ -184,7 +186,7 @@ func TestUpdatingChainsToCleanup(t *testing.T) { assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // failure to remove, so keep the same stale chains - require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[1].Name, nil)) + require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[1].PolicyKey, nil)) assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // successfully add a new policy. keep the same stale chains @@ -192,7 +194,7 @@ func TestUpdatingChainsToCleanup(t *testing.T) { assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // successful removal, so mark the policy's chains as stale - require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[2].Name, nil)) + require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[2].PolicyKey, nil)) assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain, testPolicy3EgressChain) // failure to add, so keep the same stale chains the same diff --git a/npm/pkg/dataplane/policies/policymanager_test.go b/npm/pkg/dataplane/policies/policymanager_test.go index ad84c344a6..d27ef41b63 100644 --- a/npm/pkg/dataplane/policies/policymanager_test.go +++ b/npm/pkg/dataplane/policies/policymanager_test.go @@ -14,7 +14,9 @@ var ( testNSSet = ipsets.NewIPSetMetadata("test-ns-set", ipsets.Namespace) testKeyPodSet = ipsets.NewIPSetMetadata("test-keyPod-set", ipsets.KeyLabelOfPod) testNetPol = &NPMNetworkPolicy{ - Name: "test/test-netpol", + Name: "test-netpol", + NameSpace: "x", + PolicyKey: "x/test-netpol", PodSelectorIPSets: []*ipsets.TranslatedIPSet{ { Metadata: testNSSet, @@ -74,7 +76,9 @@ func TestAddPolicy(t *testing.T) { func TestGetPolicy(t *testing.T) { netpol := &NPMNetworkPolicy{ - Name: "test", + Name: "test-netpol", + NameSpace: "x", + PolicyKey: "x/test-netpol", ACLs: []*ACLPolicy{ { PolicyID: "azure-acl-123", @@ -89,11 +93,11 @@ func TestGetPolicy(t *testing.T) { require.NoError(t, pMgr.AddPolicy(netpol, epList)) - require.True(t, pMgr.PolicyExists("test")) + require.True(t, pMgr.PolicyExists("x/test-netpol")) - policy, ok := pMgr.GetPolicy("test") + policy, ok := pMgr.GetPolicy("x/test-netpol") require.True(t, ok) - require.Equal(t, "test", policy.Name) + require.Equal(t, "x/test-netpol", policy.PolicyKey) } func TestRemovePolicy(t *testing.T) { @@ -106,3 +110,50 @@ func TestRemovePolicy(t *testing.T) { require.NoError(t, pMgr.RemovePolicy("test/test-netpol", nil)) } + +func TestNormalizeAndValidatePolicy(t *testing.T) { + tests := []struct { + name string + acl *ACLPolicy + wantErr bool + }{ + { + name: "valid policy", + acl: &ACLPolicy{ + PolicyID: "valid-acl", + Target: Dropped, + Direction: Ingress, + }, + wantErr: false, + }, + { + name: "invalid protocol", + acl: &ACLPolicy{ + PolicyID: "bad-protocol-acl", + Target: Dropped, + Direction: Ingress, + Protocol: "invalid", + }, + wantErr: true, + }, + // TODO add other invalid cases + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + netPol := &NPMNetworkPolicy{ + Name: "test-netpol", + NameSpace: "x", + PolicyKey: "x/test-netpol", + ACLs: []*ACLPolicy{tt.acl}, + } + normalizePolicy(netPol) + err := validatePolicy(netPol) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/npm/pkg/dataplane/policies/testutils.go b/npm/pkg/dataplane/policies/testutils.go index a642084917..69632baea7 100644 --- a/npm/pkg/dataplane/policies/testutils.go +++ b/npm/pkg/dataplane/policies/testutils.go @@ -6,24 +6,49 @@ var ( // TestNetworkPolicies for testing TestNetworkPolicies = []*NPMNetworkPolicy{ { - Name: "test1", + Name: "test1", + NameSpace: "x", + PolicyKey: "x/test1", PodSelectorIPSets: []*ipsets.TranslatedIPSet{ {Metadata: ipsets.TestKeyPodSet.Metadata}, }, + PodSelectorList: []SetInfo{ + { + IPSet: ipsets.TestKeyPodSet.Metadata, + Included: true, + MatchType: EitherMatch, + }, + }, ACLs: testACLs, }, { - Name: "test2", + Name: "test2", + NameSpace: "y", + PolicyKey: "y/test2", PodSelectorIPSets: []*ipsets.TranslatedIPSet{ {Metadata: ipsets.TestKeyPodSet.Metadata}, {Metadata: ipsets.TestKVPodSet.Metadata}, }, + PodSelectorList: []SetInfo{ + { + IPSet: ipsets.TestKeyPodSet.Metadata, + Included: true, + MatchType: EitherMatch, + }, + { + IPSet: ipsets.TestKVPodSet.Metadata, + Included: true, + MatchType: EitherMatch, + }, + }, ACLs: []*ACLPolicy{ testACLs[0], }, }, { - Name: "test3", + Name: "test3", + NameSpace: "z", + PolicyKey: "z/test3", ACLs: []*ACLPolicy{ testACLs[3], }, @@ -98,7 +123,7 @@ var ( }, Target: Allowed, Direction: Egress, - Protocol: AnyProtocol, + Protocol: UnspecifiedProtocol, }, } ) diff --git a/npm/pkg/dataplane/policies/testutils_linux.go b/npm/pkg/dataplane/policies/testutils_linux.go index d173bc116d..684868e45f 100644 --- a/npm/pkg/dataplane/policies/testutils_linux.go +++ b/npm/pkg/dataplane/policies/testutils_linux.go @@ -8,8 +8,8 @@ import ( ) var ( - fakeIPTablesRestoreCommand = testutils.TestCmd{Cmd: []string{"iptables-restore", "-T", "filter", "--noflush"}} - fakeIPTablesRestoreFailureCommand = testutils.TestCmd{Cmd: []string{"iptables-restore", "-T", "filter", "--noflush"}, ExitCode: 1} + fakeIPTablesRestoreCommand = testutils.TestCmd{Cmd: []string{"iptables-restore", "-w", "60", "-T", "filter", "--noflush"}} + fakeIPTablesRestoreFailureCommand = testutils.TestCmd{Cmd: []string{"iptables-restore", "-w", "60", "-T", "filter", "--noflush"}, ExitCode: 1} listLineNumbersCommandStrings = []string{"iptables", "-w", "60", "-t", "filter", "-n", "-L", "FORWARD", "--line-numbers"} listPolicyChainNamesCommandStrings = []string{"iptables", "-w", "60", "-t", "filter", "-n", "-L"} diff --git a/npm/util/errors/errors.go b/npm/util/errors/errors.go index 175c67f887..f9ded48f35 100644 --- a/npm/util/errors/errors.go +++ b/npm/util/errors/errors.go @@ -192,7 +192,7 @@ type NPMSimpleError struct { } func SimpleError(errstring string) *NPMSimpleError { - return nil + return &NPMSimpleError{fmt.Errorf("%s", errstring)} //nolint:goerr113 // need to re-structure error handler in next PR } func SimpleErrorWrapper(errstring string, err error) *NPMSimpleError {