You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/03/17 01:30:29 UTC

[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #389: [YUNIKORN-1118] Make admission controller config validation soft-succeed if scheduler is unreachable

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



##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -363,29 +364,44 @@ func (c *admissionController) shouldLabelNamespace(namespace string) bool {
 
 func (c *admissionController) validateConfigMap(cm *v1.ConfigMap) error {
 	if cm.Name == constants.DefaultConfigMapName {
-		log.Logger().Info("validating yunikorn configs")
 		if content, ok := cm.Data[c.configName]; ok {
+			checksum := fmt.Sprintf("%X", sha256.Sum256([]byte(content)))
+			log.Logger().Info("Validating YuniKorn configuration", zap.String("checksum", checksum))
+			log.Logger().Debug("Configmap data", zap.ByteString("content", []byte(content)))
 			response, err := http.Post(c.schedulerValidateConfURL, "application/json", bytes.NewBuffer([]byte(content)))
 			if err != nil {
-				return err
+				log.Logger().Error("YuniKorn scheduler is unreachable, assuming configmap is valid", zap.Error(err))
+				return nil
 			}
 			defer response.Body.Close()
+			if response.StatusCode < 200 || response.StatusCode > 299 {
+				log.Logger().Error("YuniKorn scheduler responded with unexpected status, assuming configmap is valid",
+					zap.Int("status", response.StatusCode))
+				return nil
+			}
 			responseBytes, err := io.ReadAll(response.Body)
 			if err != nil {
-				return err
+				log.Logger().Error("Unable to read response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+				return nil
 			}
 			var responseData ValidateConfResponse
 			if err := json.Unmarshal(responseBytes, &responseData); err != nil {

Review comment:
       NIT: don't shadow err

##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -363,29 +364,44 @@ func (c *admissionController) shouldLabelNamespace(namespace string) bool {
 
 func (c *admissionController) validateConfigMap(cm *v1.ConfigMap) error {
 	if cm.Name == constants.DefaultConfigMapName {
-		log.Logger().Info("validating yunikorn configs")
 		if content, ok := cm.Data[c.configName]; ok {
+			checksum := fmt.Sprintf("%X", sha256.Sum256([]byte(content)))
+			log.Logger().Info("Validating YuniKorn configuration", zap.String("checksum", checksum))
+			log.Logger().Debug("Configmap data", zap.ByteString("content", []byte(content)))
 			response, err := http.Post(c.schedulerValidateConfURL, "application/json", bytes.NewBuffer([]byte(content)))
 			if err != nil {
-				return err
+				log.Logger().Error("YuniKorn scheduler is unreachable, assuming configmap is valid", zap.Error(err))
+				return nil
 			}
 			defer response.Body.Close()
+			if response.StatusCode < 200 || response.StatusCode > 299 {
+				log.Logger().Error("YuniKorn scheduler responded with unexpected status, assuming configmap is valid",
+					zap.Int("status", response.StatusCode))
+				return nil
+			}
 			responseBytes, err := io.ReadAll(response.Body)
 			if err != nil {
-				return err
+				log.Logger().Error("Unable to read response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+				return nil
 			}
 			var responseData ValidateConfResponse
 			if err := json.Unmarshal(responseBytes, &responseData); err != nil {
-				return err
+				log.Logger().Error("Unable to parse response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+				return nil
 			}
 			if !responseData.Allowed {
-				return fmt.Errorf(responseData.Reason)
+				err = fmt.Errorf(responseData.Reason)
+				log.Logger().Error("Configmap validation failed, aborting", zap.Error(err))
+				return err
 			}
 		} else {
-			return fmt.Errorf("required config '%s' not found in this configmap", c.configName)
+			err := fmt.Errorf("required config '%s' not found in this configmap", c.configName)

Review comment:
       NIT: don't shadow err




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