fix: [NPM] adding unsupported translation check for windows NPM #1128
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| var members []string | ||
| switch op := req.Operator; op { | ||
| op := req.Operator | ||
| if invalidWindowsOperatorLimitation(op) { |
There was a problem hiding this comment.
Nitpick: slightly better unsupportedOpsInWindows instead of invalidWindowsOperatorLimitation
no strong opinion.
| var ( | ||
| errUnknownPortType = errors.New("unknown port Type") | ||
| // ErrUnsupportedTranslationFeature is returned when translation feature is not supported. | ||
| ErrUnsupportedTranslationFeature = errors.New("unsupported Windows translation feature") |
There was a problem hiding this comment.
nitpick: slightly better to have specific error message which features do not support.
For example,
ErrUnsupportedNamedPort = errors.New("unsupported namedport on windows ")
ErrUnsupportedNegativeMatch= errors.New("unsupported NotExist on windows ")no strong opinion.
| // So, start translating egress policy. | ||
| for i, rule := range egress { | ||
| translateRule(npmNetPol, policies.Egress, policies.DstMatch, i, rule.Ports, rule.To) | ||
| err := translateRule(npmNetPol, policies.Egress, policies.DstMatch, i, rule.Ports, rule.To) |
There was a problem hiding this comment.
minor: may be better to isolate err variable in if statement.
if err := ingressPolicy(npmNetPol, npObj.Spec.Ingress); err != nil {
return nil, err
}No strong opinion.
|
In the future, |
huntergregory
left a comment
huntergregory
left a comment
There was a problem hiding this comment.
just some nitpicks
| } | ||
|
|
||
| func invalidWindowsOperatorLimitation(op metav1.LabelSelectorOperator) bool { | ||
| if isWindows() && |
There was a problem hiding this comment.
nitpick:
return isWindows() && (op == ... || op == ...)
| return npmNetPol, nil | ||
| } | ||
|
|
||
| func invalidWindowsOperatorLimitation(op metav1.LabelSelectorOperator) bool { |
There was a problem hiding this comment.
nitpick: looks like we just use this in parseSelector.go. Can we put it in that file?
| return false | ||
| } | ||
|
|
||
| func isWindows() bool { |
There was a problem hiding this comment.
nitpick: we use isWindows() in this file but use runtime.GOOS == "windows" in pod controller. To keep consistency, can we either make isWindows a util func and use in pod controller or just use the boolean in all places?
There was a problem hiding this comment.
moving this to util as IsWindowsDP.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| return nil | ||
| } | ||
| klog.Errorf("Failed to translate podSelector in NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) | ||
| return errNetPolTranslationFailure |
There was a problem hiding this comment.
no preference: should we wrap the translatiion error for better context?
npmerrors.SimpleErrorWrapper("failed to translate network policy", err)
There was a problem hiding this comment.
i mean we can see this error message in logs anyway. I did not want to spam the same message multiple times.
Reason for Change:
Windows NPM dataplane does not support negative matches and named ports, so adding these two translation feature gates to warn user in controllers.
Issue Fixed:
Requirements:
Notes: