Skip to content

fix: [NPM] V2 NPM (with only linux data plane) bug fix after first run #1120

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 22 commits into from
Dec 1, 2021
Merged

fix: [NPM] V2 NPM (with only linux data plane) bug fix after first run #1120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
79c3bcc
fix: ingress allow rules jump to azure ingress allow mark chain
huntergregory Nov 22, 2021
641bb55
await for lock for iptables-restore
huntergregory Nov 22, 2021
7c739c3
FIXME: temporarily enabling v2 npm flag as default
huntergregory Nov 22, 2021
6a7f946
Revert "FIXME: temporarily enabling v2 npm flag as default"
huntergregory Nov 22, 2021
e206348
fix nil pointer problem in NPMSimpleError.
huntergregory Nov 22, 2021
a0b552e
update logging for validating policies
huntergregory Nov 23, 2021
f91c727
remove pointless check for empty string in hasKnownProtocol
huntergregory Nov 23, 2021
13a60e0
iconvert anyprotocol to unspecifiedprotocol
huntergregory Nov 23, 2021
340e825
add UT for normalizing and validating protocol
huntergregory Nov 23, 2021
ccc8e0d
Fix case-sensitivity problem on protocol
JungukCho Nov 23, 2021
a721756
fix in use by kernel problem for ipsets (TODO update UTs)
huntergregory Nov 23, 2021
72aa65d
Fix duplicated namedPort prefix in translation
JungukCho Nov 24, 2021
97ace67
Correct wrong namedPort UTs
JungukCho Nov 24, 2021
c78a143
update UTs
huntergregory Nov 24, 2021
55db298
logging fixes for restorer
huntergregory Nov 24, 2021
f5c1739
fix translating egress (previously included only the default drop rule)
huntergregory Nov 25, 2021
83533b6
Fix space issue in except information of ipblock and add its UTs
JungukCho Nov 29, 2021
a9b2010
Disable lint temporary
JungukCho Nov 29, 2021
673ca70
Use correct and consistent key in all places to clean-up delete netpol
JungukCho Nov 29, 2021
99cf5ab
fix problem of using podselectoripsets instead of podselectorlist
huntergregory Nov 30, 2021
26cd5b2
resolve UTs and golints
huntergregory Nov 30, 2021
c10e3d1
run v2 cyclonus in pipeline
huntergregory Nov 30, 2021
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
2 changes: 1 addition & 1 deletion .github/workflows/cyclonus-netpol-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 6 additions & 8 deletions npm/pkg/controlplane/translation/translatePolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand All @@ -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
}
36 changes: 18 additions & 18 deletions npm/pkg/controlplane/translation/translatePolicy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
Expand All @@ -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",
},
},
Expand Down Expand Up @@ -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",
},
},
Expand All @@ -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",
},
},
Expand Down Expand Up @@ -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",
},
Expand All @@ -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",
},
Expand Down Expand Up @@ -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{
{
Expand All @@ -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",
},
Expand All @@ -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{
{
Expand All @@ -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",
},
Expand All @@ -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{
{
Expand All @@ -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",
},
Expand All @@ -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{
{
Expand All @@ -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",
},
Expand Down
69 changes: 38 additions & 31 deletions npm/pkg/dataplane/dataplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
Expand All @@ -277,18 +278,18 @@ 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)
}

// TODO it would be ideal to calculate a diff of policies
// 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)
}
Expand Down Expand Up @@ -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()))
}
Expand Down Expand Up @@ -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()))
}
Expand All @@ -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
Expand Down
Loading