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

GameServerAllocation metadata isn't validated #2282

Closed
markmandel opened this issue Sep 29, 2021 · 1 comment · Fixed by #2449
Closed

GameServerAllocation metadata isn't validated #2282

markmandel opened this issue Sep 29, 2021 · 1 comment · Fixed by #2449
Assignees
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs.
Milestone

Comments

@markmandel
Copy link
Collaborator

What happened:

When you send a GameServerAllocation (in either CRD or gRPC/REST Allocation API) with an invalid label or annotation value, rather than returning a validation error, it responds with an UnAllocated response, suggesting it could not match a GameServer

What you expected to happen:

The response should let the user know that the format for the GameServerAllocation is invalid, and why.

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

apiVersion: "allocation.agones.dev/v1"
kind: GameServerAllocation
spec:
  metadata:
    labels:
      mode$$: deathmatch

Since "$$" is invalid, it will fail.

Anything else we need to know?:

Environment:

  • Agones version: 1.17
  • Kubernetes version (use kubectl version): Any
  • Cloud provider or hardware configuration: GKE
  • Install method (yaml/helm): Either
  • Troubleshooting guide log(s):
  • Others:

I worked out that this cause by our retry logic. Since the attempt to update the GameServer fails, it assumes the cached value is bad, and throws it away from the allocatable cache - and then retries until it runs out of game servers in the cache.

We have the ability to validate labels and annotations keys and values, so we should do that before the system attempts to actually allocate.

(I've already been working on this, PRs will slowly trickle in).

@markmandel markmandel added kind/bug These are bugs. area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Sep 29, 2021
markmandel added a commit to markmandel/agones that referenced this issue Sep 29, 2021
Initial test to ensure that all expected characters work as they should.

Work on googleforgames#2282
roberthbailey pushed a commit that referenced this issue Sep 30, 2021
Initial test to ensure that all expected characters work as they should.

Work on #2282
@markmandel markmandel self-assigned this Jan 13, 2022
@markmandel
Copy link
Collaborator Author

I have a fix for this! Got some PR's that will come in over the next few days.

markmandel added a commit to markmandel/agones that referenced this issue Jan 20, 2022
Test I made while investigating googleforgames#2282. Realised we didn't have a unit
test that ensures that when there is an empty selector, it will choose
all GameServers that are currently active in a namespace.

We have one now!
roberthbailey pushed a commit that referenced this issue Jan 20, 2022
Test I made while investigating #2282. Realised we didn't have a unit
test that ensures that when there is an empty selector, it will choose
all GameServers that are currently active in a namespace.

We have one now!
markmandel added a commit to markmandel/agones that referenced this issue Jan 20, 2022
Refactoring and cleanup I made while investigating googleforgames#2282.

Lots of the allocator tests where tied into the GameServerAllocation
controller tests, when there was no actual need for that to be the case.

This refactor de-couples those tests from the controller to have
better isolation in the testing, make it easier to create new tests, and
make test discovery for the Allocator system easier.

Also added a few more tests for extra coverage, and cleaned up a few
pieces here and there with the migration.
markmandel added a commit that referenced this issue Jan 20, 2022
Refactoring and cleanup I made while investigating #2282.

Lots of the allocator tests where tied into the GameServerAllocation
controller tests, when there was no actual need for that to be the case.

This refactor de-couples those tests from the controller to have
better isolation in the testing, make it easier to create new tests, and
make test discovery for the Allocator system easier.

Also added a few more tests for extra coverage, and cleaned up a few
pieces here and there with the migration.

Co-authored-by: Robert Bailey <robertbailey@google.com>
markmandel added a commit to markmandel/agones that referenced this issue Jan 21, 2022
With this change, if the user sends a metadata value with either
invalid labels or annotations, they will get an appropriate validation
failure result - rather than an Allocation with an `UnAllocated`
response.

Closes googleforgames#2282
roberthbailey added a commit that referenced this issue Jan 24, 2022
With this change, if the user sends a metadata value with either
invalid labels or annotations, they will get an appropriate validation
failure result - rather than an Allocation with an `UnAllocated`
response.

Closes #2282

Co-authored-by: Robert Bailey <robertbailey@google.com>
@SaitejaTamma SaitejaTamma added this to the 1.21.0 milestone Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants