Support static ipBlock queries for clustergroupmembership #2577

Dyanngg · 2021-08-11T21:49:14Z

Sample output:

curl 127.0.0.1:8001/apis/controlplane.antrea.tanzu.vmware.com/v1beta2/clustergroupmembers/ipb-cg
{
  "kind": "ClusterGroupMembers",
  "apiVersion": "controlplane.antrea.tanzu.vmware.com/v1beta2",
  "metadata": {
    "name": "ipb-cg",
    "creationTimestamp": null
  },
  "effectiveIPBlocks": [
    {
      "ip": "AAAAAAAAAAAAAP//AQAAAA==",
      "prefixLength": 8
    }
  ]
}

codecov-commenter · 2021-08-11T22:41:52Z

Codecov Report

Merging #2577 (93e7983) into main (4aaa90e) will increase coverage by 0.05%.
The diff coverage is 66.66%.

@@            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

Flag	Coverage Δ
kind-e2e-tests	`47.80% <0.00%> (+<0.01%)`	⬆️
unit-tests	`41.72% <66.66%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/apis/controlplane/types.go	`0.00% <0.00%> (ø)`
...ntroller/networkpolicy/networkpolicy_controller.go	`80.91% <ø> (-0.04%)`	⬇️
pkg/controller/networkpolicy/clustergroup.go	`65.74% <33.33%> (-0.62%)`	⬇️
.../registry/networkpolicy/clustergroupmember/rest.go	`88.23% <100.00%> (+4.90%)`	⬆️
pkg/apiserver/storage/ram/store.go	`78.94% <0.00%> (-1.51%)`	⬇️
pkg/agent/nodeportlocal/k8s/npl_controller.go	`60.34% <0.00%> (+1.03%)`	⬆️
pkg/controller/networkpolicy/status_controller.go	`73.20% <0.00%> (+1.96%)`	⬆️
...gent/controller/networkpolicy/status_controller.go	`75.34% <0.00%> (+2.73%)`	⬆️
pkg/apiserver/certificate/certificate.go	`76.71% <0.00%> (+6.84%)`	⬆️
... and 1 more

abhiraut · 2021-08-11T23:52:01Z

pkg/controller/networkpolicy/clustergroup.go

-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) {


can we instead return two structs (members, ipb), like how we return (pods, ee) instead of interface?

Pushed a new commit that uses this alternative approach

Dyanngg · 2021-08-13T00:23:00Z

Fixed lint issue /test-all

abhiraut · 2021-08-19T23:28:03Z

/test-networkpolicy

antoninbas · 2021-08-19T23:33:04Z

pkg/apis/controlplane/types.go

 // 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))


Suggested change

return ipn.IP.String() + "/" + strconv.Itoa(int(ipn.PrefixLength))

return fmt.Sprintf("%s/%d", ipn.IP, pn.PrefixLength)

antoninbas · 2021-08-19T23:35:02Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

+	if len(ipBlocks) > 0 {
+		effectiveIPBlocks := make([]controlplane.IPNet, 0, len(ipBlocks))
+		for _, ipb := range ipBlocks {
+			// ClusterGroup ipBlock cannot have except slices


I don't understand this comment?

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

antoninbas · 2021-08-19T23:39:25Z

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go

+	} 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


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 GroupMemberSets and one map for IPBlocks.

oops i missed this one

antoninbas · 2021-08-19T23:39:49Z

pkg/controller/networkpolicy/clustergroup.go

@@ -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.


s/defined by/defined with

antoninbas

LGTM, thanks for making the changes

abhiraut · 2021-08-20T18:36:20Z

/test-all

antoninbas · 2021-08-23T17:51:50Z

@abhiraut can't merge because of a conflict, PTAL

Signed-off-by: Yang Ding <dingyang@vmware.com>

Co-authored-by: abhiraut <rauta@vmware.com> Signed-off-by: abhiraut <rauta@vmware.com>

abhiraut · 2021-08-23T19:16:43Z

@abhiraut can't merge because of a conflict, PTAL

rebased

abhiraut · 2021-08-23T19:16:54Z

/test-all

antoninbas · 2021-08-23T21:44:51Z

/test-windows-networkpolicy

Dyanngg requested review from edwardbadboy, abhiraut, tnqn and liu4480 August 11, 2021 21:49

Dyanngg force-pushed the cg-show-static-member branch from 899da47 to 0b1d55f Compare August 11, 2021 23:35

abhiraut reviewed Aug 11, 2021

View reviewed changes

Dyanngg force-pushed the cg-show-static-member branch 2 times, most recently from 276c2c7 to 8468a66 Compare August 12, 2021 21:46

Dyanngg requested a review from abhiraut August 12, 2021 21:47

abhiraut previously approved these changes Aug 12, 2021

View reviewed changes

Dyanngg dismissed abhiraut’s stale review via 1ab6bbd August 13, 2021 00:22

Dyanngg force-pushed the cg-show-static-member branch from 8468a66 to 1ab6bbd Compare August 13, 2021 00:22

Dyanngg requested a review from abhiraut August 13, 2021 00:23

abhiraut previously approved these changes Aug 13, 2021

View reviewed changes

abhiraut requested a review from antoninbas August 17, 2021 18:33

abhiraut added this to the Antrea v1.3 release milestone Aug 17, 2021

antoninbas reviewed Aug 19, 2021

View reviewed changes

abhiraut dismissed their stale review via 44d6b99 August 20, 2021 00:00

abhiraut force-pushed the cg-show-static-member branch from 44d6b99 to ea3d127 Compare August 20, 2021 01:27

abhiraut requested a review from antoninbas August 20, 2021 01:29

abhiraut force-pushed the cg-show-static-member branch from ea3d127 to 656a7a2 Compare August 20, 2021 02:09

antoninbas previously approved these changes Aug 20, 2021

View reviewed changes

Dyanngg and others added 3 commits August 23, 2021 11:58

Support static ipBlock queries for clustergroupmembership

324512f

Signed-off-by: Yang Ding <dingyang@vmware.com>

Address comments and add UT

46d8730

Signed-off-by: Yang Ding <dingyang@vmware.com>

Address comments

93e7983

Co-authored-by: abhiraut <rauta@vmware.com> Signed-off-by: abhiraut <rauta@vmware.com>

abhiraut dismissed antoninbas’s stale review via 93e7983 August 23, 2021 19:12

abhiraut force-pushed the cg-show-static-member branch from 656a7a2 to 93e7983 Compare August 23, 2021 19:12

antoninbas approved these changes Aug 23, 2021

View reviewed changes

abhiraut mentioned this pull request Aug 23, 2021

Remove the restriction that an ACNP referred ClusterGroup must be created before the ACNP #2478

Merged

antoninbas merged commit 35d02ca into antrea-io:main Aug 24, 2021

abhiraut mentioned this pull request Sep 29, 2021

Support IP Address in ClusterGroup Membership and Association Query #2492

Closed

Dyanngg deleted the cg-show-static-member branch October 26, 2021 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support static ipBlock queries for clustergroupmembership #2577

Support static ipBlock queries for clustergroupmembership #2577

Dyanngg commented Aug 11, 2021

codecov-commenter commented Aug 11, 2021 •

edited

Loading

abhiraut Aug 11, 2021

Dyanngg Aug 12, 2021

Dyanngg commented Aug 13, 2021

abhiraut commented Aug 19, 2021

antoninbas Aug 19, 2021

abhiraut Aug 19, 2021

antoninbas Aug 19, 2021

abhiraut Aug 19, 2021

antoninbas Aug 19, 2021

abhiraut Aug 20, 2021

abhiraut Aug 20, 2021

antoninbas Aug 19, 2021

abhiraut Aug 19, 2021

antoninbas left a comment

abhiraut commented Aug 20, 2021

antoninbas commented Aug 23, 2021

abhiraut commented Aug 23, 2021

abhiraut commented Aug 23, 2021

antoninbas commented Aug 23, 2021

	return ipn.IP.String() + "/" + strconv.Itoa(int(ipn.PrefixLength))
	return fmt.Sprintf("%s/%d", ipn.IP, pn.PrefixLength)

Support static ipBlock queries for clustergroupmembership #2577

Support static ipBlock queries for clustergroupmembership #2577

Conversation

Dyanngg commented Aug 11, 2021

codecov-commenter commented Aug 11, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg commented Aug 13, 2021

abhiraut commented Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

abhiraut commented Aug 20, 2021

antoninbas commented Aug 23, 2021

abhiraut commented Aug 23, 2021

abhiraut commented Aug 23, 2021

antoninbas commented Aug 23, 2021

codecov-commenter commented Aug 11, 2021 •

edited

Loading