fix review comments

Signed-off-by: Periyasamy Palanisamy <[email protected]>
k8snetworkplumbingwg · Jan 12, 2021 · a03c581 · a03c581
1 parent d326bce
commit a03c581
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 32 deletions.
diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go
@@ -39,7 +39,7 @@ func main() {
 	injectHugepageDownApi := flag.Bool("injectHugepageDownApi", false, "Enable hugepage requests and limits into Downward API.")
 	flag.Var(&clientCAPaths, "client-ca", "File containing client CA. This flag is repeatable if more than one client CA needs to be added to server")
 	resourceNameKeys := flag.String("network-resource-name-keys", "k8s.v1.cni.cncf.io/resourceName", "comma separated resource name keys --network-resource-name-keys.")
-	resourceHonorFlag := flag.Bool("honor-resource", false, "Honor the existing resources --honor-resource")
+	resourcesHonorFlag := flag.Bool("honor-resources", false, "Honor the existing requested resources requests & limits --honor-resources")
 	flag.Parse()
 
 	if *port < 1024 || *port > 65535 {
@@ -71,7 +71,7 @@ func main() {
 
 	webhook.SetInjectHugepageDownApi(*injectHugepageDownApi)
 
-	webhook.SetHonorExistingResources(*resourceHonorFlag)
+	webhook.SetHonorExistingResources(*resourcesHonorFlag)
 
 	err = webhook.SetResourceNameKeys(*resourceNameKeys)
 	if err != nil {

diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go
@@ -556,23 +556,12 @@ func createResourcePatch(patch []jsonPatchOperation, Containers []corev1.Contain
 		patch = patchEmptyResources(patch, 0, "limits")
 	}
 
-	resourceList := corev1.ResourceList{}
-	for name, number := range resourceRequests {
-		resourceList[corev1.ResourceName(name)] = *resource.NewQuantity(number, resource.DecimalSI)
-	}
+	resourceList := *getResourceList(resourceRequests)
 
 	for resource, quantity := range resourceList {
-		patch = append(patch, jsonPatchOperation{
-			Operation: "add",
-			Path:      "/spec/containers/0/resources/requests/" + toSafeJsonPatchKey(resource.String()), // NOTE: in future we may want to patch specific container (not always the first one)
-			Value:     quantity,
-		})
-		patch = append(patch, jsonPatchOperation{
-			Operation: "add",
-			Path:      "/spec/containers/0/resources/limits/" + toSafeJsonPatchKey(resource.String()),
-			Value:     quantity,
-		})
+		patch = appendResource(patch, resource.String(), quantity, quantity)
 	}
+
 	return patch
 }
 
@@ -591,35 +580,47 @@ func updateResourcePatch(patch []jsonPatchOperation, Containers []corev1.Contain
 		existingLimitsMap = Containers[0].Resources.Limits
 	}
 
-	resourceList := corev1.ResourceList{}
-	for name, number := range resourceRequests {
-		resourceList[corev1.ResourceName(name)] = *resource.NewQuantity(number, resource.DecimalSI)
-	}
+	resourceList := *getResourceList(resourceRequests)
 
 	for resourceName, quantity := range resourceList {
 		reqQuantity := quantity
 		limitQuantity := quantity
 		if value, ok := existingrequestsMap[resourceName]; ok {
 			reqQuantity.Add(value)
 		}
-		patch = append(patch, jsonPatchOperation{
-			Operation: "add",
-			Path:      "/spec/containers/0/resources/requests/" + toSafeJsonPatchKey(resourceName.String()),
-			Value:     reqQuantity,
-		})
 		if value, ok := existingLimitsMap[resourceName]; ok {
 			limitQuantity.Add(value)
 		}
-		patch = append(patch, jsonPatchOperation{
-			Operation: "add",
-			Path:      "/spec/containers/0/resources/limits/" + toSafeJsonPatchKey(resourceName.String()),
-			Value:     limitQuantity,
-		})
+		patch = appendResource(patch, resourceName.String(), reqQuantity, limitQuantity)
 	}
 
 	return patch
 }
 
+func appendResource(patch []jsonPatchOperation, resourceName string, reqQuantity, limitQuantity resource.Quantity) []jsonPatchOperation {
+	patch = append(patch, jsonPatchOperation{
+		Operation: "add",
+		Path:      "/spec/containers/0/resources/requests/" + toSafeJsonPatchKey(resourceName),
+		Value:     reqQuantity,
+	})
+	patch = append(patch, jsonPatchOperation{
+		Operation: "add",
+		Path:      "/spec/containers/0/resources/limits/" + toSafeJsonPatchKey(resourceName),
+		Value:     limitQuantity,
+	})
+
+	return patch
+}
+
+func getResourceList(resourceRequests map[string]int64) *corev1.ResourceList {
+	resourceList := corev1.ResourceList{}
+	for name, number := range resourceRequests {
+		resourceList[corev1.ResourceName(name)] = *resource.NewQuantity(number, resource.DecimalSI)
+	}
+
+	return &resourceList
+}
+
 // MutateHandler handles AdmissionReview requests and sends responses back to the K8s API server
 func MutateHandler(w http.ResponseWriter, req *http.Request) {
 	glog.Infof("Received mutation request")
@@ -704,6 +705,7 @@ func MutateHandler(w http.ResponseWriter, req *http.Request) {
 			glog.Infof("pod doesn't need any custom network resources")
 		} else {
 			var patch []jsonPatchOperation
+			glog.Infof("honor-resources=%v", honorExistingResources)
 			if honorExistingResources {
 				patch = updateResourcePatch(patch, pod.Spec.Containers, resourceRequests)
 			} else {
@@ -827,6 +829,6 @@ func SetInjectHugepageDownApi(hugepageFlag bool) {
 }
 
 // SetHonorExistingResources initialize the honorExistingResources flag
-func SetHonorExistingResources(resourceHonorFlag bool) {
-	honorExistingResources = resourceHonorFlag
+func SetHonorExistingResources(resourcesHonorFlag bool) {
+	honorExistingResources = resourcesHonorFlag
 }