Skip to content

fix/refactor: [NPM] eliminate struct fields to prevent errors & reduce memory #1374

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 7 commits into from
May 13, 2022
Merged

fix/refactor: [NPM] eliminate struct fields to prevent errors & reduce memory #1374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ce494f
remove Name field from NPMNetworkPolicy
huntergregory Mar 23, 2022
90b2073
wip moving acl policy id
huntergregory Mar 29, 2022
8a43017
fix policy key typo for removing Name field
huntergregory May 12, 2022
04cb841
fix lint and log
huntergregory May 12, 2022
14606b5
temp debug logs
huntergregory May 12, 2022
48056b9
another debug log
huntergregory May 12, 2022
37531b3
update a couple UTs
huntergregory May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions npm/pkg/controlplane/goalstateprocessor/goalstateprocessor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,12 @@ func (gsp *GoalStateProcessor) processPolicyApplyEvent(goalState *protos.GoalSta
klog.Warningf("Empty Policy apply event")
continue
}
klog.Infof("Processing %s Policy ADD event", netpol.Name)
klog.Infof("Processing %s Policy ADD event", netpol.PolicyKey)
klog.Infof("Netpol: %v", netpol)

err = gsp.dp.UpdatePolicy(netpol)
if err != nil {
klog.Errorf("Error applying policy %s to dataplane with error: %s", netpol.Name, err.Error())
klog.Errorf("Error applying policy %s to dataplane with error: %s", netpol.PolicyKey, err.Error())
return nil, npmerrors.SimpleErrorWrapper("failed update policy event", err)
}
appendedPolicies[netpol.PolicyKey] = struct{}{}
Expand Down
8 changes: 3 additions & 5 deletions npm/pkg/controlplane/goalstateprocessor/goalstateprocessor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ var (
testNestedKeyPodSet = ipsets.NewIPSetMetadata("test-nestedkeyPod-set", ipsets.NestedLabelOfPod)
testNestedKeyPodCPSet = controlplane.NewControllerIPSets(testNestedKeyPodSet)
testNetPol = &policies.NPMNetworkPolicy{
Name: "test-netpol",
NameSpace: "x",
PolicyKey: "x/test-netpol",
Namespace: "x",
PolicyKey: "x/test-netpol",
ACLPolicyID: "azure-acl-x-test-netpol",
PodSelectorIPSets: []*ipsets.TranslatedIPSet{
{
Metadata: testNSSet,
Expand All @@ -49,12 +49,10 @@ var (
},
ACLs: []*policies.ACLPolicy{
{
PolicyID: "azure-acl-123",
Target: policies.Dropped,
Direction: policies.Ingress,
},
{
PolicyID: "azure-acl-234",
Target: policies.Allowed,
Direction: policies.Ingress,
SrcList: []policies.SetInfo{
Expand Down
45 changes: 23 additions & 22 deletions npm/pkg/controlplane/translation/translatePolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ func ruleExists(ports []networkingv1.NetworkPolicyPort, peer []networkingv1.Netw
// (e.g., IPBlock, podSelector, namespaceSelector, or both podSelector and namespaceSelector).
func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, setInfo []policies.SetInfo) error {
if len(ports) == 0 {
acl := policies.NewACLPolicy(npmNetPol.NameSpace, npmNetPol.Name, policies.Allowed, direction)
acl := policies.NewACLPolicy(policies.Allowed, direction)
acl.AddSetInfo(setInfo)
npmNetPol.ACLs = append(npmNetPol.ACLs, acl)
return nil
Expand All @@ -316,7 +316,7 @@ func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Di
return err
}

acl := policies.NewACLPolicy(npmNetPol.NameSpace, npmNetPol.Name, policies.Allowed, direction)
acl := policies.NewACLPolicy(policies.Allowed, direction)
acl.AddSetInfo(setInfo)
npmNetPol.RuleIPSets = portRule(npmNetPol.RuleIPSets, acl, &ports[i], portKind)
npmNetPol.ACLs = append(npmNetPol.ACLs, acl)
Expand All @@ -325,7 +325,7 @@ func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Di
}

// translateRule translates ingress or egress rules and update npmNetPol object.
func translateRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, matchType policies.MatchType, ruleIndex int,
func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, direction policies.Direction, matchType policies.MatchType, ruleIndex int,
ports []networkingv1.NetworkPolicyPort, peers []networkingv1.NetworkPolicyPeer) error {
// TODO(jungukcho): need to clean up it.
// Leave allowExternal variable now while the condition is checked before calling this function.
Expand All @@ -335,7 +335,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Dire
// The code inside if condition is to handle allowing all internal traffic, but the case is handled in #2.4.
// So, this code may not execute. After confirming this, need to delete it.
if !portRuleExists && !peerRuleExists && !allowExternal {
acl := policies.NewACLPolicy(npmNetPol.NameSpace, npmNetPol.Name, policies.Allowed, direction)
acl := policies.NewACLPolicy(policies.Allowed, direction)
ruleIPSets, allowAllInternalSetInfo := allowAllInternal(matchType)
npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ruleIPSets)
acl.AddSetInfo([]policies.SetInfo{allowAllInternalSetInfo})
Expand All @@ -351,7 +351,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Dire
return err
}

portACL := policies.NewACLPolicy(npmNetPol.NameSpace, npmNetPol.Name, policies.Allowed, direction)
portACL := policies.NewACLPolicy(policies.Allowed, direction)
npmNetPol.RuleIPSets = portRule(npmNetPol.RuleIPSets, portACL, &ports[i], portKind)
npmNetPol.ACLs = append(npmNetPol.ACLs, portACL)
}
Expand All @@ -362,7 +362,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Dire
// #2.1 Handle IPBlock and port if exist
if peer.IPBlock != nil {
if len(peer.IPBlock.CIDR) > 0 {
ipBlockIPSet, ipBlockSetInfo := ipBlockRule(npmNetPol.Name, npmNetPol.NameSpace, direction, matchType, ruleIndex, peerIdx, peer.IPBlock)
ipBlockIPSet, ipBlockSetInfo := ipBlockRule(netPolName, npmNetPol.Namespace, direction, matchType, ruleIndex, peerIdx, peer.IPBlock)
npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ipBlockIPSet)
err := peerAndPortRule(npmNetPol, direction, ports, []policies.SetInfo{ipBlockSetInfo})
if err != nil {
Expand Down Expand Up @@ -397,7 +397,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Dire

// #2.3 handle podSelector and port if exist
if peer.PodSelector != nil && peer.NamespaceSelector == nil {
podSelectorIPSets, podSelectorList, err := podSelectorWithNS(npmNetPol.NameSpace, matchType, peer.PodSelector)
podSelectorIPSets, podSelectorList, err := podSelectorWithNS(npmNetPol.Namespace, matchType, peer.PodSelector)
if err != nil {
return err
}
Expand Down Expand Up @@ -441,14 +441,14 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Dire
}

// defaultDropACL returns ACLPolicy to drop traffic which is not allowed.
func defaultDropACL(policyNS, policyName string, direction policies.Direction) *policies.ACLPolicy {
dropACL := policies.NewACLPolicy(policyNS, policyName, policies.Dropped, direction)
func defaultDropACL(direction policies.Direction) *policies.ACLPolicy {
dropACL := policies.NewACLPolicy(policies.Dropped, direction)
return dropACL
}

// allowAllPolicy adds acl to allow all traffic including internal (i.e,. K8s cluster) and external (i.e., internet)
func allowAllPolicy(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction) {
allowAllACL := policies.NewACLPolicy(npmNetPol.NameSpace, npmNetPol.Name, policies.Allowed, direction)
allowAllACL := policies.NewACLPolicy(policies.Allowed, direction)
npmNetPol.ACLs = append(npmNetPol.ACLs, allowAllACL)
}

Expand All @@ -462,7 +462,7 @@ func isAllowAllToIngress(ingress []networkingv1.NetworkPolicyIngressRule) bool {

// ingressPolicy traslates NetworkPolicyIngressRule in NetworkPolicy object
// to NPMNetworkPolicy object.
func ingressPolicy(npmNetPol *policies.NPMNetworkPolicy, ingress []networkingv1.NetworkPolicyIngressRule) error {
func ingressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, ingress []networkingv1.NetworkPolicyIngressRule) error {
// #1. Allow all traffic from both internal and external.
// In yaml file, it is specified with '{}'.
if isAllowAllToIngress(ingress) {
Expand All @@ -473,20 +473,20 @@ func ingressPolicy(npmNetPol *policies.NPMNetworkPolicy, ingress []networkingv1.
// #2. If ingress is nil (in yaml file, it is specified with '[]'), it means "Deny all" - it does not allow receiving any traffic from others.
if ingress == 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.Ingress)
dropACL := defaultDropACL(policies.Ingress)
npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL)
return nil
}

// #3. Ingress rule is not AllowAll (including internal and external) and DenyAll policy.
// So, start translating ingress policy.
for i, rule := range ingress {
if err := translateRule(npmNetPol, policies.Ingress, policies.SrcMatch, i, rule.Ports, rule.From); err != nil {
if err := translateRule(npmNetPol, netPolName, policies.Ingress, policies.SrcMatch, i, rule.Ports, rule.From); err != nil {
return err
}
}
// Except for allow all traffic case in #1, the rest of them should have default drop rules.
dropACL := defaultDropACL(npmNetPol.NameSpace, npmNetPol.Name, policies.Ingress)
dropACL := defaultDropACL(policies.Ingress)
npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL)
return nil
}
Expand All @@ -501,7 +501,7 @@ func isAllowAllToEgress(egress []networkingv1.NetworkPolicyEgressRule) bool {

// egressPolicy traslates NetworkPolicyEgressRule in networkpolicy object
// to NPMNetworkPolicy object.
func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, egress []networkingv1.NetworkPolicyEgressRule) error {
func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, egress []networkingv1.NetworkPolicyEgressRule) error {
// #1. Allow all traffic to both internal and external.
// In yaml file, it is specified with '{}'.
if isAllowAllToEgress(egress) {
Expand All @@ -512,36 +512,37 @@ 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 {
// 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)
dropACL := defaultDropACL(policies.Egress)
npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL)
return nil
}

// #3. Egress rule is not AllowAll (including internal and external) and DenyAll.
// So, start translating egress policy.
for i, rule := range egress {
err := translateRule(npmNetPol, policies.Egress, policies.DstMatch, i, rule.Ports, rule.To)
err := translateRule(npmNetPol, netPolName, policies.Egress, policies.DstMatch, i, rule.Ports, rule.To)
if err != nil {
return err
}
}

// #3. Except for allow all traffic case in #1, the rest of them should have default drop rules.
// Add drop ACL to drop the rest of traffic which is not specified in Egress Spec.
dropACL := defaultDropACL(npmNetPol.NameSpace, npmNetPol.Name, policies.Egress)
dropACL := defaultDropACL(policies.Egress)
npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL)
return nil
}

// TranslatePolicy traslates networkpolicy object to NPMNetworkPolicy object
// and return the NPMNetworkPolicy object.
func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPolicy, error) {
npmNetPol := policies.NewNPMNetworkPolicy(npObj.Name, npObj.Namespace)
netPolName := npObj.Name
npmNetPol := policies.NewNPMNetworkPolicy(netPolName, npObj.Namespace)

// podSelector in spec.PodSelector is common for ingress and egress.
// Process this podSelector first.
var err error
npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.NameSpace, policies.EitherMatch, &npObj.Spec.PodSelector)
npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector)
if err != nil {
return nil, err
}
Expand All @@ -551,12 +552,12 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPol
// and Egress will be set if the NetworkPolicy has any egress rules.
for _, ptype := range npObj.Spec.PolicyTypes {
if ptype == networkingv1.PolicyTypeIngress {
err := ingressPolicy(npmNetPol, npObj.Spec.Ingress)
err := ingressPolicy(npmNetPol, netPolName, npObj.Spec.Ingress)
if err != nil {
return nil, err
}
} else {
err := egressPolicy(npmNetPol, npObj.Spec.Egress)
err := egressPolicy(npmNetPol, netPolName, npObj.Spec.Egress)
if err != nil {
return nil, err
}
Expand Down
Loading