ADD pod resource & routes to get member-cluster namespace/deployment/pod #150

Heylosky · 2024-11-27T07:58:50Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add pod resources. Add routes for obtaining member cluster pods( list and detail ), namespaces, and deployments.
Set a router group under /api/v1 for prefix /member/:clustername, to get member cluster resources.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

warjiang · 2024-11-27T08:00:05Z

/assign

warjiang · 2024-11-27T08:00:17Z

Thansk, i'll check it today~

warjiang · 2024-11-30T16:32:46Z

cmd/api/app/routes/member/deployment/handler.go

+	if err != nil {
+		common.Fail(c, err)
+		return
+	}


duplicate check for existence of member cluster, for these kind of check, we can use middleware in gin, like :

// middleware.go func EnsureMemberClusterMiddleware() gin.HandlerFunc { return func(c *gin.Context) { karmadaClient := client.InClusterKarmadaClient() _, err := karmadaClient.ClusterV1alpha1().Clusters().Get(context.TODO(), c.Param("clustername"), metav1.GetOptions{}) if err != nil { c.AbortWithStatusJSON(http.StatusOK, common.BaseResponse{ Code: 500, Msg: err.Error(), }) return } c.Next() } } // usage in setup.go member = v1.Group("/member/:clustername") member.Use(EnsureMemberClusterMiddleware())

when the code executing in the handler, we can ensure the existence of membercluster, otherwise it will be abort in the middleware.

warjiang · 2024-11-30T16:36:51Z

I've check the code locally, the only one problem is duplicate check of membercluster. Apart from that, one question I want to talk about is that for all kind of resources in member cluster, is it need to make another copy just like the ones we implemented for the karmada apiserver. It seems that the answer is yes, but is there a better to implemented that. Just a question, i have no idea till now.

Heylosky · 2024-12-02T08:57:31Z

I've check the code locally, the only one problem is duplicate check of membercluster. Apart from that, one question I want to talk about is that for all kind of resources in member cluster, is it need to make another copy just like the ones we implemented for the karmada apiserver. It seems that the answer is yes, but is there a better to implemented that. Just a question, i have no idea till now.

Perhaps add a middleware for the karmada apiserver as well, and put the client initialization process in the middleware, and then pass the client using the c.Set and c.Get? This way we can use the same implementation across all resources in the member cluster and the karmada control plane. Just like:

func EnsureMemberClusterMiddleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		karmadaClient := client.InClusterKarmadaClient()
		_, err := karmadaClient.ClusterV1alpha1().Clusters().Get(context.TODO(), c.Param("clustername"), metav1.GetOptions{})
		if err != nil {
			c.AbortWithStatusJSON(http.StatusOK, common.BaseResponse{
				Code: 500,
				Msg:  err.Error(),
			})
			return
		}
		// Init client for member cluster
		memberClient := client.InClusterClientForMemberCluster(c.Param("clustername"))
		c.Set("client", memberClient)

		c.Next()
	}
}

func V1Middleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		// Init client for controller cluster
		k8sClient := client.InClusterClientForKarmadaApiServer()
		c.Set("client", k8sClient)

		c.Next()
	}
}

For example, to get deployments:

func handleGetDeployments(c *gin.Context) {
	if value, exists := c.Get("client"); exists {
		if memberClient, ok := value.(kubeclient.Interface); ok {
			namespace := common.ParseNamespacePathParameter(c)
			dataSelect := common.ParseDataSelectPathParameter(c)
			result, err := deployment.GetDeploymentList(memberClient, namespace, dataSelect)
			if err != nil {
				common.Fail(c, err)
				return
			}
			common.Success(c, result)
		} else {
			c.JSON(500, gin.H{"error": "Failed to assert memberClient to kubernetes.Interface"})
		}
	} else {
		c.JSON(500, gin.H{"error": "memberClient not found"})
	}
}

In this case, maybe we can avoid some copy?

warjiang · 2024-12-03T00:45:53Z

I've check the code locally, the only one problem is duplicate check of membercluster. Apart from that, one question I want to talk about is that for all kind of resources in member cluster, is it need to make another copy just like the ones we implemented for the karmada apiserver. It seems that the answer is yes, but is there a better to implemented that. Just a question, i have no idea till now.

Perhaps add a middleware for the karmada apiserver as well, and put the client initialization process in the middleware, and then pass the client using the c.Set and c.Get? This way we can use the same implementation across all resources in the member cluster and the karmada control plane. Just like:
func EnsureMemberClusterMiddleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		karmadaClient := client.InClusterKarmadaClient()
		_, err := karmadaClient.ClusterV1alpha1().Clusters().Get(context.TODO(), c.Param("clustername"), metav1.GetOptions{})
		if err != nil {
			c.AbortWithStatusJSON(http.StatusOK, common.BaseResponse{
				Code: 500,
				Msg:  err.Error(),
			})
			return
		}
		// Init client for member cluster
		memberClient := client.InClusterClientForMemberCluster(c.Param("clustername"))
		c.Set("client", memberClient)

		c.Next()
	}
}

