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

Adjust CIDRPool to use int32 instead of uint fields #48

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

vasrem
Copy link
Collaborator

@vasrem vasrem commented Aug 6, 2024

According to the API Conventions, we should not use uint type for fields.

  • All public integer fields MUST use the Go int32 or Go int64 types, not int (which is ambiguously sized, depending on target platform). Internal types may use int.
  • Do not use unsigned integers, due to inconsistent support across languages and libraries. Just validate that the integer is non-negative if that's the case.

In fact, issues using uint can also be observed when trying to DeepCopyJSON the object that panics with error panic: cannot deep copy uint64. (e.g. convert the object to unstructured.Unstructured and then call unstructuredObj.DeepCopy())

Upstream issue: kubernetes/kubernetes#62769


This PR adjusts the uint fields to int32. This means that we can fully cover the IPv4 address family, but we impose limitations for the IPv6 address family since the gatewayIndex and PerNodeNetworkPrefix can go up to 2^32.

@vasrem vasrem requested review from ykulazhenkov and rollandf and removed request for ykulazhenkov August 6, 2024 06:30
@vasrem vasrem force-pushed the bugfix/use-int-instead-of-uint branch from f069e36 to 2acb88f Compare August 6, 2024 06:36
@coveralls
Copy link

Coverage Status

coverage: 69.594% (+0.08%) from 69.514%
when pulling 2acb88f on vasrem:bugfix/use-int-instead-of-uint
into ef6ff29 on Mellanox:main.

@ykulazhenkov ykulazhenkov merged commit e033180 into Mellanox:main Aug 6, 2024
9 checks passed
@vasrem vasrem deleted the bugfix/use-int-instead-of-uint branch August 6, 2024 07:31
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.

4 participants