Skip to content

Conversation

@crobby
Copy link
Collaborator

@crobby crobby commented Nov 26, 2024

Issue:

#44825

Problem

There is no validation of the cpu/memory request/limits when creating namespaces, which leads to Rancher attempting to create invalid objects.

Solution

The webhook now validates cpu/memory request/limits, if provided.
If a request or limit is provided by itself, we just validate that it is indeed a properly formatted value.
If both a cpu request/limit or a memory request/limit pair is provided, we also check to be sure that the request is less than or equal to the limit.
If parsing any of the provided values fails or if a request is greater than the limit, the webhook will refuse the request.

CheckList

  • Test
  • Docs

@crobby crobby requested a review from a team as a code owner November 26, 2024 15:47
@crobby crobby marked this pull request as draft November 26, 2024 15:48
@crobby crobby marked this pull request as ready for review November 26, 2024 20:55
@prachidamle prachidamle requested review from joshmeranda and nflynt and removed request for a team November 26, 2024 21:42
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.

Looks ok to me. Some might suggest you could fold the 2 comparisons in validateResourceLimitsWithUnits into a separate function, because they have identical structures, but given that those are most likely the only two attributes that we'll ever compare I would leave the code as is, favoring repetition over encapsulation and more complexity (and even more complexity if you use reflection on the field names).

Copy link
Contributor

@nflynt nflynt left a comment

Choose a reason for hiding this comment

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

👍 This looks good. I might nitpick and suggest tests for an exact match between limit and requests, just to cover the edge of what's acceptable, but I won't block on that.

@crobby crobby changed the title Add resource request and limit validation when creating a namespace [main/2..10.2] Add resource request and limit validation when creating a namespace Dec 4, 2024
@crobby crobby changed the title [main/2..10.2] Add resource request and limit validation when creating a namespace [main/2.10.2] Add resource request and limit validation when creating a namespace Dec 4, 2024
@ericpromislow ericpromislow merged commit dfd30a4 into rancher:main Dec 6, 2024
2 checks passed
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