Skip to content

Re-enable the authorizer webhook by default.#364

Open
renormalize wants to merge 7 commits intoai-dynamo:mainfrom
renormalize:authorizer
Open

Re-enable the authorizer webhook by default.#364
renormalize wants to merge 7 commits intoai-dynamo:mainfrom
renormalize:authorizer

Conversation

@renormalize
Copy link
Collaborator

@renormalize renormalize commented Jan 22, 2026

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The authorizer webhook was disabled by default due to a potential regression observed around the second alpha release in #223.

However, I have been unable to replicate this issue since, and would like to re-enable it by default. It being enabled by default will also help us in catching any potential bugs before the next release as all new PRs will indirectly check the behavior of the authorizer webhook, along with the E2E tests.

This PR also fixes E2E tests and introduces a new dynamic client which creates a bearer token via TokenRequest API using the service account with which Grove operator is started. The authorizer webhook allows calls to change managed resources when it comes from grove service account.

Previous to this PR, user system:admin was making calls to scale-out/in PCLQ/PCSG and with authorizer webhook enabled this will be disallowed as system:admin is not an exempted account. Also we would not like to change the values.yaml to include test users that k3d brings in.

Does this PR introduce a API change?

N/A

Additional documentation e.g., enhancement proposals, usage docs, etc.:

N/A

Ronkahn21
Ronkahn21 previously approved these changes Jan 23, 2026
Copy link
Contributor

@Ronkahn21 Ronkahn21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@unmarshall unmarshall added component/operator Issue/PR is for grove operator module kind/enhancement Categorizes issue or PR as related to a new feature, enhancement or improvement do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 25, 2026
@unmarshall
Copy link
Collaborator

unmarshall commented Jan 25, 2026

@renormalize you are unable to see the problem because we never print the admission denied error and the response from the webhook is not captured as part of the log.

After i explicitly added a log the error is now visible:

{"level":"info","ts":"2026-01-25T12:05:14.549Z","logger":"authorizer-webhook.authorizer-webhook","msg":"create/update of managed resource denied","user":"system:admin","operation":"UPDATE","resource":{"group":"grove.io","version":"v1alpha1","resource":"podcliquescalinggroups"},"subresource":"","name":"workload2-0-sg-x","namespace":"default","resourceObjectKey":{"name":"workload2-0-sg-x","namespace":"default"}}

There seems to be a system:admin user which is attempting to update PCSG resources. It could be that this user comes from k3d. Need to check further. I also do not know what it is trying to update. Will debug further.

Only for testing, when i added system:admin as one of the allowed service accounts in the Grove operator configuration, then the test succeeded. However this should never be done since system:admin is the user in the kubeconfig that is created/used by k3d. This should be part of the test setup.

@renormalize renormalize marked this pull request as draft January 27, 2026 08:10
@unmarshall unmarshall marked this pull request as ready for review January 28, 2026 13:49
@unmarshall unmarshall self-assigned this Jan 28, 2026
@unmarshall unmarshall removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2026
Copy link
Contributor

@gflarity gflarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

A few small things in my comments. I'd like to see the Clientset and hardcoded (valid for 1 hour) fixed if you don't disagree. The printDiff is just a nit. I won't request changes so this doesn't get blocked on me. Ping me for a fast approval after, I'll also chat with @sanjaychatterjee and ask him to approve it I'm not immediately available.

renormalize and others added 6 commits February 1, 2026 16:27
Signed-off-by: Saketh Kalaga <saketh.kalaga@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
grove serviceaccount.
* Adapted setup and tests to use operator dynamic client where tests
  need to change managed resources.
* Changed the verbosity level for Info log from authorizer webhook that logs every request.

Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Signed-off-by: Saketh Kalaga <saketh.kalaga@sap.com>
Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/operator Issue/PR is for grove operator module kind/enhancement Categorizes issue or PR as related to a new feature, enhancement or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments