From 3ed2afae08c602221e2cd9ed94236d072d246a34 Mon Sep 17 00:00:00 2001 From: dmitri Date: Thu, 25 Feb 2021 11:52:16 +0100 Subject: [PATCH] Remove proto-based ServerV2 implementation of DeepCopy in favor of the manual implementation to avoid issues with proto-based type merge panics. Fixes https://github.com/gravitational/teleport/issues/5691. --- api/types/authority.go | 18 ------------- api/types/resource.go | 31 ---------------------- api/types/server.go | 14 ++++++++-- lib/services/server.go | 48 +++++------------------------------ lib/services/servers_test.go | 2 +- lib/services/services_test.go | 2 +- 6 files changed, 21 insertions(+), 94 deletions(-) diff --git a/api/types/authority.go b/api/types/authority.go index 6213dba9a7a..1b3e71e640c 100644 --- a/api/types/authority.go +++ b/api/types/authority.go @@ -24,7 +24,6 @@ import ( "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/utils" - "github.com/gogo/protobuf/proto" "github.com/gravitational/trace" ) @@ -505,23 +504,6 @@ func (r *Rotation) CheckAndSetDefaults() error { return nil } -// Merge overwrites r from src and -// is part of support for cloning Server values -// using proto.Clone. -// -// Note: this does not implement the full Merger interface, -// specifically, it assumes that r is zero value. -// See https://github.com/gogo/protobuf/blob/v1.3.1/proto/clone.go#L58-L60 -// -// Implements proto.Merger -func (r *Rotation) Merge(src proto.Message) { - s, ok := src.(*Rotation) - if !ok { - return - } - *r = *s -} - // GenerateSchedule generates schedule based on the time period, using // even time periods between rotation phases. func GenerateSchedule(now time.Time, gracePeriod time.Duration) (*RotationSchedule, error) { diff --git a/api/types/resource.go b/api/types/resource.go index 1196872dedd..8eb5646dc19 100644 --- a/api/types/resource.go +++ b/api/types/resource.go @@ -23,7 +23,6 @@ import ( "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/utils" - "github.com/gogo/protobuf/proto" "github.com/gravitational/trace" ) @@ -205,36 +204,6 @@ func (m *Metadata) CheckAndSetDefaults() error { return nil } -// Merge overwrites r from src and -// is part of support for cloning Server values -// using proto.Clone. -// -// Note: this does not implement the full Merger interface, -// specifically, it assumes that r is zero value. -// See https://github.com/gogo/protobuf/blob/v1.3.1/proto/clone.go#L58-L60 -// -// Implements proto.Merger -func (m *Metadata) Merge(src proto.Message) { - metadata, ok := src.(*Metadata) - if !ok { - return - } - *m = *metadata - // Manually clone expiry timestamp as proto.Clone - // cannot cope with values that contain unexported - // attributes (as time.Time does) - if metadata.Expires != nil { - expires := *metadata.Expires - m.Expires = &expires - } - if len(metadata.Labels) != 0 { - m.Labels = make(map[string]string) - for k, v := range metadata.Labels { - m.Labels[k] = v - } - } -} - // LabelPattern is a regexp that describes a valid label key const LabelPattern = `^[a-zA-Z/.0-9_*-]+$` diff --git a/api/types/server.go b/api/types/server.go index 10b3ce0c1ee..951896f0ae8 100644 --- a/api/types/server.go +++ b/api/types/server.go @@ -25,7 +25,6 @@ import ( "github.com/gravitational/teleport/api/utils" - "github.com/gogo/protobuf/proto" "github.com/gravitational/trace" ) @@ -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 + if s.Metadata.Expires != nil { + expires := *s.Metadata.Expires + newServer.Metadata.Expires = &expires + } + if len(s.Metadata.Labels) != 0 { + newServer.Metadata.Labels = make(map[string]string) + for k, v := range s.Metadata.Labels { + newServer.Metadata.Labels[k] = v + } + } + return &newServer } // CommandLabel is a label that has a value as a result of the diff --git a/lib/services/server.go b/lib/services/server.go index e765f4ffa44..f835589bb8b 100644 --- a/lib/services/server.go +++ b/lib/services/server.go @@ -393,47 +393,6 @@ func GetServerSchema() string { return fmt.Sprintf(V2SchemaTemplate, MetadataSchema, fmt.Sprintf(ServerSpecV2Schema, RotationSchema), DefaultDefinitions) } -// UnmarshalServerResource unmarshals role from JSON or YAML, -// sets defaults and checks the schema -func UnmarshalServerResource(data []byte, kind string, cfg *MarshalConfig) (Server, error) { - if len(data) == 0 { - return nil, trace.BadParameter("missing server data") - } - - var h ResourceHeader - err := utils.FastUnmarshal(data, &h) - if err != nil { - return nil, trace.Wrap(err) - } - - switch h.Version { - case V2: - var s ServerV2 - - if cfg.SkipValidation { - if err := utils.FastUnmarshal(data, &s); err != nil { - return nil, trace.BadParameter(err.Error()) - } - } else { - if err := utils.UnmarshalWithSchema(GetServerSchema(), &s, data); err != nil { - return nil, trace.BadParameter(err.Error()) - } - } - s.Kind = kind - if err := s.CheckAndSetDefaults(); err != nil { - return nil, trace.Wrap(err) - } - if cfg.ID != 0 { - s.SetResourceID(cfg.ID) - } - if !cfg.Expires.IsZero() { - s.SetExpiry(cfg.Expires) - } - return &s, nil - } - return nil, trace.BadParameter("server resource version %q is not supported", h.Version) -} - // UnmarshalServer unmarshals the Server resource from JSON. func UnmarshalServer(bytes []byte, kind string, opts ...MarshalOption) (Server, error) { cfg, err := CollectOptions(opts) @@ -473,6 +432,13 @@ func UnmarshalServer(bytes []byte, kind string, opts ...MarshalOption) (Server, if !cfg.Expires.IsZero() { s.SetExpiry(cfg.Expires) } + if s.Metadata.Expires != nil { + utils.UTC(s.Metadata.Expires) + } + // Force the timestamps to UTC for consistency. + // See https://github.com/gogo/protobuf/issues/519 for details on issues this causes for proto.Clone + utils.UTC(&s.Spec.Rotation.Started) + utils.UTC(&s.Spec.Rotation.LastRotated) return &s, nil } return nil, trace.BadParameter("server resource version %q is not supported", h.Version) diff --git a/lib/services/servers_test.go b/lib/services/servers_test.go index faf944c9e5e..e06ac2fa7f7 100644 --- a/lib/services/servers_test.go +++ b/lib/services/servers_test.go @@ -200,7 +200,7 @@ func TestUnmarshalServerResourceKubernetes(t *testing.T) { } for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - got, err := UnmarshalServerResource([]byte(tt.in), KindKubeService, &MarshalConfig{}) + got, err := UnmarshalServer([]byte(tt.in), KindKubeService) require.NoError(t, err) require.Empty(t, cmp.Diff(got, tt.want)) }) diff --git a/lib/services/services_test.go b/lib/services/services_test.go index d34e84daf17..d87a9f83757 100644 --- a/lib/services/services_test.go +++ b/lib/services/services_test.go @@ -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) expires := now.Add(1 * time.Hour) srv := &ServerV2{ Kind: KindNode,