apis: replace unsigned integers in CRD fields with signed integers, add validation

[awprice]

This commit replaces the unsigned integers in the HTTPProxy and IngressRoute CRDs
with signed integers. Unsigned integers are discouraged due to API compatibility
issues. Validation rules have been added to the fields to ensure that the value
provided is always positive.

Fixes #2123

Signed-off-by: Alex Price [email protected]

[davecheney]

Thanks for working on this, same comment everywhere, if we can just say int not int64 throughout I'd be fine.

Also, is it just uint32 that (can't remember the thing that complained) will uint work?

[davecheney]

why not just int?

[davecheney]

same as above

[awprice]

Upon reading https://book.kubebuilder.io/cronjob-tutorial/api-design.html?highlight=integer#designing-an-api it also recommends not using int too and only use int32/int64. I felt like changing these everywhere would be out of scope for this change.

Should I raise a second ticket for this?

[awprice]

Thanks for working on this, same comment everywhere, if we can just say int not int64 throughout I'd be fine.

Also, is it just uint32 that (can't remember the thing that complained) will uint work?

Thanks for the quick reply! See comment #2133 (comment) about usage of int/uint.

[ash2k]

Hi all :)

From https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types :

• 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.

• All public integer fields MUST use the Go (u)int32 or Go (u)int64 types, not (u)int (which is ambiguous depending on target platform). Internal types may use (u)int.

[ash2k]

Also see kubernetes/kubernetes#62981 and kubernetes/kubernetes#63522

[youngnick]

Given that Kubernetes doesn't do straight int, this LGTM. Thanks for adding the validation.

[youngnick]

@awprice if you can fix up the merge conflicts, I'll merge this one.

[codecov]

Codecov Report

Merging #2133 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #2133   +/-   ##
=======================================
  Coverage   78.08%   78.08%           
=======================================
  Files          56       56           
  Lines        4946     4946           
=======================================
  Hits         3862     3862           
  Misses       1005     1005           
  Partials       79       79

Impacted Files	Coverage Δ
internal/dag/policy.go	96.24% <100%> (ø)	⬆️
internal/dag/builder.go	92.75% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67d4105...ebfe6bb. Read the comment docs.

[awprice]

@youngnick Thanks for the reminder. Conflicts fixed.