From a2d9e90944c982de195f46354d5ae07acec0c0ed Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Fri, 10 Jan 2020 10:35:09 +0100 Subject: [PATCH] code review: replace mutable `SetVersion()` operation with an immutable `WithVersion()` --- pkg/mads/cache/cache_suite_test.go | 13 ++ pkg/mads/cache/snapshot.go | 20 ++-- pkg/mads/cache/snapshot_test.go | 185 +++++++++++++++++++++++++++++ pkg/util/xds/cache.go | 4 +- 4 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 pkg/mads/cache/cache_suite_test.go create mode 100644 pkg/mads/cache/snapshot_test.go diff --git a/pkg/mads/cache/cache_suite_test.go b/pkg/mads/cache/cache_suite_test.go new file mode 100644 index 000000000000..86e83bfbe0e7 --- /dev/null +++ b/pkg/mads/cache/cache_suite_test.go @@ -0,0 +1,13 @@ +package cache_test + +import ( + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +func TestCache(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Cache Suite") +} diff --git a/pkg/mads/cache/snapshot.go b/pkg/mads/cache/snapshot.go index 46f82c07dc0d..7763959ffa53 100644 --- a/pkg/mads/cache/snapshot.go +++ b/pkg/mads/cache/snapshot.go @@ -10,9 +10,9 @@ import ( ) // NewSnapshot creates a snapshot from response types and a version. -func NewSnapshot(version string, assignments []envoy_cache.Resource) Snapshot { - return Snapshot{ - MonitoringAssignments: envoy_cache.NewResources(version, assignments), +func NewSnapshot(version string, assignments map[string]envoy_cache.Resource) *Snapshot { + return &Snapshot{ + MonitoringAssignments: envoy_cache.Resources{Version: version, Items: assignments}, } } @@ -61,13 +61,19 @@ func (s *Snapshot) GetVersion(typ string) string { return "" } -// SetVersion sets the version for a resource type. -func (s *Snapshot) SetVersion(typ string, version string) { +// WithVersion creates a new snapshot with a different version for a given resource type. +func (s *Snapshot) WithVersion(typ string, version string) util_xds.Snapshot { if s == nil { - return + return nil + } + if s.GetVersion(typ) == version { + return s } switch typ { case mads.MonitoringAssignmentType: - s.MonitoringAssignments.Version = version + return &Snapshot{ + MonitoringAssignments: envoy_cache.Resources{Version: version, Items: s.MonitoringAssignments.Items}, + } } + return s } diff --git a/pkg/mads/cache/snapshot_test.go b/pkg/mads/cache/snapshot_test.go new file mode 100644 index 000000000000..a3514cc51a37 --- /dev/null +++ b/pkg/mads/cache/snapshot_test.go @@ -0,0 +1,185 @@ +package cache_test + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + envoy_cache "github.com/envoyproxy/go-control-plane/pkg/cache" + + . "github.com/Kong/kuma/pkg/mads/cache" + + observability_proto "github.com/Kong/kuma/api/observability/v1alpha1" +) + +var _ = Describe("Snapshot", func() { + + expectedType := "type.googleapis.com/kuma.observability.v1alpha1.MonitoringAssignment" + + Describe("GetSupportedTypes()", func() { + It("should always return ['type.googleapis.com/kuma.observability.v1alpha1.MonitoringAssignment']", func() { + // when + var snapshot *Snapshot + // then + Expect(snapshot.GetSupportedTypes()).To(Equal([]string{expectedType})) + + // when + snapshot = &Snapshot{} + // then + Expect(snapshot.GetSupportedTypes()).To(Equal([]string{expectedType})) + }) + }) + + Describe("Consistent()", func() { + It("should handle `nil`", func() { + // when + var snapshot *Snapshot + // then + Expect(snapshot.Consistent()).To(MatchError("nil snapshot")) + }) + + It("non-`nil` snapshot should be always consistet", func() { + // when + snapshot := NewSnapshot("v1", nil) + // then + Expect(snapshot.Consistent()).To(BeNil()) + + // when + snapshot = NewSnapshot("v2", map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + }) + // then + Expect(snapshot.Consistent()).To(BeNil()) + }) + }) + + Describe("GetResources()", func() { + It("should handle `nil`", func() { + // when + var snapshot *Snapshot + // then + Expect(snapshot.GetResources(expectedType)).To(BeNil()) + }) + + It("should return MonitoringAssignments", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + // when + snapshot := NewSnapshot("v1", assignments) + // then + Expect(snapshot.GetResources(expectedType)).To(Equal(assignments)) + }) + + It("should return `nil` for unsupported resource types", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + // when + snapshot := NewSnapshot("v1", assignments) + // then + Expect(snapshot.GetResources("unsupported type")).To(BeNil()) + }) + }) + + Describe("GetVersion()", func() { + It("should handle `nil`", func() { + // when + var snapshot *Snapshot + // then + Expect(snapshot.GetVersion(expectedType)).To(Equal("")) + }) + + It("should return proper version for a supported resource type", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + // when + snapshot := NewSnapshot("v1", assignments) + // then + Expect(snapshot.GetVersion(expectedType)).To(Equal("v1")) + }) + + It("should return an empty string for unsupported resource type", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + // when + snapshot := NewSnapshot("v1", assignments) + // then + Expect(snapshot.GetVersion("unsupported type")).To(Equal("")) + }) + }) + + Describe("WithVersion()", func() { + It("should handle `nil`", func() { + // given + var snapshot *Snapshot + // when + actual := snapshot.WithVersion(expectedType, "v1") + // then + Expect(actual).To(BeNil()) + }) + + It("should return a new snapshot if version has changed", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + snapshot := NewSnapshot("v1", assignments) + // when + actual := snapshot.WithVersion(expectedType, "v2") + // then + Expect(actual.GetVersion(expectedType)).To(Equal("v2")) + // and + Expect(actual).To(Equal(NewSnapshot("v2", assignments))) + }) + + It("should return the same snapshot if version has not changed", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + snapshot := NewSnapshot("v1", assignments) + // when + actual := snapshot.WithVersion(expectedType, "v1") + // then + Expect(actual.GetVersion(expectedType)).To(Equal("v1")) + // and + Expect(actual).To(BeIdenticalTo(snapshot)) + }) + + It("should return the same snapshot if resource type is not supported", func() { + // given + assignments := map[string]envoy_cache.Resource{ + "backend": &observability_proto.MonitoringAssignment{ + Name: "/meshes/default/dataplanes/backend", + }, + } + snapshot := NewSnapshot("v1", assignments) + // when + actual := snapshot.WithVersion("unsupported type", "v2") + // then + Expect(actual.GetVersion(expectedType)).To(Equal("v1")) + // and + Expect(actual).To(BeIdenticalTo(snapshot)) + }) + }) +}) diff --git a/pkg/util/xds/cache.go b/pkg/util/xds/cache.go index 6794f68cdb2a..398189f301f5 100644 --- a/pkg/util/xds/cache.go +++ b/pkg/util/xds/cache.go @@ -59,8 +59,8 @@ type Snapshot interface { // GetVersion returns the version for a resource type. GetVersion(typ string) string - // SetVersion sets the version for a resource type. - SetVersion(typ string, version string) + // WithVersion creates a new snapshot with a different version for a given resource type. + WithVersion(typ string, version string) Snapshot } // SnapshotCache is a snapshot-based cache that maintains a single versioned