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 controller service account to update finalizers #2816

Merged
merged 2 commits into from
Nov 18, 2022

Conversation

bostrt
Copy link
Contributor

@bostrt bostrt commented Nov 16, 2022

What type of PR is this?

/kind bug

What this PR does / Why we need it:

In clusters where the OwnerReferencesPermissionEnforcement admission controller is enabled, the agones-controller service account needs to have access to update finalizers on fleets, gameserversets, and gameservers.

Which issue(s) this PR fixes:

Closes #1740

Special notes for your reviewer:

I'm not sure if this is the fix you are looking for in regards to #1740 but it works great for me on OpenShift 4.11.

In clusters where the OwnerReferencesPermissionEnforcement admission controller is enabled,
the agones-controller service account needs to have access to update finalizers on
fleets, gameserversets, and gameservers.

Closes googleforgames#1740
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8271a5f5-2d78-45d1-a512-03c0f2a2b62f

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

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2816/head:pr_2816 && git checkout pr_2816
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-3c0591e-amd64

@markmandel
Copy link
Member

@thoraxe it's been a while, but does this fix look good to you? I figure you handle this more than we do 😃

@markmandel
Copy link
Member

One thing I also realised - I'm fairly sure we only use finalizers on a GameServer, not on Fleets or GameServerSets - so do we need permission on all three, or just GameServers ?

@bostrt
Copy link
Contributor Author

bostrt commented Nov 17, 2022

One thing I also realised - I'm fairly sure we only use finalizers on a GameServer, not on Fleets or GameServerSets - so do we need permission on all three, or just GameServers ?

When testing, I saw the same error when creating Fleets and GameServerSets but let me double-check!

@markmandel
Copy link
Member

But yeah - otherwise, no issues on my end with the change.

@bostrt
Copy link
Contributor Author

bostrt commented Nov 17, 2022

I just double-checked and all the new privileges will be needed from what I can tell. The owner references and finalizers begin at Pod and end at Fleet. Here's a link to more reference that's much to share in this comment :) https://gist.github.com/bostrt/aaf11005e72ec7456cf6b568653f38de

### POD 
$ oc get pod simple-game-server-nwfq2-b74cn -o yaml | yq '.kind,.metadata.finalizers,.metadata.ownerReferences'
Pod
null
- apiVersion: agones.dev/v1
  blockOwnerDeletion: true
  controller: true
  kind: GameServer
  name: simple-game-server-nwfq2-b74cn
  uid: cfe872cb-e221-4f68-890c-2b81d1eff6ac

### GAMESERVER
$ oc get gs simple-game-server-nwfq2-n756t -o yaml | yq '.kind,.metadata.finalizers,.metadata.ownerReferences'
GameServer
- agones.dev
- apiVersion: agones.dev/v1
  blockOwnerDeletion: true
  controller: true
  kind: GameServerSet
  name: simple-game-server-nwfq2
  uid: 340d0fe1-8c71-4442-b7c1-fdf235fc7088

### GAMESERVERSET
$ oc get gss simple-game-server-nwfq2 -o yaml | yq '.kind,.metadata.finalizers,.metadata.ownerReferences'
GameServerSet
null
- apiVersion: agones.dev/v1
  blockOwnerDeletion: true
  controller: true
  kind: Fleet
  name: simple-game-server
  uid: 4b3f1b14-9e5f-4e31-aabe-3627142ec52b

### FLEET
$ oc get fleet simple-game-server -o yaml | yq '.kind,.metadata.finalizers,.metadata.ownerReferences'
Fleet
null
null

@markmandel
Copy link
Member

Oh interesting! It's all about the OwnerReference, not about the actual finalizers! Fascinating.

@thoraxe if you have a chance to take a look and confirm this is good, that would be awesome, otherwise we can merge as is I think 👍🏻

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bostrt, roberthbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot removed the lgtm label Nov 18, 2022
@google-oss-prow
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e803f2fa-1ad1-4c81-bd9d-b361b4672aa3

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

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/2816/head:pr_2816 && git checkout pr_2816
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.28.0-a7bb14d-amd64

@roberthbailey roberthbailey merged commit 4ab41be into googleforgames:main Nov 18, 2022
@thoraxe
Copy link
Contributor

thoraxe commented Nov 22, 2022

I'm not sure I'm smart enough/knowledgeable enough to chime in. If this PR causes the reproducer to work, then I'm satisfied, for whatever that's worth!

@mangalpalli mangalpalli added this to the 1.28.0 milestone Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants