changed Itoa to Sprintf to allow int64 value preventing 2038 overflow #12788
Conversation
Signed-off-by: Trenton VanderWert <trenton.vanderwert@gmail.com>
|
Another option https://play.golang.com/p/944m5DvpxD- |
Signed-off-by: Trenton VanderWert <trenton.vanderwert@gmail.com>
|
found and cleaned up the occurences of the overflow in the configmap file as well. I dont see any others with a quick grep so that should cover all the use cases. switched to the sprintf. note this brings "fmt" into scope. |
There was a problem hiding this comment.
I'm not a contributor or maintainer on this project so take all of this with a grain of salt.
Since there is quite a few places where this time -> string conversion occurs it might be better to encapsulate this into a method in pkg/time something like
func (t Time) FormatAsString() string
That way all of the time formatting stuff is kept together and you won't have to import the fmt package everywhere.
Extra benefit is that you would be easily be able to add unit tests to cover the issue this is fixing 😄
|
I am an absolute noob and set up another branch have been attempting to do this (but running into problems with the imports). Thinking through it I think you're right @tleed5 - there are several other useful functions in /pkg/time so it would make a lot of sense to add this as yet another helper function or method. For the sake of this PR, I don't want to do any "large" refeactoring unless instructed by the maintainers. But I might open a seperate PR for this. Thanks for the tip! |
|
ok so while trying to reproduce - I have found that I may be fundementally misunderstanding the issue here. It seems that in go int and int64 are both 64bit while running on a 64bit system. which means the original code I'm attempting to patch in this PR doesn't actually do anything here. Here are a couple go playground links showing this: |
|
Note, the force-push contained no changes. It was just used to trigger CI. |
|
@gjenkins8 In order to test this you would need to set the system clock to a time after 2038 in order to get a unix time that can't be represented as a 32-bit int. We don't have the ability to change the clock in most testing environments. |
What this PR does / why we need it:
Prevents an overflow when getting Unix time past the year 2038
Special notes for your reviewer:
I'm very open to recommendations on how to better fix this problem. We should probably replace Itoa() used for time anywhere to avoid this problem in the future :). I tested everything here: https://play.golang.com/p/y4HMAegdQ_r
closes #12786