[NPM] Adding prefixes to IPSets in dataplane #1047
Conversation
huntergregory
left a comment
huntergregory
left a comment
There was a problem hiding this comment.
Changes look great. Noted a couple things about dataplane
npm/pkg/dataplane/dataplane.go
Outdated
| // Check if any 2nd level IPSets are generated by Controller with members | ||
| // Apply members to the list set | ||
| if set.Kind == ipsets.ListSet { | ||
| if ipsets.GetSetKind(set.Metadata.Type) == ipsets.ListSet { |
There was a problem hiding this comment.
mismatched types Type and Kind
There was a problem hiding this comment.
can we do a check in the first loop to see if len(set.MemberIPSets) > 0, then the metadata Type needs to be a list kind or we throw an error?
There was a problem hiding this comment.
same comments apply to deleteIPSetsAndReferences
There was a problem hiding this comment.
Actually we missed a case which if forgot to add, CIDR ipsets can be translated and provided to us, i will need to add CIDR block ips to corresponding IPSets, so we should not be throwing an error if it is not list
There was a problem hiding this comment.
But a CIDR is a hash set, right? why not make the members/list check still?
| IpsetNomatch string = "nomatch" | ||
|
|
||
| // Prefixes for ipsets | ||
| NamedPortIPSetPrefix string = "namedport:" |
There was a problem hiding this comment.
Is there a reason to have ":" instead of "-" for NamedPortIPsetPrefix?
There was a problem hiding this comment.
We used ":" name for this alone because k8s would not allow adding a ":" in labels. keeping it same in v2 even though there is no other reason apart from being backward compatible
| // Copyright 2017 Microsoft. All rights reserved. | ||
| // MIT License | ||
|
|
||
| //go:build windows |
There was a problem hiding this comment.
can we add the _windows suffix to this filename?
This PR includes changes to distinguish different kinds of IPSets by expanding on existing prefixes we use. This PR also updates the existing Dataplane APIs now to take in IPSetMetaData struct instead of strings