-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add the ability to turn off RBAC in helm and customize gcp test-cluster #220
add the ability to turn off RBAC in helm and customize gcp test-cluster #220
Conversation
117f9f7
to
44b8c36
Compare
Build Succeeded 👏 Build Id: 30e49dee-4147-42b4-8dd0-c2762caebff8 The following development artifacts have been built, and will exist for the next 30 days:
|
Build Succeeded 👏 Build Id: 230ba288-21b2-46ea-8b81-21837e579585 The following development artifacts have been built, and will exist for the next 30 days:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, we should also update
https://github.com/GoogleCloudPlatform/agones/blob/master/build/README.md
As well though :) At the least, in the Make Target Reference, but maybe also in the quickstart? (or have the quickstart point to the reference?)
build/Makefile
Outdated
GCP_CLUSTER_ZONE ?= us-west1-c | ||
GCP_CLUSTER_LEGACYABAC ?= false | ||
GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT ?= 3 | ||
GCP_CLUSTER_NODEPOOL_MACHINETYPE ?= n1-standard-4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move
GCP_CLUSTER_LEGACYABAC ?= false
GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT ?= 3
GCP_CLUSTER_NODEPOOL_MACHINETYPE ?= n1-standard-4
To being target specific variables to gcloud-test-cluster
, since they aren't reused across more than 1 target.
WDYT?
(here is an example, if you aren't familiar with them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will do !
b9f9e3c
to
1829750
Compare
Build Succeeded 👏 Build Id: bbc2fc4b-9d3c-437e-aa01-142100953191 The following development artifacts have been built, and will exist for the next 30 days:
|
Build Succeeded 👏 Build Id: 50719f78-96b5-46dc-bc70-55731b12a63f The following development artifacts have been built, and will exist for the next 30 days:
|
Build Succeeded 👏 Build Id: 824fc04a-cc6a-493e-9e39-40007387f1d3 The following development artifacts have been built, and will exist for the next 30 days:
|
Build Succeeded 👏 Build Id: c2dda9f4-0bac-4fdb-94d1-ff83e920ca42 The following development artifacts have been built, and will exist for the next 30 days:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh - sorry, had a couple of questions on the documentation side. Almost there, I swear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies - somehow my previous comments disappeared. Here were the two issues I found.
build/README.md
Outdated
| `GCP_CLUSTER_NODEPOOL_INITIALNODECOUNT`| The number of nodes to create in this cluster. | `3` | | ||
| `GCP_CLUSTER_NODEPOOL_MACHINETYPE` | The name of a Google Compute Engine machine type. | `n1-standard-4` | | ||
|
||
If you would like to change more settings, feel free to edit the `deployment.yaml` file before running this command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't you delete the deployment.yaml? 😊
install/helm/README.md
Outdated
@@ -47,6 +47,11 @@ $ kubectl create namespace ps4 | |||
$ helm upgrade --set "gameservers.namespaces={default,xbox,ps4}" my-release agones | |||
``` | |||
|
|||
## RBAC | |||
|
|||
If role-based access control (RBAC) is enabled in your cluster, you must set `agones.rbacEnabled` to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to suggest we change this to the following, as it sounds like I need to take action if I have RBAC enabled (which most people do), when in fact I don't. How does this sound:
By default, agones.rbacEnabled
is set to true. This enable RBAC support in Agones and must be true if RBAC is enabled in your cluster.
The chart will take care of creating the required service accounts and roles for Agones.
If you have RBAC disabled, or to put it another way, ABAC enabled, you should set this value to false
.
WDYT?
also update build documentation and move variable next to target
1829750
to
7b25a34
Compare
Build Succeeded 👏 Build Id: 2eb0acb2-2ecb-4785-b44e-4ebe7c483acf The following development artifacts have been built, and will exist for the next 30 days:
|
agones.rbacEnabled
default true)Tested on GKE with RBAC off.
Closes #211