Move GameServer validation to a ValidatingAdmissionWebhook

README.md

@@ -18,8 +18,9 @@ This software is currently alpha, and subject to change. Not to be used in produ
 - Kubernetes cluster version 1.9+
     - [Minikube](https://github.com/kubernetes/minikube) and [Google Kubernetes Engine](https://cloud.google.com/kubernetes-engine/) have been tested
     - If you are creating and managing your own Kubernetes cluster, the
-    [MutatingAdmissionWebhook](https://kubernetes.io/docs/admin/admission-controllers/#mutatingadmissionwebhook-beta-in-19)
-    admission controller is required.
+    [MutatingAdmissionWebhook](https://kubernetes.io/docs/admin/admission-controllers/#mutatingadmissionwebhook-beta-in-19), and 
+    [ValidatingAdmissionWebhook](https://kubernetes.io/docs/admin/admission-controllers/#validatingadmissionwebhook-alpha-in-18-beta-in-19)
+    admission controllers are required.
     We also recommend following the
     [recommended set of admission controllers](https://kubernetes.io/docs/admin/admission-controllers/#is-there-a-recommended-set-of-admission-controllers-to-use).
 - Firewall access for the range of ports that Game Servers can be connected to in the cluster.

build/install.yaml

@@ -196,6 +196,30 @@ webhooks:
         operations:
           - CREATE
 ---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: agones-validation-webhook
+  namespace: agones-system
+webhooks:
+  - name: validations.stable.agones.dev
+    failurePolicy: Fail
+    clientConfig:
+      service:
+        name: agones-controller-service
+        namespace: agones-system
+        path: /validate
+      caBundle: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUVLVENDQXhHZ0F3SUJBZ0lKQU9KUDY0MTB3dkdTTUEwR0NTcUdTSWIzRFFFQkN3VUFNSUdxTVFzd0NRWUQKVlFRR0V3SlZVekVUTUJFR0ExVUVDQXdLVTI5dFpTMVRkR0YwWlRFUE1BMEdBMVVFQ2d3R1FXZHZibVZ6TVE4dwpEUVlEVlFRTERBWkJaMjl1WlhNeE5EQXlCZ05WQkFNTUsyRm5iMjVsY3kxamIyNTBjbTlzYkdWeUxYTmxjblpwClkyVXVZV2R2Ym1WekxYTjVjM1JsYlM1emRtTXhMakFzQmdrcWhraUc5dzBCQ1FFV0gyRm5iMjVsY3kxa2FYTmoKZFhOelFHZHZiMmRzWldkeWIzVndjeTVqYjIwd0hoY05NVGd3TWpFME1EUTBORFEyV2hjTk1qZ3dNakV5TURRMApORFEyV2pDQnFqRUxNQWtHQTFVRUJoTUNWVk14RXpBUkJnTlZCQWdNQ2xOdmJXVXRVM1JoZEdVeER6QU5CZ05WCkJBb01Ca0ZuYjI1bGN6RVBNQTBHQTFVRUN3d0dRV2R2Ym1Wek1UUXdNZ1lEVlFRRERDdGhaMjl1WlhNdFkyOXUKZEhKdmJHeGxjaTF6WlhKMmFXTmxMbUZuYjI1bGN5MXplWE4wWlcwdWMzWmpNUzR3TEFZSktvWklodmNOQVFrQgpGaDloWjI5dVpYTXRaR2x6WTNWemMwQm5iMjluYkdWbmNtOTFjSE11WTI5dE1JSUJJakFOQmdrcWhraUc5dzBCCkFRRUZBQU9DQVE4QU1JSUJDZ0tDQVFFQXpnVlQ5MGVqeE5ud0NvL09qTUQyNmZVNGRya1NlZndkUWd3aWJpZmEKbDhyazZZMFZ2T0lWMUgrbFJvd2UwNm1XTnVSNUZPWEZBMGZYbHZ4Q0tLWVZRcFNQRUsyWVN5aC9hU25KUUw2cQpvOGVBWVRKQmtPWUxCNUNiekl6aVdlb1FmT1lOOE1sRW44YlhKZGllSmhISDhVbnlqdHlvVGx4emhabVgrcGZ0CmhVZGVhM1Zrek8yMW40K1FFM1JYNWYxMzJGVEZjdXFYT1VBL3BpOGNjQU5HYzN6akxlWkp2QTlvZFBFaEdmN2cKQzhleUE2OFNWY3NoK1BqejBsdzk1QVB2bE12MWptcVVSRldjRVNUTGFRMEZ4NUt3UnlWMHppWm1VdkFBRjJaeApEWmhIVWNvRlBIQXdUbDc1TkFobkhwTWxMTnA1TDd0Y1ZkeVQ4QjJHUnMrc2xRSURBUUFCbzFBd1RqQWRCZ05WCkhRNEVGZ1FVZ3YxblRQYVFKU04zTHFtNWpJalc0eEhtZEcwd0h3WURWUjBqQkJnd0ZvQVVndjFuVFBhUUpTTjMKTHFtNWpJalc0eEhtZEcwd0RBWURWUjBUQkFVd0F3RUIvekFOQmdrcWhraUc5dzBCQVFzRkFBT0NBUUVBSEtFQwprdEVqWU5VQ0ErbXlzejRvclc3cFJVdmhCSERWU2dzWTZlRVZSTHpmLzF5SVpFMHU2NTZrcEs2T1Q3TWhKR2xVCkt3R1NTb1VCQnpWZ1VzWmpEbTdQZ2JrNGlZem40TTF4THpiTFFCcjNNYzV6WEhlZlB2YmltaEQ1NWNMenBWRnUKVlFtQm1aVjJOalU1RHVTZFJuZGxjUGFOY2cvdU9jdlpLNEtZMUtDQkEzRW9BUUlrcHpIWDJpVU1veGlSdlpWTgpORXdnRlR0SUdCWW4wSGZML3ZnT3NIOGZWck1Va3VHMnZoR2RlWEJwWmlxL0JaSmJaZU4yckNmMmdhWDFRSXYwCkVLYmN1RnFNOThXVDVaVlpSdFgxWTNSd2V2ZzRteFlKWEN1SDZGRjlXOS9TejI5NEZ5Mk9CS0I4SkFWYUV4OW4KMS9pNmZJZmZHbkhUWFdIc1ZRPT0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=
+    rules:
+      - apiGroups:
+          - stable.agones.dev
+        resources:
+          - "gameservers"
+        apiVersions:
+          - "v1alpha1"
+        operations:
+          - CREATE
+---
 # Service account, secret, role and rolebinding for sidecar (agones-sdk) pod
 apiVersion: v1
 kind: ServiceAccount

install.yaml

@@ -195,6 +195,30 @@ webhooks:
         operations:
           - CREATE
 ---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: agones-validation-webhook
+  namespace: agones-system
+webhooks:
+  - name: validations.stable.agones.dev
+    failurePolicy: Fail
+    clientConfig:
+      service:
+        name: agones-controller-service
+        namespace: agones-system
+        path: /validate
+      caBundle: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUVLVENDQXhHZ0F3SUJBZ0lKQU9KUDY0MTB3dkdTTUEwR0NTcUdTSWIzRFFFQkN3VUFNSUdxTVFzd0NRWUQKVlFRR0V3SlZVekVUTUJFR0ExVUVDQXdLVTI5dFpTMVRkR0YwWlRFUE1BMEdBMVVFQ2d3R1FXZHZibVZ6TVE4dwpEUVlEVlFRTERBWkJaMjl1WlhNeE5EQXlCZ05WQkFNTUsyRm5iMjVsY3kxamIyNTBjbTlzYkdWeUxYTmxjblpwClkyVXVZV2R2Ym1WekxYTjVjM1JsYlM1emRtTXhMakFzQmdrcWhraUc5dzBCQ1FFV0gyRm5iMjVsY3kxa2FYTmoKZFhOelFHZHZiMmRzWldkeWIzVndjeTVqYjIwd0hoY05NVGd3TWpFME1EUTBORFEyV2hjTk1qZ3dNakV5TURRMApORFEyV2pDQnFqRUxNQWtHQTFVRUJoTUNWVk14RXpBUkJnTlZCQWdNQ2xOdmJXVXRVM1JoZEdVeER6QU5CZ05WCkJBb01Ca0ZuYjI1bGN6RVBNQTBHQTFVRUN3d0dRV2R2Ym1Wek1UUXdNZ1lEVlFRRERDdGhaMjl1WlhNdFkyOXUKZEhKdmJHeGxjaTF6WlhKMmFXTmxMbUZuYjI1bGN5MXplWE4wWlcwdWMzWmpNUzR3TEFZSktvWklodmNOQVFrQgpGaDloWjI5dVpYTXRaR2x6WTNWemMwQm5iMjluYkdWbmNtOTFjSE11WTI5dE1JSUJJakFOQmdrcWhraUc5dzBCCkFRRUZBQU9DQVE4QU1JSUJDZ0tDQVFFQXpnVlQ5MGVqeE5ud0NvL09qTUQyNmZVNGRya1NlZndkUWd3aWJpZmEKbDhyazZZMFZ2T0lWMUgrbFJvd2UwNm1XTnVSNUZPWEZBMGZYbHZ4Q0tLWVZRcFNQRUsyWVN5aC9hU25KUUw2cQpvOGVBWVRKQmtPWUxCNUNiekl6aVdlb1FmT1lOOE1sRW44YlhKZGllSmhISDhVbnlqdHlvVGx4emhabVgrcGZ0CmhVZGVhM1Zrek8yMW40K1FFM1JYNWYxMzJGVEZjdXFYT1VBL3BpOGNjQU5HYzN6akxlWkp2QTlvZFBFaEdmN2cKQzhleUE2OFNWY3NoK1BqejBsdzk1QVB2bE12MWptcVVSRldjRVNUTGFRMEZ4NUt3UnlWMHppWm1VdkFBRjJaeApEWmhIVWNvRlBIQXdUbDc1TkFobkhwTWxMTnA1TDd0Y1ZkeVQ4QjJHUnMrc2xRSURBUUFCbzFBd1RqQWRCZ05WCkhRNEVGZ1FVZ3YxblRQYVFKU04zTHFtNWpJalc0eEhtZEcwd0h3WURWUjBqQkJnd0ZvQVVndjFuVFBhUUpTTjMKTHFtNWpJalc0eEhtZEcwd0RBWURWUjBUQkFVd0F3RUIvekFOQmdrcWhraUc5dzBCQVFzRkFBT0NBUUVBSEtFQwprdEVqWU5VQ0ErbXlzejRvclc3cFJVdmhCSERWU2dzWTZlRVZSTHpmLzF5SVpFMHU2NTZrcEs2T1Q3TWhKR2xVCkt3R1NTb1VCQnpWZ1VzWmpEbTdQZ2JrNGlZem40TTF4THpiTFFCcjNNYzV6WEhlZlB2YmltaEQ1NWNMenBWRnUKVlFtQm1aVjJOalU1RHVTZFJuZGxjUGFOY2cvdU9jdlpLNEtZMUtDQkEzRW9BUUlrcHpIWDJpVU1veGlSdlpWTgpORXdnRlR0SUdCWW4wSGZML3ZnT3NIOGZWck1Va3VHMnZoR2RlWEJwWmlxL0JaSmJaZU4yckNmMmdhWDFRSXYwCkVLYmN1RnFNOThXVDVaVlpSdFgxWTNSd2V2ZzRteFlKWEN1SDZGRjlXOS9TejI5NEZ5Mk9CS0I4SkFWYUV4OW4KMS9pNmZJZmZHbkhUWFdIc1ZRPT0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=
+    rules:
+      - apiGroups:
+          - stable.agones.dev
+        resources:
+          - "gameservers"
+        apiVersions:
+          - "v1alpha1"
+        operations:
+          - CREATE
+---
 # Service account, secret, role and rolebinding for sidecar (agones-sdk) pod
 apiVersion: v1
 kind: ServiceAccount

pkg/gameservers/controller.go

@@ -110,7 +110,8 @@ func NewController(
 
 	c.workerqueue = workerqueue.NewWorkerQueue(c.syncGameServer, c.logger, stable.GroupName+".GameServerController")
 
-	wh.AddHandler("/mutate", stablev1alpha1.Kind("GameServer"), admv1beta1.Create, c.creationHandler)
+	wh.AddHandler("/mutate", stablev1alpha1.Kind("GameServer"), admv1beta1.Create, c.creationMutationHandler)
+	wh.AddHandler("/validate", stablev1alpha1.Kind("GameServer"), admv1beta1.Create, c.creationValidationHandler)
 
 	gsInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: c.workerqueue.Enqueue,
@@ -140,11 +141,11 @@ func NewController(
 	return c
 }
 
-// creationHandler is the handler for the mutating webhook that sets the
-// the default values on the GameServer, and validates the results
+// creationMutationHandler is the handler for the mutating webhook that sets the
+// the default values on the GameServer
 // Should only be called on gameserver create operations.
-func (c *Controller) creationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) {
-	c.logger.WithField("review", review).Info("creationHandler")
+func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) {
+	c.logger.WithField("review", review).Info("creationMutationHandler")
 
 	obj := review.Request.Object
 	gs := &stablev1alpha1.GameServer{}
@@ -156,25 +157,6 @@ func (c *Controller) creationHandler(review admv1beta1.AdmissionReview) (admv1be
 	// This is the main logic of this function
 	// the rest is really just json plumbing
 	gs.ApplyDefaults()
-	ok, causes := gs.Validate()
-	if !ok {
-		review.Response.Allowed = false
-		details := metav1.StatusDetails{
-			Name:   review.Request.Name,
-			Group:  review.Request.Kind.Group,
-			Kind:   review.Request.Kind.Kind,
-			Causes: causes,
-		}
-		review.Response.Result = &metav1.Status{
-			Status:  metav1.StatusFailure,
-			Message: "GameServer configuration is invalid: " + details.String(),
-			Reason:  metav1.StatusReasonInvalid,
-			Details: &details,
-		}
-
-		c.logger.WithField("review", review).Info("Invalid GameServer")
-		return review, nil
-	}
 
 	newGS, err := json.Marshal(gs)
 	if err != nil {
@@ -200,6 +182,41 @@ func (c *Controller) creationHandler(review admv1beta1.AdmissionReview) (admv1be
 	return review, nil
 }
 
+// creationValidationHandler that validates a GameServer when it is created
+// Should only be called on gameserver create operations.
+func (c *Controller) creationValidationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) {
+	c.logger.WithField("review", review).Info("creationValidationHandler")
+
+	obj := review.Request.Object
+	gs := &stablev1alpha1.GameServer{}
+	err := json.Unmarshal(obj.Raw, gs)
+	if err != nil {
+		return review, errors.Wrapf(err, "error unmarshalling original GameServer json: %s", obj.Raw)
+	}
+
+	ok, causes := gs.Validate()
+	if !ok {
+		review.Response.Allowed = false
+		details := metav1.StatusDetails{
+			Name:   review.Request.Name,
+			Group:  review.Request.Kind.Group,
+			Kind:   review.Request.Kind.Kind,
+			Causes: causes,
+		}
+		review.Response.Result = &metav1.Status{
+			Status:  metav1.StatusFailure,
+			Message: "GameServer configuration is invalid",
+			Reason:  metav1.StatusReasonInvalid,
+			Details: &details,
+		}
+
+		c.logger.WithField("review", review).Info("Invalid GameServer")
+		return review, nil
+	}
+
+	return review, nil
+}
+
 // Run the GameServer controller. Will block until stop is closed.
 // Runs threadiness number workers to process the rate limited queue
 func (c *Controller) Run(threadiness int, stop <-chan struct{}) error {

pkg/gameservers/controller_test.go

@@ -211,15 +211,62 @@ func TestControllerHealthCheck(t *testing.T) {
 	testHTTPHealth(t, "http://localhost:9090/live", "{}\n", http.StatusOK)
 }
 
-func TestControllerCreationHandler(t *testing.T) {
+func TestControllerCreationMutationHandler(t *testing.T) {
 	t.Parallel()
 
 	c, _ := newFakeController()
 	gvk := metav1.GroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("GameServer"))
 
-	t.Run("gameserver defaults", func(t *testing.T) {
+	fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"},
+		Spec: newSingleContainerSpec()}
+
+	raw, err := json.Marshal(fixture)
+	assert.Nil(t, err)
+	review := admv1beta1.AdmissionReview{
+		Request: &admv1beta1.AdmissionRequest{
+			Kind:      gvk,
+			Operation: admv1beta1.Create,
+			Object: runtime.RawExtension{
+				Raw: raw,
+			},
+		},
+		Response: &admv1beta1.AdmissionResponse{Allowed: true},
+	}
+
+	result, err := c.creationMutationHandler(review)
+	assert.Nil(t, err)
+	assert.True(t, result.Response.Allowed)
+	assert.Equal(t, admv1beta1.PatchTypeJSONPatch, *result.Response.PatchType)
+
+	patch := &jsonpatch.ByPath{}
+	err = json.Unmarshal(result.Response.Patch, patch)
+	assert.Nil(t, err)
+
+	assertContains := func(patch *jsonpatch.ByPath, op jsonpatch.JsonPatchOperation) {
+		found := false
+		for _, p := range *patch {
+			if assert.ObjectsAreEqualValues(p, op) {
+				found = true
+			}
+		}
+
+		assert.True(t, found, "Could not find operation %#v in patch %v", op, *patch)
+	}
+
+	assertContains(patch, jsonpatch.JsonPatchOperation{Operation: "add", Path: "/metadata/finalizers", Value: []interface{}{"stable.agones.dev"}})
+	assertContains(patch, jsonpatch.JsonPatchOperation{Operation: "add", Path: "/spec/protocol", Value: "UDP"})
+}
+
+func TestControllerCreationValidationHandler(t *testing.T) {
+	t.Parallel()
+
+	c, _ := newFakeController()
+	gvk := metav1.GroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("GameServer"))
+
+	t.Run("valid gameserver", func(t *testing.T) {
 		fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"},
 			Spec: newSingleContainerSpec()}
+		fixture.ApplyDefaults()
 
 		raw, err := json.Marshal(fixture)
 		assert.Nil(t, err)
@@ -234,28 +281,9 @@ func TestControllerCreationHandler(t *testing.T) {
 			Response: &admv1beta1.AdmissionResponse{Allowed: true},
 		}
 
-		result, err := c.creationHandler(review)
+		result, err := c.creationValidationHandler(review)
 		assert.Nil(t, err)
 		assert.True(t, result.Response.Allowed)
-		assert.Equal(t, admv1beta1.PatchTypeJSONPatch, *result.Response.PatchType)
-
-		patch := &jsonpatch.ByPath{}
-		err = json.Unmarshal(result.Response.Patch, patch)
-		assert.Nil(t, err)
-
-		assertContains := func(patch *jsonpatch.ByPath, op jsonpatch.JsonPatchOperation) {
-			found := false
-			for _, p := range *patch {
-				if assert.ObjectsAreEqualValues(p, op) {
-					found = true
-				}
-			}
-
-			assert.True(t, found, "Could not find operation %#v in patch %v", op, *patch)
-		}
-
-		assertContains(patch, jsonpatch.JsonPatchOperation{Operation: "add", Path: "/metadata/finalizers", Value: []interface{}{"stable.agones.dev"}})
-		assertContains(patch, jsonpatch.JsonPatchOperation{Operation: "add", Path: "/spec/protocol", Value: "UDP"})
 	})
 
 	t.Run("invalid gameserver", func(t *testing.T) {
@@ -286,7 +314,7 @@ func TestControllerCreationHandler(t *testing.T) {
 			Response: &admv1beta1.AdmissionResponse{Allowed: true},
 		}
 
-		result, err := c.creationHandler(review)
+		result, err := c.creationValidationHandler(review)
 		assert.Nil(t, err)
 		assert.False(t, result.Response.Allowed)
 		assert.Equal(t, metav1.StatusFailure, review.Response.Result.Status)