GameServerAllocation example yaml file has incorrect format for selectors

[john-haven]

What happened:

The GameServerAllocation example yaml file has the incorrect format for it's selectors:
https://github.com/googleforgames/agones/blob/release-1.27.0/examples/gameserverallocation.yaml#L38-L45
You're missing the labelSelector, I believe this is because it is missing a json:",inline" for it's json tag:
https://github.com/googleforgames/agones/blob/release-1.27.0/pkg/apis/allocation/v1/gameserverallocation.go#L103

Also the usage of the matchExpressions selector could be clearer for yaml, I've updated that below too.

What you expected to happen:
The examples should deserialize correctly from yaml to into a `v1.GameSeverAllocation object.

The following format deserializes correctly for me into a v1.GameSeverAllocation and I think is clearer for the example:

spec:
  selectors:
  - labelSelector:
      matchLabels:
        agones.dev/fleet: green-fleet
  - labelSelector:
      matchLabels:
        agones.dev/fleet: blue-fleet
  - labelSelector:
      matchLabels:
        game: my-game
  - labelSelector:
      matchExpressions:
      - key: tier
        operator: In
        values:
        - cache
  - labelSelector:
    gameServerState: Ready
  - labelSelector:
    players:
      minAvailable: 0
      maxAvailable: 99

Alternatively I think you could likely fix the json:",inline" missing from the LabelSelector to resolve the issue to match the examples, but I didn't test that.

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

Here's a short program I wrote to dump the spec

package main

import (
	"fmt"

	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
	allocv1 "agones.dev/agones/pkg/apis/allocation/v1"
	"github.com/davecgh/go-spew/spew"
	"gopkg.in/yaml.v2"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func main() {
	readyState := agonesv1.GameServerStateReady

	gsAllocation := allocv1.GameServerAllocation{
		Spec: allocv1.GameServerAllocationSpec{
			Selectors: []allocv1.GameServerSelector{
				{
					LabelSelector: metav1.LabelSelector{
						MatchLabels: map[string]string{
							"agones.dev/fleet": "green-fleet",
						},
					},
				},
				{
					LabelSelector: metav1.LabelSelector{
						MatchLabels: map[string]string{
							"agones.dev/fleet": "blue-fleet",
						},
					},
				},
				{
					LabelSelector: metav1.LabelSelector{
						MatchLabels: map[string]string{
							"game": "my-game",
						},
					},
				},
				{
					LabelSelector: metav1.LabelSelector{
						MatchExpressions: []metav1.LabelSelectorRequirement{
							{
								Key:      "tier",
								Operator: "In",
								Values:   []string{"cache"},
							},
						},
					},
				},
				{
					GameServerState: &readyState,
				},
				{
					Players: &allocv1.PlayerSelector{
						MinAvailable: 0,
						MaxAvailable: 99,
					},
				},
			},
		},
	}
	spew.Dump(gsAllocation)

	out, err := yaml.Marshal(gsAllocation)
	if err != nil {
		fmt.Printf("error: %v", err)
		return
	}
	fmt.Println(string(out))
}

Anything else we need to know?:

Due to my internal policies I am unable to open a PR for this. My apologies.

Environment:

• Agones version: 1.27.0
• Kubernetes version (use kubectl version): n/a
• Cloud provider or hardware configuration: n/a
• Install method (yaml/helm): n/a
• Troubleshooting guide log(s): n/a
• Others: n/a

[markmandel]

Oooh! Interesting, because https://github.com/googleforgames/agones/blob/main/examples/xonotic/gameserverallocation.yaml works just fine 🤔 I use that demo all the time, but that's going through k8s.

I wonder if this is an issue with gopkg.in/yaml.v2 more than anything else? Or at least an inconsistency in how it works?

Do you have the same issue if you use the same library as Kubernetes: https://pkg.go.dev/k8s.io/apimachinery/pkg/util/yaml ?

[john-haven]

You're correct, it unmarshalls successfully with the Kubernetes library.

I'm not sure what the right thing to do here is - the Kubernetes yaml library only offers deserialization, not serialization, so it seems like it's a common footgun for users here since if you're doing anything with yaml in go you're likely not to be using the Kubernetes yaml library due to this limitation.

Maybe you could consider adding the json:",inline" but I'm not sure, if this works or how this affects compatibility with either library, or backwards compatibility for that matter. Sadly (?) I'm not a yaml expert, but both are seemingly valid yaml.

It's just a shame to have such a footgun in the examples. Feel free to close this issue anyway.

[markmandel]

I feel like we could definitely add json:",inline" and just see if everything works. It wouldn't be a change in API surface I don't think. @roberthbailey WDYT?

Out of curiosity, do you have a use case to go from Go -> YAML for what you are doing?

[john-haven]

Not for Agones, but we use (and serialize) yaml for other things so we were using the go-yaml library for that.
It's ok to have multiple yaml libraries, but I would generally try to unify around one in a codebase - it's nasty to have bugs because you had a wrong import and all the packages have the same "yaml" name - it's a bit of a go issue really. Aliases can help if you want to use both but it's all a bit of a mess at that point.

[markmandel]

Yeah - that makes sense!

[roberthbailey]

@markmandel - is there a downside in updating the example yaml files (as an alternative to adding the inline tag)?

Do you think it's clearer to not specify the labelSelector "wrapper" field? I initially found it confusing why the list of selectors went straight to matchLabels or matchExpressions but since it worked I went with it.

The code in our e2e tests where we use selectors has to include the labelSelector (e.g. here) so it seems like making the yaml match that would make things consistent (and also work with other yaml parsing libraries).

[markmandel]

@markmandel - is there a downside in updating the example yaml files (as an alternative to adding the inline tag)?

Other than it's a change in the API surface? 😄 (well not really, since the original still works).

Do you think it's clearer to not specify the labelSelector "wrapper" field? I initially found it confusing why the list of selectors went straight to matchLabels or matchExpressions but since it worked I went with it.

Personally, I do - selectors is an array of LabelSelectors. Adding a top level element doesn't actually do anything other than add some extra typing. You have an item in the yaml, and you specify which types of selection operations you want to use.

But also, Kubernetes as as whole doesn't expose the LabelSelector (we steal that data structure from them), although it calls it an individual "selector" in each of the cases below:

• https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#resources-that-support-set-based-requirements
• https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#creating-a-deployment

[roberthbailey]

In that case, adding inline seems like the best solution to be consistent with kubernetes.