func V1Middleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		// Init client for controller cluster
		k8sClient := client.InClusterClientForKarmadaApiServer()
		c.Set("client", k8sClient)

		c.Next()
	}
}
For example, to get deployments:
func handleGetDeployments(c *gin.Context) {
	if value, exists := c.Get("client"); exists {
		if memberClient, ok := value.(kubeclient.Interface); ok {
			namespace := common.ParseNamespacePathParameter(c)
			dataSelect := common.ParseDataSelectPathParameter(c)
			result, err := deployment.GetDeploymentList(memberClient, namespace, dataSelect)
			if err != nil {
				common.Fail(c, err)
				return
			}
			common.Success(c, result)
		} else {
			c.JSON(500, gin.H{"error": "Failed to assert memberClient to kubernetes.Interface"})
		}
	} else {
		c.JSON(500, gin.H{"error": "memberClient not found"})
	}
}
In this case, maybe we can avoid some copy?

+1, I think it's a good practice. One more suggestion is that, maybe we can use different client name to distinguish client for k8s and client for karmada.

Heylosky · 2024-12-03T03:36:15Z

I've check the code locally, the only one problem is duplicate check of membercluster. Apart from that, one question I want to talk about is that for all kind of resources in member cluster, is it need to make another copy just like the ones we implemented for the karmada apiserver. It seems that the answer is yes, but is there a better to implemented that. Just a question, i have no idea till now.

Perhaps add a middleware for the karmada apiserver as well, and put the client initialization process in the middleware, and then pass the client using the c.Set and c.Get? This way we can use the same implementation across all resources in the member cluster and the karmada control plane. Just like:
func EnsureMemberClusterMiddleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		karmadaClient := client.InClusterKarmadaClient()
		_, err := karmadaClient.ClusterV1alpha1().Clusters().Get(context.TODO(), c.Param("clustername"), metav1.GetOptions{})
		if err != nil {
			c.AbortWithStatusJSON(http.StatusOK, common.BaseResponse{
				Code: 500,
				Msg:  err.Error(),
			})
			return
		}
		// Init client for member cluster
		memberClient := client.InClusterClientForMemberCluster(c.Param("clustername"))
		c.Set("client", memberClient)

		c.Next()
	}
}

func V1Middleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		// Init client for controller cluster
		k8sClient := client.InClusterClientForKarmadaApiServer()
		c.Set("client", k8sClient)

		c.Next()
	}
}
For example, to get deployments:
func handleGetDeployments(c *gin.Context) {
	if value, exists := c.Get("client"); exists {
		if memberClient, ok := value.(kubeclient.Interface); ok {
			namespace := common.ParseNamespacePathParameter(c)
			dataSelect := common.ParseDataSelectPathParameter(c)
			result, err := deployment.GetDeploymentList(memberClient, namespace, dataSelect)
			if err != nil {
				common.Fail(c, err)
				return
			}
			common.Success(c, result)
		} else {
			c.JSON(500, gin.H{"error": "Failed to assert memberClient to kubernetes.Interface"})
		}
	} else {
		c.JSON(500, gin.H{"error": "memberClient not found"})
	}
}
In this case, maybe we can avoid some copy?
+1, I think it's a good practice. One more suggestion is that, maybe we can use different client name to distinguish client for k8s and client for karmada.

You mean c.Get("client") this client name? But if the name in the context is different, the func is different also I think. (so can not reuse by member and karmada?)

warjiang · 2024-12-03T07:21:04Z

I've check the code locally, the only one problem is duplicate check of membercluster. Apart from that, one question I want to talk about is that for all kind of resources in member cluster, is it need to make another copy just like the ones we implemented for the karmada apiserver. It seems that the answer is yes, but is there a better to implemented that. Just a question, i have no idea till now.

Perhaps add a middleware for the karmada apiserver as well, and put the client initialization process in the middleware, and then pass the client using the c.Set and c.Get? This way we can use the same implementation across all resources in the member cluster and the karmada control plane. Just like:
func EnsureMemberClusterMiddleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		karmadaClient := client.InClusterKarmadaClient()
		_, err := karmadaClient.ClusterV1alpha1().Clusters().Get(context.TODO(), c.Param("clustername"), metav1.GetOptions{})
		if err != nil {
			c.AbortWithStatusJSON(http.StatusOK, common.BaseResponse{
				Code: 500,
				Msg:  err.Error(),
			})
			return
		}
		// Init client for member cluster
		memberClient := client.InClusterClientForMemberCluster(c.Param("clustername"))
		c.Set("client", memberClient)

		c.Next()
	}
}

func V1Middleware() gin.HandlerFunc {
	return func(c *gin.Context) {
		// Init client for controller cluster
		k8sClient := client.InClusterClientForKarmadaApiServer()
		c.Set("client", k8sClient)

		c.Next()
	}
}
For example, to get deployments:
func handleGetDeployments(c *gin.Context) {
	if value, exists := c.Get("client"); exists {
		if memberClient, ok := value.(kubeclient.Interface); ok {
			namespace := common.ParseNamespacePathParameter(c)
			dataSelect := common.ParseDataSelectPathParameter(c)
			result, err := deployment.GetDeploymentList(memberClient, namespace, dataSelect)
			if err != nil {
				common.Fail(c, err)
				return
			}
			common.Success(c, result)
		} else {
			c.JSON(500, gin.H{"error": "Failed to assert memberClient to kubernetes.Interface"})
		}
	} else {
		c.JSON(500, gin.H{"error": "memberClient not found"})
	}
}
In this case, maybe we can avoid some copy?
+1, I think it's a good practice. One more suggestion is that, maybe we can use different client name to distinguish client for k8s and client for karmada.
You mean c.Get("client") this client name? But if the name in the context is different, the func is different also I think. (so can not reuse by member and karmada?)

I mean for k8s client we can name the key as kuberneters-client, for karmada client we can name the key as karmada-client, for different member cluster client, we can name the key as member-client

Heylosky · 2024-12-03T10:28:16Z

I mean for k8s client we can name the key as kuberneters-client, for karmada client we can name the key as karmada-client, for different member cluster client, we can name the key as member-client

But in this case, how can we know which key should retrieve. For example, getting deployments

func handleGetDeployments(c *gin.Context) {
	c.Get( kuberneters-client?  or  karmada-client?  or member-client?  )
}

I think the route maybe like this

r := router.V1()
r.GET("/deployment", handleGetDeployments)
r := router.MemberV1()
r.GET("/deployment", handleGetDeployments)

warjiang · 2024-12-04T16:29:19Z

I mean for k8s client we can name the key as kuberneters-client, for karmada client we can name the key as karmada-client, for different member cluster client, we can name the key as member-client

But in this case, how can we know which key should retrieve. For example, getting deployments
func handleGetDeployments(c *gin.Context) {
	c.Get( kuberneters-client?  or  karmada-client?  or member-client?  )
}
I think the route maybe like this
r := router.V1()
r.GET("/deployment", handleGetDeployments)
r := router.MemberV1()
r.GET("/deployment", handleGetDeployments)

Yeah, to take these two problems into consideration at the same time is a little bit hard, we can init the clients(for kuberneters、for karmada and for member-cluster ) in the middleware functions, and use them on demand. If we cannot reuse the resource handler currently, we can just hold on these problem, just some more duplicate code, maybe we can reduce these kinds of code in the future~

warjiang · 2024-12-06T06:47:26Z

@Heylosky can you rebase your code and push again ?

Signed-off-by: Heylosky <2547226479@qq.com>

Heylosky · 2024-12-06T08:06:46Z

@Heylosky can you rebase your code and push again ?

Sorry, I just push again. I have added the middleware and removed the duplicate code for the member-cluster check. I didn't add the init clients in the middleware functions in this PR. I think it can be add in the future code optimization for resource handler together.

warjiang · 2024-12-06T12:48:50Z

/lgtm
/approve
thanks~ @Heylosky

karmada-bot · 2024-12-06T12:48:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: warjiang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [warjiang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 27, 2024

karmada-bot requested review from devadapter and warjiang November 27, 2024 07:58

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 27, 2024

karmada-bot assigned warjiang Nov 27, 2024

warjiang reviewed Nov 30, 2024

View reviewed changes

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Dec 6, 2024

ADD pod resource & routes to get member-cluster namespace/deployment/pod

e16cf8f

Signed-off-by: Heylosky <2547226479@qq.com>

Heylosky force-pushed the member-cluster-api branch from 3201af6 to e16cf8f Compare December 6, 2024 07:40

karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Dec 6, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2024

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2024

karmada-bot merged commit 3afef47 into karmada-io:main Dec 6, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD pod resource & routes to get member-cluster namespace/deployment/pod #150

ADD pod resource & routes to get member-cluster namespace/deployment/pod #150

Heylosky commented Nov 27, 2024

warjiang commented Nov 27, 2024

warjiang commented Nov 27, 2024

warjiang Nov 30, 2024

warjiang commented Nov 30, 2024

Heylosky commented Dec 2, 2024

warjiang commented Dec 3, 2024

Heylosky commented Dec 3, 2024

warjiang commented Dec 3, 2024

Heylosky commented Dec 3, 2024

warjiang commented Dec 4, 2024

warjiang commented Dec 6, 2024

Heylosky commented Dec 6, 2024

warjiang commented Dec 6, 2024

karmada-bot commented Dec 6, 2024

ADD pod resource & routes to get member-cluster namespace/deployment/pod #150

ADD pod resource & routes to get member-cluster namespace/deployment/pod #150

Conversation

Heylosky commented Nov 27, 2024

warjiang commented Nov 27, 2024

warjiang commented Nov 27, 2024

warjiang Nov 30, 2024

Choose a reason for hiding this comment

warjiang commented Nov 30, 2024

Heylosky commented Dec 2, 2024

warjiang commented Dec 3, 2024

Heylosky commented Dec 3, 2024

warjiang commented Dec 3, 2024

Heylosky commented Dec 3, 2024

warjiang commented Dec 4, 2024

warjiang commented Dec 6, 2024

Heylosky commented Dec 6, 2024

warjiang commented Dec 6, 2024

karmada-bot commented Dec 6, 2024