[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #345: [YUNIKORN-928] refactor admission controller code

[GitBox <gi...@apache.org>]

wilfred-s commented on a change in pull request #345:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/345#discussion_r771091316



##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -69,9 +70,23 @@ type ValidateConfResponse struct {
 	Reason  string `json:"reason"`
 }
 
-func (c *admissionController) mutate(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
-	req := ar.Request
-	namespace := ar.Request.Namespace
+func admissionResponseBuilder(uid string, allowed bool, resultMessage string, patch []byte) *v1beta1.AdmissionResponse {
+	res := &v1beta1.AdmissionResponse{
+		Allowed: allowed,
+		UID:     types.UID(uid),
+		Result: &metav1.Status{
+			Message: resultMessage,
+		},

Review comment:
       If the result message is empty, `len(message) == 0` we should not add it to the response. We should keep the response as clean as possible.

##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -69,9 +70,23 @@ type ValidateConfResponse struct {
 	Reason  string `json:"reason"`
 }
 
-func (c *admissionController) mutate(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
-	req := ar.Request
-	namespace := ar.Request.Namespace
+func admissionResponseBuilder(uid string, allowed bool, resultMessage string, patch []byte) *v1beta1.AdmissionResponse {
+	res := &v1beta1.AdmissionResponse{
+		Allowed: allowed,
+		UID:     types.UID(uid),
+		Result: &metav1.Status{
+			Message: resultMessage,
+		},
+		Patch: patch,
+		PatchType: func() *v1beta1.PatchType {
+			pt := v1beta1.PatchTypeJSONPatch
+			return &pt
+		}(),

Review comment:
       If the patch is empty, i.e. `len(patch) == 0` we should not add the patch and type to the response.
   Using a function to generate the variable is a lot of overhead. It will not change using a var defined at the file level is easier.

##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -81,9 +96,7 @@ func (c *admissionController) mutate(ar *v1beta1.AdmissionReview) *v1beta1.Admis
 		log.Logger().Warn("request kind is not pod", zap.String("uid", uid),
 			zap.String("requestKind", requestKind))
 
-		return &v1beta1.AdmissionResponse{
-			Allowed: true,
-		}
+		return admissionResponseBuilder(uid, true, "", []byte(""))

Review comment:
       The `[]byte("")` should be replaced with a nil. That lines up with the comment earlier about not adding the patch details if not set.
   This should be applied to all calls that pass in the empty array.

##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -311,20 +286,18 @@ func (c *admissionController) serve(w http.ResponseWriter, r *http.Request) {
 
 	var admissionResponse *v1beta1.AdmissionResponse
 	ar := v1beta1.AdmissionReview{}
+	req := ar.Request
 	urlPath := r.URL.Path
 
 	if _, _, err := deserializer.Decode(body, nil, &ar); err != nil {
 		log.Logger().Error("Can't decode the body", zap.Error(err))
-		admissionResponse = &v1beta1.AdmissionResponse{
-			Allowed: false,
-			Result: &metav1.Status{
-				Message: err.Error(),
-			},
+		admissionResponse = admissionResponseBuilder(string(req.UID), false, err.Error(), []byte(""))
+	} else if req != nil {
+		if urlPath == mutateURL {
+			admissionResponse = c.mutate(req)
+		} else if urlPath == validateConfURL {
+			admissionResponse = c.validateConf(req)

Review comment:
       if the deserialiser fails we must not assume that `ar` or `req` are valid and can be used. We should provide a fixed ID as part of the response.
   

##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -213,51 +207,32 @@ func isConfigMapUpdateAllowed(userInfo string) bool {
 	return false
 }
 
-func (c *admissionController) validateConf(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
-	req := ar.Request
+func (c *admissionController) validateConf(req *v1beta1.AdmissionRequest) *v1beta1.AdmissionResponse {
+	uid := string(req.UID)
 	if !isConfigMapUpdateAllowed(req.UserInfo.Username) {
-		return &v1beta1.AdmissionResponse{
-			Allowed: false,
-			Result: &metav1.Status{
-				Message: fmt.Sprintf("ConfigHotRefresh is disabled. " +
-					"Please use the REST API to update the configuration, or enable configHotRefresh"),
-			},
-		}
+		return admissionResponseBuilder(uid, false, fmt.Sprintf("ConfigHotRefresh is disabled. "+
+			"Please use the REST API to update the configuration, or enable configHotRefresh"), []byte(""))

Review comment:
       Not introduced here but can we replace this with a constant in the file? The text will never change and generating a new string is just overhead. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org