-
Notifications
You must be signed in to change notification settings - Fork 366
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.
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
Support static ipBlock queries for clustergroupmembership #2577
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2577 +/- ##
==========================================
+ Coverage 60.73% 60.78% +0.05%
==========================================
Files 285 286 +1
Lines 23045 23053 +8
==========================================
+ Hits 13996 14013 +17
+ Misses 7577 7571 -6
+ Partials 1472 1469 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
899da47
to
0b1d55f
Compare
func (n *NetworkPolicyController) GetGroupMembers(cgName string) (controlplane.GroupMemberSet, error) { | ||
// If the ClusterGroup is defined by IPBlocks, the returned members will be []controlplane.IPBlock. | ||
// Otherwise, the returned members will be of type controlplane.GroupMemberSet. | ||
func (n *NetworkPolicyController) GetGroupMembers(cgName string) (interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we instead return two structs (members, ipb), like how we return (pods, ee) instead of interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a new commit that uses this alternative approach
276c2c7
to
8468a66
Compare
8468a66
to
1ab6bbd
Compare
Fixed lint issue /test-all |
/test-networkpolicy |
pkg/apis/controlplane/types.go
Outdated
// IPNet describes an IP network. | ||
type IPNet struct { | ||
IP IPAddress | ||
PrefixLength int32 | ||
} | ||
|
||
func (ipn IPNet) String() string { | ||
return ipn.IP.String() + "/" + strconv.Itoa(int(ipn.PrefixLength)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ipn.IP.String() + "/" + strconv.Itoa(int(ipn.PrefixLength)) | |
return fmt.Sprintf("%s/%d", ipn.IP, pn.PrefixLength) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if len(ipBlocks) > 0 { | ||
effectiveIPBlocks := make([]controlplane.IPNet, 0, len(ipBlocks)) | ||
for _, ipb := range ipBlocks { | ||
// ClusterGroup ipBlock cannot have except slices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the motivation was to showcase that the effective IPs can be calculated easily without the need to remove except slices from the allowed CIDR because ClusterGroup IPBlocks do not support an Except field, unlike K8s NetPol IPblocks
} else if uid == "cgIPBlock" { | ||
testCIDR := controlplane.IPNet{ | ||
IP: controlplane.IPAddress(net.ParseIP("10.0.0.1")), | ||
PrefixLength: int32(24), | ||
} | ||
return nil, []controlplane.IPBlock{{CIDR: testCIDR}}, nil | ||
} | ||
return controlplane.GroupMemberSet{}, nil | ||
return nil, nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a great way to write that test, hardcoding the uid used in a test case below like this
it feels like fakeQuerier
could have 2 maps instead of 1: one map for GroupMemberSet
s and one map for IPBlock
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops i missed this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -427,11 +427,16 @@ func (n *NetworkPolicyController) getAssociatedGroupsByName(grpName string) []an | |||
} | |||
|
|||
// GetGroupMembers returns the current members of a ClusterGroup. | |||
func (n *NetworkPolicyController) GetGroupMembers(cgName string) (controlplane.GroupMemberSet, error) { | |||
// If the ClusterGroup is defined by IPBlocks, the returned members will be []controlplane.IPBlock. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/defined by/defined with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
44d6b99
to
ea3d127
Compare
ea3d127
to
656a7a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for making the changes
/test-all |
@abhiraut can't merge because of a conflict, PTAL |
Signed-off-by: Yang Ding <dingyang@vmware.com>
Signed-off-by: Yang Ding <dingyang@vmware.com>
Co-authored-by: abhiraut <rauta@vmware.com> Signed-off-by: abhiraut <rauta@vmware.com>
656a7a2
to
93e7983
Compare
rebased |
/test-all |
/test-windows-networkpolicy |
Sample output: