Update documentation to describe why the Agones sidecar uses the prefix agones.dev/sdk-

[domgreen]

What happened:

When setting labels/annotations from a dedicated game server (DGS) via the SDKs (Unreal specifically, but applies to all) a prefix is added to those labels and annotations agones.dev/sdk- this is the metaPrefix defined in the sdkserver. Which is used when setting or updating a label/annotation from the DGS here.

Issue:

• When I set a label n6k.games/mylabel
• I get the following error:

{"message":"GameServer.agones.dev \"fleet-dev-playtest-dcj9z-4rrxv\" is invalid: metadata.labels: Invalid value: \"agones.dev/sdk-n6k.games/mylabel\": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')","severity":"error","time":"2021-04-06T10:11:27.460918192Z"}

What you expected to happen:

• I expect to be able to set a label with any supplied prefix
• no prefix is added when not supplied on the call
  • this is specifically as getting the gameserver state afterwards means the label is different to the one the engineer thinks they set

How to reproduce it (as minimally and precisely as possible):

• Spin up any DGS call via the SDKs to add a label notice a prefix is added, get the state to see if your label is present.

Anything else we need to know?:

Reading Set Labels

As eluded to in the what you expect to happen section this becomes very strange when you both wish to set/read/update labels from within the DGS.

In my case, I wish to set a label, read periodically if it has changed and the server react to it, this could have been changed outside of the DGS or within the process.

An example:

• set a label from within the DGS foo
• saved as agones.dev/sdk-foo
• get the state of the DGS via Agones
• not able to find the foo label
• engineer has to know that the prefix is added (it is documented but none intuative)

Further:

• create a DGS and set a label n6k.games/instance.foo=value or instance.foo=value
• read this from within the DGS
• try to update this value
• n6k.games/instance.foo=123 < will error due to prefix existing
• instance.foo=newvalue < will be saved as agones.dev/sdk-instance.foo=newvalue
  • reading instance.foo will still give you the old value value hiding the update.

Adding Labels during Allocation

Additionally, this becomes slightly more confusing (at least for me) as I also use labels at allocation time to tell the DGS to perform various behaviours on startup. I use AllocationRequest and MetaPatch to set labels on the DGS these do not react in the same way by prefixing the agones.dev/sdk- prefix on the label I added.

Therefore I now to work around this have to change my labels to always be prefixed with agones./dev-sdk.

Use of Labels

It could be that I am overusing labels for how I do instance within a DGS (trying not to spin up and down huge UE4 servers) and use labels for this as it is a good store of data that is only lived as long as the UE4 instance. The labels that I use allow me to identify the UE4 and their contained instances.

[roberthbailey]

I think the fact that read / write isn't symmetric is a problem. If the system is going to add a prefix during write, it should add it during read as well (and also during allocation requests), so that the DGS has a consistent view of labels. (This wouldn't fix your issue with namespacing though.)

I think that the prefix was put there to protect users from label name collisions, but it seems like it's causing more problems than it may be preventing.

Since Agones users that want to use labels need to be aware of how k8s labels work and namespace collision issues, I wonder if dropping the prefix is better than trying to better hide by adding it in more places.

@markmandel - WDYT?

[domgreen]

Thanks @roberthbailey my 2cents is also that dropping the prefix would be most beneficial as it would also allow me to do my own namespacing and interact with labels as expected.

However, I also see this as a breaking change so might complicate things depending on how you deal with those.

[mthssdrbrg]

+1 for dropping the prefix, or at least making it configurable. I imagine adding a flag (be it a boolean or a prefix that can be set to the empty string or something) underneath the sdkServer part of the GameServer spec could work?

[markmandel]

To give some context, here are the original design discussions:
#277
#279

Totally understood the prefix can be confusing, and we could likely have chosen a better prefix, and also better docs (you both aren't the first one to bring this up -- and I'm slowly getting broken down to let this battle go 😄 ). The big issue here is isolation though, so it's worth rehashing.

Probably the smaller aspect is: Because of the prefix, you always know if you are accessing or reading a value that could have come, or may be changed by the client SDK. So much like private vs public scope in a programming language - we are only giving access to part of the set of labels and annotations that exist on a GameServer. While I admit it's confusing from an initial development setup, I hope it's easier to reconcile things when you are viewing history of when something has gone wrong -- as you know what labels and annotation could possibly be changed by the SDK and those that couldn't be.

