Enable RBAC #86

dzlier-gcp · 2018-02-10T01:32:58Z

Enable RBAC, and create service accounts/roles/rolebindings for sidecar and controller as appropriate.

Currently this creates a service account in the default namespace for sidecar, and another for controller.

It then gives permission to sidecar to get and update GameServers, and access to events, pods, nodes, gameservers, and custom resource definitions for controller.

There are more permissions for controller here than we first discussed; I came by the list of resources and the access required for each by adding them one-at-a-time after the controller failed to start with error messages saying different access was required. Everything there now is needed for one reason or another.

Note: You may need to delete all running GameServers before installing this update, otherwise you may get stuck with GameServers that cannot be deleted.

Closes #57

cyriltovena · 2018-02-10T02:51:23Z

Looks good.

I think service account name are too generic and could easily clash with user service account especially on the default namespace. Should we prefix them with agones- ?

Another thought, should we pass the sidecar service account name as an environement variable to the controller ? I have a feeling that if they want to change the name they have to also re-compile the controller.

Also @markmandel should we create an issue for creating a helm chart to ease users installation ?

markmandel

This is looking good - but I agree there should be some naming changes.

Also think there should be a rejig of where the service account gets applied.

See inline comments, but let me know if anything doesn't make sense.

markmandel · 2018-02-10T18:19:27Z

build/Makefile

@@ -233,6 +233,7 @@ gcloud-auth-cluster: ensure-build-image
 	docker run --rm $(common_mounts) --entrypoint="gcloud" $(build_tag) config set compute/zone \
 		`grep zone: $(build_path)/gke-test-cluster/deployment.yml | sed 's/zone: //'`
 	docker run --rm $(common_mounts) --entrypoint="gcloud" $(build_tag) container clusters get-credentials $(CLUSTER_NAME)
+	docker run --rm $(common_mounts) $(build_tag) kubectl create clusterrolebinding cluster-admin-binding --clusterrole cluster-admin --user $$(gcloud config get-value account)


let's put a - in front of this, as if you re-run this target, it will fail the second time, but I think it's best if it fails gracefully (as maybe you've already setup this before)

Context: https://www.gnu.org/software/make/manual/html_node/Errors.html

I was wondering if there was a way to do that, thanks. Done.

markmandel · 2018-02-10T18:21:57Z

build/install.yaml

+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: sidecar


Agree with @Kuqd - this name is a bit generic.

I would change it all the sidecar names to agones-sdk (I previously changed the sidecar items to sdk, as what if we want more sidecars later on? Better to be explicit)

markmandel · 2018-02-10T18:22:34Z

build/install.yaml

+kind: ClusterRole
+metadata:
+  namespace: default
+  name: sidecar


Same as above agones-sdk (etc) - I won't mark them all, but just referencing the pattern.

markmandel · 2018-02-10T18:22:51Z

build/install.yaml

+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: controller


agones-controller across the board.

markmandel · 2018-02-10T18:36:01Z

pkg/apis/stable/v1alpha1/types.go

@@ -192,9 +194,11 @@ func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) {
 // Pod creates a new Pod from the PodTemplateSpec
 // attached for the
 func (gs *GameServer) Pod(sidecars ...corev1.Container) (*corev1.Pod, error) {
+	podSpec := *gs.Spec.Template.Spec.DeepCopy()
+	podSpec.ServiceAccountName = SidecarServiceAccountName


Also agree with @Kuqd this should be an flag/env var that gets passed to the Controller (and maybe the 'sidecar' flag should be renamed to sdk ?) - see example

My current thought is the the Pod() method below should go back to how it was, and the controller should add the ServiceAccountName around here. WDYT?

I'm all right with setting the sidecar account name as a flag (I considered that originally), but am curious when a user would want to be changing it?

Also why shouldn't we be setting the Pod's service account within the Pod() method? It sets everything else about its own metadata, spec, and containers. Don't we want to initialize everything about it in one place?

I could see a user having their own service accounts setup (down the line) and wanting to change the value. I think it should still have our own name as the default, but being able to have it able to be overwritten would be good.

Yeah, I'm going to rescind my idea to do the addition in the controller. For some reason I added the health checks in the controller - and I'm not sure why 😕 so yeah, leave it in gs.Pod()

Actually now that I'm looking at this, I think it should be something that is specified in GameServer.Spec.Template.Spec (type PodSpec), instead of something that is overwritten manually here. Then it would go in the GameServer type creation yaml files, where a serviceAccountName field is allowed in the first place.

I would say that it would be a case of - if it has been set in the pod template, then leave it alone. If it hasn't been set, then set it to our rbac value.

Then users can overwrite it if they so desire, but it has a sensible default. Which is pretty much what is done elsewhere. WDYT?

But both options are good to have as well - redefine it globally, but also be able to change it on a per-GameServer basis if needed.

That's fine, it's just a question of how to decide the default - should it still just be a constant in the types.go file? If we want it to be a value controlled from main.go, then it'll need to come in as a param value which means changing the signature.

I'd lean on having a constant that matches the default sidecar service account over having a flag, personally. If the user wants to modify the sidecar service account name (without rebuilding), then the way to do that would be to provide it in the definition file.

I'd lean on having a constant that matches the default sidecar service account over having a flag, personally. If the user wants to modify the sidecar service account name (without rebuilding), then the way to do that would be to provide it in the definition file.

Why don't we go this way for now, for simplicity sake - and if having the flag becomes something that users want, we can add it at a later date.

markmandel · 2018-02-10T18:37:44Z

build/install.yaml

+  verbs: ["get"]
+- apiGroups: ["stable.agones.dev"]
+  resources: ["gameservers"]
+  verbs: ["get", "list", "update", "watch"]


Super interesting to see all the actual permissions required.

markmandel · 2018-02-10T18:42:08Z

@Kuqd A design ticket on whether helm/ksonnet or nothing at all is probably worth talking about.

I deliberately haven't gone down that path as it added extra complexity and tooling for the user to know about - but maybe we're at a point were we should look at it? It's worth the discussion at least.

markmandel · 2018-02-12T22:51:24Z

build/Makefile

@@ -258,7 +259,7 @@ clean-gcloud-config:
 # Switches to an "agones" profile, and starts a kubernetes cluster
 # of the right version.
 #
-# Use MINIKUBE_DRIVER variable to change the VM driver 
+# Use MINIKUBE_DRIVER variable to change the VM driver
 # (defaults virtualbox for Linux and OSX, hyperv for windows) if you so desire.
 minikube-test-cluster: minikube-agones-profile
 	$(MINIKUBE) start --kubernetes-version v1.8.0 --vm-driver $(MINIKUBE_DRIVER)


From: https://kubernetes.io/docs/getting-started-guides/minikube/#examples

To set the AuthorizationMode on the apiserver to RBAC, you can use: --extra-config=apiserver.Authorization.Mode=RBAC.

So we should add the setting --extra-config=apiserver.Authorization.Mode=RBAC to the minikube-test-cluster script so that RBAC is enabled there as well.

minikube-test-cluster: minikube-agones-profile $(MINIKUBE) start --kubernetes-version v1.8.0 --vm-driver $(MINIKUBE_DRIVER) \ --extra-config=apiserver.Authorization.Mode=RBAC

Don't add this yet - this seems to be what is causing the issue. I'll investigate.

markmandel · 2018-02-12T22:53:19Z

pkg/apis/stable/v1alpha1/types.go

@@ -204,6 +206,9 @@ func (gs *GameServer) Pod(sidecars ...corev1.Container) (*corev1.Pod, error) {
 	pod.ObjectMeta.Namespace = gs.ObjectMeta.Namespace
 	// Make sure these are blank, just in case
 	pod.ResourceVersion = ""
+	if len(pod.Spec.ServiceAccountName) == 0 {


Super nitpicky, but just for consistency's sake across the codebase, this should be:
if pod.Spec.ServiceAccountName != ""

Oh, and this really needs to be included in the tests for Pod() - make sure it's actually set.
https://github.com/googleprivate/agones/blob/master/pkg/apis/stable/v1alpha1/types_test.go#L151

markmandel · 2018-02-12T23:35:57Z

Looks like the make gcloud-test-cluster failed on me on the last step:

build git:(feature/rbac) ✗ make gcloud-test-cluster


mkdir -p ~/.kube
mkdir -p /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud
docker run --rm -it -v /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube:/root/.kube -v /home/markmandel/workspace/agones/src/agones.dev/agones:/go/src/agones.dev/agones agones-build:0229fec933 gcloud \
                deployment-manager deployments create test-cluster --config=/go/src/agones.dev/agones/build/gke-test-cluster/deployment.yml
The fingerprint of the deployment is 2-LRaPoxhb4S6UenbRtrxg==
Waiting for create [operation-1518478067241-5650c39cc1029-5858e97d-ea9c9db5]...done.
Create operation operation-1518478067241-5650c39cc1029-5858e97d-ea9c9db5 completed successfully.
NAME                  TYPE                   STATE      ERRORS  INTENT
game-server-firewall  compute.beta.firewall  COMPLETED  []
test-cluster          container.v1.cluster   COMPLETED  []
make gcloud-auth-cluster
make[1]: Entering directory `/home/markmandel/workspace/agones/src/agones.dev/agones/build'
mkdir -p ~/.kube
mkdir -p /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud
docker run --rm -v /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube:/root/.kube -v /home/markmandel/workspace/agones/src/agones.dev/agones:/go/src/agones.dev/agones agones-build:0229fec933 gcloud config set container/cluster test-cluster
Updated property [container/cluster].
docker run --rm -v /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube:/root/.kube -v /home/markmandel/workspace/agones/src/agones.dev/agones:/go/src/agones.dev/agones agones-build:0229fec933 gcloud config set compute/zone \
                `grep zone: /home/markmandel/workspace/agones/src/agones.dev/agones/build//gke-test-cluster/deployment.yml | sed 's/zone: //'`
Updated property [compute/zone].
docker run --rm -v /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube:/root/.kube -v /home/markmandel/workspace/agones/src/agones.dev/agones:/go/src/agones.dev/agones agones-build:0229fec933 gcloud container clusters get-credentials test-cluster
Fetching cluster endpoint and auth data.
kubeconfig entry generated for test-cluster.
docker run --rm -v /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube:/root/.kube -v /home/markmandel/workspace/agones/src/agones.dev/agones:/go/src/agones.dev/agones agones-build:0229fec933 kubectl create clusterrolebinding cluster-admin-binding --clusterrole cluster-admin --user $(gcloud config get-value account)
/bin/sh: gcloud: command not found
Error: flag needs an argument: --user
Examples:

# Create a ClusterRoleBinding for user1, user2, and group1 using the cluster-admin ClusterRole

kubectl create clusterrolebinding cluster-admin --clusterrole=cluster-admin --user=user1 --user=user2 --group=group1
Options:

--allow-missing-template-keys=true: If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats.

--clusterrole='': ClusterRole this ClusterRoleBinding should reference

--dry-run=false: If true, only print the object that would be sent, without sending it.

--generator='clusterrolebinding.rbac.authorization.k8s.io/v1alpha1': The name of the API generator to use.

--group=[]: Groups to bind to the role

--no-headers=false: When using the default or custom-column output format, don't print headers (default print headers).

--openapi-validation=true: If true, use openapi rather than swagger for validation.

-o, --output='': Output format. One of: json|yaml|wide|name|custom-columns=...|custom-columns-file=...|go-template=...|go-template-file=...|jsonpath=...|jsonpath-file=... See custom columns [http://kubernetes.io/docs/user-guide/kubectl-overview/#custom-columns], golang template [http://golang.org/pkg/text/template/#pkg-overview] and jsonpath template [http://kubernetes.io/docs/user-guide/jsonpath].

--save-config=false: If true, the configuration of current object will be saved in its annotation. Otherwise, the annotation will be unchanged. This flag is useful when you want to perform kubectl apply on this object in the future.

--schema-cache-dir='~/.kube/schema': If non-empty, load/store cached API schemas in this directory, default is '$HOME/.kube/schema'

--serviceaccount=[]: Service accounts to bind to the role, in the format :

-a, --show-all=false: When printing, show all resources (default hide terminated pods.)

--show-labels=false: When printing, show all labels as the last column (default hide labels column)

--sort-by='': If non-empty, sort list types using this field specification.  The field specification is expressed as a JSONPath expression (e.g. '{.metadata.name}'). The field in the API resource specified by this JSONPath expression must be an integer or a string.

--template='': Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].

--user=[]: Usernames to bind to the role

--validate=true: If true, use a schema to validate the input before sending it
Usage:

kubectl create clusterrolebinding NAME --clusterrole=NAME [--user=username] [--group=groupname] [--serviceaccount=namespace:serviceaccountname] [--dry-run] [options]
Use "kubectl options" for a list of global command-line options (applies to all commands).

make[1]: [gcloud-auth-cluster] Error 1 (ignored) make[1]: Leaving directory `/home/markmandel/workspace/agones/src/agones.dev/agones/build'

markmandel · 2018-02-12T23:39:33Z

The other thing I'm running into (at least on minikube, I haven't test on gke yet), is that the grpc SDK can't connect sdk-sidecar process. All I see is this in the logs for the cpp gameserver:

root@markmandel:/go/src/agones.dev/agones# kubectl logs cpp-simple-wx2v2-bqvtt -c cpp-simple
C++ Game Server has started!
Getting the instance of the SDK!
Attempting to connect...

dzlier-gcp · 2018-02-12T23:43:55Z

Is RBAC enabled in your cluster?

dzlier-gcp · 2018-02-12T23:44:48Z

As for gRPC not being able to connect, can you tell which resource/verb pair is missing exactly?

markmandel · 2018-02-12T23:45:20Z

build/Makefile

@@ -233,6 +233,7 @@ gcloud-auth-cluster: ensure-build-image
 	docker run --rm $(common_mounts) $(build_tag) gcloud config set compute/zone \
 		`grep zone: $(build_path)/gke-test-cluster/deployment.yml | sed 's/zone: //'`
 	docker run --rm $(common_mounts) $(build_tag) gcloud container clusters get-credentials $(CLUSTER_NAME)
+	-docker run --rm $(common_mounts) $(build_tag) kubectl create clusterrolebinding cluster-admin-binding --clusterrole cluster-admin --user $$(gcloud config get-value account)


Worked out the bug. This should be:
-docker run --rm $(common_mounts) $(build_tag) bash -c 'kubectl create clusterrolebinding cluster-admin-binding --clusterrole cluster-admin --user $$(gcloud config get-value account)'

This pushes all the command down into the container, so the gcloud command doesn't get evaluated beforehand.

markmandel · 2018-02-12T23:47:33Z

@dzlier-gcp yeah, I have RBAC enabled, I started my minikube cluster with:

$(MINIKUBE) start --kubernetes-version v1.8.0 --vm-driver $(MINIKUBE_DRIVER) \
		--extra-config=apiserver.Authorization.Mode=RBAC

as per https://kubernetes.io/docs/getting-started-guides/minikube/#examples

Testing on GKE next, see if it's just a minikube issue.

markmandel · 2018-02-12T23:56:25Z

@dzlier-gcp works fine on GKE - so it's a minikube issue! Investigating.

markmandel · 2018-02-13T01:43:49Z

Fixed the final minikube issues on commit af9ed94, feature/rbac-mark-fixes branch.

If you are happy, I can also merge them here and/or rebase against master, or you can cherry pick the change across and rebase. Up to you.

…ar and controller as appropriate.

markmandel requested changes Feb 10, 2018

View reviewed changes

markmandel mentioned this pull request Feb 12, 2018

Namespace for Agones infrastructure #89

Closed

dzlier-gcp force-pushed the feature/rbac branch 8 times, most recently from 0aa7224 to 64faa73 Compare February 12, 2018 23:08

markmandel requested changes Feb 12, 2018

View reviewed changes

markmandel reviewed Feb 12, 2018

View reviewed changes

dzlier-gcp force-pushed the feature/rbac branch from 64faa73 to aafe57b Compare February 12, 2018 23:45

markmandel added a commit that referenced this pull request Feb 13, 2018

Fixes for RBAC + Minikube issues in #86

af9ed94

markmandel force-pushed the feature/rbac branch from af9ed94 to c8d9b63 Compare February 13, 2018 17:27

markmandel approved these changes Feb 13, 2018

View reviewed changes

Enable RBAC, and create service accounts/roles/rolebindings for sidec…

cd98e93

…ar and controller as appropriate.

markmandel force-pushed the feature/rbac branch from c8d9b63 to cd98e93 Compare February 13, 2018 17:47

markmandel merged commit 098f5a3 into master Feb 13, 2018

markmandel deleted the feature/rbac branch February 13, 2018 18:07

markmandel added this to the 0.1 milestone May 19, 2018

markmandel mentioned this pull request May 22, 2018

Target for generating a CHANGELOG from GitHub Milestones. #217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable RBAC #86

Enable RBAC #86

dzlier-gcp commented Feb 10, 2018 •

edited by markmandel

Loading

cyriltovena commented Feb 10, 2018

markmandel left a comment

markmandel Feb 10, 2018

dzlier-gcp Feb 12, 2018

markmandel Feb 10, 2018

markmandel Feb 10, 2018 •

edited

Loading

dzlier-gcp Feb 12, 2018

markmandel Feb 10, 2018

dzlier-gcp Feb 12, 2018

markmandel Feb 10, 2018

dzlier-gcp Feb 12, 2018

markmandel Feb 12, 2018

dzlier-gcp Feb 12, 2018

markmandel Feb 12, 2018 •

edited

Loading

dzlier-gcp Feb 12, 2018

markmandel Feb 12, 2018

markmandel Feb 10, 2018

markmandel commented Feb 10, 2018

markmandel Feb 12, 2018

markmandel Feb 13, 2018

markmandel Feb 12, 2018

markmandel Feb 12, 2018

markmandel commented Feb 12, 2018

markmandel commented Feb 12, 2018

dzlier-gcp commented Feb 12, 2018

dzlier-gcp commented Feb 12, 2018

markmandel Feb 12, 2018

markmandel commented Feb 12, 2018

markmandel commented Feb 12, 2018

markmandel commented Feb 13, 2018

Enable RBAC #86

Enable RBAC #86

Conversation

dzlier-gcp commented Feb 10, 2018 • edited by markmandel Loading

cyriltovena commented Feb 10, 2018

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel commented Feb 12, 2018

markmandel commented Feb 12, 2018

dzlier-gcp commented Feb 12, 2018

dzlier-gcp commented Feb 12, 2018

Choose a reason for hiding this comment

markmandel commented Feb 12, 2018

markmandel commented Feb 12, 2018

markmandel commented Feb 13, 2018

dzlier-gcp commented Feb 10, 2018 •

edited by markmandel

Loading

markmandel Feb 10, 2018 •

edited

Loading

markmandel Feb 12, 2018 •

edited

Loading