Add Namespaced Group Membership API #5380

qiyueyao · 2023-08-10T06:03:24Z

Adds membership API for namespaced Group at groupmembers.controlplane.antrea.io. The group association API is still available for both ClusterGroup and namespaced Group.

# curl "<k8s-apiserver>:8001/apis/controlplane.antrea.io/v1beta2/namespaces/<ns>/groupmembers/<name>?page=<page#>&limit=<limit#>"
{
  "kind": "GroupMembers",
  "apiVersion": "controlplane.antrea.io/v1beta2",
  "metadata": {
    "name": "test-group",
    "namespace": "default",
    "creationTimestamp": null
  },
  "effectiveMembers": [
    {
      "pod": {
        "name": "pod1",
        "namespace": "test"
      },
      "ips": [
        "AAAAAAAAAAAAAP//CgoBBg=="
      ]
    }
  ],
  "effectiveIPBlocks": null,
  "totalMembers": 1,
  "totalPages": 1,
  "currentPage": 1
}

Fixes #5269

jianjuns · 2023-08-17T18:58:07Z

Question - I remember we have another API to query Groups given a Pod/ExternalEntity. Do we support that for Namespaced Groups? @qiyueyao @Dyanngg

Dyanngg · 2023-08-17T21:10:02Z

Question - I remember we have another API to query Groups given a Pod/ExternalEntity. Do we support that for Namespaced Groups? @qiyueyao @Dyanngg

That API already works for namespaced groups

pkg/controller/networkpolicy/group.go

pkg/apis/controlplane/v1beta2/types.go

pkg/apiserver/registry/networkpolicy/util.go

jianjuns · 2023-08-17T21:36:59Z

pkg/apiserver/registry/networkpolicy/util.go

+// PaginateMemberList returns paginated results if meaningful options are provided, options should never be nil.
+// Paginated results are continuous only when there is no member change across multiple calls.
+// Pagination is not processed if either page number or limit = 0, thus returns full member list.
+// Returns an error for invalid options; returns empty list for page number beyond the total pages range.


An error is returned for invalid options, and an empty list is returned for a page number out of the pages range.

pkg/apiserver/registry/networkpolicy/util.go

jianjuns · 2023-08-21T23:05:18Z

pkg/apis/controlplane/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// GroupMembers is a list of GroupMember objects or IPBlocks that are currently selected by a Group.
+type GroupMembers struct {


This struct is the same as ClusterGroupMembers. Do we need to redefine?

And could not we reuse GroupMembers defined in pkg/apis/controlplane/v1beta2/types.go?

I remember controlplane/types.go and v1beta2/types.go where intentionally kept the same, is this outdated?

Humm.. I do not know this. Let us see what @tnqn would recommend.

For ClusterGroupMembers and GroupMembers, it's correct to redefine the struct even they are the same because every struct maps its own API, many generated code are based on the struct definition. Besides, it's a convention in K8s API, see https://github.com/kubernetes/kubernetes/blob/fb785f1f42183e26e2b9f042474391c4d58433bb/pkg/apis/rbac/types.go#L109-L120 and https://github.com/kubernetes/kubernetes/blob/fb785f1f42183e26e2b9f042474391c4d58433bb/pkg/apis/rbac/types.go#L174-L186

For controlplane/types.go and v1beta2/types.go, it's also correct to redefine the struct because the former is the internal representation of the data while the latter is the versioned one. The internal representation needs to be convertible with multiple versions so it may change along with the change of the API while the versioned one should remain compatible. For a new API, its internal struct and versioned struct is normally the same but they will diverge when the API evolves. The following docs has more informaiton:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md

jianjuns · 2023-08-21T23:08:05Z

pkg/apis/controlplane/v1beta2/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// GroupMembers is a list of GroupMember objects or IPBlocks that are currently selected by a Group.
+type GroupMembers struct {


Same question - why we need to redefine a struct?

I redefined because on line 97 // +genclient:nonNamespaced, ClusterGroupMembers API is non namespaced and GroupMembers API is namespaced, and the genclient is auto generated. So I only generalized the pagination function not the Get(). Is there any suggestion to solve the problem? I'm not aware of, thanks!

Could we do:
type ClusterGroupMember GroupMember?

Here? Then they are still not differentiated in terms of nonNamespaced and namespaced 🤔

jianjuns · 2023-08-21T23:10:50Z

pkg/apiserver/registry/groupmember/rest.go

+		return nil, errors.NewInternalError(err)
+	}
+	// Retrieve options used for pagination.
+	getOptions, ok := options.(*controlplane.PaginationGetOptions)


If we use a shared type for Group and ClusterGroup, we can also share the Get() code with ClusterGroup?

I think we could extract the common code even when there are two different structs. It just needs a function like GetPaginatedMembers(querier groupMembershipQuerier, name string, options runtime.Object) (members []GroupMember, ipNets []IPNet, totalMembers, totalPages, currentPage int64)

tnqn

LGTM overall

tnqn · 2023-08-23T10:49:14Z

pkg/apis/controlplane/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// GroupMembers is a list of GroupMember objects or IPBlocks that are currently selected by a Group.
+type GroupMembers struct {


For ClusterGroupMembers and GroupMembers, it's correct to redefine the struct even they are the same because every struct maps its own API, many generated code are based on the struct definition. Besides, it's a convention in K8s API, see https://github.com/kubernetes/kubernetes/blob/fb785f1f42183e26e2b9f042474391c4d58433bb/pkg/apis/rbac/types.go#L109-L120 and https://github.com/kubernetes/kubernetes/blob/fb785f1f42183e26e2b9f042474391c4d58433bb/pkg/apis/rbac/types.go#L174-L186

For controlplane/types.go and v1beta2/types.go, it's also correct to redefine the struct because the former is the internal representation of the data while the latter is the versioned one. The internal representation needs to be convertible with multiple versions so it may change along with the change of the API while the versioned one should remain compatible. For a new API, its internal struct and versioned struct is normally the same but they will diverge when the API evolves. The following docs has more informaiton:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md

tnqn · 2023-08-23T11:17:39Z

pkg/apiserver/registry/groupmember/rest.go

+		return nil, errors.NewInternalError(err)
+	}
+	// Retrieve options used for pagination.
+	getOptions, ok := options.(*controlplane.PaginationGetOptions)


I think we could extract the common code even when there are two different structs. It just needs a function like GetPaginatedMembers(querier groupMembershipQuerier, name string, options runtime.Object) (members []GroupMember, ipNets []IPNet, totalMembers, totalPages, currentPage int64)

tnqn · 2023-08-23T11:22:45Z

pkg/apiserver/registry/groupmember/rest.go

+}
+
+type groupMembershipQuerier interface {
+	GetGroupMembersByNamespace(name, namespace string) (controlplane.GroupMemberSet, []controlplane.IPBlock, error)


I feel the "ByNamespace" is a bit misleading. Although making it more accurate like ByNamespacedName could fix it, I wonder if we could just merge the two arguments and just use GetGroupMembers given the callee doesn't need to separate it anyway, we could comment that the method accepts both ClusterGroup name and Group name.

Or we could rename existing GetGroupMembers to GetClusterGroupMembers which takes ClusterGroup name and add GetGroupMembers which takes Group Namespace and Name.

Done, generalized GroupMembershipQuerier and the common codes.

tnqn · 2023-08-28T08:32:45Z

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

-func paginateMemberList(memberList *controlplane.ClusterGroupMembers, pageInfo *controlplane.PaginationGetOptions) error {
+// Pagination is not enabled if either page number or limit = 0, in which the full member list is returned.
+// An error is returned for invalid options, and an empty list is returned for a page number out of the pages range.
+func PaginateMemberList(effectiveMembers *[]controlplane.GroupMember, pageInfo *controlplane.PaginationGetOptions) (int64, int64, error) {


Does it need to be public?

Makes sense, updated.

Currently Antrea supports ClusterGroup and namespaced Group CRD, but only provides API for ClusterGroup membership. This solution adds membership API for namespaced Group at namespacedgroupmembers.controlplane.antrea.io. The group association API is still available for both ClusterGroup and namespaced Group. Fixes antrea-io#5269 Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao · 2023-08-29T01:41:43Z

/test-all

tnqn

LGTM

tnqn · 2023-08-29T04:51:15Z

@jianjuns any other comments? Do we need a pre-release like 1.14.0-alpha.0 for this API?

jianjuns · 2023-08-29T17:14:34Z

LGTM

qiyueyao force-pushed the group-api branch 3 times, most recently from 0a79296 to 6203a0f Compare August 10, 2023 23:12

qiyueyao added the area/api Issues or PRs related to an API. label Aug 10, 2023

qiyueyao added this to the Antrea v1.14 release milestone Aug 10, 2023

qiyueyao requested review from Dyanngg, tnqn and GraysonWu August 11, 2023 00:16

shi0rik0 mentioned this pull request Aug 14, 2023

Switch traceflow CRD validation to webhook validation. #5230

Merged

qiyueyao requested a review from jianjuns August 17, 2023 13:28

Dyanngg reviewed Aug 17, 2023

View reviewed changes

pkg/controller/networkpolicy/group.go Outdated Show resolved Hide resolved

jianjuns reviewed Aug 17, 2023

View reviewed changes

pkg/apis/controlplane/v1beta2/types.go Outdated Show resolved Hide resolved

jianjuns reviewed Aug 17, 2023

View reviewed changes

jianjuns reviewed Aug 21, 2023

View reviewed changes

tnqn reviewed Aug 23, 2023

View reviewed changes

jianjuns mentioned this pull request Aug 23, 2023

Release 1.13.1 #5430

Merged

qiyueyao force-pushed the group-api branch 2 times, most recently from 2f4e2a8 to 064b669 Compare August 24, 2023 17:51

qiyueyao requested review from tnqn, Dyanngg and jianjuns August 25, 2023 00:31

tnqn reviewed Aug 28, 2023

View reviewed changes

qiyueyao force-pushed the group-api branch from 064b669 to b1f4de7 Compare August 28, 2023 19:47

tnqn approved these changes Aug 29, 2023

View reviewed changes

jianjuns merged commit 2135110 into antrea-io:main Aug 29, 2023

qiyueyao mentioned this pull request Sep 28, 2023

Support GroupMembers API Pagination for Client #5533

Merged

qiyueyao deleted the group-api branch October 12, 2023 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Namespaced Group Membership API #5380

Add Namespaced Group Membership API #5380

qiyueyao commented Aug 10, 2023 •

edited

Loading

jianjuns commented Aug 17, 2023

Dyanngg commented Aug 17, 2023

jianjuns Aug 17, 2023

jianjuns Aug 21, 2023

jianjuns Aug 21, 2023

qiyueyao Aug 21, 2023

jianjuns Aug 22, 2023

tnqn Aug 23, 2023

jianjuns Aug 21, 2023

qiyueyao Aug 21, 2023

jianjuns Aug 21, 2023

qiyueyao Aug 22, 2023

jianjuns Aug 21, 2023

tnqn Aug 23, 2023

tnqn left a comment

tnqn Aug 23, 2023

tnqn Aug 23, 2023

tnqn Aug 23, 2023

qiyueyao Aug 24, 2023

tnqn Aug 28, 2023

qiyueyao Aug 28, 2023

qiyueyao commented Aug 29, 2023

tnqn left a comment

tnqn commented Aug 29, 2023

jianjuns commented Aug 29, 2023

Add Namespaced Group Membership API #5380

Add Namespaced Group Membership API #5380

Conversation

qiyueyao commented Aug 10, 2023 • edited Loading

jianjuns commented Aug 17, 2023

Dyanngg commented Aug 17, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a 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

qiyueyao commented Aug 29, 2023

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 29, 2023

jianjuns commented Aug 29, 2023

qiyueyao commented Aug 10, 2023 •

edited

Loading