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

[v0.7|main] Make admission rejection message more useful for namespace creation #572

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crobby
Copy link
Contributor

@crobby crobby commented Dec 17, 2024

Issue:

rancher/rancher#47585

Problem

Webhook rejection message was not useful in this situation

Solution

Include user, verb, resource in the rejection message.

Testing notes

To make this situation show up, you can create a roletemplate like the one below

apiVersion: management.cattle.io/v3
builtin: false
context: project
description: ""
displayName: nomanagenamespace
external: false
hidden: false
kind: RoleTemplate
metadata:
  generateName: rt-
rules:
- apiGroups:
  - ""
  resources:
  - namespaces
  verbs:
  - '*'
- apiGroups:
  - management.cattle.io
  resources:
  - projects
  verbs:
  - get
  - list
  - watch
  - updatepsa

Then you can create a clusterroletemplatebinding to bind that role to a user that has no other permissions on a given cluster.

With that in place, login as the user to which the crtb was bound and navigate to the cluster and try to create a namespace in a project.

Prior to this fix, the error message provided just said "Unauthorized".
After this fix, the error message will look like: admission webhook "rancher.cattle.io.namespaces.create-non-kubesystem" denied the request: User "u-7qhg2" does not have permission "manage-namespaces" on project "p-w7252"

@crobby crobby requested a review from a team as a code owner December 17, 2024 17:22
@crobby crobby requested a review from nflynt December 19, 2024 09:44
@crobby crobby requested a review from a team December 19, 2024 13:06
@crobby crobby requested a review from ericpromislow January 27, 2025 18:28
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Would be good if there could be a unit test for this. I tried messing with the test file, but always get an actual error "error when creating sar, server unavailable" instead of a fabricated one. Not sure if the underlying function is properly mocked.

Looks fine otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants