Skip to content
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

Allow GameServers to be created in non-default namespace. #154

Closed
wants to merge 1 commit into from

Conversation

dzlier-gcp
Copy link
Contributor

Closes #146

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 42507786-b1bb-42c0-b201-27b18c9bf112

The following development artifacts have been built, and will exist for the next 30 days:

@markmandel markmandel added this to the 0.2 milestone Mar 29, 2018
@markmandel
Copy link
Member

markmandel commented Mar 29, 2018

Just took this for a spin - built a new image, and installed from build tools, but it didn't work for me :(

This was in the default namespace.

{
    "error": "error creating Pod for GameServer cpp-simple-zx7m2: pods \"cpp-simple-zx7m2-\" is forbidden: error looking up service account default/agones-sdk: serviceaccount \"agones-sdk\" not found",
    "level": "error",
    "msg": "",
    "obj": "default/cpp-simple-zx7m2",
    "queue": "stable.agones.dev.GameServerController",
    "source": "*gameservers.Controller",
    "time": "2018-03-29T00:32:45Z"
}
{
    "level": "error",
    "msg": "error creating Pod for GameServer cpp-simple-zx7m2: pods \"cpp-simple-zx7m2-\" is forbidden: error looking up service account default/agones-sdk: serviceaccount \"agones-sdk\" not found",
    "stack": [
        "agones.dev/agones/pkg/gameservers.(*Controller).syncGameServerCreatingState\n\t/go/src/agones.dev/agones/pkg/gameservers/controller.go:387",
        "agones.dev/agones/pkg/gameservers.(*Controller).syncGameServer\n\t/go/src/agones.dev/agones/pkg/gameservers/controller.go:273",
        "agones.dev/agones/pkg/gameservers.(*Controller).(agones.dev/agones/pkg/gameservers.syncGameServer)-fm\n\t/go/src/agones.dev/agones/pkg/gameservers/controller.go:111",
        "agones.dev/agones/pkg/util/workerqueue.(*WorkerQueue).processNextWorkItem\n\t/go/src/agones.dev/agones/pkg/util/workerqueue/workerqueue.go:97",
        "agones.dev/agones/pkg/util/workerqueue.(*WorkerQueue).runWorker\n\t/go/src/agones.dev/agones/pkg/util/workerqueue/workerqueue.go:73",
        "agones.dev/agones/pkg/util/workerqueue.(*WorkerQueue).(agones.dev/agones/pkg/util/workerqueue.runWorker)-fm\n\t/go/src/agones.dev/agones/pkg/util/workerqueue/workerqueue.go:115",
        "agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1\n\t/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133",
        "agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134",
        "agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait.Until\n\t/go/src/agones.dev/agones/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88",
        "runtime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:2361"
    ],
    "time": "2018-03-29T00:32:45Z"
}

@dzlier-gcp dzlier-gcp requested review from markmandel and enocom and removed request for markmandel March 29, 2018 16:09
@dzlier-gcp
Copy link
Contributor Author

OK so here's the challenge.

In types.go, it says the pod created by a gameserver has to be in the same namespace as the gameserver.

However, I cannot find a way to make give the PodSpec a service account in a namespace other than the one that it's in. So if we want the pod spec to use the agones-sdk namespace, then the GameServer pod has to be in the same namespace as that service account.

This would be why, answering Cyril's question from weeks ago, I didn't originally put agones-sdk in agones-system. Because then the GameServers had to be in agones-system as well.

Now, if "Pods for GameServers need to stay in the same namespace" is a hard requirement, I'm not sure of any way around this other than creating a new service account specifically for the pod. Since that sounds like a terrible idea, are there any other suggestions? Am I missing some way to configure a pod to use a service account in a namespace other than the one it's in?

@enocom
Copy link
Contributor

enocom commented Mar 30, 2018

It sounds like there are some possible improvements to how Agones handles namespaces.

For now, I think moving Agones into the non-default namespace is a nice change.

@markmandel Any objections to just merging this?

@dzlier-gcp
Copy link
Contributor Author

The result of merging this would be that all GameServer + Pods would have to be created in the agones-system namespace, so if people are already accustomed to creating them in default, it will break some things.

@enocom
Copy link
Contributor

enocom commented Apr 2, 2018

Thinking about upgrade paths is a good point. Since we're in alpha, I think we can still make breaking changes, but we should provide a note in the release notes about it.

@markmandel
Copy link
Member

So it's not a hard requirement that the Pod be created in the same namespace as the GameServer - but I think logically it makes sense, and it would be confusing to the end user to find them in different places. If I make a Deployment, my Pods stay in the same Namespace as well.

Forcing people to create GameServers in a specific namespace (other than default) also feels very counter intuitive for any Kubernetes user.

On upgrades - this is alpha -- we can break things, as long as we document them. It's in the root README 😄

I feel like there has to be a way to do this - the kubectl user has access to all namespaces, so there has to be a way. May be time to reach out and see who we know who works on RBAC, and get them to review?

@markmandel
Copy link
Member

...and apparently I'm totally wrong. Chatting with a few people:

service accounts are namespaced. pods are namespaced. podspecs can only references pods, secrets, service accounts that are local to the namespace.

So this sounds like something we should solve at the #101 (Yaml/Helm packaging) - wherein the user selects which namespaces GameServers can be used at install/update time, and all the service accounts that are needed are created at that point. WDYT?

@markmandel
Copy link
Member

Just to clean up - should we close this PR, and make a note to revisit this in #146 when #101 has the first part of it's work complete (a Helm install that works)

@enocom
Copy link
Contributor

enocom commented Apr 3, 2018

Yep. I think that makes sense. @dzlier-gcp What do you think?

@enocom
Copy link
Contributor

enocom commented Apr 4, 2018

I'm going to close this in favor of #101.

@enocom enocom closed this Apr 4, 2018
@markmandel markmandel mentioned this pull request Apr 4, 2018
@markmandel markmandel deleted the auth branch April 4, 2018 19:07
@markmandel markmandel added the kind/feature New features for Agones label May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants