Fix wrong condition check for Memory limit (googleforgames#1418)

* Fix wrong condition check for Memory limit
* Add a function to validate Resource config

Add test function for Agones Controller validate function.
Function ValidateResource() would be reused for Fleet, GSS and GS PodTemplate CPU and Memory Resources validation.

cmd/controller/main.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"agones.dev/agones/pkg"
+	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	"agones.dev/agones/pkg/client/clientset/versioned"
 	"agones.dev/agones/pkg/client/informers/externalversions"
 	"agones.dev/agones/pkg/fleetautoscalers"
@@ -44,6 +45,7 @@ import (
 	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
 	"gopkg.in/natefinch/lumberjack.v2"
+	corev1 "k8s.io/api/core/v1"
 	extclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/client-go/informers"
@@ -119,8 +121,11 @@ func main() {
 	logger.WithField("version", pkg.Version).WithField("featureGates", runtime.EncodeFeatures()).
 		WithField("ctlConf", ctlConf).Info("starting gameServer operator...")
 
-	if err := ctlConf.validate(); err != nil {
-		logger.WithError(err).Fatal("Could not create controller from environment or flags")
+	if errs := ctlConf.validate(); len(errs) != 0 {
+		for _, err := range errs {
+			logger.WithError(err)
+		}
+		logger.Fatal("Could not create controller from environment or flags")
 	}
 
 	// if the kubeconfig fails BuildConfigFromFlags will try in cluster config
@@ -382,14 +387,19 @@ type config struct {
 }
 
 // validate ensures the ctlConfig data is valid.
-func (c *config) validate() error {
+func (c *config) validate() []error {
+	validationErrors := make([]error, 0)
 	if c.MinPort <= 0 || c.MaxPort <= 0 {
-		return errors.New("min Port and Max Port values are required")
+		validationErrors = append(validationErrors, errors.New("min Port and Max Port values are required"))
 	}
 	if c.MaxPort < c.MinPort {
-		return errors.New("max Port cannot be set less that the Min Port")
+		validationErrors = append(validationErrors, errors.New("max Port cannot be set less that the Min Port"))
 	}
-	return nil
+	resourceErrors := agonesv1.ValidateResource(c.SidecarCPURequest, c.SidecarCPULimit, corev1.ResourceCPU)
+	validationErrors = append(validationErrors, resourceErrors...)
+	resourceErrors = agonesv1.ValidateResource(c.SidecarMemoryRequest, c.SidecarMemoryLimit, corev1.ResourceMemory)
+	validationErrors = append(validationErrors, resourceErrors...)
+	return validationErrors
 }
 
 type runner interface {

cmd/controller/main_test.go

@@ -0,0 +1,62 @@
+// Copyright 2020 Google LLC All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+func TestControllerConfigValidation(t *testing.T) {
+	t.Parallel()
+
+	c := config{MinPort: 10, MaxPort: 2}
+	errs := c.validate()
+	assert.Len(t, errs, 1)
+	errorsContainString(t, errs, "max Port cannot be set less that the Min Port")
+
+	c.SidecarMemoryRequest = resource.MustParse("2Gi")
+	c.SidecarMemoryLimit = resource.MustParse("1Gi")
+	errs = c.validate()
+	assert.Len(t, errs, 2)
+	errorsContainString(t, errs, "Request must be less than or equal to memory limit")
+
+	c.SidecarMemoryLimit = resource.MustParse("2Gi")
+	c.SidecarCPURequest = resource.MustParse("2m")
+	c.SidecarCPULimit = resource.MustParse("1m")
+	errs = c.validate()
+	assert.Len(t, errs, 2)
+	errorsContainString(t, errs, "Request must be less than or equal to cpu limit")
+
+	c.SidecarMemoryLimit = resource.MustParse("2Gi")
+	c.SidecarCPURequest = resource.MustParse("-2m")
+	c.SidecarCPULimit = resource.MustParse("2m")
+	errs = c.validate()
+	assert.Len(t, errs, 2)
+	errorsContainString(t, errs, "Resource cpu request value must be non negative")
+}
+
+func errorsContainString(t *testing.T, errs []error, expected string) {
+	found := false
+	for _, v := range errs {
+		if expected == v.Error() {
+			found = true
+			break
+		}
+	}
+	assert.True(t, found, "Was not able to find '%s'", expected)
+}

pkg/apis/agones/v1/gameserver.go

@@ -27,6 +27,7 @@ import (
 	"github.com/pkg/errors"
 	admregv1b "k8s.io/api/admissionregistration/v1beta1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 )
@@ -456,6 +457,24 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo
 	return causes, len(causes) == 0
 }
 
+// ValidateResource validates limit or Memory CPU resources used for containers in pods
+// If a GameServer is invalid there will be > 0 values in
+// the returned array
+func ValidateResource(request resource.Quantity, limit resource.Quantity, resourceName corev1.ResourceName) []error {
+	validationErrors := make([]error, 0)
+	if !limit.IsZero() && request.Cmp(limit) > 0 {
+		validationErrors = append(validationErrors, errors.New(fmt.Sprintf("Request must be less than or equal to %s limit", resourceName)))
+	}
+	if request.Cmp(resource.Quantity{}) < 0 {
+		validationErrors = append(validationErrors, errors.New(fmt.Sprintf("Resource %s request value must be non negative", resourceName)))
+	}
+	if limit.Cmp(resource.Quantity{}) < 0 {
+		validationErrors = append(validationErrors, errors.New(fmt.Sprintf("Resource %s limit value must be non negative", resourceName)))
+	}
+
+	return validationErrors
+}
+
 // Validate validates the GameServer configuration.
 // If a GameServer is invalid there will be > 0 values in
 // the returned array

pkg/gameservers/controller.go

@@ -652,7 +652,7 @@ func (c *Controller) sidecar(gs *agonesv1.GameServer) corev1.Container {
 	if !c.sidecarCPULimit.IsZero() {
 		limits[corev1.ResourceCPU] = c.sidecarCPULimit
 	}
-	if !c.sidecarCPULimit.IsZero() {
+	if !c.sidecarMemoryLimit.IsZero() {
 		limits[corev1.ResourceMemory] = c.sidecarMemoryLimit
 	}
 	sidecar.Resources.Limits = limits