NPM: Linux Policy Manager #1069
Conversation
InitializeNPMChains, RemoveNPMChains, and ReconcileChains WIP fix compile errors in composer WIP UTs and finished but no retry logic yet assert equal file strings remove unused stuff func to test equality of translatedipsets UTs final adds
|
We have to think how to redesign UTs for generic policy/ipsetmanager since the fakeexec calls will be specific to the OS. This PR fails the generic pMgr UTs currently. |
npm/pkg/dataplane/policies/policymanager-chain-management_linux.go
Outdated
Show resolved
Hide resolved
npm/pkg/dataplane/policies/policymanager-chain-management_linux.go
Outdated
Show resolved
Hide resolved
npm/pkg/dataplane/policies/policymanager-chain-management_linux.go
Outdated
Show resolved
Hide resolved
…ane interface calls later)
…or reporting error on chain destroy failures
…ant delete jump rules from ingress/egress chain to policy chain
| return nil | ||
| } | ||
|
|
||
| func checkForErrors(networkPolicies ...*NPMNetworkPolicy) error { |
There was a problem hiding this comment.
Great, i can a OSSpecific function inside this to see if acl policy supports a certain feature.
JungukCho
left a comment
JungukCho
left a comment
There was a problem hiding this comment.
Great PR and left some comments. Excited to test end-to-end later with new translation and linux dataplane.
One impression is that structures and files in policy package are slightly complex to manage linux and windows. It may be due to less time for me to read policy packages. Also I did not walk through codes related to windows in policy package in details.
npm/pkg/dataplane/parse/parser.go
Outdated
| // parseChainNameFromRuleLine gets the chain name from given rule line. | ||
| func parseChainNameFromRuleLine(ruleLine []byte) (string, int) { | ||
| // GetChainNameFromRuleLine gets the chain name from given rule line. | ||
| func GetChainNameFromRuleLine(ruleLine []byte) (chainName string, ruleReadIndex int) { |
There was a problem hiding this comment.
Minor: for consistency parseChainNameFromRuleLine is better?
I thought since the package name is parse, we can ideally drop all parse- prefix in the functions while it is not scope of this PR.
There was a problem hiding this comment.
I will revert it to the old name for now (unexported since we don't need it elsewhere), but golint complains about stuttering when you have parse.ParseChain...
There was a problem hiding this comment.
Actually, I prefer dropping all "Parse-" prefix in the code with the same reasons. If you are ok and it does not require heavy change, we can drop it in this PR or put it in a next small PR. It's up to you.
There was a problem hiding this comment.
Ok, I'll drop for this PR
npm/pkg/dataplane/policies/policy.go
Outdated
| DstSrcMatch MatchType = 5 | ||
| ) | ||
|
|
||
| func (policy *ACLPolicy) hasKnownDirection() bool { |
There was a problem hiding this comment.
Minor: would we put these receiver functions close to ACLPolicy strcut definition?
Also, may just use (aclp (or ap) *ACLPolicy) instead of (policy *ACLPolicy)? While all functions are very small, there are another policy here - NPMNetworkPolicy. So, explicitly use some prefixes.
There was a problem hiding this comment.
I rearranged these. Let me know if you have any suggestions
| IptablesAzureEgressToPodChain string = "AZURE-NPM-EGRESS-TO-POD" | ||
|
|
||
| // Below are the skb->mark NPM will use for different criteria | ||
| IptablesAzureClearMarkHex string = "0x0" |
There was a problem hiding this comment.
Suggestion: now there are multiple dataplanes. So, is it better to these specific const variables in specific dataplane for better maintanance?
There was a problem hiding this comment.
That's a good idea. Update in another PR?
…ing, add windows UT files
JungukCho
left a comment
JungukCho
left a comment
There was a problem hiding this comment.
Put minor comments and they can be revised in your next PR if they make sense.
|
|
||
| // will make a similar func for on update eventually | ||
| func (pMgr *PolicyManager) deleteOldJumpRulesOnRemove(policy *NPMNetworkPolicy) error { | ||
| fmt.Println(policy.ACLs[0]) |
There was a problem hiding this comment.
nitpick: forget removing this one?
| fmt.Println(policy.ACLs[0]) | ||
|
|
||
| shouldDeleteIngress, shouldDeleteEgress := policy.hasIngressAndEgress() | ||
| fmt.Println(shouldDeleteIngress, shouldDeleteEgress) |
There was a problem hiding this comment.
nitpick: forget removing this one?
|
|
||
| // add AZURE-NPM chain rules | ||
| creator.AddLine("", nil, util.IptablesAppendFlag, util.IptablesAzureChain, util.IptablesJumpFlag, util.IptablesAzureIngressChain) | ||
|
|
There was a problem hiding this comment.
nitPick: remove unneeded newlines in this function if there is no specific reason.
redesign iptables flow for NPM and add/remove ACLs in batch OS calls
TODO in next iteration: