MutatingWebHookConfiguration for GameServer creation & Validation. by markmandel · Pull Request #95 · agones-dev/agones

markmandel · 2018-02-17T01:15:55Z

Webhook library to make k8s webhooks easy(er) to use, as well as setting default values on GameServers via it for when they are first created.

Some refactoring of GameServer sync in the controller was required and a new PortAllocation state was created.

This is also makes #70 and #10 possible to implement.

markmandel · 2018-02-17T01:17:56Z

/cc @Kuqd - you might find this interesting as well. I implemented a 1 rule server side validation on a GameServer, to provide an example:
https://github.com/googleprivate/agones/pull/95/files#diff-4e4f035f1a95e5d6eddc9dd39d2738efR190
https://github.com/googleprivate/agones/pull/95/files#diff-3c5d2dc69cf8a604d58f0be1215b08dcR182

As an aside - if anyone wants to understand how the MutatingWebhookConfiguration parts of K8s work, drop me a line, happy to organise a hangout to talk it through.

cyriltovena · 2018-02-17T20:21:00Z

Interesting I ´ll have look

enocom

Mostly small little things. Otherwise, LGTM.

enocom · 2018-02-20T17:17:51Z

pkg/apis/stable/v1alpha1/types.go

+// Validate validates the GameServer configuration.
+// If a GameServer is invalid there will be > 0 values in
+// the returned array
+func (gs *GameServer) Validate() []metav1.StatusCause {


The callers to Validate will have to check the length of the returned slice to determine if the GameServer has valid configuration. Instead, I think it might be nicer to return (bool, []metav1.StatusCause), so callers could simple check an ok status, and only then look at the slice if necessary, e.g.:

if ok, causes := gs.Validate(); !ok { // do something with causes here }

What do you think?

I like this idea - it's far more explicit. I'll make this change.

enocom · 2018-02-20T17:21:21Z

pkg/gameservers/controller.go

+	// This is the main logic of this function
+	// the rest is really just json plumbing
+	gs.ApplyDefaults()
+	causes := gs.Validate()


Again, an ok value in addition to causes would be nice.

enocom · 2018-02-20T17:26:56Z

build/install.yaml

  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
-  name: agones-controller
+  name: agones-controller


A small thing, but probably worth fixing. Git wants newlines on all files (see here), but not all editors insert them by default. Do we want to include an .editorconfig file in this project? See here.

I noticed that in Gopkg.toml as well.

Want to do this as a separate PR? Sounds like you have much more experience with this than I do.

But this sounds like a great idea.

Yeah, sure. I'll pick it up. 👍

enocom · 2018-02-20T17:30:45Z

cmd/controller/main.go

+		logrus.WithError(err).Fatal("Could not get executable path")
+	}
+
+	base := filepath.Dir(exec)


This is a cool trick. Haven't see this before. 👍

enocom · 2018-02-20T17:34:09Z

pkg/gameservers/controller.go

+		return review, nil
+	}
+
+	new, err := json.Marshal(gs)


Since new is a built-in function, maybe newGS would be better here.

You are righ! Will change.

enocom · 2018-02-20T17:38:12Z

pkg/util/webhooks/webhooks.go

+		})
+	}
+	logrus.WithField("path", path).WithField("groupKind", gk).WithField("op", op).Info("Added webhook handler")
+	wh.handlers[path] = append(wh.handlers[path], operationHandler{groupKind: gk, operation: op, handler: h})


Does this need to be thread-safe?

Interesting question. It shouldn't have to be, since controllers (will) get created in sequence, and not at the same time. But worth keeping an eye on.

dzlier-gcp · 2018-02-20T18:29:20Z

cmd/controller/main.go

-	c := gameservers.NewController(minPort, maxPort, sidecarImage, alwaysPullSidecar, kubeClient, kubeInformationFactory, extClient, agonesClient, agonesInformerFactory)
+
+	wh := webhooks.NewWebHook(certFile, keyFile)
+	c := gameservers.NewController(wh, minPort, maxPort, sidecarImage, alwaysPullSidecar, kubeClient, kubeInformationFactory, extClient, agonesClient, agonesInformerFactory)


Not necessarily an issue for this CL, but is there precedent in Go to support the Builder paradigm? Whenever I have to look at adding a param to this constructor it makes me nervous, and a ControllerBuilder might be a way to alleviate potential problems here.

If there is support for this I will create an issue for it.

We could always do functional options. It's become a standard pattern.

Something like:

gameserves.NewController(minPort, maxPort, gameserves.WithWebHook(wh))

where the last argument to the constructor is variadic.

See here for more.

I would say for Go, functional options are the more prevalent pattern:
https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

It's an interesting question. I decided not to go that route, since default values are set in the cmd binaries, and it seemed odd to have defaults in both places and keep them in sync.

@enocom what do you think?

It's a straightforward refactor. Once we hit the 0.1 milestone, I would be interested in revisiting it, especially once I understand Agones better.

Yeah it's less critical if we don't expect users to be calling this function directly, if all their config are provided as k8s type config. If it was a public API I'd definitely push for it, but with this I could see it either way. Would make it less error prone in the future though.

stupid question :

Does go recommend a line return for long line of code ?

Interesting question - from https://golang.org/doc/effective_go.html

Line length Go has no line length limit. Don't worry about overflowing a punched card. If a line feels too long, wrap it and indent with an extra tab.

That being said, it is fairly common practice to wrap on long lines.

cyriltovena · 2018-02-21T03:43:55Z

cmd/controller/main.go

 	pflag.Bool(pullSidecarFlag, viper.GetBool(pullSidecarFlag), "For development purposes, set the sidecar image to have a ImagePullPolicy of Always. Can also use ALWAYS_PULL_SIDECAR env variable")
 	pflag.Int32(minPortFlag, 0, "Required. The minimum port that that a GameServer can be allocated to. Can also use MIN_PORT env variable.")
 	pflag.Int32(maxPortFlag, 0, "Required. The maximum port that that a GameServer can be allocated to. Can also use MAX_PORT env variable")
+	pflag.String(keyFileFlag, viper.GetString(certFileFlag), "Optional. Path to the key file")


copy/paste issue :) , should be

pflag.String(keyFileFlag, viper.GetString(keyFileFlag), "Optional. Path to the key file")

Well spotted!

cyriltovena · 2018-02-21T03:46:34Z

cmd/controller/main.go

-	c := gameservers.NewController(minPort, maxPort, sidecarImage, alwaysPullSidecar, kubeClient, kubeInformationFactory, extClient, agonesClient, agonesInformerFactory)
+
+	wh := webhooks.NewWebHook(certFile, keyFile)
+	c := gameservers.NewController(wh, minPort, maxPort, sidecarImage, alwaysPullSidecar, kubeClient, kubeInformationFactory, extClient, agonesClient, agonesInformerFactory)


stupid question :

Does go recommend a line return for long line of code ?

cyriltovena · 2018-02-21T03:54:20Z

pkg/gameservers/controller.go

+		}
+
+		logrus.WithField("review", review).Info("Invalid GameServer")
+		return review, nil


I didn't know you could validate also in the mutating webhook. I thought you would need another webhook.

So the validation hook runs after the mutation hook (and all the mutations are complete) - but the data structure that comes in and out is the same on both (AdmissionReview). But you can cancel an operation through either. I figured since we're already processing the data, it feels like overkill to do it twice.

make sense, thanks for clarifying

cyriltovena · 2018-02-21T04:00:45Z

pkg/util/webhooks/webhooks.go

+
+	err := wh.server.ListenAndServeTLS(wh.certFile, wh.keyFile)
+	if err == http.ErrServerClosed {
+		logrus.WithError(err).Info("webhook: https server closed")


should you return nil here ? the server has been closed by request of the user so you should not return an error.

oooh! Also good catch!

Webhook library to make k8s webhooks easy(er) to use, as well as setting default values on GameServers via it for when they are first created. Some refactoring of GameServer sync in the controller was required and a new PortAllocation state was created. This is also makes #70 and #10 possible to implement.

markmandel added the kind/feature New features for Agones label Feb 17, 2018

markmandel added this to the 0.1 milestone Feb 17, 2018

markmandel requested a review from enocom February 17, 2018 01:15

markmandel force-pushed the feature/gameserver-mutate-webhook branch 2 times, most recently from 7a3c34d to af43e71 Compare February 20, 2018 01:54

enocom approved these changes Feb 20, 2018

View reviewed changes

dzlier-gcp reviewed Feb 20, 2018

View reviewed changes

enocom mentioned this pull request Feb 20, 2018

Use github.com/heptio/healthcheck #98

Merged

cyriltovena reviewed Feb 21, 2018

View reviewed changes

markmandel force-pushed the feature/gameserver-mutate-webhook branch from af43e71 to 495420e Compare February 21, 2018 04:49

markmandel merged commit d226a24 into master Feb 21, 2018

markmandel deleted the feature/gameserver-mutate-webhook branch February 21, 2018 05:05

markmandel mentioned this pull request May 22, 2018

Target for generating a CHANGELOG from GitHub Milestones. #217

Merged

Conversation

markmandel commented Feb 17, 2018

Uh oh!

markmandel commented Feb 17, 2018

Uh oh!

cyriltovena commented Feb 17, 2018

Uh oh!

enocom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants