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

Admission Controller example panics on objects with no name #942

Closed
Alibirb opened this issue Jun 30, 2022 · 5 comments · Fixed by #945
Closed

Admission Controller example panics on objects with no name #942

Alibirb opened this issue Jun 30, 2022 · 5 comments · Fixed by #945
Assignees
Labels
bug Something isn't working docs unclear documentation errors error handling or error ergonomics

Comments

@Alibirb
Copy link
Contributor

Alibirb commented Jun 30, 2022

Current and expected behavior

If the admission controller example is given an object without a "name" attribute, it will panic on this line (or the similar line for when it rejects the object):
info!("accepted: {:?} on Foo {}", req.operation, obj.name());

This is because the name() method is only supposed to be used when you KNOW that the object has a name set (which is not the case e.g. for Pods created by a ReplicaSet, which will have the generate_name field set instead).

I know it's just an example application, but the lack of name seems like a common enough condition to run into, so the example ought to start people off on the right foot in terms of how to use the name() method.

Possible solution

Check whether a name is set, and if not, use generate_name if that exists, or leave the name blank if there is none.

Additional context

No response

Environment

$ kubectl version --short
Client Version: v1.20.5
Server Version: v1.23.3

Configuration and features

No response

Affected crates

No response

Would you like to work on fixing this bug?

yes

@Alibirb Alibirb added the bug Something isn't working label Jun 30, 2022
@nightkr
Copy link
Member

nightkr commented Jun 30, 2022

Is this something you have ran into in the wild? I agree that it's not great (and I'd like to deprecate name() entirely, see #634), but I'd expect the API server to reject unnamed objects before they reach the admission controller in the first place.

@nightkr nightkr added errors error handling or error ergonomics docs unclear documentation labels Jun 30, 2022
@Alibirb
Copy link
Contributor Author

Alibirb commented Jun 30, 2022

Yes, I ran into this on a shared development cluster at work. According to the docs, the name field can be missing as long as generate_name is present, and then Kubernetes will generate a name for it when it actually creates the resource.

@clux
Copy link
Member

clux commented Jul 1, 2022

Thanks for the report. I have reworked the ResourceExt trait in #945 to add a .name_or_generatename() (provisionally) and start work on .name() deprecation in favour of .name_unchecked().

EDIT: just .name_any now.

@clux
Copy link
Member

clux commented Jul 1, 2022

By the way, what happens when you apply an object without either a name or generateName? Is that passed through to the admission controller or is that handled by Kubernetes first?

@Alibirb
Copy link
Contributor Author

Alibirb commented Jul 1, 2022

I tried applying a Pod without a name or generateName, and Kubernetes rejected it before it reached the webhook.

Also, I agree with renaming .name() to .name_unchecked(). That way you're reminded every time you use it that you need to be careful with it.

@clux clux self-assigned this Jul 2, 2022
@clux clux closed this as completed in #945 Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs unclear documentation errors error handling or error ergonomics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants