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.

Sign up for GitHub

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

Jump to bottom

Use UTC for timestamps consistently to avoid proto merge panics #5712

Merged
merged 3 commits into from
Mar 12, 2021
Merged

Use UTC for timestamps consistently to avoid proto merge panics #5712

merged 3 commits into from
Mar 12, 2021

Conversation

a-palchikov
Copy link
Contributor

@a-palchikov a-palchikov commented Feb 25, 2021 •
edited
Loading

Use UTC for timestamps when unmarshaling server values to avoid issues with proto-based type merge panics.

Fixes #5691.

@a-palchikov a-palchikov requested a review from awly February 25, 2021 10:58
a-palchikov
a-palchikov commented Feb 25, 2021
View reviewed changes
lib/services/services_test.go Outdated
@@ -108,7 +108,7 @@ func (s *ServicesSuite) TestLabelKeyValidation(c *check.C) {
func TestServerDeepCopy(t *testing.T) {
t.Parallel()
// setup
now := time.Date(1984, time.April, 4, 0, 0, 0, 0, time.UTC)
now := time.Date(1984, time.April, 4, 0, 0, 0, 0, time.Local)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests the proto-based implementation deficiency (if we continued to use the implementation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to use UTC again as this triggers panic.

api/types/server.go Outdated
@@ -335,7 +334,18 @@ func (s *ServerV2) CheckAndSetDefaults() error {

// DeepCopy creates a clone of this server value
func (s *ServerV2) DeepCopy() Server {
return proto.Clone(s).(*ServerV2)
newServer := *s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potential drawback is this needs attention if the server type is modified to include more pointer fields that would require cloning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to proto.Clone provided that timestamps are all UTC.

@russjones
Copy link
Contributor

@r0mant @awly Do you two mind reviewing this one?

awly
awly reviewed Feb 25, 2021
View reviewed changes
Copy link
Contributor

@awly awly left a comment

Choose a reason for hiding this comment

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

It's a real shame we can't use proto.Clone, this manual cloning adds maintenance burden and subtle bug risks.

I think the main reason is that we use gogoprotobuf generator, mixed with the official protobuf libraries. If we could use only the official one, a lot of these problems could go away.
But gogoproto adds some conveniences that would be hard to give up.

lib/services/servers_test.go
got, err := UnmarshalServerResource([]byte(tt.in), KindKubeService, &MarshalConfig{})
got, err := UnmarshalServer([]byte(tt.in), KindKubeService)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: update the test name accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in aec288b

api/types/server.go Outdated
@@ -335,7 +334,18 @@ func (s *ServerV2) CheckAndSetDefaults() error {

// DeepCopy creates a clone of this server value
func (s *ServerV2) DeepCopy() Server {
Copy link
Contributor

Choose a reason for hiding this comment

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

A few other things to copy manually:

  • s.Spec.CmdLabels
  • s.Spec.Apps
  • s.Spec.KubernetesClusters

@a-palchikov
Copy link
Contributor Author

It's a real shame we can't use proto.Clone, this manual cloning adds maintenance burden and subtle bug risks.

I think the main reason is that we use gogoprotobuf generator, mixed with the official protobuf libraries. If we could use only the official one, a lot of these problems could go away.
But gogoproto adds some conveniences that would be hard to give up.

In fact, I may have rushed things a bit - the reason I removed the proto.Clone is that I was wary of other possible sources of inconsistencies with unsupported types. The "unsupported" at this point was the time.zone which only shows when one uses a time zone instead of UTC. If we decide we can patch all places where this might be a problem - e.g. in this case, servers were marshaled with a local timezone setting for LastRotated which triggered the panic after unmarshaling and subsequent proto.Clone. The workaround was consistently using UTC for timestamps. Even the custom Merger implementation aren't necessary as previously.

@a-palchikov a-palchikov force-pushed the dmitri/5691-server-deep-copy branch from 2345412 to aec288b Compare February 26, 2021 13:40
@a-palchikov a-palchikov changed the title Remove proto-based DeepCopy implementation for server USe UTC for timestamps consistently to avoid proto merge panics Feb 26, 2021
@a-palchikov a-palchikov requested a review from awly February 26, 2021 13:43
@a-palchikov a-palchikov changed the title USe UTC for timestamps consistently to avoid proto merge panics Use UTC for timestamps consistently to avoid proto merge panics Feb 26, 2021
@a-palchikov
Remove proto-based ServerV2 implementation of DeepCopy in favor of the
bfc6c0a 
manual implementation to avoid issues with proto-based type merge
panics.

Fixes #5691.
@a-palchikov
Address review comments
c03cb31
@a-palchikov a-palchikov force-pushed the dmitri/5691-server-deep-copy branch from aec288b to c03cb31 Compare March 4, 2021 13:39
@russjones
Copy link
Contributor

@fspmarshall Can you review?

@russjones russjones enabled auto-merge (rebase) March 12, 2021 00:53
@russjones russjones merged commit 83c2808 into master Mar 12, 2021
@russjones russjones deleted the dmitri/5691-server-deep-copy branch March 12, 2021 01:05
This was referenced May 4, 2021
Merged
Merged
@espadolini espadolini mentioned this pull request Apr 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awly awly awly approved these changes

@russjones russjones russjones approved these changes

@klizhentas klizhentas Awaiting requested review from klizhentas

@r0mant r0mant Awaiting requested review from r0mant

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

panic in proto merge when deep-copying server value
3 participants
@a-palchikov @russjones @awly