The larger aspect: It's a smaller security vector if the prefix remains in place, and the GameServer container gets compromised. Since the game container is (generally) externally exposed, and we don't control the binary that is run within it - limiting exposure if something goes horribly wrong and it becomes compromised, is, I think a good thing. And potentially a good thing that is worth the friction around initial development.

While editing all labels and annotations doesn't seem like a significant attack vector (you could take a GameServer out of the Fleet management, maybe?) -- I don't know if we want to be in the business of trying to come up with all the ways having access to all labels and annotations in for all releases of Agones forever is not going to be able to be abused in a worst case scenario.

As a side note: The GameServer container deliberately doesn't have the Pod's service account mounted, so it can't access the K8s api directly for this exact reason. It only has access to SDK API. To get access to the service account, one would have to escape the game server binary container, and somehow then get into the SDK server container to execute code (maybe a good reason to move the SDK server to something like a distroless image, as suggested in #909, even though this seems unlikely).

Thoughts?

[domgreen]

I think you won me over, as I mentioned I have worked around this by setting anything on my GS with the expected prefix 👍

Probably the smaller aspect is: Because of the prefix, you always know if you are accessing or reading a value that could have come, or may be changed by the client SDK. So much like private vs public scope in a programming language - we are only giving access to part of the set of labels and annotations that exist on a GameServer. While I admit it's confusing from an initial development setup, I hope it's easier to reconcile things when you are viewing history of when something has gone wrong -- as you know what labels and annotation could possibly be changed by the SDK and those that couldn't be.

This makes alot of sense to me, we have changed to adding the prefix on all things we are allowing the GS to alter on our instances so that they can indeed alter what we expect.

The larger aspect: It's a smaller security vector if the prefix remains in place, and the GameServer container gets compromised. Since the game container is (generally) externally exposed, and we don't control the binary that is run within it - limiting exposure if something goes horribly wrong and it becomes compromised, is, I think a good thing. And potentially a good thing that is worth the friction around initial development.

Didn't initially give much thought to this but is an excellent point which is what won me over, the fact we are running arbitary binaries that can be doing anything restricting them to a prefix is indeed a good solution.

Totally understood the prefix can be confusing, and we could likely have chosen a better prefix, and also better docs (you both aren't the first one to bring this up -- and I'm slowly getting broken down to let this battle go 😄 ). The big issue here is isolation though, so it's worth rehashing.

After reading though this response, I am won over to the argument of keeping a prefix that is specifically for allowing a GS to write or edit a label/annotation in isolation. I would be tempted to port alot of your reasons here into the documenation on why the prefix is present 👍

My only concern is that the reading / writing of labels is not the symetrical, this is something that maybe documentation can help with, clearly pointing out the expected behaviour.

The other option is to add a GetLabels set of functions to the SDKs however, I do not think this would be benificial as I would be wanting to read all labels not just hte ones set by hte SDK.

Personally happy to close this isse.

[markmandel]

I would be tempted to port alot of your reasons here into the documenation on why the prefix is present +1

Personally happy to close this isse.

Maybe we pivot this issue slightly to be about what documentation fixes would be appropriate (or at least for a PR to be pointed at)?

[mthssdrbrg]

Thanks for the context, and taking the time to write it all up, very much appreciated!

The larger aspect: It's a smaller security vector if the prefix remains in place, and the GameServer container gets compromised. Since the game container is (generally) externally exposed, and we don't control the binary that is run within it - limiting exposure if something goes horribly wrong and it becomes compromised, is, I think a good thing. And potentially a good thing that is worth the friction around initial development.

Excellent point, I hadn't thought too much about non-trusted workloads to be completely honest 😅.

While editing all labels and annotations doesn't seem like a significant attack vector (you could take a GameServer out of the Fleet management, maybe?) -- I don't know if we want to be in the business of trying to come up with all the ways having access to all labels and annotations in for all releases of Agones forever is not going to be able to be abused in a worst case scenario.

Yep, very much agree.

Thinking about it, for the use case I had in mind where I'd like to not have the prefix, I've come up with an alternative solution that could work just as well.

+1 on expanding the documentation with regards to isolation and motivation for the prefix.

[markmandel]

Created a PR: #2057

PTAL, let me know if you think that covers things 👍🏻

[roberthbailey]

I've updated the issue title and labels.