From 4139ea3a9b56210cdc806f6609b7efaffad51624 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 11 Jun 2019 13:07:54 +0300 Subject: [PATCH 01/34] Async health check --- Gopkg.lock | 21 +++++ Gopkg.toml | 6 +- api/healthcheck/composite_indicator.go | 68 ---------------- api/healthcheck/composite_indicator_test.go | 87 --------------------- api/healthcheck/healthcheck_controller.go | 16 ++-- pkg/health/aggregation_policy.go | 19 ++++- pkg/health/ping.go | 4 +- pkg/health/types.go | 9 ++- pkg/sm/sm.go | 42 +++++++++- storage/healthcheck.go | 26 ++++-- storage/interfaces.go | 14 +--- storage/postgres/storage.go | 2 +- 12 files changed, 120 insertions(+), 194 deletions(-) delete mode 100644 api/healthcheck/composite_indicator.go delete mode 100644 api/healthcheck/composite_indicator_test.go diff --git a/Gopkg.lock b/Gopkg.lock index 3d8c4cd45..95199d98a 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -8,6 +8,25 @@ pruneopts = "UT" revision = "852fc940e4b9b895dc144b88ee8f6e39228127b0" +[[projects]] + digest = "1:12156f9d8523585d9d875a03c4b5c6c6374f7370716cd814aa6781732ac942c5" + name = "github.com/InVisionApp/go-health" + packages = [ + ".", + "checkers", + ] + pruneopts = "UT" + revision = "06e1878ab5a8acf6e841861c6cbfcc4d3036add0" + version = "v2.1.0" + +[[projects]] + digest = "1:137a441b9903a0179439f1c497406765c3d3758a0f1e2922174dc61ae3c21fe6" + name = "github.com/InVisionApp/go-logger" + packages = ["."] + pruneopts = "UT" + revision = "c377c6c3f6a48a6366517e86f77bafea18454ff1" + version = "v1.0.1" + [[projects]] digest = "1:f780d408067189c4c42b53f7bb24ebf8fd2a1e4510b813ed6e79dd6563e38cc5" name = "github.com/ajg/form" @@ -680,6 +699,8 @@ analyzer-version = 1 input-imports = [ "github.com/DATA-DOG/go-sqlmock", + "github.com/InVisionApp/go-health", + "github.com/InVisionApp/go-health/checkers", "github.com/benjamintf1/unmarshalledmatchers", "github.com/cloudfoundry-community/go-cfenv", "github.com/coreos/go-oidc", diff --git a/Gopkg.toml b/Gopkg.toml index 9a361e541..4b182bedb 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -108,4 +108,8 @@ # Refer to issue https://github.com/golang/dep/issues/1799 [[override]] name = "gopkg.in/fsnotify.v1" -source = "https://github.com/fsnotify/fsnotify.git" \ No newline at end of file +source = "https://github.com/fsnotify/fsnotify.git" + +[[constraint]] + name = "github.com/InVisionApp/go-health" + version = "v2.1.0" \ No newline at end of file diff --git a/api/healthcheck/composite_indicator.go b/api/healthcheck/composite_indicator.go deleted file mode 100644 index 1e2a9962c..000000000 --- a/api/healthcheck/composite_indicator.go +++ /dev/null @@ -1,68 +0,0 @@ -/* - * Copyright 2018 The Service Manager Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package healthcheck - -import ( - "strings" - - "github.com/Peripli/service-manager/pkg/health" -) - -const defaultCompositeName = "composite" - -// compositeIndicator aggregates multiple health indicators and provides one detailed health -type compositeIndicator struct { - aggregationPolicy health.AggregationPolicy - indicators []health.Indicator -} - -// newCompositeIndicator returns a new compositeIndicator for the provided health indicators -func newCompositeIndicator(indicators []health.Indicator, aggregator health.AggregationPolicy) health.Indicator { - return &compositeIndicator{ - aggregationPolicy: aggregator, - indicators: indicators, - } -} - -// Name returns the name of the compositeIndicator -func (i *compositeIndicator) Name() string { - if len(i.indicators) == 0 { - return defaultCompositeName - } - return aggregateIndicatorNames(i.indicators) -} - -// Health returns the aggregated health of all health indicators -func (i *compositeIndicator) Health() *health.Health { - healths := make(map[string]*health.Health) - for _, indicator := range i.indicators { - healths[indicator.Name()] = indicator.Health() - } - return i.aggregationPolicy.Apply(healths) -} - -func aggregateIndicatorNames(indicators []health.Indicator) string { - indicatorsCnt := len(indicators) - builder := strings.Builder{} - for index, indicator := range indicators { - builder.WriteString(indicator.Name()) - if index < indicatorsCnt-1 { - builder.WriteString(", ") - } - } - return builder.String() -} diff --git a/api/healthcheck/composite_indicator_test.go b/api/healthcheck/composite_indicator_test.go deleted file mode 100644 index d27df5259..000000000 --- a/api/healthcheck/composite_indicator_test.go +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Copyright 2018 The Service Manager Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package healthcheck - -import ( - "testing" - - "github.com/Peripli/service-manager/pkg/health/healthfakes" - - "github.com/Peripli/service-manager/pkg/health" - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -func TestNewCompositeIndicator(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Healthcheck Suite") -} - -var _ = Describe("Healthcheck Composite indicator", func() { - - Context("New Composite indicator", func() { - When("No indicators are provided ", func() { - It("Has default name", func() { - indicator := newCompositeIndicator(nil, nil) - Expect(indicator.Name()).To(Equal(defaultCompositeName)) - }) - }) - - When("Has indicators", func() { - It("Has the name of the indicator", func() { - fakeIndicator1 := &healthfakes.FakeIndicator{} - fakeIndicator1.NameReturns("fake1") - - fakeIndicator2 := &healthfakes.FakeIndicator{} - fakeIndicator2.NameReturns("fake2") - indicators := []health.Indicator{fakeIndicator1, fakeIndicator2} - indicator := newCompositeIndicator(indicators, nil) - - Expect(indicator.Name()).To(Equal(aggregateIndicatorNames(indicators))) - }) - }) - }) - - When("Checking health", func() { - Context("With empty indicators", func() { - It("Returns unknown status", func() { - indicator := newCompositeIndicator(nil, &health.DefaultAggregationPolicy{}) - h := indicator.Health() - Expect(h.Status).To(Equal(health.StatusUnknown)) - Expect(h.Details["error"]).ToNot(BeNil()) - }) - }) - - Context("With provided indicators", func() { - It("Aggregates the healths", func() { - testIndicator := &healthfakes.FakeIndicator{} - testIndicator.HealthReturns(health.New().Up()) - testIndicator.NameReturns("fake") - - aggregationPolicy := &healthfakes.FakeAggregationPolicy{} - defaultAggregationPolicy := &health.DefaultAggregationPolicy{} - aggregationPolicy.ApplyStub = defaultAggregationPolicy.Apply - - indicator := newCompositeIndicator([]health.Indicator{testIndicator}, aggregationPolicy) - invocationsCnt := aggregationPolicy.ApplyCallCount() - health := indicator.Health() - Expect(aggregationPolicy.ApplyCallCount()).To(Equal(invocationsCnt + 1)) - Expect(health.Details[testIndicator.Name()]).ToNot(BeNil()) - }) - }) - }) -}) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index 90f7ddcbf..de811333a 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -17,23 +17,26 @@ package healthcheck import ( + h "github.com/InVisionApp/go-health" + "github.com/Peripli/service-manager/pkg/health" "github.com/Peripli/service-manager/pkg/util" "net/http" - "github.com/Peripli/service-manager/pkg/health" "github.com/Peripli/service-manager/pkg/log" "github.com/Peripli/service-manager/pkg/web" ) // controller platform controller type controller struct { - indicator health.Indicator + health h.IHealth + aggPolicy health.AggregationPolicy } // NewController returns a new healthcheck controller with the given indicators and aggregation policy -func NewController(indicators []health.Indicator, aggregator health.AggregationPolicy) web.Controller { +func NewController(health h.IHealth, aggPolict health.AggregationPolicy) web.Controller { return &controller{ - indicator: newCompositeIndicator(indicators, aggregator), + health: health, + aggPolicy: aggPolict, } } @@ -41,8 +44,9 @@ func NewController(indicators []health.Indicator, aggregator health.AggregationP func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { ctx := r.Context() logger := log.C(ctx) - logger.Debugf("Performing health check with %s...", c.indicator.Name()) - healthResult := c.indicator.Health() + logger.Debugf("Performing health check...") + healthState, _, _ := c.health.State() + healthResult := c.aggPolicy.Apply(healthState) var status int if healthResult.Status == health.StatusUp { status = http.StatusOK diff --git a/pkg/health/aggregation_policy.go b/pkg/health/aggregation_policy.go index 3fba18ec1..15b4aed22 100644 --- a/pkg/health/aggregation_policy.go +++ b/pkg/health/aggregation_policy.go @@ -16,26 +16,39 @@ package health +import "github.com/InVisionApp/go-health" + // DefaultAggregationPolicy aggregates the healths by constructing a new Health based on the given // where the overall health status is negative if one of the healths is negative and positive if all are positive type DefaultAggregationPolicy struct { } // Apply aggregates the given healths -func (*DefaultAggregationPolicy) Apply(healths map[string]*Health) *Health { +func (*DefaultAggregationPolicy) Apply(healths map[string]health.State) *Health { if len(healths) == 0 { return New().WithDetail("error", "no health indicators registered").Unknown() } overallStatus := StatusUp for _, health := range healths { - if health.Status == StatusDown { + if health.Status == "failed" && health.ContiguousFailures > 3 { overallStatus = StatusDown break } } details := make(map[string]interface{}) for k, v := range healths { - details[k] = v + details[k] = convertStatus(v.Status) } return New().WithStatus(overallStatus).WithDetails(details) } + +func convertStatus(status string) Status { + switch status { + case "ok": + return StatusUp + case "failed": + return StatusDown + default: + return StatusUnknown + } +} diff --git a/pkg/health/ping.go b/pkg/health/ping.go index c145376ea..6331a89fb 100644 --- a/pkg/health/ping.go +++ b/pkg/health/ping.go @@ -24,6 +24,6 @@ func (*pingIndicator) Name() string { return "ping" } -func (*pingIndicator) Health() *Health { - return New().Up() +func (*pingIndicator) Status() (interface{}, error) { + return nil, nil } diff --git a/pkg/health/types.go b/pkg/health/types.go index 6ed339b17..f9cff7243 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -16,6 +16,8 @@ package health +import "github.com/InVisionApp/go-health" + // Status represents the overall health status of a component type Status string @@ -91,15 +93,16 @@ func (h *Health) WithDetails(details map[string]interface{}) *Health { type Indicator interface { // Name returns the name of the component Name() string - // Health returns the health of the component - Health() *Health + + // Status returns the health information of the component + Status() (interface{}, error) } // AggregationPolicy is an interface to provide aggregated health information //go:generate counterfeiter . AggregationPolicy type AggregationPolicy interface { // Apply processes the given healths to build a single health - Apply(healths map[string]*Health) *Health + Apply(healths map[string]health.State) *Health } // NewDefaultRegistry returns a default health registry with a single ping indicator and a default aggregation policy diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 4cf1e70ed..3059f48ea 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -21,9 +21,11 @@ import ( "crypto/tls" "database/sql" "fmt" + health "github.com/InVisionApp/go-health" "net" "net/http" "sync" + "time" "github.com/Peripli/service-manager/api/osb" @@ -132,7 +134,12 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( return nil, fmt.Errorf("error creating core api: %s", err) } - API.HealthIndicators = append(API.HealthIndicators, &storage.HealthIndicator{Pinger: storage.PingFunc(smStorage.Ping)}) + storageHealthIndicator, err := storage.NewStorageHealthIndicator(storage.PingFunc(smStorage.Ping)) + if err != nil { + return nil, fmt.Errorf("error creating storage health indicator: %s", err) + } + + API.HealthIndicators = append(API.HealthIndicators, storageHealthIndicator) notificationCleaner := &storage.NotificationCleaner{ Storage: interceptableRepository, @@ -149,6 +156,11 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( cfg: cfg, } + err = smb.installHealth() + if err != nil { + return nil, fmt.Errorf("error adding health chech to sm: %s", err) + } + // Register default interceptors that represent the core SM business logic smb. WithCreateInterceptorProvider(types.ServiceBrokerType, &interceptors.BrokerCreateCatalogInterceptorProvider{ @@ -189,10 +201,32 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager { } } -func (smb *ServiceManagerBuilder) installHealth() { - if len(smb.HealthIndicators) > 0 { - smb.RegisterControllers(healthcheck.NewController(smb.HealthIndicators, smb.HealthAggregationPolicy)) +func (smb *ServiceManagerBuilder) installHealth() error { + if len(smb.HealthIndicators) == 0 { + return nil + } + + h := health.New() + + for _, indicator := range smb.HealthIndicators { + err := h.AddCheck(&health.Config{ + Name: indicator.Name(), + Checker: indicator, + Interval: time.Duration(1) * time.Minute, + }) + + if err != nil { + return err + } } + smb.RegisterControllers(healthcheck.NewController(h, smb.HealthAggregationPolicy)) + + err := h.Start() + if err != nil { + return err + } + + return nil } // Run starts the Service Manager diff --git a/storage/healthcheck.go b/storage/healthcheck.go index 3d70320ca..3d6988e45 100644 --- a/storage/healthcheck.go +++ b/storage/healthcheck.go @@ -16,11 +16,14 @@ package storage -import "github.com/Peripli/service-manager/pkg/health" +import ( + "github.com/InVisionApp/go-health/checkers" + "github.com/Peripli/service-manager/pkg/health" +) // HealthIndicator returns a new indicator for the storage type HealthIndicator struct { - Pinger Pinger + checkers.SQL } // Name returns the name of the storage component @@ -28,12 +31,19 @@ func (i *HealthIndicator) Name() string { return "storage" } -// Health returns the health of the storage component -func (i *HealthIndicator) Health() *health.Health { - err := i.Pinger.Ping() - healthz := health.New() +func NewStorageHealthIndicator(pingFunc PingFunc) (health.Indicator, error) { + sqlConfig := &checkers.SQLConfig{ + Pinger: pingFunc, + } + sqlChecker, err := checkers.NewSQL(sqlConfig) if err != nil { - return healthz.WithError(err).WithDetail("message", "TransactionalRepository ping failed") + return nil, err + } + + indicator := &HealthIndicator{ + SQL: *sqlChecker, } - return healthz.Up() + + return indicator, nil + } diff --git a/storage/interfaces.go b/storage/interfaces.go index 508324e42..9d047e55a 100644 --- a/storage/interfaces.go +++ b/storage/interfaces.go @@ -135,19 +135,12 @@ type OpenCloser interface { Close() error } -// Pinger allows pinging the storage to check liveliness -//go:generate counterfeiter . Pinger -type Pinger interface { - // Ping verifies a connection to the database is still alive, establishing a connection if necessary. - Ping() error -} - // PingFunc is an adapter that allows to use regular functions as Pinger -type PingFunc func() error +type PingFunc func(context.Context) error // Ping allows PingFunc to act as a Pinger -func (mf PingFunc) Ping() error { - return mf() +func (mf PingFunc) PingContext(ctx context.Context) error { + return mf(ctx) } type Repository interface { @@ -182,7 +175,6 @@ type TransactionalRepositoryDecorator func(TransactionalRepository) (Transaction //go:generate counterfeiter . Storage type Storage interface { OpenCloser - Pinger TransactionalRepository Introduce(entity Entity) diff --git a/storage/postgres/storage.go b/storage/postgres/storage.go index 9072977f0..4fe2ab9f5 100644 --- a/storage/postgres/storage.go +++ b/storage/postgres/storage.go @@ -138,7 +138,7 @@ func (ps *Storage) updateSchema(migrationsURL, pgDriverName string) error { return err } -func (ps *Storage) Ping() error { +func (ps *Storage) Ping(ctx context.Context) error { ps.checkOpen() return ps.state.Get() } From 144ea916b3527bd177c6a7ed090688fdc35fb8e9 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 11 Jun 2019 16:01:25 +0300 Subject: [PATCH 02/34] Tests adaptation --- api/healthcheck/healthcheck_controller.go | 4 +- .../healthcheck_controller_test.go | 43 ++++- pkg/health/aggregation_policy_test.go | 21 ++- .../healthfakes/fake_aggregation_policy.go | 13 +- pkg/health/healthfakes/fake_indicator.go | 165 ------------------ pkg/health/registry_test.go | 7 +- pkg/health/types.go | 1 - storage/healthcheck_test.go | 33 ++-- storage/postgres/storage_test.go | 2 +- storage/storagefakes/fake_pinger.go | 101 ----------- storage/storagefakes/fake_storage.go | 64 ------- 11 files changed, 77 insertions(+), 377 deletions(-) delete mode 100644 pkg/health/healthfakes/fake_indicator.go delete mode 100644 storage/storagefakes/fake_pinger.go diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index de811333a..fa1db8076 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -33,10 +33,10 @@ type controller struct { } // NewController returns a new healthcheck controller with the given indicators and aggregation policy -func NewController(health h.IHealth, aggPolict health.AggregationPolicy) web.Controller { +func NewController(health h.IHealth, aggPolicy health.AggregationPolicy) web.Controller { return &controller{ health: health, - aggPolicy: aggPolict, + aggPolicy: aggPolicy, } } diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index 6d39f3a3c..f7e049ae1 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -17,13 +17,12 @@ package healthcheck import ( "fmt" + h "github.com/InVisionApp/go-health" + "github.com/Peripli/service-manager/pkg/health" + "github.com/Peripli/service-manager/pkg/health/healthfakes" "net/http" "testing" - "github.com/Peripli/service-manager/pkg/health/healthfakes" - - "github.com/Peripli/service-manager/pkg/health" - "github.com/Peripli/service-manager/pkg/web" . "github.com/onsi/ginkgo" @@ -70,9 +69,39 @@ var _ = Describe("Healthcheck controller", func() { }) func createController(status health.Status) *controller { - indicator := &healthfakes.FakeIndicator{} - indicator.HealthReturns(health.New().WithStatus(status)) + aggPolicy := &healthfakes.FakeAggregationPolicy{} + aggPolicy.ApplyReturns(&health.Health{Status: status}) return &controller{ - indicator: indicator, + health: HealthFake{}, + aggPolicy: aggPolicy, } } + +type HealthFake struct { + state map[string]h.State + failed bool + err error +} + +func (hf HealthFake) AddChecks(cfgs []*h.Config) error { + return nil +} + +func (hf HealthFake) AddCheck(cfg *h.Config) error { + return nil +} + +func (hf HealthFake) Start() error { + return nil +} + +func (hf HealthFake) Stop() error { + return nil +} + +func (hf HealthFake) State() (map[string]h.State, bool, error) { + return hf.state, hf.failed, hf.err +} +func (hf HealthFake) Failed() bool { + return hf.failed +} diff --git a/pkg/health/aggregation_policy_test.go b/pkg/health/aggregation_policy_test.go index da9c0d365..048129d0f 100644 --- a/pkg/health/aggregation_policy_test.go +++ b/pkg/health/aggregation_policy_test.go @@ -17,6 +17,7 @@ package health import ( + "github.com/InVisionApp/go-health" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -24,12 +25,12 @@ import ( var _ = Describe("Healthcheck AggregationPolicy", func() { aggregationPolicy := &DefaultAggregationPolicy{} - var healths map[string]*Health + var healths map[string]health.State BeforeEach(func() { - healths = map[string]*Health{ - "test1": New().Up(), - "test2": New().Up(), + healths = map[string]health.State{ + "test1": {Status: "ok"}, + "test2": {Status: "ok"}, } }) @@ -43,12 +44,20 @@ var _ = Describe("Healthcheck AggregationPolicy", func() { When("At least one health is DOWN", func() { It("Returns DOWN", func() { - healths["test3"] = New().Down() + healths["test3"] = health.State{Status: "failed", ContiguousFailures: 4} aggregatedHealth := aggregationPolicy.Apply(healths) Expect(aggregatedHealth.Status).To(Equal(StatusDown)) }) }) + When("There is DOWN healths but not more than 3 times in a row", func() { + It("Returns UP", func() { + healths["test3"] = health.State{Status: "failed"} + aggregatedHealth := aggregationPolicy.Apply(healths) + Expect(aggregatedHealth.Status).To(Equal(StatusUp)) + }) + }) + When("All healths are UP", func() { It("Returns UP", func() { aggregatedHealth := aggregationPolicy.Apply(healths) @@ -60,7 +69,7 @@ var _ = Describe("Healthcheck AggregationPolicy", func() { It("Includes them as overall details", func() { aggregatedHealth := aggregationPolicy.Apply(healths) for name, h := range healths { - Expect(aggregatedHealth.Details[name]).To(Equal(h)) + Expect(aggregatedHealth.Details[name]).To(Equal(convertStatus(h.Status))) } }) }) diff --git a/pkg/health/healthfakes/fake_aggregation_policy.go b/pkg/health/healthfakes/fake_aggregation_policy.go index 45a1f9504..f11b40a81 100644 --- a/pkg/health/healthfakes/fake_aggregation_policy.go +++ b/pkg/health/healthfakes/fake_aggregation_policy.go @@ -4,14 +4,15 @@ package healthfakes import ( "sync" + healtha "github.com/InVisionApp/go-health" "github.com/Peripli/service-manager/pkg/health" ) type FakeAggregationPolicy struct { - ApplyStub func(map[string]*health.Health) *health.Health + ApplyStub func(map[string]healtha.State) *health.Health applyMutex sync.RWMutex applyArgsForCall []struct { - arg1 map[string]*health.Health + arg1 map[string]healtha.State } applyReturns struct { result1 *health.Health @@ -23,11 +24,11 @@ type FakeAggregationPolicy struct { invocationsMutex sync.RWMutex } -func (fake *FakeAggregationPolicy) Apply(arg1 map[string]*health.Health) *health.Health { +func (fake *FakeAggregationPolicy) Apply(arg1 map[string]healtha.State) *health.Health { fake.applyMutex.Lock() ret, specificReturn := fake.applyReturnsOnCall[len(fake.applyArgsForCall)] fake.applyArgsForCall = append(fake.applyArgsForCall, struct { - arg1 map[string]*health.Health + arg1 map[string]healtha.State }{arg1}) fake.recordInvocation("Apply", []interface{}{arg1}) fake.applyMutex.Unlock() @@ -47,13 +48,13 @@ func (fake *FakeAggregationPolicy) ApplyCallCount() int { return len(fake.applyArgsForCall) } -func (fake *FakeAggregationPolicy) ApplyCalls(stub func(map[string]*health.Health) *health.Health) { +func (fake *FakeAggregationPolicy) ApplyCalls(stub func(map[string]healtha.State) *health.Health) { fake.applyMutex.Lock() defer fake.applyMutex.Unlock() fake.ApplyStub = stub } -func (fake *FakeAggregationPolicy) ApplyArgsForCall(i int) map[string]*health.Health { +func (fake *FakeAggregationPolicy) ApplyArgsForCall(i int) map[string]healtha.State { fake.applyMutex.RLock() defer fake.applyMutex.RUnlock() argsForCall := fake.applyArgsForCall[i] diff --git a/pkg/health/healthfakes/fake_indicator.go b/pkg/health/healthfakes/fake_indicator.go deleted file mode 100644 index 5959dc549..000000000 --- a/pkg/health/healthfakes/fake_indicator.go +++ /dev/null @@ -1,165 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package healthfakes - -import ( - "sync" - - "github.com/Peripli/service-manager/pkg/health" -) - -type FakeIndicator struct { - HealthStub func() *health.Health - healthMutex sync.RWMutex - healthArgsForCall []struct { - } - healthReturns struct { - result1 *health.Health - } - healthReturnsOnCall map[int]struct { - result1 *health.Health - } - NameStub func() string - nameMutex sync.RWMutex - nameArgsForCall []struct { - } - nameReturns struct { - result1 string - } - nameReturnsOnCall map[int]struct { - result1 string - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeIndicator) Health() *health.Health { - fake.healthMutex.Lock() - ret, specificReturn := fake.healthReturnsOnCall[len(fake.healthArgsForCall)] - fake.healthArgsForCall = append(fake.healthArgsForCall, struct { - }{}) - fake.recordInvocation("Health", []interface{}{}) - fake.healthMutex.Unlock() - if fake.HealthStub != nil { - return fake.HealthStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.healthReturns - return fakeReturns.result1 -} - -func (fake *FakeIndicator) HealthCallCount() int { - fake.healthMutex.RLock() - defer fake.healthMutex.RUnlock() - return len(fake.healthArgsForCall) -} - -func (fake *FakeIndicator) HealthCalls(stub func() *health.Health) { - fake.healthMutex.Lock() - defer fake.healthMutex.Unlock() - fake.HealthStub = stub -} - -func (fake *FakeIndicator) HealthReturns(result1 *health.Health) { - fake.healthMutex.Lock() - defer fake.healthMutex.Unlock() - fake.HealthStub = nil - fake.healthReturns = struct { - result1 *health.Health - }{result1} -} - -func (fake *FakeIndicator) HealthReturnsOnCall(i int, result1 *health.Health) { - fake.healthMutex.Lock() - defer fake.healthMutex.Unlock() - fake.HealthStub = nil - if fake.healthReturnsOnCall == nil { - fake.healthReturnsOnCall = make(map[int]struct { - result1 *health.Health - }) - } - fake.healthReturnsOnCall[i] = struct { - result1 *health.Health - }{result1} -} - -func (fake *FakeIndicator) Name() string { - fake.nameMutex.Lock() - ret, specificReturn := fake.nameReturnsOnCall[len(fake.nameArgsForCall)] - fake.nameArgsForCall = append(fake.nameArgsForCall, struct { - }{}) - fake.recordInvocation("Name", []interface{}{}) - fake.nameMutex.Unlock() - if fake.NameStub != nil { - return fake.NameStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.nameReturns - return fakeReturns.result1 -} - -func (fake *FakeIndicator) NameCallCount() int { - fake.nameMutex.RLock() - defer fake.nameMutex.RUnlock() - return len(fake.nameArgsForCall) -} - -func (fake *FakeIndicator) NameCalls(stub func() string) { - fake.nameMutex.Lock() - defer fake.nameMutex.Unlock() - fake.NameStub = stub -} - -func (fake *FakeIndicator) NameReturns(result1 string) { - fake.nameMutex.Lock() - defer fake.nameMutex.Unlock() - fake.NameStub = nil - fake.nameReturns = struct { - result1 string - }{result1} -} - -func (fake *FakeIndicator) NameReturnsOnCall(i int, result1 string) { - fake.nameMutex.Lock() - defer fake.nameMutex.Unlock() - fake.NameStub = nil - if fake.nameReturnsOnCall == nil { - fake.nameReturnsOnCall = make(map[int]struct { - result1 string - }) - } - fake.nameReturnsOnCall[i] = struct { - result1 string - }{result1} -} - -func (fake *FakeIndicator) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.healthMutex.RLock() - defer fake.healthMutex.RUnlock() - fake.nameMutex.RLock() - defer fake.nameMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeIndicator) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ health.Indicator = new(FakeIndicator) diff --git a/pkg/health/registry_test.go b/pkg/health/registry_test.go index faf4d8525..d73d65e25 100644 --- a/pkg/health/registry_test.go +++ b/pkg/health/registry_test.go @@ -17,6 +17,7 @@ package health import ( + "github.com/InVisionApp/go-health" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -68,7 +69,7 @@ var _ = Describe("Healthcheck Registry", func() { type testAggregationPolicy struct { } -func (*testAggregationPolicy) Apply(healths map[string]*Health) *Health { +func (*testAggregationPolicy) Apply(healths map[string]health.State) *Health { return New().Up() } @@ -79,6 +80,6 @@ func (*testIndicator) Name() string { return "test" } -func (*testIndicator) Health() *Health { - return New().Up() +func (*testIndicator) Status() (interface{}, error) { + return nil, nil } diff --git a/pkg/health/types.go b/pkg/health/types.go index f9cff7243..fb1d09869 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -89,7 +89,6 @@ func (h *Health) WithDetails(details map[string]interface{}) *Health { } // Indicator is an interface to provide the health of a component -//go:generate counterfeiter . Indicator type Indicator interface { // Name returns the name of the component Name() string diff --git a/storage/healthcheck_test.go b/storage/healthcheck_test.go index 4d6eb8490..4b7d15a56 100644 --- a/storage/healthcheck_test.go +++ b/storage/healthcheck_test.go @@ -17,27 +17,22 @@ package storage_test import ( + "context" "fmt" - "github.com/Peripli/service-manager/pkg/health" "github.com/Peripli/service-manager/storage" - "github.com/Peripli/service-manager/storage/storagefakes" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) var _ = Describe("Healthcheck", func() { - var healthIndicator *storage.HealthIndicator - var pinger *storagefakes.FakePinger + var healthIndicator health.Indicator BeforeEach(func() { - pinger = &storagefakes.FakePinger{} - pinger.PingStub = func() error { + ping := func(ctx context.Context) error { return nil } - healthIndicator = &storage.HealthIndicator{ - Pinger: pinger, - } + healthIndicator, _ = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) }) Context("Name", func() { @@ -47,27 +42,23 @@ var _ = Describe("Healthcheck", func() { }) Context("Ping does not return error", func() { - It("Returns health status UP", func() { - healthz := healthIndicator.Health() - Expect(healthz.Status).To(Equal(health.StatusUp)) + It("Status doest not contains error", func() { + _, err := healthIndicator.Status() + Expect(err).ShouldNot(HaveOccurred()) }) }) Context("Ping returns error", func() { expectedError := fmt.Errorf("could not connect to database") BeforeEach(func() { - pinger.PingStub = func() error { + ping := func(ctx context.Context) error { return expectedError } - }) - It("Returns status DOWN", func() { - healthz := healthIndicator.Health() - Expect(healthz.Status).To(Equal(health.StatusDown)) + healthIndicator, _ = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) }) It("Contains error", func() { - healthz := healthIndicator.Health() - errorDetails := healthz.Details["error"] - Expect(errorDetails).To(Equal(expectedError)) + _, err := healthIndicator.Status() + Expect(err).Should(HaveOccurred()) + Expect(err).To(Equal(expectedError)) }) - }) }) diff --git a/storage/postgres/storage_test.go b/storage/postgres/storage_test.go index 5b0a8a351..1078313da 100644 --- a/storage/postgres/storage_test.go +++ b/storage/postgres/storage_test.go @@ -70,7 +70,7 @@ var _ = Describe("Postgres Storage", func() { Describe("Ping", func() { Context("Called with uninitialized db", func() { It("Should panic", func() { - Expect(func() { pgStorage.Ping() }).To(Panic()) + Expect(func() { pgStorage.Ping(context.Background()) }).To(Panic()) }) }) }) diff --git a/storage/storagefakes/fake_pinger.go b/storage/storagefakes/fake_pinger.go deleted file mode 100644 index a8549274c..000000000 --- a/storage/storagefakes/fake_pinger.go +++ /dev/null @@ -1,101 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package storagefakes - -import ( - "sync" - - "github.com/Peripli/service-manager/storage" -) - -type FakePinger struct { - PingStub func() error - pingMutex sync.RWMutex - pingArgsForCall []struct { - } - pingReturns struct { - result1 error - } - pingReturnsOnCall map[int]struct { - result1 error - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakePinger) Ping() error { - fake.pingMutex.Lock() - ret, specificReturn := fake.pingReturnsOnCall[len(fake.pingArgsForCall)] - fake.pingArgsForCall = append(fake.pingArgsForCall, struct { - }{}) - fake.recordInvocation("Ping", []interface{}{}) - fake.pingMutex.Unlock() - if fake.PingStub != nil { - return fake.PingStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.pingReturns - return fakeReturns.result1 -} - -func (fake *FakePinger) PingCallCount() int { - fake.pingMutex.RLock() - defer fake.pingMutex.RUnlock() - return len(fake.pingArgsForCall) -} - -func (fake *FakePinger) PingCalls(stub func() error) { - fake.pingMutex.Lock() - defer fake.pingMutex.Unlock() - fake.PingStub = stub -} - -func (fake *FakePinger) PingReturns(result1 error) { - fake.pingMutex.Lock() - defer fake.pingMutex.Unlock() - fake.PingStub = nil - fake.pingReturns = struct { - result1 error - }{result1} -} - -func (fake *FakePinger) PingReturnsOnCall(i int, result1 error) { - fake.pingMutex.Lock() - defer fake.pingMutex.Unlock() - fake.PingStub = nil - if fake.pingReturnsOnCall == nil { - fake.pingReturnsOnCall = make(map[int]struct { - result1 error - }) - } - fake.pingReturnsOnCall[i] = struct { - result1 error - }{result1} -} - -func (fake *FakePinger) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.pingMutex.RLock() - defer fake.pingMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakePinger) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ storage.Pinger = new(FakePinger) diff --git a/storage/storagefakes/fake_storage.go b/storage/storagefakes/fake_storage.go index dc16ce5de..4ec5ff95d 100644 --- a/storage/storagefakes/fake_storage.go +++ b/storage/storagefakes/fake_storage.go @@ -108,16 +108,6 @@ type FakeStorage struct { openReturnsOnCall map[int]struct { result1 error } - PingStub func() error - pingMutex sync.RWMutex - pingArgsForCall []struct { - } - pingReturns struct { - result1 error - } - pingReturnsOnCall map[int]struct { - result1 error - } UpdateStub func(context.Context, types.Object, query.LabelChanges, ...query.Criterion) (types.Object, error) updateMutex sync.RWMutex updateArgsForCall []struct { @@ -601,58 +591,6 @@ func (fake *FakeStorage) OpenReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeStorage) Ping() error { - fake.pingMutex.Lock() - ret, specificReturn := fake.pingReturnsOnCall[len(fake.pingArgsForCall)] - fake.pingArgsForCall = append(fake.pingArgsForCall, struct { - }{}) - fake.recordInvocation("Ping", []interface{}{}) - fake.pingMutex.Unlock() - if fake.PingStub != nil { - return fake.PingStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.pingReturns - return fakeReturns.result1 -} - -func (fake *FakeStorage) PingCallCount() int { - fake.pingMutex.RLock() - defer fake.pingMutex.RUnlock() - return len(fake.pingArgsForCall) -} - -func (fake *FakeStorage) PingCalls(stub func() error) { - fake.pingMutex.Lock() - defer fake.pingMutex.Unlock() - fake.PingStub = stub -} - -func (fake *FakeStorage) PingReturns(result1 error) { - fake.pingMutex.Lock() - defer fake.pingMutex.Unlock() - fake.PingStub = nil - fake.pingReturns = struct { - result1 error - }{result1} -} - -func (fake *FakeStorage) PingReturnsOnCall(i int, result1 error) { - fake.pingMutex.Lock() - defer fake.pingMutex.Unlock() - fake.PingStub = nil - if fake.pingReturnsOnCall == nil { - fake.pingReturnsOnCall = make(map[int]struct { - result1 error - }) - } - fake.pingReturnsOnCall[i] = struct { - result1 error - }{result1} -} - func (fake *FakeStorage) Update(arg1 context.Context, arg2 types.Object, arg3 query.LabelChanges, arg4 ...query.Criterion) (types.Object, error) { fake.updateMutex.Lock() ret, specificReturn := fake.updateReturnsOnCall[len(fake.updateArgsForCall)] @@ -738,8 +676,6 @@ func (fake *FakeStorage) Invocations() map[string][][]interface{} { defer fake.listMutex.RUnlock() fake.openMutex.RLock() defer fake.openMutex.RUnlock() - fake.pingMutex.RLock() - defer fake.pingMutex.RUnlock() fake.updateMutex.RLock() defer fake.updateMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} From 61157b439c73a224fe2d40b117cca06a8c0e404b Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 17 Jun 2019 15:24:28 +0300 Subject: [PATCH 03/34] Make health configurable externally --- api/healthcheck/healthcheck_controller.go | 14 ++++---- .../healthcheck_controller_test.go | 5 +-- config/config.go | 3 ++ pkg/health/aggregation_policy.go | 4 +-- pkg/health/aggregation_policy_test.go | 13 ++++---- .../healthfakes/fake_aggregation_policy.go | 18 ++++++----- pkg/health/registry_test.go | 2 +- pkg/health/types.go | 32 +++++++++++++++++-- pkg/sm/sm.go | 17 +++++----- 9 files changed, 73 insertions(+), 35 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index fa1db8076..c13d90ffb 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -28,15 +28,17 @@ import ( // controller platform controller type controller struct { - health h.IHealth - aggPolicy health.AggregationPolicy + health h.IHealth + aggPolicy health.AggregationPolicy + failuresTreshold int64 } // NewController returns a new healthcheck controller with the given indicators and aggregation policy -func NewController(health h.IHealth, aggPolicy health.AggregationPolicy) web.Controller { +func NewController(health h.IHealth, aggPolicy health.AggregationPolicy, failuresTreshold int64) web.Controller { return &controller{ - health: health, - aggPolicy: aggPolicy, + health: health, + aggPolicy: aggPolicy, + failuresTreshold: failuresTreshold, } } @@ -46,7 +48,7 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { logger := log.C(ctx) logger.Debugf("Performing health check...") healthState, _, _ := c.health.State() - healthResult := c.aggPolicy.Apply(healthState) + healthResult := c.aggPolicy.Apply(healthState, c.failuresTreshold) var status int if healthResult.Status == health.StatusUp { status = http.StatusOK diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index f7e049ae1..e4a1b7916 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -72,8 +72,9 @@ func createController(status health.Status) *controller { aggPolicy := &healthfakes.FakeAggregationPolicy{} aggPolicy.ApplyReturns(&health.Health{Status: status}) return &controller{ - health: HealthFake{}, - aggPolicy: aggPolicy, + health: HealthFake{}, + aggPolicy: aggPolicy, + failuresTreshold: 3, } } diff --git a/config/config.go b/config/config.go index f9e28f561..9a39e9025 100644 --- a/config/config.go +++ b/config/config.go @@ -23,6 +23,7 @@ import ( "github.com/Peripli/service-manager/api" "github.com/Peripli/service-manager/pkg/env" + "github.com/Peripli/service-manager/pkg/health" "github.com/Peripli/service-manager/pkg/log" "github.com/Peripli/service-manager/pkg/server" "github.com/Peripli/service-manager/pkg/ws" @@ -38,6 +39,7 @@ type Settings struct { API *api.Settings WebSocket *ws.Settings HTTPClient *httpclient.Settings + Health *health.Settings } // AddPFlags adds the SM config flags to the provided flag set @@ -55,6 +57,7 @@ func DefaultSettings() *Settings { API: api.DefaultSettings(), WebSocket: ws.DefaultSettings(), HTTPClient: httpclient.DefaultSettings(), + Health: health.DefaultSettings(), } } diff --git a/pkg/health/aggregation_policy.go b/pkg/health/aggregation_policy.go index 15b4aed22..553db911d 100644 --- a/pkg/health/aggregation_policy.go +++ b/pkg/health/aggregation_policy.go @@ -24,13 +24,13 @@ type DefaultAggregationPolicy struct { } // Apply aggregates the given healths -func (*DefaultAggregationPolicy) Apply(healths map[string]health.State) *Health { +func (*DefaultAggregationPolicy) Apply(healths map[string]health.State, failureTreshold int64) *Health { if len(healths) == 0 { return New().WithDetail("error", "no health indicators registered").Unknown() } overallStatus := StatusUp for _, health := range healths { - if health.Status == "failed" && health.ContiguousFailures > 3 { + if health.Status == "failed" && health.ContiguousFailures > failureTreshold { overallStatus = StatusDown break } diff --git a/pkg/health/aggregation_policy_test.go b/pkg/health/aggregation_policy_test.go index 048129d0f..f288dba9f 100644 --- a/pkg/health/aggregation_policy_test.go +++ b/pkg/health/aggregation_policy_test.go @@ -26,6 +26,7 @@ var _ = Describe("Healthcheck AggregationPolicy", func() { aggregationPolicy := &DefaultAggregationPolicy{} var healths map[string]health.State + var failuresTreshold int64 = 3 BeforeEach(func() { healths = map[string]health.State{ @@ -36,7 +37,7 @@ var _ = Describe("Healthcheck AggregationPolicy", func() { When("No healths are provided", func() { It("Returns UNKNOWN and an error detail", func() { - aggregatedHealth := aggregationPolicy.Apply(nil) + aggregatedHealth := aggregationPolicy.Apply(nil, failuresTreshold) Expect(aggregatedHealth.Status).To(Equal(StatusUnknown)) Expect(aggregatedHealth.Details["error"]).ToNot(BeNil()) }) @@ -45,29 +46,29 @@ var _ = Describe("Healthcheck AggregationPolicy", func() { When("At least one health is DOWN", func() { It("Returns DOWN", func() { healths["test3"] = health.State{Status: "failed", ContiguousFailures: 4} - aggregatedHealth := aggregationPolicy.Apply(healths) + aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) Expect(aggregatedHealth.Status).To(Equal(StatusDown)) }) }) - When("There is DOWN healths but not more than 3 times in a row", func() { + When("There is DOWN healths but not more than treshold times in a row", func() { It("Returns UP", func() { healths["test3"] = health.State{Status: "failed"} - aggregatedHealth := aggregationPolicy.Apply(healths) + aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) Expect(aggregatedHealth.Status).To(Equal(StatusUp)) }) }) When("All healths are UP", func() { It("Returns UP", func() { - aggregatedHealth := aggregationPolicy.Apply(healths) + aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) Expect(aggregatedHealth.Status).To(Equal(StatusUp)) }) }) When("Aggregating healths", func() { It("Includes them as overall details", func() { - aggregatedHealth := aggregationPolicy.Apply(healths) + aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) for name, h := range healths { Expect(aggregatedHealth.Details[name]).To(Equal(convertStatus(h.Status))) } diff --git a/pkg/health/healthfakes/fake_aggregation_policy.go b/pkg/health/healthfakes/fake_aggregation_policy.go index f11b40a81..69a1220cc 100644 --- a/pkg/health/healthfakes/fake_aggregation_policy.go +++ b/pkg/health/healthfakes/fake_aggregation_policy.go @@ -9,10 +9,11 @@ import ( ) type FakeAggregationPolicy struct { - ApplyStub func(map[string]healtha.State) *health.Health + ApplyStub func(map[string]healtha.State, int64) *health.Health applyMutex sync.RWMutex applyArgsForCall []struct { arg1 map[string]healtha.State + arg2 int64 } applyReturns struct { result1 *health.Health @@ -24,16 +25,17 @@ type FakeAggregationPolicy struct { invocationsMutex sync.RWMutex } -func (fake *FakeAggregationPolicy) Apply(arg1 map[string]healtha.State) *health.Health { +func (fake *FakeAggregationPolicy) Apply(arg1 map[string]healtha.State, arg2 int64) *health.Health { fake.applyMutex.Lock() ret, specificReturn := fake.applyReturnsOnCall[len(fake.applyArgsForCall)] fake.applyArgsForCall = append(fake.applyArgsForCall, struct { arg1 map[string]healtha.State - }{arg1}) - fake.recordInvocation("Apply", []interface{}{arg1}) + arg2 int64 + }{arg1, arg2}) + fake.recordInvocation("Apply", []interface{}{arg1, arg2}) fake.applyMutex.Unlock() if fake.ApplyStub != nil { - return fake.ApplyStub(arg1) + return fake.ApplyStub(arg1, arg2) } if specificReturn { return ret.result1 @@ -48,17 +50,17 @@ func (fake *FakeAggregationPolicy) ApplyCallCount() int { return len(fake.applyArgsForCall) } -func (fake *FakeAggregationPolicy) ApplyCalls(stub func(map[string]healtha.State) *health.Health) { +func (fake *FakeAggregationPolicy) ApplyCalls(stub func(map[string]healtha.State, int64) *health.Health) { fake.applyMutex.Lock() defer fake.applyMutex.Unlock() fake.ApplyStub = stub } -func (fake *FakeAggregationPolicy) ApplyArgsForCall(i int) map[string]healtha.State { +func (fake *FakeAggregationPolicy) ApplyArgsForCall(i int) (map[string]healtha.State, int64) { fake.applyMutex.RLock() defer fake.applyMutex.RUnlock() argsForCall := fake.applyArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeAggregationPolicy) ApplyReturns(result1 *health.Health) { diff --git a/pkg/health/registry_test.go b/pkg/health/registry_test.go index d73d65e25..a0d543446 100644 --- a/pkg/health/registry_test.go +++ b/pkg/health/registry_test.go @@ -69,7 +69,7 @@ var _ = Describe("Healthcheck Registry", func() { type testAggregationPolicy struct { } -func (*testAggregationPolicy) Apply(healths map[string]health.State) *Health { +func (*testAggregationPolicy) Apply(healths map[string]health.State, failuresTreshold int64) *Health { return New().Up() } diff --git a/pkg/health/types.go b/pkg/health/types.go index fb1d09869..5d5806f8f 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -16,7 +16,35 @@ package health -import "github.com/InVisionApp/go-health" +import ( + "fmt" + "github.com/InVisionApp/go-health" +) + +// Settings type to be loaded from the environment +type Settings struct { + FailuresTreshold int64 `mapstructure:"failures_treshold" description:"maximum failures in a row until component is considered down"` + Interval int64 `description:"seconds between health checks of components"` +} + +// DefaultSettings returns default values for health settings +func DefaultSettings() *Settings { + return &Settings{ + FailuresTreshold: 3, + Interval: 60, + } +} + +// Validate validates the health settings +func (s *Settings) Validate() error { + if s.FailuresTreshold <= 0 { + return fmt.Errorf("validate Settings: FailuresTreshold must be > 0") + } + if s.Interval <= 0 { + return fmt.Errorf("valudate Settings: Interval must be > 0") + } + return nil +} // Status represents the overall health status of a component type Status string @@ -101,7 +129,7 @@ type Indicator interface { //go:generate counterfeiter . AggregationPolicy type AggregationPolicy interface { // Apply processes the given healths to build a single health - Apply(healths map[string]health.State) *Health + Apply(healths map[string]health.State, failureTreshold int64) *Health } // NewDefaultRegistry returns a default health registry with a single ping indicator and a default aggregation policy diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 3059f48ea..0462953c8 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -21,7 +21,8 @@ import ( "crypto/tls" "database/sql" "fmt" - health "github.com/InVisionApp/go-health" + h "github.com/InVisionApp/go-health" + "github.com/Peripli/service-manager/pkg/health" "net" "net/http" "sync" @@ -156,7 +157,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( cfg: cfg, } - err = smb.installHealth() + err = smb.installHealth(cfg.Health) if err != nil { return nil, fmt.Errorf("error adding health chech to sm: %s", err) } @@ -201,27 +202,27 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager { } } -func (smb *ServiceManagerBuilder) installHealth() error { +func (smb *ServiceManagerBuilder) installHealth(cfg *health.Settings) error { if len(smb.HealthIndicators) == 0 { return nil } - h := health.New() + healthz := h.New() for _, indicator := range smb.HealthIndicators { - err := h.AddCheck(&health.Config{ + err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, - Interval: time.Duration(1) * time.Minute, + Interval: time.Duration(cfg.Interval) * time.Second, }) if err != nil { return err } } - smb.RegisterControllers(healthcheck.NewController(h, smb.HealthAggregationPolicy)) + smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthAggregationPolicy, cfg.FailuresTreshold)) - err := h.Start() + err := healthz.Start() if err != nil { return err } From 9f62a8120135b84ec9bbb8d8e014b51914452be9 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 17 Jun 2019 16:40:11 +0300 Subject: [PATCH 04/34] Install health later --- pkg/sm/sm.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 0462953c8..8dc6bb99b 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -187,6 +187,11 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( // Build builds the Service Manager func (smb *ServiceManagerBuilder) Build() *ServiceManager { + err := smb.installHealth() + if err != nil { + panic(err) + } + // setup server and add relevant global middleware smb.installHealth() @@ -202,7 +207,7 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager { } } -func (smb *ServiceManagerBuilder) installHealth(cfg *health.Settings) error { +func (smb *ServiceManagerBuilder) installHealth() error { if len(smb.HealthIndicators) == 0 { return nil } @@ -213,14 +218,14 @@ func (smb *ServiceManagerBuilder) installHealth(cfg *health.Settings) error { err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, - Interval: time.Duration(cfg.Interval) * time.Second, + Interval: time.Duration(smb.health.Interval) * time.Second, }) if err != nil { return err } } - smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthAggregationPolicy, cfg.FailuresTreshold)) + smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthAggregationPolicy, smb.health.FailuresTreshold)) err := healthz.Start() if err != nil { From 81ff6653695fe906156d883badbb945a665e5cd0 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 17 Jun 2019 16:49:46 +0300 Subject: [PATCH 05/34] Export ConvertStatus function --- pkg/health/aggregation_policy.go | 5 +++-- pkg/health/aggregation_policy_test.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/health/aggregation_policy.go b/pkg/health/aggregation_policy.go index 553db911d..8c73ca395 100644 --- a/pkg/health/aggregation_policy.go +++ b/pkg/health/aggregation_policy.go @@ -37,12 +37,13 @@ func (*DefaultAggregationPolicy) Apply(healths map[string]health.State, failureT } details := make(map[string]interface{}) for k, v := range healths { - details[k] = convertStatus(v.Status) + details[k] = ConvertStatus(v.Status) } return New().WithStatus(overallStatus).WithDetails(details) } -func convertStatus(status string) Status { +// ConvertStatus converts go-health status to Status +func ConvertStatus(status string) Status { switch status { case "ok": return StatusUp diff --git a/pkg/health/aggregation_policy_test.go b/pkg/health/aggregation_policy_test.go index f288dba9f..c16a24d10 100644 --- a/pkg/health/aggregation_policy_test.go +++ b/pkg/health/aggregation_policy_test.go @@ -70,7 +70,7 @@ var _ = Describe("Healthcheck AggregationPolicy", func() { It("Includes them as overall details", func() { aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) for name, h := range healths { - Expect(aggregatedHealth.Details[name]).To(Equal(convertStatus(h.Status))) + Expect(aggregatedHealth.Details[name]).To(Equal(ConvertStatus(h.Status))) } }) }) From 115e2a429f2efb737f27aee0cbd9fda463249a4c Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 17 Jun 2019 17:07:25 +0300 Subject: [PATCH 06/34] Fix health settings validation --- pkg/health/types.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/health/types.go b/pkg/health/types.go index 5d5806f8f..d0408cb73 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -37,11 +37,11 @@ func DefaultSettings() *Settings { // Validate validates the health settings func (s *Settings) Validate() error { - if s.FailuresTreshold <= 0 { - return fmt.Errorf("validate Settings: FailuresTreshold must be > 0") + if s.FailuresTreshold < 0 { + return fmt.Errorf("validate Settings: FailuresTreshold must be >= 0") } - if s.Interval <= 0 { - return fmt.Errorf("valudate Settings: Interval must be > 0") + if s.Interval < 0 { + return fmt.Errorf("valudate Settings: Interval must be >= 0") } return nil } From 37f9bc426947a2598c255541888cadec520235ef Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 18 Jun 2019 12:37:09 +0300 Subject: [PATCH 07/34] Attach logger to health --- Gopkg.lock | 8 ++++++-- pkg/sm/sm.go | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 95199d98a..9d7656807 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -20,9 +20,12 @@ version = "v2.1.0" [[projects]] - digest = "1:137a441b9903a0179439f1c497406765c3d3758a0f1e2922174dc61ae3c21fe6" + digest = "1:be2deb3cba3d2526b3692d80483f59df6462aabc875aace2db263f2664f30670" name = "github.com/InVisionApp/go-logger" - packages = ["."] + packages = [ + ".", + "shims/logrus", + ] pruneopts = "UT" revision = "c377c6c3f6a48a6366517e86f77bafea18454ff1" version = "v1.0.1" @@ -701,6 +704,7 @@ "github.com/DATA-DOG/go-sqlmock", "github.com/InVisionApp/go-health", "github.com/InVisionApp/go-health/checkers", + "github.com/InVisionApp/go-logger/shims/logrus", "github.com/benjamintf1/unmarshalledmatchers", "github.com/cloudfoundry-community/go-cfenv", "github.com/coreos/go-oidc", diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 8dc6bb99b..0bd7af54a 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -22,6 +22,7 @@ import ( "database/sql" "fmt" h "github.com/InVisionApp/go-health" + l "github.com/InVisionApp/go-logger/shims/logrus" "github.com/Peripli/service-manager/pkg/health" "net" "net/http" @@ -213,6 +214,8 @@ func (smb *ServiceManagerBuilder) installHealth() error { } healthz := h.New() + logger := log.C(smb.ctx).Logger + healthz.Logger = l.New(logger) for _, indicator := range smb.HealthIndicators { err := healthz.AddCheck(&h.Config{ From 4f21ed987ff6accbf464b0a91e4af4a40d0984e9 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 18 Jun 2019 18:09:20 +0300 Subject: [PATCH 08/34] Address PR comments --- config/config.go | 2 +- pkg/health/types.go | 9 ++- pkg/sm/sm.go | 17 ++-- storage/interfaces.go | 10 ++- storage/postgres/storage.go | 2 +- storage/postgres/storage_test.go | 2 +- storage/storagefakes/fake_pinger.go | 111 +++++++++++++++++++++++++++ storage/storagefakes/fake_storage.go | 73 ++++++++++++++++++ 8 files changed, 213 insertions(+), 13 deletions(-) create mode 100644 storage/storagefakes/fake_pinger.go diff --git a/config/config.go b/config/config.go index 9a39e9025..2d156f401 100644 --- a/config/config.go +++ b/config/config.go @@ -85,7 +85,7 @@ func NewForEnv(env env.Environment) (*Settings, error) { func (c *Settings) Validate() error { validatable := []interface { Validate() error - }{c.Server, c.Storage, c.Log, c.API, c.WebSocket} + }{c.Server, c.Storage, c.Log, c.Health, c.API, c.WebSocket} for _, item := range validatable { if err := item.Validate(); err != nil { diff --git a/pkg/health/types.go b/pkg/health/types.go index d0408cb73..240379427 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -19,12 +19,13 @@ package health import ( "fmt" "github.com/InVisionApp/go-health" + "time" ) // Settings type to be loaded from the environment type Settings struct { - FailuresTreshold int64 `mapstructure:"failures_treshold" description:"maximum failures in a row until component is considered down"` - Interval int64 `description:"seconds between health checks of components"` + FailuresTreshold int64 `mapstructure:"failures_treshold" description:"maximum failures in a row until component is considered down"` + Interval time.Duration `mapstructure:"interval" description:"seconds between health checks of components"` } // DefaultSettings returns default values for health settings @@ -40,8 +41,8 @@ func (s *Settings) Validate() error { if s.FailuresTreshold < 0 { return fmt.Errorf("validate Settings: FailuresTreshold must be >= 0") } - if s.Interval < 0 { - return fmt.Errorf("valudate Settings: Interval must be >= 0") + if s.Interval < 30 { + return fmt.Errorf("validate Settings: Minimum interval is 30 seconds") } return nil } diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 0bd7af54a..db07d7af8 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -23,7 +23,6 @@ import ( "fmt" h "github.com/InVisionApp/go-health" l "github.com/InVisionApp/go-logger/shims/logrus" - "github.com/Peripli/service-manager/pkg/health" "net" "net/http" "sync" @@ -136,7 +135,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( return nil, fmt.Errorf("error creating core api: %s", err) } - storageHealthIndicator, err := storage.NewStorageHealthIndicator(storage.PingFunc(smStorage.Ping)) + storageHealthIndicator, err := storage.NewStorageHealthIndicator(storage.PingFunc(smStorage.PingContext)) if err != nil { return nil, fmt.Errorf("error creating storage health indicator: %s", err) } @@ -221,7 +220,7 @@ func (smb *ServiceManagerBuilder) installHealth() error { err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, - Interval: time.Duration(smb.health.Interval) * time.Second, + Interval: smb.health.Interval * time.Second, }) if err != nil { @@ -230,11 +229,19 @@ func (smb *ServiceManagerBuilder) installHealth() error { } smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthAggregationPolicy, smb.health.FailuresTreshold)) - err := healthz.Start() - if err != nil { + if err := healthz.Start(); err != nil { return err } + // Handles safe termination of sm + util.StartInWaitGroupWithContext(smb.ctx, func(c context.Context) { + <-c.Done() + log.C(c).Debug("Context cancelled. Stopping health checks...") + if err := healthz.Stop(); err != nil { + log.D().Error(err) + } + }, smb.wg) + return nil } diff --git a/storage/interfaces.go b/storage/interfaces.go index 9d047e55a..7eb24b0dd 100644 --- a/storage/interfaces.go +++ b/storage/interfaces.go @@ -135,10 +135,17 @@ type OpenCloser interface { Close() error } +// Pinger allows pinging the storage to check liveliness +//go:generate counterfeiter . Pinger +type Pinger interface { + // PingContext verifies a connection to the database is still alive, establishing a connection if necessary. + PingContext(context.Context) error +} + // PingFunc is an adapter that allows to use regular functions as Pinger type PingFunc func(context.Context) error -// Ping allows PingFunc to act as a Pinger +// PingContext allows PingFunc to act as a Pinger func (mf PingFunc) PingContext(ctx context.Context) error { return mf(ctx) } @@ -176,6 +183,7 @@ type TransactionalRepositoryDecorator func(TransactionalRepository) (Transaction type Storage interface { OpenCloser TransactionalRepository + Pinger Introduce(entity Entity) } diff --git a/storage/postgres/storage.go b/storage/postgres/storage.go index 4fe2ab9f5..3c7681d9b 100644 --- a/storage/postgres/storage.go +++ b/storage/postgres/storage.go @@ -138,7 +138,7 @@ func (ps *Storage) updateSchema(migrationsURL, pgDriverName string) error { return err } -func (ps *Storage) Ping(ctx context.Context) error { +func (ps *Storage) PingContext(ctx context.Context) error { ps.checkOpen() return ps.state.Get() } diff --git a/storage/postgres/storage_test.go b/storage/postgres/storage_test.go index 1078313da..ac1cecd98 100644 --- a/storage/postgres/storage_test.go +++ b/storage/postgres/storage_test.go @@ -70,7 +70,7 @@ var _ = Describe("Postgres Storage", func() { Describe("Ping", func() { Context("Called with uninitialized db", func() { It("Should panic", func() { - Expect(func() { pgStorage.Ping(context.Background()) }).To(Panic()) + Expect(func() { pgStorage.PingContext(context.Background()) }).To(Panic()) }) }) }) diff --git a/storage/storagefakes/fake_pinger.go b/storage/storagefakes/fake_pinger.go new file mode 100644 index 000000000..ea4f34a7e --- /dev/null +++ b/storage/storagefakes/fake_pinger.go @@ -0,0 +1,111 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package storagefakes + +import ( + "context" + "sync" + + "github.com/Peripli/service-manager/storage" +) + +type FakePinger struct { + PingContextStub func(context.Context) error + pingContextMutex sync.RWMutex + pingContextArgsForCall []struct { + arg1 context.Context + } + pingContextReturns struct { + result1 error + } + pingContextReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakePinger) PingContext(arg1 context.Context) error { + fake.pingContextMutex.Lock() + ret, specificReturn := fake.pingContextReturnsOnCall[len(fake.pingContextArgsForCall)] + fake.pingContextArgsForCall = append(fake.pingContextArgsForCall, struct { + arg1 context.Context + }{arg1}) + fake.recordInvocation("PingContext", []interface{}{arg1}) + fake.pingContextMutex.Unlock() + if fake.PingContextStub != nil { + return fake.PingContextStub(arg1) + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.pingContextReturns + return fakeReturns.result1 +} + +func (fake *FakePinger) PingContextCallCount() int { + fake.pingContextMutex.RLock() + defer fake.pingContextMutex.RUnlock() + return len(fake.pingContextArgsForCall) +} + +func (fake *FakePinger) PingContextCalls(stub func(context.Context) error) { + fake.pingContextMutex.Lock() + defer fake.pingContextMutex.Unlock() + fake.PingContextStub = stub +} + +func (fake *FakePinger) PingContextArgsForCall(i int) context.Context { + fake.pingContextMutex.RLock() + defer fake.pingContextMutex.RUnlock() + argsForCall := fake.pingContextArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakePinger) PingContextReturns(result1 error) { + fake.pingContextMutex.Lock() + defer fake.pingContextMutex.Unlock() + fake.PingContextStub = nil + fake.pingContextReturns = struct { + result1 error + }{result1} +} + +func (fake *FakePinger) PingContextReturnsOnCall(i int, result1 error) { + fake.pingContextMutex.Lock() + defer fake.pingContextMutex.Unlock() + fake.PingContextStub = nil + if fake.pingContextReturnsOnCall == nil { + fake.pingContextReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.pingContextReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakePinger) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.pingContextMutex.RLock() + defer fake.pingContextMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakePinger) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ storage.Pinger = new(FakePinger) diff --git a/storage/storagefakes/fake_storage.go b/storage/storagefakes/fake_storage.go index 4ec5ff95d..1cb6fa4f7 100644 --- a/storage/storagefakes/fake_storage.go +++ b/storage/storagefakes/fake_storage.go @@ -108,6 +108,17 @@ type FakeStorage struct { openReturnsOnCall map[int]struct { result1 error } + PingContextStub func(context.Context) error + pingContextMutex sync.RWMutex + pingContextArgsForCall []struct { + arg1 context.Context + } + pingContextReturns struct { + result1 error + } + pingContextReturnsOnCall map[int]struct { + result1 error + } UpdateStub func(context.Context, types.Object, query.LabelChanges, ...query.Criterion) (types.Object, error) updateMutex sync.RWMutex updateArgsForCall []struct { @@ -591,6 +602,66 @@ func (fake *FakeStorage) OpenReturnsOnCall(i int, result1 error) { }{result1} } +func (fake *FakeStorage) PingContext(arg1 context.Context) error { + fake.pingContextMutex.Lock() + ret, specificReturn := fake.pingContextReturnsOnCall[len(fake.pingContextArgsForCall)] + fake.pingContextArgsForCall = append(fake.pingContextArgsForCall, struct { + arg1 context.Context + }{arg1}) + fake.recordInvocation("PingContext", []interface{}{arg1}) + fake.pingContextMutex.Unlock() + if fake.PingContextStub != nil { + return fake.PingContextStub(arg1) + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.pingContextReturns + return fakeReturns.result1 +} + +func (fake *FakeStorage) PingContextCallCount() int { + fake.pingContextMutex.RLock() + defer fake.pingContextMutex.RUnlock() + return len(fake.pingContextArgsForCall) +} + +func (fake *FakeStorage) PingContextCalls(stub func(context.Context) error) { + fake.pingContextMutex.Lock() + defer fake.pingContextMutex.Unlock() + fake.PingContextStub = stub +} + +func (fake *FakeStorage) PingContextArgsForCall(i int) context.Context { + fake.pingContextMutex.RLock() + defer fake.pingContextMutex.RUnlock() + argsForCall := fake.pingContextArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeStorage) PingContextReturns(result1 error) { + fake.pingContextMutex.Lock() + defer fake.pingContextMutex.Unlock() + fake.PingContextStub = nil + fake.pingContextReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeStorage) PingContextReturnsOnCall(i int, result1 error) { + fake.pingContextMutex.Lock() + defer fake.pingContextMutex.Unlock() + fake.PingContextStub = nil + if fake.pingContextReturnsOnCall == nil { + fake.pingContextReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.pingContextReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeStorage) Update(arg1 context.Context, arg2 types.Object, arg3 query.LabelChanges, arg4 ...query.Criterion) (types.Object, error) { fake.updateMutex.Lock() ret, specificReturn := fake.updateReturnsOnCall[len(fake.updateArgsForCall)] @@ -676,6 +747,8 @@ func (fake *FakeStorage) Invocations() map[string][][]interface{} { defer fake.listMutex.RUnlock() fake.openMutex.RLock() defer fake.openMutex.RUnlock() + fake.pingContextMutex.RLock() + defer fake.pingContextMutex.RUnlock() fake.updateMutex.RLock() defer fake.updateMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} From 421d88fe7345bbd59c983da33e0a795e7fdfd073 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 24 Jun 2019 18:06:40 +0300 Subject: [PATCH 09/34] Configuration per indicator and refactoring --- api/healthcheck/healthcheck_controller.go | 50 ++- .../healthcheck_controller_test.go | 130 +++++- pkg/health/aggregation_policy.go | 55 --- pkg/health/aggregation_policy_test.go | 77 ---- .../healthfakes/fake_aggregation_policy.go | 113 ----- .../fake_configurable_indicator.go | 401 ++++++++++++++++++ pkg/health/healthfakes/fake_indicator.go | 363 ++++++++++++++++ pkg/health/ping.go | 14 + pkg/health/registry_test.go | 75 ++-- pkg/health/types.go | 86 +++- pkg/sm/sm.go | 20 +- storage/healthcheck.go | 23 +- storage/healthcheck_test.go | 8 +- 13 files changed, 1099 insertions(+), 316 deletions(-) delete mode 100644 pkg/health/aggregation_policy.go delete mode 100644 pkg/health/aggregation_policy_test.go delete mode 100644 pkg/health/healthfakes/fake_aggregation_policy.go create mode 100644 pkg/health/healthfakes/fake_configurable_indicator.go create mode 100644 pkg/health/healthfakes/fake_indicator.go diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index c13d90ffb..0d10c134f 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -28,17 +28,19 @@ import ( // controller platform controller type controller struct { - health h.IHealth - aggPolicy health.AggregationPolicy - failuresTreshold int64 + health h.IHealth + tresholds map[string]int64 } -// NewController returns a new healthcheck controller with the given indicators and aggregation policy -func NewController(health h.IHealth, aggPolicy health.AggregationPolicy, failuresTreshold int64) web.Controller { +// NewController returns a new healthcheck controller with the given health and tresholds +func NewController(health h.IHealth, indicators []health.Indicator) web.Controller { + tresholds := make(map[string]int64) + for _, v := range indicators { + tresholds[v.Name()] = v.FailuresTreshold() + } return &controller{ - health: health, - aggPolicy: aggPolicy, - failuresTreshold: failuresTreshold, + health: health, + tresholds: tresholds, } } @@ -48,7 +50,7 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { logger := log.C(ctx) logger.Debugf("Performing health check...") healthState, _, _ := c.health.State() - healthResult := c.aggPolicy.Apply(healthState, c.failuresTreshold) + healthResult := c.aggregate(healthState) var status int if healthResult.Status == health.StatusUp { status = http.StatusOK @@ -57,3 +59,33 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { } return util.NewJSONResponse(status, healthResult) } + +func (c *controller) aggregate(state map[string]h.State) *health.Health { + if len(state) == 0 { + return health.New().WithDetail("error", "no health indicators registered").Unknown() + } + overallStatus := health.StatusUp + for i, v := range state { + if v.Fatal && v.ContiguousFailures >= c.tresholds[i] { + overallStatus = health.StatusDown + break + } + } + details := make(map[string]interface{}) + for k, v := range state { + v.Status = convertStatus(v.Status) + details[k] = v + } + return health.New().WithStatus(overallStatus).WithDetails(details) +} + +func convertStatus(status string) string { + switch status { + case "ok": + return string(health.StatusUp) + case "failed": + return string(health.StatusDown) + default: + return string(health.StatusUnknown) + } +} diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index e4a1b7916..a91325e25 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -20,10 +20,11 @@ import ( h "github.com/InVisionApp/go-health" "github.com/Peripli/service-manager/pkg/health" "github.com/Peripli/service-manager/pkg/health/healthfakes" + "github.com/Peripli/service-manager/pkg/web" "net/http" "testing" - "github.com/Peripli/service-manager/pkg/web" + //"github.com/Peripli/service-manager/pkg/web" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -66,15 +67,132 @@ var _ = Describe("Healthcheck controller", func() { }) }) }) + + Describe("aggregation", func() { + var c *controller + var healths map[string]h.State + var tresholds map[string]int64 + + BeforeEach(func() { + healths = map[string]h.State{ + "test1": {Status: "ok"}, + "test2": {Status: "ok"}, + } + tresholds = map[string]int64{ + "test1": 3, + "test2": 3, + } + c = &controller{ + health: HealthFake{}, + tresholds: tresholds, + } + }) + + When("No healths are provided", func() { + It("Returns UNKNOWN and an error detail", func() { + aggregatedHealth := c.aggregate(nil) + Expect(aggregatedHealth.Status).To(Equal(health.StatusUnknown)) + Expect(aggregatedHealth.Details["error"]).ToNot(BeNil()) + }) + }) + + When("At least one health is DOWN more than treshold times and is Fatal", func() { + It("Returns DOWN", func() { + healths["test3"] = h.State{Status: "failed", Fatal: true, ContiguousFailures: 4} + c.tresholds["test3"] = 3 + aggregatedHealth := c.aggregate(healths) + Expect(aggregatedHealth.Status).To(Equal(health.StatusDown)) + }) + }) + + When("At least one health is DOWN more than treshold times and is not Fatal", func() { + It("Returns UP", func() { + healths["test3"] = h.State{Status: "failed", Fatal: false, ContiguousFailures: 4} + c.tresholds["test3"] = 3 + aggregatedHealth := c.aggregate(healths) + Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) + }) + }) + + When("There is DOWN healths but not more than treshold times in a row", func() { + It("Returns UP", func() { + healths["test3"] = h.State{Status: "failed"} + c.tresholds["test3"] = 3 + aggregatedHealth := c.aggregate(healths) + Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) + }) + }) + + When("All healths are UP", func() { + It("Returns UP", func() { + aggregatedHealth := c.aggregate(healths) + Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) + }) + }) + + When("Aggregating healths", func() { + It("Includes them as overall details", func() { + aggregatedHealth := c.aggregate(healths) + for name, h := range healths { + h.Status = convertStatus(h.Status) + Expect(aggregatedHealth.Details[name]).To(Equal(h)) + } + }) + }) + }) + + Describe("create controller", func() { + var c web.Controller + tresholds := map[string]int64{ + "test1": 2, + "test2": 3, + } + + BeforeEach(func() { + indicators := make([]health.Indicator, 0, len(tresholds)) + for i, v := range tresholds { + indicator := &healthfakes.FakeIndicator{} + indicator.NameReturns(i) + indicator.FailuresTresholdReturns(v) + + indicators = append(indicators, indicator) + } + c = NewController(HealthFake{}, indicators) + }) + + When("Controller created with given indicators", func() { + It("Should extract tresholds", func() { + controllerStruct := c.(*controller) + + Expect(controllerStruct.tresholds).To(Equal(tresholds)) + }) + }) + }) }) func createController(status health.Status) *controller { - aggPolicy := &healthfakes.FakeAggregationPolicy{} - aggPolicy.ApplyReturns(&health.Health{Status: status}) + if status == health.StatusUnknown { + return &controller{ + health: HealthFake{}, + } + } + + stringStatus := "ok" + var contiguousFailures int64 = 0 + if status == health.StatusDown { + stringStatus = "failed" + contiguousFailures = 1 + } + return &controller{ - health: HealthFake{}, - aggPolicy: aggPolicy, - failuresTreshold: 3, + health: HealthFake{ + state: map[string]h.State{ + "test1": {Status: stringStatus, Fatal: true, ContiguousFailures: contiguousFailures}, + }, + }, + tresholds: map[string]int64{ + "test1": 1, + }, } } diff --git a/pkg/health/aggregation_policy.go b/pkg/health/aggregation_policy.go deleted file mode 100644 index 8c73ca395..000000000 --- a/pkg/health/aggregation_policy.go +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright 2018 The Service Manager Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package health - -import "github.com/InVisionApp/go-health" - -// DefaultAggregationPolicy aggregates the healths by constructing a new Health based on the given -// where the overall health status is negative if one of the healths is negative and positive if all are positive -type DefaultAggregationPolicy struct { -} - -// Apply aggregates the given healths -func (*DefaultAggregationPolicy) Apply(healths map[string]health.State, failureTreshold int64) *Health { - if len(healths) == 0 { - return New().WithDetail("error", "no health indicators registered").Unknown() - } - overallStatus := StatusUp - for _, health := range healths { - if health.Status == "failed" && health.ContiguousFailures > failureTreshold { - overallStatus = StatusDown - break - } - } - details := make(map[string]interface{}) - for k, v := range healths { - details[k] = ConvertStatus(v.Status) - } - return New().WithStatus(overallStatus).WithDetails(details) -} - -// ConvertStatus converts go-health status to Status -func ConvertStatus(status string) Status { - switch status { - case "ok": - return StatusUp - case "failed": - return StatusDown - default: - return StatusUnknown - } -} diff --git a/pkg/health/aggregation_policy_test.go b/pkg/health/aggregation_policy_test.go deleted file mode 100644 index c16a24d10..000000000 --- a/pkg/health/aggregation_policy_test.go +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Copyright 2018 The Service Manager Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package health - -import ( - "github.com/InVisionApp/go-health" - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("Healthcheck AggregationPolicy", func() { - - aggregationPolicy := &DefaultAggregationPolicy{} - var healths map[string]health.State - var failuresTreshold int64 = 3 - - BeforeEach(func() { - healths = map[string]health.State{ - "test1": {Status: "ok"}, - "test2": {Status: "ok"}, - } - }) - - When("No healths are provided", func() { - It("Returns UNKNOWN and an error detail", func() { - aggregatedHealth := aggregationPolicy.Apply(nil, failuresTreshold) - Expect(aggregatedHealth.Status).To(Equal(StatusUnknown)) - Expect(aggregatedHealth.Details["error"]).ToNot(BeNil()) - }) - }) - - When("At least one health is DOWN", func() { - It("Returns DOWN", func() { - healths["test3"] = health.State{Status: "failed", ContiguousFailures: 4} - aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) - Expect(aggregatedHealth.Status).To(Equal(StatusDown)) - }) - }) - - When("There is DOWN healths but not more than treshold times in a row", func() { - It("Returns UP", func() { - healths["test3"] = health.State{Status: "failed"} - aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) - Expect(aggregatedHealth.Status).To(Equal(StatusUp)) - }) - }) - - When("All healths are UP", func() { - It("Returns UP", func() { - aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) - Expect(aggregatedHealth.Status).To(Equal(StatusUp)) - }) - }) - - When("Aggregating healths", func() { - It("Includes them as overall details", func() { - aggregatedHealth := aggregationPolicy.Apply(healths, failuresTreshold) - for name, h := range healths { - Expect(aggregatedHealth.Details[name]).To(Equal(ConvertStatus(h.Status))) - } - }) - }) -}) diff --git a/pkg/health/healthfakes/fake_aggregation_policy.go b/pkg/health/healthfakes/fake_aggregation_policy.go deleted file mode 100644 index 69a1220cc..000000000 --- a/pkg/health/healthfakes/fake_aggregation_policy.go +++ /dev/null @@ -1,113 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package healthfakes - -import ( - "sync" - - healtha "github.com/InVisionApp/go-health" - "github.com/Peripli/service-manager/pkg/health" -) - -type FakeAggregationPolicy struct { - ApplyStub func(map[string]healtha.State, int64) *health.Health - applyMutex sync.RWMutex - applyArgsForCall []struct { - arg1 map[string]healtha.State - arg2 int64 - } - applyReturns struct { - result1 *health.Health - } - applyReturnsOnCall map[int]struct { - result1 *health.Health - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeAggregationPolicy) Apply(arg1 map[string]healtha.State, arg2 int64) *health.Health { - fake.applyMutex.Lock() - ret, specificReturn := fake.applyReturnsOnCall[len(fake.applyArgsForCall)] - fake.applyArgsForCall = append(fake.applyArgsForCall, struct { - arg1 map[string]healtha.State - arg2 int64 - }{arg1, arg2}) - fake.recordInvocation("Apply", []interface{}{arg1, arg2}) - fake.applyMutex.Unlock() - if fake.ApplyStub != nil { - return fake.ApplyStub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.applyReturns - return fakeReturns.result1 -} - -func (fake *FakeAggregationPolicy) ApplyCallCount() int { - fake.applyMutex.RLock() - defer fake.applyMutex.RUnlock() - return len(fake.applyArgsForCall) -} - -func (fake *FakeAggregationPolicy) ApplyCalls(stub func(map[string]healtha.State, int64) *health.Health) { - fake.applyMutex.Lock() - defer fake.applyMutex.Unlock() - fake.ApplyStub = stub -} - -func (fake *FakeAggregationPolicy) ApplyArgsForCall(i int) (map[string]healtha.State, int64) { - fake.applyMutex.RLock() - defer fake.applyMutex.RUnlock() - argsForCall := fake.applyArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeAggregationPolicy) ApplyReturns(result1 *health.Health) { - fake.applyMutex.Lock() - defer fake.applyMutex.Unlock() - fake.ApplyStub = nil - fake.applyReturns = struct { - result1 *health.Health - }{result1} -} - -func (fake *FakeAggregationPolicy) ApplyReturnsOnCall(i int, result1 *health.Health) { - fake.applyMutex.Lock() - defer fake.applyMutex.Unlock() - fake.ApplyStub = nil - if fake.applyReturnsOnCall == nil { - fake.applyReturnsOnCall = make(map[int]struct { - result1 *health.Health - }) - } - fake.applyReturnsOnCall[i] = struct { - result1 *health.Health - }{result1} -} - -func (fake *FakeAggregationPolicy) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.applyMutex.RLock() - defer fake.applyMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeAggregationPolicy) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ health.AggregationPolicy = new(FakeAggregationPolicy) diff --git a/pkg/health/healthfakes/fake_configurable_indicator.go b/pkg/health/healthfakes/fake_configurable_indicator.go new file mode 100644 index 000000000..05876bd5a --- /dev/null +++ b/pkg/health/healthfakes/fake_configurable_indicator.go @@ -0,0 +1,401 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package healthfakes + +import ( + "sync" + "time" + + "github.com/Peripli/service-manager/pkg/health" +) + +type FakeConfigurableIndicator struct { + ConfigureStub func(*health.IndicatorSettings) + configureMutex sync.RWMutex + configureArgsForCall []struct { + arg1 *health.IndicatorSettings + } + FailuresTresholdStub func() int64 + failuresTresholdMutex sync.RWMutex + failuresTresholdArgsForCall []struct { + } + failuresTresholdReturns struct { + result1 int64 + } + failuresTresholdReturnsOnCall map[int]struct { + result1 int64 + } + FatalStub func() bool + fatalMutex sync.RWMutex + fatalArgsForCall []struct { + } + fatalReturns struct { + result1 bool + } + fatalReturnsOnCall map[int]struct { + result1 bool + } + IntervalStub func() time.Duration + intervalMutex sync.RWMutex + intervalArgsForCall []struct { + } + intervalReturns struct { + result1 time.Duration + } + intervalReturnsOnCall map[int]struct { + result1 time.Duration + } + NameStub func() string + nameMutex sync.RWMutex + nameArgsForCall []struct { + } + nameReturns struct { + result1 string + } + nameReturnsOnCall map[int]struct { + result1 string + } + StatusStub func() (interface{}, error) + statusMutex sync.RWMutex + statusArgsForCall []struct { + } + statusReturns struct { + result1 interface{} + result2 error + } + statusReturnsOnCall map[int]struct { + result1 interface{} + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeConfigurableIndicator) Configure(arg1 *health.IndicatorSettings) { + fake.configureMutex.Lock() + fake.configureArgsForCall = append(fake.configureArgsForCall, struct { + arg1 *health.IndicatorSettings + }{arg1}) + fake.recordInvocation("Configure", []interface{}{arg1}) + fake.configureMutex.Unlock() + if fake.ConfigureStub != nil { + fake.ConfigureStub(arg1) + } +} + +func (fake *FakeConfigurableIndicator) ConfigureCallCount() int { + fake.configureMutex.RLock() + defer fake.configureMutex.RUnlock() + return len(fake.configureArgsForCall) +} + +func (fake *FakeConfigurableIndicator) ConfigureCalls(stub func(*health.IndicatorSettings)) { + fake.configureMutex.Lock() + defer fake.configureMutex.Unlock() + fake.ConfigureStub = stub +} + +func (fake *FakeConfigurableIndicator) ConfigureArgsForCall(i int) *health.IndicatorSettings { + fake.configureMutex.RLock() + defer fake.configureMutex.RUnlock() + argsForCall := fake.configureArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeConfigurableIndicator) FailuresTreshold() int64 { + fake.failuresTresholdMutex.Lock() + ret, specificReturn := fake.failuresTresholdReturnsOnCall[len(fake.failuresTresholdArgsForCall)] + fake.failuresTresholdArgsForCall = append(fake.failuresTresholdArgsForCall, struct { + }{}) + fake.recordInvocation("FailuresTreshold", []interface{}{}) + fake.failuresTresholdMutex.Unlock() + if fake.FailuresTresholdStub != nil { + return fake.FailuresTresholdStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.failuresTresholdReturns + return fakeReturns.result1 +} + +func (fake *FakeConfigurableIndicator) FailuresTresholdCallCount() int { + fake.failuresTresholdMutex.RLock() + defer fake.failuresTresholdMutex.RUnlock() + return len(fake.failuresTresholdArgsForCall) +} + +func (fake *FakeConfigurableIndicator) FailuresTresholdCalls(stub func() int64) { + fake.failuresTresholdMutex.Lock() + defer fake.failuresTresholdMutex.Unlock() + fake.FailuresTresholdStub = stub +} + +func (fake *FakeConfigurableIndicator) FailuresTresholdReturns(result1 int64) { + fake.failuresTresholdMutex.Lock() + defer fake.failuresTresholdMutex.Unlock() + fake.FailuresTresholdStub = nil + fake.failuresTresholdReturns = struct { + result1 int64 + }{result1} +} + +func (fake *FakeConfigurableIndicator) FailuresTresholdReturnsOnCall(i int, result1 int64) { + fake.failuresTresholdMutex.Lock() + defer fake.failuresTresholdMutex.Unlock() + fake.FailuresTresholdStub = nil + if fake.failuresTresholdReturnsOnCall == nil { + fake.failuresTresholdReturnsOnCall = make(map[int]struct { + result1 int64 + }) + } + fake.failuresTresholdReturnsOnCall[i] = struct { + result1 int64 + }{result1} +} + +func (fake *FakeConfigurableIndicator) Fatal() bool { + fake.fatalMutex.Lock() + ret, specificReturn := fake.fatalReturnsOnCall[len(fake.fatalArgsForCall)] + fake.fatalArgsForCall = append(fake.fatalArgsForCall, struct { + }{}) + fake.recordInvocation("Fatal", []interface{}{}) + fake.fatalMutex.Unlock() + if fake.FatalStub != nil { + return fake.FatalStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.fatalReturns + return fakeReturns.result1 +} + +func (fake *FakeConfigurableIndicator) FatalCallCount() int { + fake.fatalMutex.RLock() + defer fake.fatalMutex.RUnlock() + return len(fake.fatalArgsForCall) +} + +func (fake *FakeConfigurableIndicator) FatalCalls(stub func() bool) { + fake.fatalMutex.Lock() + defer fake.fatalMutex.Unlock() + fake.FatalStub = stub +} + +func (fake *FakeConfigurableIndicator) FatalReturns(result1 bool) { + fake.fatalMutex.Lock() + defer fake.fatalMutex.Unlock() + fake.FatalStub = nil + fake.fatalReturns = struct { + result1 bool + }{result1} +} + +func (fake *FakeConfigurableIndicator) FatalReturnsOnCall(i int, result1 bool) { + fake.fatalMutex.Lock() + defer fake.fatalMutex.Unlock() + fake.FatalStub = nil + if fake.fatalReturnsOnCall == nil { + fake.fatalReturnsOnCall = make(map[int]struct { + result1 bool + }) + } + fake.fatalReturnsOnCall[i] = struct { + result1 bool + }{result1} +} + +func (fake *FakeConfigurableIndicator) Interval() time.Duration { + fake.intervalMutex.Lock() + ret, specificReturn := fake.intervalReturnsOnCall[len(fake.intervalArgsForCall)] + fake.intervalArgsForCall = append(fake.intervalArgsForCall, struct { + }{}) + fake.recordInvocation("Interval", []interface{}{}) + fake.intervalMutex.Unlock() + if fake.IntervalStub != nil { + return fake.IntervalStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.intervalReturns + return fakeReturns.result1 +} + +func (fake *FakeConfigurableIndicator) IntervalCallCount() int { + fake.intervalMutex.RLock() + defer fake.intervalMutex.RUnlock() + return len(fake.intervalArgsForCall) +} + +func (fake *FakeConfigurableIndicator) IntervalCalls(stub func() time.Duration) { + fake.intervalMutex.Lock() + defer fake.intervalMutex.Unlock() + fake.IntervalStub = stub +} + +func (fake *FakeConfigurableIndicator) IntervalReturns(result1 time.Duration) { + fake.intervalMutex.Lock() + defer fake.intervalMutex.Unlock() + fake.IntervalStub = nil + fake.intervalReturns = struct { + result1 time.Duration + }{result1} +} + +func (fake *FakeConfigurableIndicator) IntervalReturnsOnCall(i int, result1 time.Duration) { + fake.intervalMutex.Lock() + defer fake.intervalMutex.Unlock() + fake.IntervalStub = nil + if fake.intervalReturnsOnCall == nil { + fake.intervalReturnsOnCall = make(map[int]struct { + result1 time.Duration + }) + } + fake.intervalReturnsOnCall[i] = struct { + result1 time.Duration + }{result1} +} + +func (fake *FakeConfigurableIndicator) Name() string { + fake.nameMutex.Lock() + ret, specificReturn := fake.nameReturnsOnCall[len(fake.nameArgsForCall)] + fake.nameArgsForCall = append(fake.nameArgsForCall, struct { + }{}) + fake.recordInvocation("Name", []interface{}{}) + fake.nameMutex.Unlock() + if fake.NameStub != nil { + return fake.NameStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.nameReturns + return fakeReturns.result1 +} + +func (fake *FakeConfigurableIndicator) NameCallCount() int { + fake.nameMutex.RLock() + defer fake.nameMutex.RUnlock() + return len(fake.nameArgsForCall) +} + +func (fake *FakeConfigurableIndicator) NameCalls(stub func() string) { + fake.nameMutex.Lock() + defer fake.nameMutex.Unlock() + fake.NameStub = stub +} + +func (fake *FakeConfigurableIndicator) NameReturns(result1 string) { + fake.nameMutex.Lock() + defer fake.nameMutex.Unlock() + fake.NameStub = nil + fake.nameReturns = struct { + result1 string + }{result1} +} + +func (fake *FakeConfigurableIndicator) NameReturnsOnCall(i int, result1 string) { + fake.nameMutex.Lock() + defer fake.nameMutex.Unlock() + fake.NameStub = nil + if fake.nameReturnsOnCall == nil { + fake.nameReturnsOnCall = make(map[int]struct { + result1 string + }) + } + fake.nameReturnsOnCall[i] = struct { + result1 string + }{result1} +} + +func (fake *FakeConfigurableIndicator) Status() (interface{}, error) { + fake.statusMutex.Lock() + ret, specificReturn := fake.statusReturnsOnCall[len(fake.statusArgsForCall)] + fake.statusArgsForCall = append(fake.statusArgsForCall, struct { + }{}) + fake.recordInvocation("Status", []interface{}{}) + fake.statusMutex.Unlock() + if fake.StatusStub != nil { + return fake.StatusStub() + } + if specificReturn { + return ret.result1, ret.result2 + } + fakeReturns := fake.statusReturns + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeConfigurableIndicator) StatusCallCount() int { + fake.statusMutex.RLock() + defer fake.statusMutex.RUnlock() + return len(fake.statusArgsForCall) +} + +func (fake *FakeConfigurableIndicator) StatusCalls(stub func() (interface{}, error)) { + fake.statusMutex.Lock() + defer fake.statusMutex.Unlock() + fake.StatusStub = stub +} + +func (fake *FakeConfigurableIndicator) StatusReturns(result1 interface{}, result2 error) { + fake.statusMutex.Lock() + defer fake.statusMutex.Unlock() + fake.StatusStub = nil + fake.statusReturns = struct { + result1 interface{} + result2 error + }{result1, result2} +} + +func (fake *FakeConfigurableIndicator) StatusReturnsOnCall(i int, result1 interface{}, result2 error) { + fake.statusMutex.Lock() + defer fake.statusMutex.Unlock() + fake.StatusStub = nil + if fake.statusReturnsOnCall == nil { + fake.statusReturnsOnCall = make(map[int]struct { + result1 interface{} + result2 error + }) + } + fake.statusReturnsOnCall[i] = struct { + result1 interface{} + result2 error + }{result1, result2} +} + +func (fake *FakeConfigurableIndicator) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.configureMutex.RLock() + defer fake.configureMutex.RUnlock() + fake.failuresTresholdMutex.RLock() + defer fake.failuresTresholdMutex.RUnlock() + fake.fatalMutex.RLock() + defer fake.fatalMutex.RUnlock() + fake.intervalMutex.RLock() + defer fake.intervalMutex.RUnlock() + fake.nameMutex.RLock() + defer fake.nameMutex.RUnlock() + fake.statusMutex.RLock() + defer fake.statusMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeConfigurableIndicator) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ health.ConfigurableIndicator = new(FakeConfigurableIndicator) diff --git a/pkg/health/healthfakes/fake_indicator.go b/pkg/health/healthfakes/fake_indicator.go new file mode 100644 index 000000000..f3b08c113 --- /dev/null +++ b/pkg/health/healthfakes/fake_indicator.go @@ -0,0 +1,363 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package healthfakes + +import ( + "sync" + "time" + + "github.com/Peripli/service-manager/pkg/health" +) + +type FakeIndicator struct { + FailuresTresholdStub func() int64 + failuresTresholdMutex sync.RWMutex + failuresTresholdArgsForCall []struct { + } + failuresTresholdReturns struct { + result1 int64 + } + failuresTresholdReturnsOnCall map[int]struct { + result1 int64 + } + FatalStub func() bool + fatalMutex sync.RWMutex + fatalArgsForCall []struct { + } + fatalReturns struct { + result1 bool + } + fatalReturnsOnCall map[int]struct { + result1 bool + } + IntervalStub func() time.Duration + intervalMutex sync.RWMutex + intervalArgsForCall []struct { + } + intervalReturns struct { + result1 time.Duration + } + intervalReturnsOnCall map[int]struct { + result1 time.Duration + } + NameStub func() string + nameMutex sync.RWMutex + nameArgsForCall []struct { + } + nameReturns struct { + result1 string + } + nameReturnsOnCall map[int]struct { + result1 string + } + StatusStub func() (interface{}, error) + statusMutex sync.RWMutex + statusArgsForCall []struct { + } + statusReturns struct { + result1 interface{} + result2 error + } + statusReturnsOnCall map[int]struct { + result1 interface{} + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeIndicator) FailuresTreshold() int64 { + fake.failuresTresholdMutex.Lock() + ret, specificReturn := fake.failuresTresholdReturnsOnCall[len(fake.failuresTresholdArgsForCall)] + fake.failuresTresholdArgsForCall = append(fake.failuresTresholdArgsForCall, struct { + }{}) + fake.recordInvocation("FailuresTreshold", []interface{}{}) + fake.failuresTresholdMutex.Unlock() + if fake.FailuresTresholdStub != nil { + return fake.FailuresTresholdStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.failuresTresholdReturns + return fakeReturns.result1 +} + +func (fake *FakeIndicator) FailuresTresholdCallCount() int { + fake.failuresTresholdMutex.RLock() + defer fake.failuresTresholdMutex.RUnlock() + return len(fake.failuresTresholdArgsForCall) +} + +func (fake *FakeIndicator) FailuresTresholdCalls(stub func() int64) { + fake.failuresTresholdMutex.Lock() + defer fake.failuresTresholdMutex.Unlock() + fake.FailuresTresholdStub = stub +} + +func (fake *FakeIndicator) FailuresTresholdReturns(result1 int64) { + fake.failuresTresholdMutex.Lock() + defer fake.failuresTresholdMutex.Unlock() + fake.FailuresTresholdStub = nil + fake.failuresTresholdReturns = struct { + result1 int64 + }{result1} +} + +func (fake *FakeIndicator) FailuresTresholdReturnsOnCall(i int, result1 int64) { + fake.failuresTresholdMutex.Lock() + defer fake.failuresTresholdMutex.Unlock() + fake.FailuresTresholdStub = nil + if fake.failuresTresholdReturnsOnCall == nil { + fake.failuresTresholdReturnsOnCall = make(map[int]struct { + result1 int64 + }) + } + fake.failuresTresholdReturnsOnCall[i] = struct { + result1 int64 + }{result1} +} + +func (fake *FakeIndicator) Fatal() bool { + fake.fatalMutex.Lock() + ret, specificReturn := fake.fatalReturnsOnCall[len(fake.fatalArgsForCall)] + fake.fatalArgsForCall = append(fake.fatalArgsForCall, struct { + }{}) + fake.recordInvocation("Fatal", []interface{}{}) + fake.fatalMutex.Unlock() + if fake.FatalStub != nil { + return fake.FatalStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.fatalReturns + return fakeReturns.result1 +} + +func (fake *FakeIndicator) FatalCallCount() int { + fake.fatalMutex.RLock() + defer fake.fatalMutex.RUnlock() + return len(fake.fatalArgsForCall) +} + +func (fake *FakeIndicator) FatalCalls(stub func() bool) { + fake.fatalMutex.Lock() + defer fake.fatalMutex.Unlock() + fake.FatalStub = stub +} + +func (fake *FakeIndicator) FatalReturns(result1 bool) { + fake.fatalMutex.Lock() + defer fake.fatalMutex.Unlock() + fake.FatalStub = nil + fake.fatalReturns = struct { + result1 bool + }{result1} +} + +func (fake *FakeIndicator) FatalReturnsOnCall(i int, result1 bool) { + fake.fatalMutex.Lock() + defer fake.fatalMutex.Unlock() + fake.FatalStub = nil + if fake.fatalReturnsOnCall == nil { + fake.fatalReturnsOnCall = make(map[int]struct { + result1 bool + }) + } + fake.fatalReturnsOnCall[i] = struct { + result1 bool + }{result1} +} + +func (fake *FakeIndicator) Interval() time.Duration { + fake.intervalMutex.Lock() + ret, specificReturn := fake.intervalReturnsOnCall[len(fake.intervalArgsForCall)] + fake.intervalArgsForCall = append(fake.intervalArgsForCall, struct { + }{}) + fake.recordInvocation("Interval", []interface{}{}) + fake.intervalMutex.Unlock() + if fake.IntervalStub != nil { + return fake.IntervalStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.intervalReturns + return fakeReturns.result1 +} + +func (fake *FakeIndicator) IntervalCallCount() int { + fake.intervalMutex.RLock() + defer fake.intervalMutex.RUnlock() + return len(fake.intervalArgsForCall) +} + +func (fake *FakeIndicator) IntervalCalls(stub func() time.Duration) { + fake.intervalMutex.Lock() + defer fake.intervalMutex.Unlock() + fake.IntervalStub = stub +} + +func (fake *FakeIndicator) IntervalReturns(result1 time.Duration) { + fake.intervalMutex.Lock() + defer fake.intervalMutex.Unlock() + fake.IntervalStub = nil + fake.intervalReturns = struct { + result1 time.Duration + }{result1} +} + +func (fake *FakeIndicator) IntervalReturnsOnCall(i int, result1 time.Duration) { + fake.intervalMutex.Lock() + defer fake.intervalMutex.Unlock() + fake.IntervalStub = nil + if fake.intervalReturnsOnCall == nil { + fake.intervalReturnsOnCall = make(map[int]struct { + result1 time.Duration + }) + } + fake.intervalReturnsOnCall[i] = struct { + result1 time.Duration + }{result1} +} + +func (fake *FakeIndicator) Name() string { + fake.nameMutex.Lock() + ret, specificReturn := fake.nameReturnsOnCall[len(fake.nameArgsForCall)] + fake.nameArgsForCall = append(fake.nameArgsForCall, struct { + }{}) + fake.recordInvocation("Name", []interface{}{}) + fake.nameMutex.Unlock() + if fake.NameStub != nil { + return fake.NameStub() + } + if specificReturn { + return ret.result1 + } + fakeReturns := fake.nameReturns + return fakeReturns.result1 +} + +func (fake *FakeIndicator) NameCallCount() int { + fake.nameMutex.RLock() + defer fake.nameMutex.RUnlock() + return len(fake.nameArgsForCall) +} + +func (fake *FakeIndicator) NameCalls(stub func() string) { + fake.nameMutex.Lock() + defer fake.nameMutex.Unlock() + fake.NameStub = stub +} + +func (fake *FakeIndicator) NameReturns(result1 string) { + fake.nameMutex.Lock() + defer fake.nameMutex.Unlock() + fake.NameStub = nil + fake.nameReturns = struct { + result1 string + }{result1} +} + +func (fake *FakeIndicator) NameReturnsOnCall(i int, result1 string) { + fake.nameMutex.Lock() + defer fake.nameMutex.Unlock() + fake.NameStub = nil + if fake.nameReturnsOnCall == nil { + fake.nameReturnsOnCall = make(map[int]struct { + result1 string + }) + } + fake.nameReturnsOnCall[i] = struct { + result1 string + }{result1} +} + +func (fake *FakeIndicator) Status() (interface{}, error) { + fake.statusMutex.Lock() + ret, specificReturn := fake.statusReturnsOnCall[len(fake.statusArgsForCall)] + fake.statusArgsForCall = append(fake.statusArgsForCall, struct { + }{}) + fake.recordInvocation("Status", []interface{}{}) + fake.statusMutex.Unlock() + if fake.StatusStub != nil { + return fake.StatusStub() + } + if specificReturn { + return ret.result1, ret.result2 + } + fakeReturns := fake.statusReturns + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeIndicator) StatusCallCount() int { + fake.statusMutex.RLock() + defer fake.statusMutex.RUnlock() + return len(fake.statusArgsForCall) +} + +func (fake *FakeIndicator) StatusCalls(stub func() (interface{}, error)) { + fake.statusMutex.Lock() + defer fake.statusMutex.Unlock() + fake.StatusStub = stub +} + +func (fake *FakeIndicator) StatusReturns(result1 interface{}, result2 error) { + fake.statusMutex.Lock() + defer fake.statusMutex.Unlock() + fake.StatusStub = nil + fake.statusReturns = struct { + result1 interface{} + result2 error + }{result1, result2} +} + +func (fake *FakeIndicator) StatusReturnsOnCall(i int, result1 interface{}, result2 error) { + fake.statusMutex.Lock() + defer fake.statusMutex.Unlock() + fake.StatusStub = nil + if fake.statusReturnsOnCall == nil { + fake.statusReturnsOnCall = make(map[int]struct { + result1 interface{} + result2 error + }) + } + fake.statusReturnsOnCall[i] = struct { + result1 interface{} + result2 error + }{result1, result2} +} + +func (fake *FakeIndicator) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.failuresTresholdMutex.RLock() + defer fake.failuresTresholdMutex.RUnlock() + fake.fatalMutex.RLock() + defer fake.fatalMutex.RUnlock() + fake.intervalMutex.RLock() + defer fake.intervalMutex.RUnlock() + fake.nameMutex.RLock() + defer fake.nameMutex.RUnlock() + fake.statusMutex.RLock() + defer fake.statusMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeIndicator) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ health.Indicator = new(FakeIndicator) diff --git a/pkg/health/ping.go b/pkg/health/ping.go index 6331a89fb..ce55fbcaf 100644 --- a/pkg/health/ping.go +++ b/pkg/health/ping.go @@ -16,6 +16,8 @@ package health +import "time" + // pingIndicator is a default indicator that always returns up type pingIndicator struct { } @@ -24,6 +26,18 @@ func (*pingIndicator) Name() string { return "ping" } +func (*pingIndicator) Interval() time.Duration { + return 30 +} + +func (*pingIndicator) FailuresTreshold() int64 { + return 1 +} + +func (*pingIndicator) Fatal() bool { + return true +} + func (*pingIndicator) Status() (interface{}, error) { return nil, nil } diff --git a/pkg/health/registry_test.go b/pkg/health/registry_test.go index a0d543446..cd38ad351 100644 --- a/pkg/health/registry_test.go +++ b/pkg/health/registry_test.go @@ -17,9 +17,9 @@ package health import ( - "github.com/InVisionApp/go-health" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "time" ) var _ = Describe("Healthcheck Registry", func() { @@ -31,27 +31,13 @@ var _ = Describe("Healthcheck Registry", func() { }) When("Constructing default registry", func() { - It("Has ping indicator and default aggregation policy", func() { + It("Has ping indicator", func() { indicators := registry.HealthIndicators Expect(indicators).To(ConsistOf(&pingIndicator{})) - - policy := registry.HealthAggregationPolicy - Expect(policy).To(BeAssignableToTypeOf(&DefaultAggregationPolicy{})) - }) - }) - - Context("Register aggregation policy", func() { - It("Overrides the previous", func() { - policy := registry.HealthAggregationPolicy - Expect(policy).To(BeAssignableToTypeOf(&DefaultAggregationPolicy{})) - - registry.HealthAggregationPolicy = &testAggregationPolicy{} - policy = registry.HealthAggregationPolicy - Expect(policy).To(BeAssignableToTypeOf(&testAggregationPolicy{})) }) }) - Context("Register health indicator", func() { + When("Register health indicator", func() { It("Adds a new indicator", func() { preAddIndicators := registry.HealthIndicators @@ -64,22 +50,63 @@ var _ = Describe("Healthcheck Registry", func() { Expect(postAddIndicators).To(ContainElement(newIndicator)) }) }) + + When("When configure indicators", func() { + It("Should configure with default settings if settings not provided", func() { + newIndicator := &testIndicator{} + registry.HealthIndicators = append(registry.HealthIndicators, newIndicator) + + registry.ConfigureIndicators() + + Expect(newIndicator.Interval()).To(Equal(DefaultIndicatorSettings().Interval)) + Expect(newIndicator.FailuresTreshold()).To(Equal(DefaultIndicatorSettings().FailuresTreshold)) + }) + }) + + When("When configure indicators", func() { + It("Should configure with provided settings", func() { + newIndicator := &testIndicator{} + registry.HealthIndicators = append(registry.HealthIndicators, newIndicator) + + settings := &IndicatorSettings{ + Interval: 50, + FailuresTreshold: 2, + } + + registry.HealthSettings[newIndicator.Name()] = settings + + registry.ConfigureIndicators() + + Expect(newIndicator.Interval()).To(Equal(settings.Interval)) + Expect(newIndicator.FailuresTreshold()).To(Equal(settings.FailuresTreshold)) + }) + }) }) -type testAggregationPolicy struct { +type testIndicator struct { + settings *IndicatorSettings } -func (*testAggregationPolicy) Apply(healths map[string]health.State, failuresTreshold int64) *Health { - return New().Up() +func (i *testIndicator) Name() string { + return "test" } -type testIndicator struct { +func (i *testIndicator) Interval() time.Duration { + return i.settings.Interval } -func (*testIndicator) Name() string { - return "test" +func (i *testIndicator) FailuresTreshold() int64 { + return i.settings.FailuresTreshold } -func (*testIndicator) Status() (interface{}, error) { +func (i *testIndicator) Fatal() bool { + return i.settings.Fatal +} + +func (i *testIndicator) Status() (interface{}, error) { return nil, nil } + +func (i *testIndicator) Configure(settings *IndicatorSettings) { + i.settings = settings +} diff --git a/pkg/health/types.go b/pkg/health/types.go index 240379427..440f54b5f 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -18,30 +18,54 @@ package health import ( "fmt" - "github.com/InVisionApp/go-health" "time" ) // Settings type to be loaded from the environment type Settings struct { - FailuresTreshold int64 `mapstructure:"failures_treshold" description:"maximum failures in a row until component is considered down"` - Interval time.Duration `mapstructure:"interval" description:"seconds between health checks of components"` + IndicatorsSettings map[string]*IndicatorSettings `mapstructure:"indicators,omitempty"` } // DefaultSettings returns default values for health settings func DefaultSettings() *Settings { + emptySettings := make(map[string]*IndicatorSettings) return &Settings{ + IndicatorsSettings: emptySettings, + } +} + +// Validate validates health settings +func (s *Settings) Validate() error { + for _, v := range s.IndicatorsSettings { + if err := v.Validate(); err != nil { + return err + } + } + return nil +} + +// IndicatorSettings type to be loaded from the environment +type IndicatorSettings struct { + Fatal bool `mapstructure:"fatal" description:"if the indicator affects the overall status "` + FailuresTreshold int64 `mapstructure:"failures_treshold" description:"maximum failures in a row until component is considered down"` + Interval time.Duration `mapstructure:"interval" description:"time between health checks of components"` +} + +// DefaultIndicatorSettings returns default values for indicator settings +func DefaultIndicatorSettings() *IndicatorSettings { + return &IndicatorSettings{ + Fatal: true, FailuresTreshold: 3, Interval: 60, } } -// Validate validates the health settings -func (s *Settings) Validate() error { - if s.FailuresTreshold < 0 { - return fmt.Errorf("validate Settings: FailuresTreshold must be >= 0") +// Validate validates indicator settings +func (is *IndicatorSettings) Validate() error { + if is.FailuresTreshold <= 0 { + return fmt.Errorf("validate Settings: FailuresTreshold must be > 0") } - if s.Interval < 30 { + if is.Interval < 30 { return fmt.Errorf("validate Settings: Minimum interval is 30 seconds") } return nil @@ -118,26 +142,39 @@ func (h *Health) WithDetails(details map[string]interface{}) *Health { } // Indicator is an interface to provide the health of a component +//go:generate counterfeiter . Indicator type Indicator interface { // Name returns the name of the component Name() string + // Interval returns settings of the indicator + Interval() time.Duration + + // FailuresTreshold returns the maximum failures in a row until component is considered down + FailuresTreshold() int64 + + // Fatal returns if the health indicator is fatal for the overall status + Fatal() bool + // Status returns the health information of the component Status() (interface{}, error) } -// AggregationPolicy is an interface to provide aggregated health information -//go:generate counterfeiter . AggregationPolicy -type AggregationPolicy interface { - // Apply processes the given healths to build a single health - Apply(healths map[string]health.State, failureTreshold int64) *Health +// ConfigurableIndicator is an interface to provide configurable health of a component +//go:generate counterfeiter . ConfigurableIndicator +type ConfigurableIndicator interface { + Indicator + + // Configure configures the indicator with given settings + Configure(*IndicatorSettings) } -// NewDefaultRegistry returns a default health registry with a single ping indicator and a default aggregation policy +// NewDefaultRegistry returns a default health registry with a single ping indicator func NewDefaultRegistry() *Registry { + emptySettings := make(map[string]*IndicatorSettings) return &Registry{ - HealthIndicators: []Indicator{&pingIndicator{}}, - HealthAggregationPolicy: &DefaultAggregationPolicy{}, + HealthIndicators: []Indicator{&pingIndicator{}}, + HealthSettings: emptySettings, } } @@ -146,6 +183,19 @@ type Registry struct { // HealthIndicators are the currently registered health indicators HealthIndicators []Indicator - // HealthAggregationPolicy is the registered health aggregationPolicy - HealthAggregationPolicy AggregationPolicy + // Indicator Settings of the registry + HealthSettings map[string]*IndicatorSettings +} + +// ConfigureIndicators configures registry's indicators with provided settings +func (r *Registry) ConfigureIndicators() { + for _, hi := range r.HealthIndicators { + if indicator, ok := hi.(ConfigurableIndicator); ok { + if settings, ok := r.HealthSettings[indicator.Name()]; ok { + indicator.Configure(settings) + } else { + indicator.Configure(DefaultIndicatorSettings()) + } + } + } } diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index db07d7af8..d2394f753 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -141,6 +141,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( } API.HealthIndicators = append(API.HealthIndicators, storageHealthIndicator) + API.HealthSettings = cfg.Health.IndicatorsSettings notificationCleaner := &storage.NotificationCleaner{ Storage: interceptableRepository, @@ -187,8 +188,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( // Build builds the Service Manager func (smb *ServiceManagerBuilder) Build() *ServiceManager { - err := smb.installHealth() - if err != nil { + if err := smb.installHealth(); err != nil { panic(err) } @@ -212,33 +212,33 @@ func (smb *ServiceManagerBuilder) installHealth() error { return nil } + smb.ConfigureIndicators() + healthz := h.New() logger := log.C(smb.ctx).Logger healthz.Logger = l.New(logger) for _, indicator := range smb.HealthIndicators { - err := healthz.AddCheck(&h.Config{ + if err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, - Interval: smb.health.Interval * time.Second, - }) - - if err != nil { + Interval: indicator.Interval() * time.Second, + Fatal: indicator.Fatal(), + }); err != nil { return err } } - smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthAggregationPolicy, smb.health.FailuresTreshold)) + smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthIndicators)) if err := healthz.Start(); err != nil { return err } - // Handles safe termination of sm util.StartInWaitGroupWithContext(smb.ctx, func(c context.Context) { <-c.Done() log.C(c).Debug("Context cancelled. Stopping health checks...") if err := healthz.Stop(); err != nil { - log.D().Error(err) + log.C(c).Error(err) } }, smb.wg) diff --git a/storage/healthcheck.go b/storage/healthcheck.go index 3d6988e45..f2847ae42 100644 --- a/storage/healthcheck.go +++ b/storage/healthcheck.go @@ -19,11 +19,14 @@ package storage import ( "github.com/InVisionApp/go-health/checkers" "github.com/Peripli/service-manager/pkg/health" + "time" ) // HealthIndicator returns a new indicator for the storage type HealthIndicator struct { - checkers.SQL + *checkers.SQL + + settings *health.IndicatorSettings } // Name returns the name of the storage component @@ -31,6 +34,22 @@ func (i *HealthIndicator) Name() string { return "storage" } +func (i *HealthIndicator) Configure(settings *health.IndicatorSettings) { + i.settings = settings +} + +func (i *HealthIndicator) Interval() time.Duration { + return i.settings.Interval +} + +func (i *HealthIndicator) FailuresTreshold() int64 { + return i.settings.FailuresTreshold +} + +func (i *HealthIndicator) Fatal() bool { + return i.settings.Fatal +} + func NewStorageHealthIndicator(pingFunc PingFunc) (health.Indicator, error) { sqlConfig := &checkers.SQLConfig{ Pinger: pingFunc, @@ -41,7 +60,7 @@ func NewStorageHealthIndicator(pingFunc PingFunc) (health.Indicator, error) { } indicator := &HealthIndicator{ - SQL: *sqlChecker, + SQL: sqlChecker, } return indicator, nil diff --git a/storage/healthcheck_test.go b/storage/healthcheck_test.go index 4b7d15a56..0a178d0c7 100644 --- a/storage/healthcheck_test.go +++ b/storage/healthcheck_test.go @@ -32,7 +32,9 @@ var _ = Describe("Healthcheck", func() { ping := func(ctx context.Context) error { return nil } - healthIndicator, _ = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) + var err error + healthIndicator, err = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) + Expect(err).ShouldNot(HaveOccurred()) }) Context("Name", func() { @@ -53,7 +55,9 @@ var _ = Describe("Healthcheck", func() { ping := func(ctx context.Context) error { return expectedError } - healthIndicator, _ = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) + var err error + healthIndicator, err = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) + Expect(err).ShouldNot(HaveOccurred()) }) It("Contains error", func() { _, err := healthIndicator.Status() From 30ab52e0f53878c66a0472a5fb1df2bd82b10d03 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 25 Jun 2019 11:43:37 +0300 Subject: [PATCH 10/34] Add health status listener --- pkg/health/settings_test.go | 87 +++++++++++++++++++++++++++++++++++++ pkg/health/types.go | 12 +++++ pkg/sm/sm.go | 4 ++ 3 files changed, 103 insertions(+) create mode 100644 pkg/health/settings_test.go diff --git a/pkg/health/settings_test.go b/pkg/health/settings_test.go new file mode 100644 index 000000000..f40b3f930 --- /dev/null +++ b/pkg/health/settings_test.go @@ -0,0 +1,87 @@ +/* + * Copyright 2018 The Service Manager Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package health + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "time" +) + +var _ = Describe("Healthcheck Settings", func() { + + var settings *Settings + var fatal bool + var failuresTreshold int64 + var interval time.Duration + + BeforeEach(func() { + settings = DefaultSettings() + + fatal = false + failuresTreshold = 1 + interval = 30 + }) + + var registerIndicatorSettings = func() { + indicatorSettings := &IndicatorSettings{ + Fatal: fatal, + FailuresTreshold: failuresTreshold, + Interval: interval, + } + + settings.IndicatorsSettings["test"] = indicatorSettings + } + + var assertValidationErrorOccured = func(err error) { + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("validate Settings")) + } + + When("Indicator with negative treshold", func() { + It("Should be invalid", func() { + failuresTreshold = -1 + registerIndicatorSettings() + + err := settings.Validate() + + assertValidationErrorOccured(err) + }) + }) + + When("Indicator with 0 treshold", func() { + It("Should be invalid", func() { + failuresTreshold = 0 + registerIndicatorSettings() + + err := settings.Validate() + + assertValidationErrorOccured(err) + }) + }) + + When("Indicator with interval less than 30", func() { + It("Should be invalid", func() { + interval = 15 + registerIndicatorSettings() + + err := settings.Validate() + + assertValidationErrorOccured(err) + }) + }) +}) diff --git a/pkg/health/types.go b/pkg/health/types.go index 440f54b5f..4f8c451de 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -18,6 +18,8 @@ package health import ( "fmt" + "github.com/InVisionApp/go-health" + "github.com/Peripli/service-manager/pkg/log" "time" ) @@ -83,6 +85,16 @@ const ( StatusUnknown Status = "UNKNOWN" ) +type StatusListener struct{} + +func (sl *StatusListener) HealthCheckFailed(state *health.State) { + log.D().Errorf("Health check for %v failed with: %v", state.Name, state.Err) +} + +func (sl *StatusListener) HealthCheckRecovered(state *health.State, numberOfFailures int64, unavailableDuration float64) { + log.D().Infof("Health check for %v recovered after %v failures and was unavailable for %v seconds roughly", state.Name, numberOfFailures, unavailableDuration) +} + // Health contains information about the health of a component. type Health struct { Status Status `json:"status"` diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index d2394f753..e318081ea 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -23,6 +23,7 @@ import ( "fmt" h "github.com/InVisionApp/go-health" l "github.com/InVisionApp/go-logger/shims/logrus" + "github.com/Peripli/service-manager/pkg/health" "net" "net/http" "sync" @@ -216,7 +217,9 @@ func (smb *ServiceManagerBuilder) installHealth() error { healthz := h.New() logger := log.C(smb.ctx).Logger + healthz.Logger = l.New(logger) + healthz.StatusListener = &health.StatusListener{} for _, indicator := range smb.HealthIndicators { if err := healthz.AddCheck(&h.Config{ @@ -228,6 +231,7 @@ func (smb *ServiceManagerBuilder) installHealth() error { return err } } + smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthIndicators)) if err := healthz.Start(); err != nil { From d25a87dc9917926891b0c3a84e9d738a6fe78f19 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 25 Jun 2019 12:41:08 +0300 Subject: [PATCH 11/34] Minor tweaks --- api/healthcheck/healthcheck_controller.go | 2 +- pkg/health/registry_test.go | 2 -- pkg/health/settings_test.go | 12 ++++++++++++ pkg/health/types.go | 18 ------------------ 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index 0d10c134f..95f4975a7 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -62,7 +62,7 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { func (c *controller) aggregate(state map[string]h.State) *health.Health { if len(state) == 0 { - return health.New().WithDetail("error", "no health indicators registered").Unknown() + return health.New().WithDetail("error", "no health indicators registered") } overallStatus := health.StatusUp for i, v := range state { diff --git a/pkg/health/registry_test.go b/pkg/health/registry_test.go index cd38ad351..f8c98da6a 100644 --- a/pkg/health/registry_test.go +++ b/pkg/health/registry_test.go @@ -61,9 +61,7 @@ var _ = Describe("Healthcheck Registry", func() { Expect(newIndicator.Interval()).To(Equal(DefaultIndicatorSettings().Interval)) Expect(newIndicator.FailuresTreshold()).To(Equal(DefaultIndicatorSettings().FailuresTreshold)) }) - }) - When("When configure indicators", func() { It("Should configure with provided settings", func() { newIndicator := &testIndicator{} registry.HealthIndicators = append(registry.HealthIndicators, newIndicator) diff --git a/pkg/health/settings_test.go b/pkg/health/settings_test.go index f40b3f930..4436f3d90 100644 --- a/pkg/health/settings_test.go +++ b/pkg/health/settings_test.go @@ -84,4 +84,16 @@ var _ = Describe("Healthcheck Settings", func() { assertValidationErrorOccured(err) }) }) + + When("Indicator with positive treshold and interval > 30", func() { + It("Should be considered valid", func() { + interval = 30 + failuresTreshold = 3 + registerIndicatorSettings() + + err := settings.Validate() + + Expect(err).ShouldNot(HaveOccurred()) + }) + }) }) diff --git a/pkg/health/types.go b/pkg/health/types.go index 4f8c451de..54c105fe4 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -127,24 +127,6 @@ func (h *Health) WithDetail(key string, val interface{}) *Health { return h } -// Up sets the health status to up -func (h *Health) Up() *Health { - h.Status = StatusUp - return h -} - -// Down sets the health status to down -func (h *Health) Down() *Health { - h.Status = StatusDown - return h -} - -// Unknown sets the health status to unknown -func (h *Health) Unknown() *Health { - h.Status = StatusUnknown - return h -} - // WithDetails adds the given details to the health func (h *Health) WithDetails(details map[string]interface{}) *Health { for k, v := range details { From 794c3b08a0c53bb2612f2599d7589ed475f6c3bc Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 8 Jul 2019 09:44:54 +0300 Subject: [PATCH 12/34] Fix indicator interval type --- pkg/health/types.go | 2 +- pkg/sm/sm.go | 13 ++----------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/pkg/health/types.go b/pkg/health/types.go index 54c105fe4..0d127cc6b 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -67,7 +67,7 @@ func (is *IndicatorSettings) Validate() error { if is.FailuresTreshold <= 0 { return fmt.Errorf("validate Settings: FailuresTreshold must be > 0") } - if is.Interval < 30 { + if is.Interval < 30*time.Second { return fmt.Errorf("validate Settings: Minimum interval is 30 seconds") } return nil diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index e318081ea..d433860c1 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -23,13 +23,11 @@ import ( "fmt" h "github.com/InVisionApp/go-health" l "github.com/InVisionApp/go-logger/shims/logrus" + "github.com/Peripli/service-manager/api/osb" "github.com/Peripli/service-manager/pkg/health" "net" "net/http" "sync" - "time" - - "github.com/Peripli/service-manager/api/osb" "github.com/Peripli/service-manager/storage/catalog" @@ -159,11 +157,6 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( cfg: cfg, } - err = smb.installHealth(cfg.Health) - if err != nil { - return nil, fmt.Errorf("error adding health chech to sm: %s", err) - } - // Register default interceptors that represent the core SM business logic smb. WithCreateInterceptorProvider(types.ServiceBrokerType, &interceptors.BrokerCreateCatalogInterceptorProvider{ @@ -194,8 +187,6 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager { } // setup server and add relevant global middleware - smb.installHealth() - srv := server.New(smb.cfg.Server, smb.API) srv.Use(filters.NewRecoveryMiddleware()) @@ -225,7 +216,7 @@ func (smb *ServiceManagerBuilder) installHealth() error { if err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, - Interval: indicator.Interval() * time.Second, + Interval: indicator.Interval(), Fatal: indicator.Fatal(), }); err != nil { return err From 40fb02b9a6d757e1c7659bd7f5daf41faac327f6 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 8 Jul 2019 10:15:20 +0300 Subject: [PATCH 13/34] Fix tests --- config/config_test.go | 55 +++++++++++++++++++++ pkg/health/ping.go | 2 +- pkg/health/settings_test.go | 99 ------------------------------------- 3 files changed, 56 insertions(+), 100 deletions(-) delete mode 100644 pkg/health/settings_test.go diff --git a/config/config_test.go b/config/config_test.go index ac4a24a7e..1ebe67b18 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -18,6 +18,7 @@ package config_test import ( "fmt" + "github.com/Peripli/service-manager/pkg/health" "testing" "time" @@ -44,11 +45,25 @@ var _ = Describe("config", func() { ) Describe("Validate", func() { + var fatal bool + var failuresTreshold int64 + var interval time.Duration + assertErrorDuringValidate := func() { err = config.Validate() Expect(err).To(HaveOccurred()) } + registerIndicatorSettings := func() { + indicatorSettings := &health.IndicatorSettings{ + Fatal: fatal, + FailuresTreshold: failuresTreshold, + Interval: interval, + } + + config.Health.IndicatorsSettings["test"] = indicatorSettings + } + BeforeEach(func() { config = cfg.DefaultSettings() config.Storage.URI = "postgres://postgres:postgres@localhost:5555/postgres?sslmode=disable" @@ -56,6 +71,46 @@ var _ = Describe("config", func() { config.API.ClientID = "sm" config.API.SkipSSLValidation = true config.Storage.EncryptionKey = "ejHjRNHbS0NaqARSRvnweVV9zcmhQEa8" + + fatal = false + failuresTreshold = 1 + interval = 30 * time.Second + }) + + Context("health indicator with negative treshold", func() { + It("should be considered invalid", func() { + failuresTreshold = -1 + registerIndicatorSettings() + assertErrorDuringValidate() + }) + }) + + Context("health indicator with 0 treshold", func() { + It("should be considered invalid", func() { + failuresTreshold = 0 + registerIndicatorSettings() + assertErrorDuringValidate() + }) + }) + + Context("health indicator with interval less than 30", func() { + It("should be considered invalid", func() { + interval = 15 * time.Second + registerIndicatorSettings() + assertErrorDuringValidate() + }) + }) + + Context("health indicator with positive treshold and interval >= 30", func() { + It("should be considered valid", func() { + interval = 30 * time.Second + failuresTreshold = 3 + registerIndicatorSettings() + + err := config.Validate() + + Expect(err).ShouldNot(HaveOccurred()) + }) }) Context("when config is valid", func() { diff --git a/pkg/health/ping.go b/pkg/health/ping.go index ce55fbcaf..f1e169b22 100644 --- a/pkg/health/ping.go +++ b/pkg/health/ping.go @@ -27,7 +27,7 @@ func (*pingIndicator) Name() string { } func (*pingIndicator) Interval() time.Duration { - return 30 + return 30 * time.Second } func (*pingIndicator) FailuresTreshold() int64 { diff --git a/pkg/health/settings_test.go b/pkg/health/settings_test.go deleted file mode 100644 index 4436f3d90..000000000 --- a/pkg/health/settings_test.go +++ /dev/null @@ -1,99 +0,0 @@ -/* - * Copyright 2018 The Service Manager Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package health - -import ( - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" - "time" -) - -var _ = Describe("Healthcheck Settings", func() { - - var settings *Settings - var fatal bool - var failuresTreshold int64 - var interval time.Duration - - BeforeEach(func() { - settings = DefaultSettings() - - fatal = false - failuresTreshold = 1 - interval = 30 - }) - - var registerIndicatorSettings = func() { - indicatorSettings := &IndicatorSettings{ - Fatal: fatal, - FailuresTreshold: failuresTreshold, - Interval: interval, - } - - settings.IndicatorsSettings["test"] = indicatorSettings - } - - var assertValidationErrorOccured = func(err error) { - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("validate Settings")) - } - - When("Indicator with negative treshold", func() { - It("Should be invalid", func() { - failuresTreshold = -1 - registerIndicatorSettings() - - err := settings.Validate() - - assertValidationErrorOccured(err) - }) - }) - - When("Indicator with 0 treshold", func() { - It("Should be invalid", func() { - failuresTreshold = 0 - registerIndicatorSettings() - - err := settings.Validate() - - assertValidationErrorOccured(err) - }) - }) - - When("Indicator with interval less than 30", func() { - It("Should be invalid", func() { - interval = 15 - registerIndicatorSettings() - - err := settings.Validate() - - assertValidationErrorOccured(err) - }) - }) - - When("Indicator with positive treshold and interval > 30", func() { - It("Should be considered valid", func() { - interval = 30 - failuresTreshold = 3 - registerIndicatorSettings() - - err := settings.Validate() - - Expect(err).ShouldNot(HaveOccurred()) - }) - }) -}) From 37c03ddb81a31880e51bb94f0a7ecfd514613168 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 20 Aug 2019 11:22:49 +0300 Subject: [PATCH 14/34] Remove unused import --- api/healthcheck/healthcheck_controller_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index a91325e25..e12546c0f 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -24,8 +24,6 @@ import ( "net/http" "testing" - //"github.com/Peripli/service-manager/pkg/web" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) From f1388078c8b5b747a2a8020f69bc0b9e5eca67be Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 20 Aug 2019 14:39:32 +0300 Subject: [PATCH 15/34] Extract indicator configuration and address PR comments --- api/healthcheck/healthcheck_controller.go | 6 +--- .../healthcheck_controller_test.go | 29 ------------------- pkg/health/registry_test.go | 29 ------------------- pkg/health/types.go | 13 --------- pkg/sm/sm.go | 16 +++++++--- 5 files changed, 13 insertions(+), 80 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index 95f4975a7..b560aec9d 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -33,11 +33,7 @@ type controller struct { } // NewController returns a new healthcheck controller with the given health and tresholds -func NewController(health h.IHealth, indicators []health.Indicator) web.Controller { - tresholds := make(map[string]int64) - for _, v := range indicators { - tresholds[v.Name()] = v.FailuresTreshold() - } +func NewController(health h.IHealth, tresholds map[string]int64) web.Controller { return &controller{ health: health, tresholds: tresholds, diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index e12546c0f..5de22ea69 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -19,7 +19,6 @@ import ( "fmt" h "github.com/InVisionApp/go-health" "github.com/Peripli/service-manager/pkg/health" - "github.com/Peripli/service-manager/pkg/health/healthfakes" "github.com/Peripli/service-manager/pkg/web" "net/http" "testing" @@ -138,34 +137,6 @@ var _ = Describe("Healthcheck controller", func() { }) }) }) - - Describe("create controller", func() { - var c web.Controller - tresholds := map[string]int64{ - "test1": 2, - "test2": 3, - } - - BeforeEach(func() { - indicators := make([]health.Indicator, 0, len(tresholds)) - for i, v := range tresholds { - indicator := &healthfakes.FakeIndicator{} - indicator.NameReturns(i) - indicator.FailuresTresholdReturns(v) - - indicators = append(indicators, indicator) - } - c = NewController(HealthFake{}, indicators) - }) - - When("Controller created with given indicators", func() { - It("Should extract tresholds", func() { - controllerStruct := c.(*controller) - - Expect(controllerStruct.tresholds).To(Equal(tresholds)) - }) - }) - }) }) func createController(status health.Status) *controller { diff --git a/pkg/health/registry_test.go b/pkg/health/registry_test.go index f8c98da6a..7b1f8044b 100644 --- a/pkg/health/registry_test.go +++ b/pkg/health/registry_test.go @@ -50,35 +50,6 @@ var _ = Describe("Healthcheck Registry", func() { Expect(postAddIndicators).To(ContainElement(newIndicator)) }) }) - - When("When configure indicators", func() { - It("Should configure with default settings if settings not provided", func() { - newIndicator := &testIndicator{} - registry.HealthIndicators = append(registry.HealthIndicators, newIndicator) - - registry.ConfigureIndicators() - - Expect(newIndicator.Interval()).To(Equal(DefaultIndicatorSettings().Interval)) - Expect(newIndicator.FailuresTreshold()).To(Equal(DefaultIndicatorSettings().FailuresTreshold)) - }) - - It("Should configure with provided settings", func() { - newIndicator := &testIndicator{} - registry.HealthIndicators = append(registry.HealthIndicators, newIndicator) - - settings := &IndicatorSettings{ - Interval: 50, - FailuresTreshold: 2, - } - - registry.HealthSettings[newIndicator.Name()] = settings - - registry.ConfigureIndicators() - - Expect(newIndicator.Interval()).To(Equal(settings.Interval)) - Expect(newIndicator.FailuresTreshold()).To(Equal(settings.FailuresTreshold)) - }) - }) }) type testIndicator struct { diff --git a/pkg/health/types.go b/pkg/health/types.go index 0d127cc6b..985b36f6d 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -180,16 +180,3 @@ type Registry struct { // Indicator Settings of the registry HealthSettings map[string]*IndicatorSettings } - -// ConfigureIndicators configures registry's indicators with provided settings -func (r *Registry) ConfigureIndicators() { - for _, hi := range r.HealthIndicators { - if indicator, ok := hi.(ConfigurableIndicator); ok { - if settings, ok := r.HealthSettings[indicator.Name()]; ok { - indicator.Configure(settings) - } else { - indicator.Configure(DefaultIndicatorSettings()) - } - } - } -} diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index d433860c1..56182b91e 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -183,7 +183,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( // Build builds the Service Manager func (smb *ServiceManagerBuilder) Build() *ServiceManager { if err := smb.installHealth(); err != nil { - panic(err) + log.C(smb.ctx).Panic() } // setup server and add relevant global middleware @@ -204,15 +204,22 @@ func (smb *ServiceManagerBuilder) installHealth() error { return nil } - smb.ConfigureIndicators() - healthz := h.New() logger := log.C(smb.ctx).Logger healthz.Logger = l.New(logger) healthz.StatusListener = &health.StatusListener{} + tresholds := make(map[string]int64) + for _, indicator := range smb.HealthIndicators { + if configurableIndicator, ok := indicator.(health.ConfigurableIndicator); ok { + if settings, ok := smb.HealthSettings[configurableIndicator.Name()]; ok { + configurableIndicator.Configure(settings) + } else { + configurableIndicator.Configure(health.DefaultIndicatorSettings()) + } + } if err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, @@ -221,9 +228,10 @@ func (smb *ServiceManagerBuilder) installHealth() error { }); err != nil { return err } + tresholds[indicator.Name()] = indicator.FailuresTreshold() } - smb.RegisterControllers(healthcheck.NewController(healthz, smb.HealthIndicators)) + smb.RegisterControllers(healthcheck.NewController(healthz, tresholds)) if err := healthz.Start(); err != nil { return err From ce4d4bbe2c076cd2b261788fb27a6a917681af63 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 20 Aug 2019 14:55:36 +0300 Subject: [PATCH 16/34] Add error to panic --- pkg/sm/sm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 56182b91e..a3d0cabe5 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -183,7 +183,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( // Build builds the Service Manager func (smb *ServiceManagerBuilder) Build() *ServiceManager { if err := smb.installHealth(); err != nil { - log.C(smb.ctx).Panic() + log.C(smb.ctx).Panic(err) } // setup server and add relevant global middleware From 3a3438123593a014ff57eb01b2ab04354327762d Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 21 Aug 2019 12:12:37 +0300 Subject: [PATCH 17/34] Address PR comments --- api/healthcheck/healthcheck_controller.go | 28 +- .../healthcheck_controller_test.go | 38 +- config/config_test.go | 47 +- .../fake_configurable_indicator.go | 401 ------------------ pkg/health/healthfakes/fake_indicator.go | 193 --------- pkg/health/ping.go | 43 -- pkg/health/registry_test.go | 22 +- pkg/health/types.go | 50 +-- pkg/sm/sm.go | 26 +- storage/healthcheck.go | 40 +- storage/healthcheck_test.go | 4 +- 11 files changed, 97 insertions(+), 795 deletions(-) delete mode 100644 pkg/health/healthfakes/fake_configurable_indicator.go delete mode 100644 pkg/health/ping.go diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index b560aec9d..9e55f611e 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -28,15 +28,15 @@ import ( // controller platform controller type controller struct { - health h.IHealth - tresholds map[string]int64 + health h.IHealth + thresholds map[string]int64 } -// NewController returns a new healthcheck controller with the given health and tresholds -func NewController(health h.IHealth, tresholds map[string]int64) web.Controller { +// NewController returns a new healthcheck controller with the given health and thresholds +func NewController(health h.IHealth, thresholds map[string]int64) web.Controller { return &controller{ - health: health, - tresholds: tresholds, + health: health, + thresholds: thresholds, } } @@ -56,21 +56,21 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { return util.NewJSONResponse(status, healthResult) } -func (c *controller) aggregate(state map[string]h.State) *health.Health { - if len(state) == 0 { - return health.New().WithDetail("error", "no health indicators registered") +func (c *controller) aggregate(overallState map[string]h.State) *health.Health { + if len(overallState) == 0 { + return health.New().WithStatus(health.StatusUp) } overallStatus := health.StatusUp - for i, v := range state { - if v.Fatal && v.ContiguousFailures >= c.tresholds[i] { + for name, state := range overallState { + if state.Fatal && state.ContiguousFailures >= c.thresholds[name] { overallStatus = health.StatusDown break } } details := make(map[string]interface{}) - for k, v := range state { - v.Status = convertStatus(v.Status) - details[k] = v + for name, state := range overallState { + state.Status = convertStatus(state.Status) + details[name] = state } return health.New().WithStatus(overallStatus).WithDetails(details) } diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index 5de22ea69..fc353d341 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -52,12 +52,6 @@ var _ = Describe("Healthcheck controller", func() { }) }) - When("health returns unknown", func() { - It("should respond with 503", func() { - assertResponse(health.StatusUnknown, http.StatusServiceUnavailable) - }) - }) - When("health returns up", func() { It("should respond with 200", func() { assertResponse(health.StatusUp, http.StatusOK) @@ -68,53 +62,51 @@ var _ = Describe("Healthcheck controller", func() { Describe("aggregation", func() { var c *controller var healths map[string]h.State - var tresholds map[string]int64 + var thresholds map[string]int64 BeforeEach(func() { healths = map[string]h.State{ "test1": {Status: "ok"}, "test2": {Status: "ok"}, } - tresholds = map[string]int64{ + thresholds = map[string]int64{ "test1": 3, "test2": 3, } c = &controller{ - health: HealthFake{}, - tresholds: tresholds, + health: HealthFake{}, + thresholds: thresholds, } }) When("No healths are provided", func() { - It("Returns UNKNOWN and an error detail", func() { + It("Returns UP", func() { aggregatedHealth := c.aggregate(nil) - Expect(aggregatedHealth.Status).To(Equal(health.StatusUnknown)) - Expect(aggregatedHealth.Details["error"]).ToNot(BeNil()) + Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) }) - When("At least one health is DOWN more than treshold times and is Fatal", func() { + When("At least one health is DOWN more than threshold times and is Fatal", func() { It("Returns DOWN", func() { healths["test3"] = h.State{Status: "failed", Fatal: true, ContiguousFailures: 4} - c.tresholds["test3"] = 3 + c.thresholds["test3"] = 3 aggregatedHealth := c.aggregate(healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusDown)) }) }) - When("At least one health is DOWN more than treshold times and is not Fatal", func() { + When("At least one health is DOWN and is not Fatal", func() { It("Returns UP", func() { healths["test3"] = h.State{Status: "failed", Fatal: false, ContiguousFailures: 4} - c.tresholds["test3"] = 3 aggregatedHealth := c.aggregate(healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) }) - When("There is DOWN healths but not more than treshold times in a row", func() { + When("There is DOWN healths but not more than threshold times in a row", func() { It("Returns UP", func() { healths["test3"] = h.State{Status: "failed"} - c.tresholds["test3"] = 3 + c.thresholds["test3"] = 3 aggregatedHealth := c.aggregate(healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) @@ -140,12 +132,6 @@ var _ = Describe("Healthcheck controller", func() { }) func createController(status health.Status) *controller { - if status == health.StatusUnknown { - return &controller{ - health: HealthFake{}, - } - } - stringStatus := "ok" var contiguousFailures int64 = 0 if status == health.StatusDown { @@ -159,7 +145,7 @@ func createController(status health.Status) *controller { "test1": {Status: stringStatus, Fatal: true, ContiguousFailures: contiguousFailures}, }, }, - tresholds: map[string]int64{ + thresholds: map[string]int64{ "test1": 1, }, } diff --git a/config/config_test.go b/config/config_test.go index 1ebe67b18..a4f32fd3f 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -46,7 +46,7 @@ var _ = Describe("config", func() { Describe("Validate", func() { var fatal bool - var failuresTreshold int64 + var failuresThreshold int64 var interval time.Duration assertErrorDuringValidate := func() { @@ -56,12 +56,12 @@ var _ = Describe("config", func() { registerIndicatorSettings := func() { indicatorSettings := &health.IndicatorSettings{ - Fatal: fatal, - FailuresTreshold: failuresTreshold, - Interval: interval, + Fatal: fatal, + FailuresThreshold: failuresThreshold, + Interval: interval, } - config.Health.IndicatorsSettings["test"] = indicatorSettings + config.Health.Indicators["test"] = indicatorSettings } BeforeEach(func() { @@ -72,22 +72,41 @@ var _ = Describe("config", func() { config.API.SkipSSLValidation = true config.Storage.EncryptionKey = "ejHjRNHbS0NaqARSRvnweVV9zcmhQEa8" - fatal = false - failuresTreshold = 1 + fatal = true + failuresThreshold = 1 interval = 30 * time.Second }) - Context("health indicator with negative treshold", func() { + Context("health indicator with negative threshold", func() { It("should be considered invalid", func() { - failuresTreshold = -1 + failuresThreshold = -1 registerIndicatorSettings() assertErrorDuringValidate() }) }) - Context("health indicator with 0 treshold", func() { - It("should be considered invalid", func() { - failuresTreshold = 0 + Context("health indicator with 0 threshold", func() { + It("should be considered invalid if it is fatal", func() { + failuresThreshold = 0 + registerIndicatorSettings() + assertErrorDuringValidate() + }) + }) + + Context("health indicator with 0 threshold", func() { + It("should be considered valid if it is not fatal", func() { + fatal = false + failuresThreshold = 0 + registerIndicatorSettings() + err := config.Validate() + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + + Context("health indicator with positive threshold", func() { + It("should be considered invalid if it is not fatal", func() { + fatal = false + failuresThreshold = 3 registerIndicatorSettings() assertErrorDuringValidate() }) @@ -101,10 +120,10 @@ var _ = Describe("config", func() { }) }) - Context("health indicator with positive treshold and interval >= 30", func() { + Context("health indicator with positive threshold and interval >= 30", func() { It("should be considered valid", func() { interval = 30 * time.Second - failuresTreshold = 3 + failuresThreshold = 3 registerIndicatorSettings() err := config.Validate() diff --git a/pkg/health/healthfakes/fake_configurable_indicator.go b/pkg/health/healthfakes/fake_configurable_indicator.go deleted file mode 100644 index 05876bd5a..000000000 --- a/pkg/health/healthfakes/fake_configurable_indicator.go +++ /dev/null @@ -1,401 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package healthfakes - -import ( - "sync" - "time" - - "github.com/Peripli/service-manager/pkg/health" -) - -type FakeConfigurableIndicator struct { - ConfigureStub func(*health.IndicatorSettings) - configureMutex sync.RWMutex - configureArgsForCall []struct { - arg1 *health.IndicatorSettings - } - FailuresTresholdStub func() int64 - failuresTresholdMutex sync.RWMutex - failuresTresholdArgsForCall []struct { - } - failuresTresholdReturns struct { - result1 int64 - } - failuresTresholdReturnsOnCall map[int]struct { - result1 int64 - } - FatalStub func() bool - fatalMutex sync.RWMutex - fatalArgsForCall []struct { - } - fatalReturns struct { - result1 bool - } - fatalReturnsOnCall map[int]struct { - result1 bool - } - IntervalStub func() time.Duration - intervalMutex sync.RWMutex - intervalArgsForCall []struct { - } - intervalReturns struct { - result1 time.Duration - } - intervalReturnsOnCall map[int]struct { - result1 time.Duration - } - NameStub func() string - nameMutex sync.RWMutex - nameArgsForCall []struct { - } - nameReturns struct { - result1 string - } - nameReturnsOnCall map[int]struct { - result1 string - } - StatusStub func() (interface{}, error) - statusMutex sync.RWMutex - statusArgsForCall []struct { - } - statusReturns struct { - result1 interface{} - result2 error - } - statusReturnsOnCall map[int]struct { - result1 interface{} - result2 error - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeConfigurableIndicator) Configure(arg1 *health.IndicatorSettings) { - fake.configureMutex.Lock() - fake.configureArgsForCall = append(fake.configureArgsForCall, struct { - arg1 *health.IndicatorSettings - }{arg1}) - fake.recordInvocation("Configure", []interface{}{arg1}) - fake.configureMutex.Unlock() - if fake.ConfigureStub != nil { - fake.ConfigureStub(arg1) - } -} - -func (fake *FakeConfigurableIndicator) ConfigureCallCount() int { - fake.configureMutex.RLock() - defer fake.configureMutex.RUnlock() - return len(fake.configureArgsForCall) -} - -func (fake *FakeConfigurableIndicator) ConfigureCalls(stub func(*health.IndicatorSettings)) { - fake.configureMutex.Lock() - defer fake.configureMutex.Unlock() - fake.ConfigureStub = stub -} - -func (fake *FakeConfigurableIndicator) ConfigureArgsForCall(i int) *health.IndicatorSettings { - fake.configureMutex.RLock() - defer fake.configureMutex.RUnlock() - argsForCall := fake.configureArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeConfigurableIndicator) FailuresTreshold() int64 { - fake.failuresTresholdMutex.Lock() - ret, specificReturn := fake.failuresTresholdReturnsOnCall[len(fake.failuresTresholdArgsForCall)] - fake.failuresTresholdArgsForCall = append(fake.failuresTresholdArgsForCall, struct { - }{}) - fake.recordInvocation("FailuresTreshold", []interface{}{}) - fake.failuresTresholdMutex.Unlock() - if fake.FailuresTresholdStub != nil { - return fake.FailuresTresholdStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.failuresTresholdReturns - return fakeReturns.result1 -} - -func (fake *FakeConfigurableIndicator) FailuresTresholdCallCount() int { - fake.failuresTresholdMutex.RLock() - defer fake.failuresTresholdMutex.RUnlock() - return len(fake.failuresTresholdArgsForCall) -} - -func (fake *FakeConfigurableIndicator) FailuresTresholdCalls(stub func() int64) { - fake.failuresTresholdMutex.Lock() - defer fake.failuresTresholdMutex.Unlock() - fake.FailuresTresholdStub = stub -} - -func (fake *FakeConfigurableIndicator) FailuresTresholdReturns(result1 int64) { - fake.failuresTresholdMutex.Lock() - defer fake.failuresTresholdMutex.Unlock() - fake.FailuresTresholdStub = nil - fake.failuresTresholdReturns = struct { - result1 int64 - }{result1} -} - -func (fake *FakeConfigurableIndicator) FailuresTresholdReturnsOnCall(i int, result1 int64) { - fake.failuresTresholdMutex.Lock() - defer fake.failuresTresholdMutex.Unlock() - fake.FailuresTresholdStub = nil - if fake.failuresTresholdReturnsOnCall == nil { - fake.failuresTresholdReturnsOnCall = make(map[int]struct { - result1 int64 - }) - } - fake.failuresTresholdReturnsOnCall[i] = struct { - result1 int64 - }{result1} -} - -func (fake *FakeConfigurableIndicator) Fatal() bool { - fake.fatalMutex.Lock() - ret, specificReturn := fake.fatalReturnsOnCall[len(fake.fatalArgsForCall)] - fake.fatalArgsForCall = append(fake.fatalArgsForCall, struct { - }{}) - fake.recordInvocation("Fatal", []interface{}{}) - fake.fatalMutex.Unlock() - if fake.FatalStub != nil { - return fake.FatalStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.fatalReturns - return fakeReturns.result1 -} - -func (fake *FakeConfigurableIndicator) FatalCallCount() int { - fake.fatalMutex.RLock() - defer fake.fatalMutex.RUnlock() - return len(fake.fatalArgsForCall) -} - -func (fake *FakeConfigurableIndicator) FatalCalls(stub func() bool) { - fake.fatalMutex.Lock() - defer fake.fatalMutex.Unlock() - fake.FatalStub = stub -} - -func (fake *FakeConfigurableIndicator) FatalReturns(result1 bool) { - fake.fatalMutex.Lock() - defer fake.fatalMutex.Unlock() - fake.FatalStub = nil - fake.fatalReturns = struct { - result1 bool - }{result1} -} - -func (fake *FakeConfigurableIndicator) FatalReturnsOnCall(i int, result1 bool) { - fake.fatalMutex.Lock() - defer fake.fatalMutex.Unlock() - fake.FatalStub = nil - if fake.fatalReturnsOnCall == nil { - fake.fatalReturnsOnCall = make(map[int]struct { - result1 bool - }) - } - fake.fatalReturnsOnCall[i] = struct { - result1 bool - }{result1} -} - -func (fake *FakeConfigurableIndicator) Interval() time.Duration { - fake.intervalMutex.Lock() - ret, specificReturn := fake.intervalReturnsOnCall[len(fake.intervalArgsForCall)] - fake.intervalArgsForCall = append(fake.intervalArgsForCall, struct { - }{}) - fake.recordInvocation("Interval", []interface{}{}) - fake.intervalMutex.Unlock() - if fake.IntervalStub != nil { - return fake.IntervalStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.intervalReturns - return fakeReturns.result1 -} - -func (fake *FakeConfigurableIndicator) IntervalCallCount() int { - fake.intervalMutex.RLock() - defer fake.intervalMutex.RUnlock() - return len(fake.intervalArgsForCall) -} - -func (fake *FakeConfigurableIndicator) IntervalCalls(stub func() time.Duration) { - fake.intervalMutex.Lock() - defer fake.intervalMutex.Unlock() - fake.IntervalStub = stub -} - -func (fake *FakeConfigurableIndicator) IntervalReturns(result1 time.Duration) { - fake.intervalMutex.Lock() - defer fake.intervalMutex.Unlock() - fake.IntervalStub = nil - fake.intervalReturns = struct { - result1 time.Duration - }{result1} -} - -func (fake *FakeConfigurableIndicator) IntervalReturnsOnCall(i int, result1 time.Duration) { - fake.intervalMutex.Lock() - defer fake.intervalMutex.Unlock() - fake.IntervalStub = nil - if fake.intervalReturnsOnCall == nil { - fake.intervalReturnsOnCall = make(map[int]struct { - result1 time.Duration - }) - } - fake.intervalReturnsOnCall[i] = struct { - result1 time.Duration - }{result1} -} - -func (fake *FakeConfigurableIndicator) Name() string { - fake.nameMutex.Lock() - ret, specificReturn := fake.nameReturnsOnCall[len(fake.nameArgsForCall)] - fake.nameArgsForCall = append(fake.nameArgsForCall, struct { - }{}) - fake.recordInvocation("Name", []interface{}{}) - fake.nameMutex.Unlock() - if fake.NameStub != nil { - return fake.NameStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.nameReturns - return fakeReturns.result1 -} - -func (fake *FakeConfigurableIndicator) NameCallCount() int { - fake.nameMutex.RLock() - defer fake.nameMutex.RUnlock() - return len(fake.nameArgsForCall) -} - -func (fake *FakeConfigurableIndicator) NameCalls(stub func() string) { - fake.nameMutex.Lock() - defer fake.nameMutex.Unlock() - fake.NameStub = stub -} - -func (fake *FakeConfigurableIndicator) NameReturns(result1 string) { - fake.nameMutex.Lock() - defer fake.nameMutex.Unlock() - fake.NameStub = nil - fake.nameReturns = struct { - result1 string - }{result1} -} - -func (fake *FakeConfigurableIndicator) NameReturnsOnCall(i int, result1 string) { - fake.nameMutex.Lock() - defer fake.nameMutex.Unlock() - fake.NameStub = nil - if fake.nameReturnsOnCall == nil { - fake.nameReturnsOnCall = make(map[int]struct { - result1 string - }) - } - fake.nameReturnsOnCall[i] = struct { - result1 string - }{result1} -} - -func (fake *FakeConfigurableIndicator) Status() (interface{}, error) { - fake.statusMutex.Lock() - ret, specificReturn := fake.statusReturnsOnCall[len(fake.statusArgsForCall)] - fake.statusArgsForCall = append(fake.statusArgsForCall, struct { - }{}) - fake.recordInvocation("Status", []interface{}{}) - fake.statusMutex.Unlock() - if fake.StatusStub != nil { - return fake.StatusStub() - } - if specificReturn { - return ret.result1, ret.result2 - } - fakeReturns := fake.statusReturns - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeConfigurableIndicator) StatusCallCount() int { - fake.statusMutex.RLock() - defer fake.statusMutex.RUnlock() - return len(fake.statusArgsForCall) -} - -func (fake *FakeConfigurableIndicator) StatusCalls(stub func() (interface{}, error)) { - fake.statusMutex.Lock() - defer fake.statusMutex.Unlock() - fake.StatusStub = stub -} - -func (fake *FakeConfigurableIndicator) StatusReturns(result1 interface{}, result2 error) { - fake.statusMutex.Lock() - defer fake.statusMutex.Unlock() - fake.StatusStub = nil - fake.statusReturns = struct { - result1 interface{} - result2 error - }{result1, result2} -} - -func (fake *FakeConfigurableIndicator) StatusReturnsOnCall(i int, result1 interface{}, result2 error) { - fake.statusMutex.Lock() - defer fake.statusMutex.Unlock() - fake.StatusStub = nil - if fake.statusReturnsOnCall == nil { - fake.statusReturnsOnCall = make(map[int]struct { - result1 interface{} - result2 error - }) - } - fake.statusReturnsOnCall[i] = struct { - result1 interface{} - result2 error - }{result1, result2} -} - -func (fake *FakeConfigurableIndicator) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.configureMutex.RLock() - defer fake.configureMutex.RUnlock() - fake.failuresTresholdMutex.RLock() - defer fake.failuresTresholdMutex.RUnlock() - fake.fatalMutex.RLock() - defer fake.fatalMutex.RUnlock() - fake.intervalMutex.RLock() - defer fake.intervalMutex.RUnlock() - fake.nameMutex.RLock() - defer fake.nameMutex.RUnlock() - fake.statusMutex.RLock() - defer fake.statusMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeConfigurableIndicator) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ health.ConfigurableIndicator = new(FakeConfigurableIndicator) diff --git a/pkg/health/healthfakes/fake_indicator.go b/pkg/health/healthfakes/fake_indicator.go index f3b08c113..5636134e2 100644 --- a/pkg/health/healthfakes/fake_indicator.go +++ b/pkg/health/healthfakes/fake_indicator.go @@ -3,42 +3,11 @@ package healthfakes import ( "sync" - "time" "github.com/Peripli/service-manager/pkg/health" ) type FakeIndicator struct { - FailuresTresholdStub func() int64 - failuresTresholdMutex sync.RWMutex - failuresTresholdArgsForCall []struct { - } - failuresTresholdReturns struct { - result1 int64 - } - failuresTresholdReturnsOnCall map[int]struct { - result1 int64 - } - FatalStub func() bool - fatalMutex sync.RWMutex - fatalArgsForCall []struct { - } - fatalReturns struct { - result1 bool - } - fatalReturnsOnCall map[int]struct { - result1 bool - } - IntervalStub func() time.Duration - intervalMutex sync.RWMutex - intervalArgsForCall []struct { - } - intervalReturns struct { - result1 time.Duration - } - intervalReturnsOnCall map[int]struct { - result1 time.Duration - } NameStub func() string nameMutex sync.RWMutex nameArgsForCall []struct { @@ -65,162 +34,6 @@ type FakeIndicator struct { invocationsMutex sync.RWMutex } -func (fake *FakeIndicator) FailuresTreshold() int64 { - fake.failuresTresholdMutex.Lock() - ret, specificReturn := fake.failuresTresholdReturnsOnCall[len(fake.failuresTresholdArgsForCall)] - fake.failuresTresholdArgsForCall = append(fake.failuresTresholdArgsForCall, struct { - }{}) - fake.recordInvocation("FailuresTreshold", []interface{}{}) - fake.failuresTresholdMutex.Unlock() - if fake.FailuresTresholdStub != nil { - return fake.FailuresTresholdStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.failuresTresholdReturns - return fakeReturns.result1 -} - -func (fake *FakeIndicator) FailuresTresholdCallCount() int { - fake.failuresTresholdMutex.RLock() - defer fake.failuresTresholdMutex.RUnlock() - return len(fake.failuresTresholdArgsForCall) -} - -func (fake *FakeIndicator) FailuresTresholdCalls(stub func() int64) { - fake.failuresTresholdMutex.Lock() - defer fake.failuresTresholdMutex.Unlock() - fake.FailuresTresholdStub = stub -} - -func (fake *FakeIndicator) FailuresTresholdReturns(result1 int64) { - fake.failuresTresholdMutex.Lock() - defer fake.failuresTresholdMutex.Unlock() - fake.FailuresTresholdStub = nil - fake.failuresTresholdReturns = struct { - result1 int64 - }{result1} -} - -func (fake *FakeIndicator) FailuresTresholdReturnsOnCall(i int, result1 int64) { - fake.failuresTresholdMutex.Lock() - defer fake.failuresTresholdMutex.Unlock() - fake.FailuresTresholdStub = nil - if fake.failuresTresholdReturnsOnCall == nil { - fake.failuresTresholdReturnsOnCall = make(map[int]struct { - result1 int64 - }) - } - fake.failuresTresholdReturnsOnCall[i] = struct { - result1 int64 - }{result1} -} - -func (fake *FakeIndicator) Fatal() bool { - fake.fatalMutex.Lock() - ret, specificReturn := fake.fatalReturnsOnCall[len(fake.fatalArgsForCall)] - fake.fatalArgsForCall = append(fake.fatalArgsForCall, struct { - }{}) - fake.recordInvocation("Fatal", []interface{}{}) - fake.fatalMutex.Unlock() - if fake.FatalStub != nil { - return fake.FatalStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.fatalReturns - return fakeReturns.result1 -} - -func (fake *FakeIndicator) FatalCallCount() int { - fake.fatalMutex.RLock() - defer fake.fatalMutex.RUnlock() - return len(fake.fatalArgsForCall) -} - -func (fake *FakeIndicator) FatalCalls(stub func() bool) { - fake.fatalMutex.Lock() - defer fake.fatalMutex.Unlock() - fake.FatalStub = stub -} - -func (fake *FakeIndicator) FatalReturns(result1 bool) { - fake.fatalMutex.Lock() - defer fake.fatalMutex.Unlock() - fake.FatalStub = nil - fake.fatalReturns = struct { - result1 bool - }{result1} -} - -func (fake *FakeIndicator) FatalReturnsOnCall(i int, result1 bool) { - fake.fatalMutex.Lock() - defer fake.fatalMutex.Unlock() - fake.FatalStub = nil - if fake.fatalReturnsOnCall == nil { - fake.fatalReturnsOnCall = make(map[int]struct { - result1 bool - }) - } - fake.fatalReturnsOnCall[i] = struct { - result1 bool - }{result1} -} - -func (fake *FakeIndicator) Interval() time.Duration { - fake.intervalMutex.Lock() - ret, specificReturn := fake.intervalReturnsOnCall[len(fake.intervalArgsForCall)] - fake.intervalArgsForCall = append(fake.intervalArgsForCall, struct { - }{}) - fake.recordInvocation("Interval", []interface{}{}) - fake.intervalMutex.Unlock() - if fake.IntervalStub != nil { - return fake.IntervalStub() - } - if specificReturn { - return ret.result1 - } - fakeReturns := fake.intervalReturns - return fakeReturns.result1 -} - -func (fake *FakeIndicator) IntervalCallCount() int { - fake.intervalMutex.RLock() - defer fake.intervalMutex.RUnlock() - return len(fake.intervalArgsForCall) -} - -func (fake *FakeIndicator) IntervalCalls(stub func() time.Duration) { - fake.intervalMutex.Lock() - defer fake.intervalMutex.Unlock() - fake.IntervalStub = stub -} - -func (fake *FakeIndicator) IntervalReturns(result1 time.Duration) { - fake.intervalMutex.Lock() - defer fake.intervalMutex.Unlock() - fake.IntervalStub = nil - fake.intervalReturns = struct { - result1 time.Duration - }{result1} -} - -func (fake *FakeIndicator) IntervalReturnsOnCall(i int, result1 time.Duration) { - fake.intervalMutex.Lock() - defer fake.intervalMutex.Unlock() - fake.IntervalStub = nil - if fake.intervalReturnsOnCall == nil { - fake.intervalReturnsOnCall = make(map[int]struct { - result1 time.Duration - }) - } - fake.intervalReturnsOnCall[i] = struct { - result1 time.Duration - }{result1} -} - func (fake *FakeIndicator) Name() string { fake.nameMutex.Lock() ret, specificReturn := fake.nameReturnsOnCall[len(fake.nameArgsForCall)] @@ -331,12 +144,6 @@ func (fake *FakeIndicator) StatusReturnsOnCall(i int, result1 interface{}, resul func (fake *FakeIndicator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.failuresTresholdMutex.RLock() - defer fake.failuresTresholdMutex.RUnlock() - fake.fatalMutex.RLock() - defer fake.fatalMutex.RUnlock() - fake.intervalMutex.RLock() - defer fake.intervalMutex.RUnlock() fake.nameMutex.RLock() defer fake.nameMutex.RUnlock() fake.statusMutex.RLock() diff --git a/pkg/health/ping.go b/pkg/health/ping.go deleted file mode 100644 index f1e169b22..000000000 --- a/pkg/health/ping.go +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright 2018 The Service Manager Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package health - -import "time" - -// pingIndicator is a default indicator that always returns up -type pingIndicator struct { -} - -func (*pingIndicator) Name() string { - return "ping" -} - -func (*pingIndicator) Interval() time.Duration { - return 30 * time.Second -} - -func (*pingIndicator) FailuresTreshold() int64 { - return 1 -} - -func (*pingIndicator) Fatal() bool { - return true -} - -func (*pingIndicator) Status() (interface{}, error) { - return nil, nil -} diff --git a/pkg/health/registry_test.go b/pkg/health/registry_test.go index 7b1f8044b..1bdbbc203 100644 --- a/pkg/health/registry_test.go +++ b/pkg/health/registry_test.go @@ -19,7 +19,6 @@ package health import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "time" ) var _ = Describe("Healthcheck Registry", func() { @@ -31,9 +30,9 @@ var _ = Describe("Healthcheck Registry", func() { }) When("Constructing default registry", func() { - It("Has ping indicator", func() { + It("Has empty indicators", func() { indicators := registry.HealthIndicators - Expect(indicators).To(ConsistOf(&pingIndicator{})) + Expect(len(indicators)).To(Equal(0)) }) }) @@ -53,29 +52,12 @@ var _ = Describe("Healthcheck Registry", func() { }) type testIndicator struct { - settings *IndicatorSettings } func (i *testIndicator) Name() string { return "test" } -func (i *testIndicator) Interval() time.Duration { - return i.settings.Interval -} - -func (i *testIndicator) FailuresTreshold() int64 { - return i.settings.FailuresTreshold -} - -func (i *testIndicator) Fatal() bool { - return i.settings.Fatal -} - func (i *testIndicator) Status() (interface{}, error) { return nil, nil } - -func (i *testIndicator) Configure(settings *IndicatorSettings) { - i.settings = settings -} diff --git a/pkg/health/types.go b/pkg/health/types.go index 985b36f6d..27fd497f1 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -25,20 +25,20 @@ import ( // Settings type to be loaded from the environment type Settings struct { - IndicatorsSettings map[string]*IndicatorSettings `mapstructure:"indicators,omitempty"` + Indicators map[string]*IndicatorSettings `mapstructure:"indicators,omitempty"` } // DefaultSettings returns default values for health settings func DefaultSettings() *Settings { emptySettings := make(map[string]*IndicatorSettings) return &Settings{ - IndicatorsSettings: emptySettings, + Indicators: emptySettings, } } // Validate validates health settings func (s *Settings) Validate() error { - for _, v := range s.IndicatorsSettings { + for _, v := range s.Indicators { if err := v.Validate(); err != nil { return err } @@ -48,24 +48,27 @@ func (s *Settings) Validate() error { // IndicatorSettings type to be loaded from the environment type IndicatorSettings struct { - Fatal bool `mapstructure:"fatal" description:"if the indicator affects the overall status "` - FailuresTreshold int64 `mapstructure:"failures_treshold" description:"maximum failures in a row until component is considered down"` - Interval time.Duration `mapstructure:"interval" description:"time between health checks of components"` + Fatal bool `mapstructure:"fatal" description:"if the indicator affects the overall status, if false not failures_threshold expected"` + FailuresThreshold int64 `mapstructure:"failures_threshold" description:"number of failures in a row that will affect overall status"` + Interval time.Duration `mapstructure:"interval" description:"time between health checks of components"` } // DefaultIndicatorSettings returns default values for indicator settings func DefaultIndicatorSettings() *IndicatorSettings { return &IndicatorSettings{ - Fatal: true, - FailuresTreshold: 3, - Interval: 60, + Fatal: true, + FailuresThreshold: 3, + Interval: 60 * time.Second, } } // Validate validates indicator settings func (is *IndicatorSettings) Validate() error { - if is.FailuresTreshold <= 0 { - return fmt.Errorf("validate Settings: FailuresTreshold must be > 0") + if !is.Fatal && is.FailuresThreshold != 0 { + return fmt.Errorf("validate Settings: FailuresThreshold not applicable for non-fatal indicators") + } + if is.Fatal && is.FailuresThreshold <= 0 { + return fmt.Errorf("validate Settings: FailuresThreshold must be > 0 for fatal indicators") } if is.Interval < 30*time.Second { return fmt.Errorf("validate Settings: Minimum interval is 30 seconds") @@ -141,34 +144,14 @@ type Indicator interface { // Name returns the name of the component Name() string - // Interval returns settings of the indicator - Interval() time.Duration - - // FailuresTreshold returns the maximum failures in a row until component is considered down - FailuresTreshold() int64 - - // Fatal returns if the health indicator is fatal for the overall status - Fatal() bool - // Status returns the health information of the component Status() (interface{}, error) } -// ConfigurableIndicator is an interface to provide configurable health of a component -//go:generate counterfeiter . ConfigurableIndicator -type ConfigurableIndicator interface { - Indicator - - // Configure configures the indicator with given settings - Configure(*IndicatorSettings) -} - // NewDefaultRegistry returns a default health registry with a single ping indicator func NewDefaultRegistry() *Registry { - emptySettings := make(map[string]*IndicatorSettings) return &Registry{ - HealthIndicators: []Indicator{&pingIndicator{}}, - HealthSettings: emptySettings, + HealthIndicators: []Indicator{}, } } @@ -176,7 +159,4 @@ func NewDefaultRegistry() *Registry { type Registry struct { // HealthIndicators are the currently registered health indicators HealthIndicators []Indicator - - // Indicator Settings of the registry - HealthSettings map[string]*IndicatorSettings } diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index a3d0cabe5..5b92b68a6 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -134,13 +134,12 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( return nil, fmt.Errorf("error creating core api: %s", err) } - storageHealthIndicator, err := storage.NewStorageHealthIndicator(storage.PingFunc(smStorage.PingContext)) + storageHealthIndicator, err := storage.NewHealthIndicator(storage.PingFunc(smStorage.PingContext)) if err != nil { return nil, fmt.Errorf("error creating storage health indicator: %s", err) } API.HealthIndicators = append(API.HealthIndicators, storageHealthIndicator) - API.HealthSettings = cfg.Health.IndicatorsSettings notificationCleaner := &storage.NotificationCleaner{ Storage: interceptableRepository, @@ -200,38 +199,31 @@ func (smb *ServiceManagerBuilder) Build() *ServiceManager { } func (smb *ServiceManagerBuilder) installHealth() error { - if len(smb.HealthIndicators) == 0 { - return nil - } - healthz := h.New() logger := log.C(smb.ctx).Logger healthz.Logger = l.New(logger) healthz.StatusListener = &health.StatusListener{} - tresholds := make(map[string]int64) + thresholds := make(map[string]int64) for _, indicator := range smb.HealthIndicators { - if configurableIndicator, ok := indicator.(health.ConfigurableIndicator); ok { - if settings, ok := smb.HealthSettings[configurableIndicator.Name()]; ok { - configurableIndicator.Configure(settings) - } else { - configurableIndicator.Configure(health.DefaultIndicatorSettings()) - } + settings, ok := smb.cfg.Health.Indicators[indicator.Name()] + if !ok { + settings = health.DefaultIndicatorSettings() } if err := healthz.AddCheck(&h.Config{ Name: indicator.Name(), Checker: indicator, - Interval: indicator.Interval(), - Fatal: indicator.Fatal(), + Interval: settings.Interval, + Fatal: settings.Fatal, }); err != nil { return err } - tresholds[indicator.Name()] = indicator.FailuresTreshold() + thresholds[indicator.Name()] = settings.FailuresThreshold } - smb.RegisterControllers(healthcheck.NewController(healthz, tresholds)) + smb.RegisterControllers(healthcheck.NewController(healthz, thresholds)) if err := healthz.Start(); err != nil { return err diff --git a/storage/healthcheck.go b/storage/healthcheck.go index f2847ae42..d44b73a87 100644 --- a/storage/healthcheck.go +++ b/storage/healthcheck.go @@ -19,38 +19,9 @@ package storage import ( "github.com/InVisionApp/go-health/checkers" "github.com/Peripli/service-manager/pkg/health" - "time" ) -// HealthIndicator returns a new indicator for the storage -type HealthIndicator struct { - *checkers.SQL - - settings *health.IndicatorSettings -} - -// Name returns the name of the storage component -func (i *HealthIndicator) Name() string { - return "storage" -} - -func (i *HealthIndicator) Configure(settings *health.IndicatorSettings) { - i.settings = settings -} - -func (i *HealthIndicator) Interval() time.Duration { - return i.settings.Interval -} - -func (i *HealthIndicator) FailuresTreshold() int64 { - return i.settings.FailuresTreshold -} - -func (i *HealthIndicator) Fatal() bool { - return i.settings.Fatal -} - -func NewStorageHealthIndicator(pingFunc PingFunc) (health.Indicator, error) { +func NewHealthIndicator(pingFunc PingFunc) (health.Indicator, error) { sqlConfig := &checkers.SQLConfig{ Pinger: pingFunc, } @@ -64,5 +35,14 @@ func NewStorageHealthIndicator(pingFunc PingFunc) (health.Indicator, error) { } return indicator, nil +} + +// HealthIndicator returns a new indicator for the storage +type HealthIndicator struct { + *checkers.SQL +} +// Name returns the name of the storage component +func (i *HealthIndicator) Name() string { + return "storage" } diff --git a/storage/healthcheck_test.go b/storage/healthcheck_test.go index 0a178d0c7..1ea4d0ac2 100644 --- a/storage/healthcheck_test.go +++ b/storage/healthcheck_test.go @@ -33,7 +33,7 @@ var _ = Describe("Healthcheck", func() { return nil } var err error - healthIndicator, err = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) + healthIndicator, err = storage.NewHealthIndicator(storage.PingFunc(ping)) Expect(err).ShouldNot(HaveOccurred()) }) @@ -56,7 +56,7 @@ var _ = Describe("Healthcheck", func() { return expectedError } var err error - healthIndicator, err = storage.NewStorageHealthIndicator(storage.PingFunc(ping)) + healthIndicator, err = storage.NewHealthIndicator(storage.PingFunc(ping)) Expect(err).ShouldNot(HaveOccurred()) }) It("Contains error", func() { From 91eb2a9efe22dabe0a2571d521935af6d7f6e5f7 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 21 Aug 2019 12:25:58 +0300 Subject: [PATCH 18/34] minor fix --- pkg/health/types.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/health/types.go b/pkg/health/types.go index 27fd497f1..7c1dfba13 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -148,14 +148,14 @@ type Indicator interface { Status() (interface{}, error) } -// NewDefaultRegistry returns a default health registry with a single ping indicator +// NewDefaultRegistry returns a default empty health registry func NewDefaultRegistry() *Registry { return &Registry{ - HealthIndicators: []Indicator{}, + HealthIndicators: make([]Indicator, 0), } } -// Registry is an interface to store and fetch health indicators +// Registry is a struct to store health indicators type Registry struct { // HealthIndicators are the currently registered health indicators HealthIndicators []Indicator From 31f86b7120902973a92328db081dd978bb6b6496 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 21 Aug 2019 10:28:36 +0300 Subject: [PATCH 19/34] Add active status to platform --- api/healthcheck/platforms_indicators.go | 33 ++++++++++++++ pkg/sm/sm.go | 2 +- pkg/types/platform.go | 3 ++ .../012_add_platforms_status.down.sql | 6 +++ .../012_add_platforms_status.up.sql | 6 +++ storage/postgres/notificator.go | 43 ++++++++++++++----- storage/postgres/platform.go | 7 +++ 7 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 api/healthcheck/platforms_indicators.go create mode 100644 storage/postgres/migrations/012_add_platforms_status.down.sql create mode 100644 storage/postgres/migrations/012_add_platforms_status.up.sql diff --git a/api/healthcheck/platforms_indicators.go b/api/healthcheck/platforms_indicators.go new file mode 100644 index 000000000..bcdb0d95e --- /dev/null +++ b/api/healthcheck/platforms_indicators.go @@ -0,0 +1,33 @@ +package healthcheck + +import ( + "github.com/Peripli/service-manager/storage" + "time" +) + +type platformsIndicator struct { + repository storage.Repository +} + +// Name returns the name of the storage component +func (i *platformsIndicator) Name() string { + return "platforms" +} + +func (i *platformsIndicator) Interval() time.Duration { + return 60 * time.Second +} + +func (i *platformsIndicator) FailuresTreshold() int64 { + return 1 +} + +func (i *platformsIndicator) Fatal() bool { + return false +} + +func (i *platformsIndicator) Status() (interface{}, error) { + return nil, nil +} + +//func CreatePlatformsHealthIndicators() diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 5b92b68a6..679e3bf0e 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -118,7 +118,7 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( // Setup core API log.C(ctx).Info("Setting up Service Manager core API...") - pgNotificator, err := postgres.NewNotificator(smStorage, cfg.Storage) + pgNotificator, err := postgres.NewNotificator(smStorage, interceptableRepository, cfg.Storage) if err != nil { return nil, fmt.Errorf("could not create notificator: %v", err) } diff --git a/pkg/types/platform.go b/pkg/types/platform.go index 0653b6e69..4372aeda7 100644 --- a/pkg/types/platform.go +++ b/pkg/types/platform.go @@ -19,6 +19,7 @@ package types import ( "errors" "fmt" + "time" "github.com/Peripli/service-manager/pkg/util" ) @@ -32,6 +33,8 @@ type Platform struct { Name string `json:"name"` Description string `json:"description"` Credentials *Credentials `json:"credentials,omitempty"` + Active bool `json:"-"` + LastActive time.Time `json:"-"` } func (e *Platform) SetCredentials(credentials *Credentials) { diff --git a/storage/postgres/migrations/012_add_platforms_status.down.sql b/storage/postgres/migrations/012_add_platforms_status.down.sql new file mode 100644 index 000000000..0627176db --- /dev/null +++ b/storage/postgres/migrations/012_add_platforms_status.down.sql @@ -0,0 +1,6 @@ +BEGIN; + +ALTER TABLE platforms DROP COLUMN active; +ALTER TABLE platforms DROP COLUMN last_active; + +COMMIT; \ No newline at end of file diff --git a/storage/postgres/migrations/012_add_platforms_status.up.sql b/storage/postgres/migrations/012_add_platforms_status.up.sql new file mode 100644 index 000000000..328cbddfd --- /dev/null +++ b/storage/postgres/migrations/012_add_platforms_status.up.sql @@ -0,0 +1,6 @@ +BEGIN; + +ALTER TABLE platforms ADD COLUMN active BOOLEAN; +ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP; + +COMMIT; \ No newline at end of file diff --git a/storage/postgres/notificator.go b/storage/postgres/notificator.go index 123c9ccde..1b1360061 100644 --- a/storage/postgres/notificator.go +++ b/storage/postgres/notificator.go @@ -66,7 +66,7 @@ type Notificator struct { } // NewNotificator returns new Notificator based on a given NotificatorStorage and desired queue size -func NewNotificator(st storage.Storage, settings *storage.Settings) (*Notificator, error) { +func NewNotificator(st storage.Storage, repository storage.Repository, settings *storage.Settings) (*Notificator, error) { ns, err := NewNotificationStorage(st) connectionCreator := ¬ificationConnectionCreatorImpl{ storageURI: settings.URI, @@ -82,8 +82,9 @@ func NewNotificator(st storage.Storage, settings *storage.Settings) (*Notificato connectionMutex: &sync.Mutex{}, consumersMutex: &sync.Mutex{}, consumers: &consumers{ - queues: make(map[string][]storage.NotificationQueue), - platforms: make([]*types.Platform, 0), + repository: repository, + queues: make(map[string][]storage.NotificationQueue), + platforms: make([]*types.Platform, 0), }, storage: ns, connectionCreator: connectionCreator, @@ -99,6 +100,7 @@ func (n *Notificator) Start(ctx context.Context, group *sync.WaitGroup) error { return errors.New("notificator already started") } n.ctx = ctx + n.consumers.ctx = ctx n.setConnection(n.connectionCreator.NewConnection(func(isConnected bool, err error) { if isConnected { atomic.StoreInt32(&n.isConnected, aTrue) @@ -145,7 +147,9 @@ func (n *Notificator) addConsumer(platform *types.Platform, queue storage.Notifi } n.consumersMutex.Lock() defer n.consumersMutex.Unlock() - n.consumers.Add(platform, queue) + if err := n.consumers.Add(platform, queue); err != nil { + return types.InvalidRevision, err + } return atomic.LoadInt64(&n.lastKnownRevision), nil } @@ -259,7 +263,9 @@ func (n *Notificator) UnregisterConsumer(queue storage.NotificationQueue) error if n.consumers.Len() == 0 { return nil // Consumer already unregistered } - n.consumers.Delete(queue) + if err := n.consumers.Delete(queue); err != nil { + return err + } if n.consumers.Len() == 0 { log.C(n.ctx).Debugf("No notification consumers left. Stop listening to channel %s", postgresChannel) n.stopProcessing() // stop processing notifications as there are no consumers @@ -419,8 +425,10 @@ func (n *Notificator) stopConnection() { } type consumers struct { - queues map[string][]storage.NotificationQueue - platforms []*types.Platform + repository storage.Repository + ctx context.Context + queues map[string][]storage.NotificationQueue + platforms []*types.Platform } func (c *consumers) find(queueID string) (string, int) { @@ -443,10 +451,10 @@ func (c *consumers) ReplaceQueue(queueID string, newQueue storage.NotificationQu return nil } -func (c *consumers) Delete(queue storage.NotificationQueue) { +func (c *consumers) Delete(queue storage.NotificationQueue) error { platformIDToDelete, queueIndex := c.find(queue.ID()) if queueIndex == -1 { - return + return nil } platformConsumers := c.queues[platformIDToDelete] c.queues[platformIDToDelete] = append(platformConsumers[:queueIndex], platformConsumers[queueIndex+1:]...) @@ -456,17 +464,32 @@ func (c *consumers) Delete(queue storage.NotificationQueue) { for index, platform := range c.platforms { if platform.ID == platformIDToDelete { c.platforms = append(c.platforms[:index], c.platforms[index+1:]...) + + platform.Active = false + platform.LastActive = time.Now() + + if _, err := c.repository.Update(c.ctx, platform, nil); err != nil { + return err + } break } } } + return nil } -func (c *consumers) Add(platform *types.Platform, queue storage.NotificationQueue) { +func (c *consumers) Add(platform *types.Platform, queue storage.NotificationQueue) error { if len(c.queues[platform.ID]) == 0 { c.platforms = append(c.platforms, platform) + + platform.Active = true + + if _, err := c.repository.Update(c.ctx, platform, nil); err != nil { + return err + } } c.queues[platform.ID] = append(c.queues[platform.ID], queue) + return nil } func (c *consumers) Clear() map[string][]storage.NotificationQueue { diff --git a/storage/postgres/platform.go b/storage/postgres/platform.go index 2f43e19b1..4921c02f6 100644 --- a/storage/postgres/platform.go +++ b/storage/postgres/platform.go @@ -18,6 +18,7 @@ package postgres import ( "database/sql" + "time" "github.com/Peripli/service-manager/storage" @@ -33,6 +34,8 @@ type Platform struct { Description sql.NullString `db:"description"` Username string `db:"username"` Password string `db:"password"` + Active bool `db:"active"` + LastActive time.Time `db:"last_active"` } func (p *Platform) FromObject(object types.Object) (storage.Entity, bool) { @@ -49,6 +52,8 @@ func (p *Platform) FromObject(object types.Object) (storage.Entity, bool) { Type: platform.Type, Name: platform.Name, Description: toNullString(platform.Description), + Active: platform.Active, + LastActive: platform.LastActive, } if platform.Description != "" { @@ -77,5 +82,7 @@ func (p *Platform) ToObject() types.Object { Password: p.Password, }, }, + Active: p.Active, + LastActive: p.LastActive, } } From 0ac6fb5c6d57ed962c31f99fd5b29231c26f4347 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Thu, 22 Aug 2019 15:03:25 +0300 Subject: [PATCH 20/34] Platforms connection in health --- api/healthcheck/platform_indicator.go | 58 +++++++++++++++++++ api/healthcheck/platforms_indicators.go | 33 ----------- pkg/health/types.go | 13 ++++- pkg/sm/sm.go | 3 + storage/postgres/keystore_test.go | 2 +- storage/postgres/notificator.go | 47 +++++++++++---- storage/postgres/notificator_test.go | 54 +++++++++-------- .../ws_notification_test.go | 49 ++++++++++++++++ 8 files changed, 186 insertions(+), 73 deletions(-) create mode 100644 api/healthcheck/platform_indicator.go delete mode 100644 api/healthcheck/platforms_indicators.go diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go new file mode 100644 index 000000000..5cc2351e5 --- /dev/null +++ b/api/healthcheck/platform_indicator.go @@ -0,0 +1,58 @@ +package healthcheck + +import ( + "context" + "fmt" + "github.com/Peripli/service-manager/pkg/health" + "github.com/Peripli/service-manager/pkg/query" + "github.com/Peripli/service-manager/pkg/types" + "github.com/Peripli/service-manager/storage" +) + +//TODO: Add Tests + +// NewPlatformIndicator returns new health indicator for platforms of given type +func NewPlatformIndicator(ctx context.Context, repository storage.Repository, platformType string) health.Indicator { + return &platformIndicator{ + ctx: ctx, + repository: repository, + platformType: platformType, + } +} + +type platformIndicator struct { + repository storage.Repository + ctx context.Context + platformType string +} + +// Name returns the name of the indicator +func (pi *platformIndicator) Name() string { + return pi.platformType + "_platforms" // e.g. cf_platforms, k8s_platforms ... +} + +// Status returns status of the health check +func (pi *platformIndicator) Status() (interface{}, error) { + cfCriteria := query.Criterion{ + LeftOp: "type", + Operator: query.EqualsOperator, + RightOp: []string{pi.platformType}, + Type: query.FieldQuery, + } + objList, err := pi.repository.List(pi.ctx, types.PlatformType, cfCriteria) + if err != nil { + return nil, fmt.Errorf("could not fetch platforms health from storage: %v", err) + } + + details := make(map[string]interface{}) + for i := 0; i < objList.Len(); i++ { + platform := objList.ItemAt(i).(*types.Platform) + if platform.Active { + details[platform.Name] = health.New().WithStatus(health.StatusUp) + } else { + details[platform.Name] = health.New().WithStatus(health.StatusDown).WithDetail("since", platform.LastActive) + err = fmt.Errorf("there is inactive %s platforms", pi.platformType) + } + } + return details, err +} diff --git a/api/healthcheck/platforms_indicators.go b/api/healthcheck/platforms_indicators.go deleted file mode 100644 index bcdb0d95e..000000000 --- a/api/healthcheck/platforms_indicators.go +++ /dev/null @@ -1,33 +0,0 @@ -package healthcheck - -import ( - "github.com/Peripli/service-manager/storage" - "time" -) - -type platformsIndicator struct { - repository storage.Repository -} - -// Name returns the name of the storage component -func (i *platformsIndicator) Name() string { - return "platforms" -} - -func (i *platformsIndicator) Interval() time.Duration { - return 60 * time.Second -} - -func (i *platformsIndicator) FailuresTreshold() int64 { - return 1 -} - -func (i *platformsIndicator) Fatal() bool { - return false -} - -func (i *platformsIndicator) Status() (interface{}, error) { - return nil, nil -} - -//func CreatePlatformsHealthIndicators() diff --git a/pkg/health/types.go b/pkg/health/types.go index 7c1dfba13..16e3ce3f9 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -25,14 +25,16 @@ import ( // Settings type to be loaded from the environment type Settings struct { - Indicators map[string]*IndicatorSettings `mapstructure:"indicators,omitempty"` + PlatformTypes []string `mapstructure:"platform_types,omitempty" description:"platform types which health will be measured"` + Indicators map[string]*IndicatorSettings `mapstructure:"indicators,omitempty"` } // DefaultSettings returns default values for health settings func DefaultSettings() *Settings { emptySettings := make(map[string]*IndicatorSettings) return &Settings{ - Indicators: emptySettings, + PlatformTypes: make([]string, 0), + Indicators: emptySettings, } } @@ -91,7 +93,12 @@ const ( type StatusListener struct{} func (sl *StatusListener) HealthCheckFailed(state *health.State) { - log.D().Errorf("Health check for %v failed with: %v", state.Name, state.Err) + msg := fmt.Sprintf("Health check for %v failed with: %v", state.Name, state.Err) + if state.Fatal { + log.D().Error(msg) + } else { + log.D().Warn(msg) + } } func (sl *StatusListener) HealthCheckRecovered(state *health.State, numberOfFailures int64, unavailableDuration float64) { diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 679e3bf0e..d24b07686 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -140,6 +140,9 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( } API.HealthIndicators = append(API.HealthIndicators, storageHealthIndicator) + for _, platformType := range cfg.Health.PlatformTypes { + API.HealthIndicators = append(API.HealthIndicators, healthcheck.NewPlatformIndicator(ctx, interceptableRepository, platformType)) + } notificationCleaner := &storage.NotificationCleaner{ Storage: interceptableRepository, diff --git a/storage/postgres/keystore_test.go b/storage/postgres/keystore_test.go index 73a29b0a9..ecaf74081 100644 --- a/storage/postgres/keystore_test.go +++ b/storage/postgres/keystore_test.go @@ -59,7 +59,7 @@ var _ = Describe("Secured Storage", func() { mock.ExpectQuery(`SELECT CURRENT_DATABASE()`).WillReturnRows(sqlmock.NewRows([]string{"mock"}).FromCSVString("mock")) mock.ExpectQuery(`SELECT COUNT(1)*`).WillReturnRows(sqlmock.NewRows([]string{"mock"}).FromCSVString("1")) mock.ExpectExec("SELECT pg_advisory_lock*").WithArgs(sqlmock.AnyArg()).WillReturnResult(sqlmock.NewResult(1, 1)) - mock.ExpectQuery(`SELECT version, dirty FROM "schema_migrations" LIMIT 1`).WillReturnRows(sqlmock.NewRows([]string{"version", "dirty"}).FromCSVString("11,false")) + mock.ExpectQuery(`SELECT version, dirty FROM "schema_migrations" LIMIT 1`).WillReturnRows(sqlmock.NewRows([]string{"version", "dirty"}).FromCSVString("12,false")) mock.ExpectExec("SELECT pg_advisory_unlock*").WithArgs(sqlmock.AnyArg()).WillReturnResult(sqlmock.NewResult(1, 1)) options := storage.DefaultSettings() options.EncryptionKey = string(envEncryptionKey) diff --git a/storage/postgres/notificator.go b/storage/postgres/notificator.go index 1b1360061..ca323d3c2 100644 --- a/storage/postgres/notificator.go +++ b/storage/postgres/notificator.go @@ -21,6 +21,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/Peripli/service-manager/pkg/query" "sync" "sync/atomic" "time" @@ -66,7 +67,7 @@ type Notificator struct { } // NewNotificator returns new Notificator based on a given NotificatorStorage and desired queue size -func NewNotificator(st storage.Storage, repository storage.Repository, settings *storage.Settings) (*Notificator, error) { +func NewNotificator(st storage.Storage, repository storage.TransactionalRepository, settings *storage.Settings) (*Notificator, error) { ns, err := NewNotificationStorage(st) connectionCreator := ¬ificationConnectionCreatorImpl{ storageURI: settings.URI, @@ -425,7 +426,7 @@ func (n *Notificator) stopConnection() { } type consumers struct { - repository storage.Repository + repository storage.TransactionalRepository ctx context.Context queues map[string][]storage.NotificationQueue platforms []*types.Platform @@ -464,11 +465,10 @@ func (c *consumers) Delete(queue storage.NotificationQueue) error { for index, platform := range c.platforms { if platform.ID == platformIDToDelete { c.platforms = append(c.platforms[:index], c.platforms[index+1:]...) - - platform.Active = false - platform.LastActive = time.Now() - - if _, err := c.repository.Update(c.ctx, platform, nil); err != nil { + if err := c.updatePlatform(platform.ID, func(p *types.Platform) { + p.Active = false + p.LastActive = time.Now() + }); err != nil { return err } break @@ -481,10 +481,9 @@ func (c *consumers) Delete(queue storage.NotificationQueue) error { func (c *consumers) Add(platform *types.Platform, queue storage.NotificationQueue) error { if len(c.queues[platform.ID]) == 0 { c.platforms = append(c.platforms, platform) - - platform.Active = true - - if _, err := c.repository.Update(c.ctx, platform, nil); err != nil { + if err := c.updatePlatform(platform.ID, func(p *types.Platform) { + p.Active = true + }); err != nil { return err } } @@ -515,3 +514,29 @@ func (c *consumers) GetPlatform(platformID string) *types.Platform { func (c *consumers) GetQueuesForPlatform(platformID string) []storage.NotificationQueue { return c.queues[platformID] } + +func (c *consumers) updatePlatform(platformID string, updatePlatformFunc func(p *types.Platform)) error { + if err := c.repository.InTransaction(c.ctx, func(ctx context.Context, storage storage.Repository) error { + idCriteria := query.Criterion{ + LeftOp: "id", + Operator: query.EqualsOperator, + RightOp: []string{platformID}, + Type: query.FieldQuery, + } + obj, err := storage.Get(ctx, types.PlatformType, idCriteria) + if err != nil { + return err + } + + platform := obj.(*types.Platform) + updatePlatformFunc(platform) + + if _, err := storage.Update(ctx, platform, nil); err != nil { + return err + } + return nil + }); err != nil { + return err + } + return nil +} diff --git a/storage/postgres/notificator_test.go b/storage/postgres/notificator_test.go index c3b3a3072..da9e99b85 100644 --- a/storage/postgres/notificator_test.go +++ b/storage/postgres/notificator_test.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "errors" + "github.com/Peripli/service-manager/storage/storagefakes" "sync" "time" @@ -52,7 +53,8 @@ var _ = Describe("Notificator", func() { ctx context.Context cancel context.CancelFunc wg *sync.WaitGroup - fakeStorage *postgresfakes.FakeNotificationStorage + fakeStorage *storagefakes.FakeStorage + fakeNotificationStorage *postgresfakes.FakeNotificationStorage fakeConnectionCreator *postgresfakes.FakeNotificationConnectionCreator testNotificator storage.Notificator fakeNotificationConnection *notificationConnectionFakes.FakeNotificationConnection @@ -95,10 +97,11 @@ var _ = Describe("Notificator", func() { connectionMutex: &sync.Mutex{}, consumersMutex: &sync.Mutex{}, consumers: &consumers{ - queues: make(map[string][]storage.NotificationQueue), - platforms: make([]*types.Platform, 0), + repository: fakeStorage, + queues: make(map[string][]storage.NotificationQueue), + platforms: make([]*types.Platform, 0), }, - storage: fakeStorage, + storage: fakeNotificationStorage, connectionCreator: fakeConnectionCreator, stopProcessing: func() {}, lastKnownRevision: types.InvalidRevision, @@ -149,8 +152,9 @@ var _ = Describe("Notificator", func() { ID: "platformID", }, } - fakeStorage = &postgresfakes.FakeNotificationStorage{} - fakeStorage.GetLastRevisionReturns(defaultLastRevision, nil) + fakeStorage = &storagefakes.FakeStorage{} + fakeNotificationStorage = &postgresfakes.FakeNotificationStorage{} + fakeNotificationStorage.GetLastRevisionReturns(defaultLastRevision, nil) fakeNotificationConnection = ¬ificationConnectionFakes.FakeNotificationConnection{} fakeNotificationConnection.ListenReturns(nil) fakeNotificationConnection.UnlistenReturns(nil) @@ -215,9 +219,9 @@ var _ = Describe("Notificator", func() { } }) notification = createNotification("") - fakeStorage.GetNotificationByRevisionReturns(notification, nil) - fakeStorage.ListNotificationsReturns([]*types.Notification{notification}, nil) - fakeStorage.GetNotificationReturns(notification, nil) + fakeNotificationStorage.GetNotificationByRevisionReturns(notification, nil) + fakeNotificationStorage.ListNotificationsReturns([]*types.Notification{notification}, nil) + fakeNotificationStorage.GetNotificationReturns(notification, nil) Expect(testNotificator.Start(ctx, wg)).ToNot(HaveOccurred()) runningFunc(true, nil) @@ -333,7 +337,7 @@ var _ = Describe("Notificator", func() { Context("When storage GetLastRevision fails", func() { BeforeEach(func() { - fakeStorage.GetLastRevisionReturns(types.InvalidRevision, expectedError) + fakeNotificationStorage.GetLastRevisionReturns(types.InvalidRevision, expectedError) }) It("Should return error", func() { @@ -373,7 +377,7 @@ var _ = Describe("Notificator", func() { unlistenCalled <- struct{}{} return nil } - fakeStorage.GetNotificationByRevisionReturns(nil, expectedError) + fakeNotificationStorage.GetNotificationByRevisionReturns(nil, expectedError) expectRegisterConsumerFail(expectedError.Error(), defaultLastRevision-1) expectUnlistenCalled(unlistenCalled) }) @@ -387,7 +391,7 @@ var _ = Describe("Notificator", func() { unlistenCalled <- struct{}{} return errors.New("unlisten error") } - fakeStorage.GetNotificationByRevisionReturns(nil, expectedError) + fakeNotificationStorage.GetNotificationByRevisionReturns(nil, expectedError) expectRegisterConsumerFail(expectedError.Error(), defaultLastRevision-1) expectUnlistenCalled(unlistenCalled) }) @@ -395,14 +399,14 @@ var _ = Describe("Notificator", func() { Context("When storage returns \"not found\" error when getting notification with revision", func() { It("Should return ErrInvalidNotificationRevision", func() { - fakeStorage.GetNotificationByRevisionReturns(nil, util.ErrNotFoundInStorage) + fakeNotificationStorage.GetNotificationByRevisionReturns(nil, util.ErrNotFoundInStorage) expectRegisterConsumerFail(util.ErrInvalidNotificationRevision.Error(), defaultLastRevision-1) }) }) Context("When storage returns error on notification list", func() { It("Should return the error", func() { - fakeStorage.ListNotificationsReturns(nil, expectedError) + fakeNotificationStorage.ListNotificationsReturns(nil, expectedError) expectRegisterConsumerFail(expectedError.Error(), defaultLastRevision-1) }) }) @@ -413,7 +417,7 @@ var _ = Describe("Notificator", func() { for i := 0; i < defaultQueueSize+1; i++ { notificationsToReturn = append(notificationsToReturn, createNotification("")) } - fakeStorage.ListNotificationsReturns(notificationsToReturn, nil) + fakeNotificationStorage.ListNotificationsReturns(notificationsToReturn, nil) expectRegisterConsumerFail(util.ErrInvalidNotificationRevision.Error(), defaultLastRevision-1) }) }) @@ -422,9 +426,9 @@ var _ = Describe("Notificator", func() { It("Should be in the returned queue", func() { n1 := createNotification("") n2 := createNotification("") - fakeStorage.GetNotificationByRevisionReturns(n1, nil) - fakeStorage.ListNotificationsReturns([]*types.Notification{n1}, nil) - fakeStorage.GetNotificationReturns(n2, nil) + fakeNotificationStorage.GetNotificationByRevisionReturns(n1, nil) + fakeNotificationStorage.ListNotificationsReturns([]*types.Notification{n1}, nil) + fakeNotificationStorage.GetNotificationReturns(n2, nil) queue = expectRegisterConsumerSuccess(defaultPlatform, defaultLastRevision-1) queueChannel := queue.Channel() Expect(<-queueChannel).To(Equal(n1)) @@ -477,7 +481,7 @@ var _ = Describe("Notificator", func() { Context("When notification is sent", func() { It("Should be received in the queue", func() { notification := createNotification(defaultPlatform.ID) - fakeStorage.GetNotificationReturns(notification, nil) + fakeNotificationStorage.GetNotificationReturns(notification, nil) notificationChannel <- &pq.Notification{ Extra: createNotificationPayload(defaultPlatform.ID, notification.ID), } @@ -487,7 +491,7 @@ var _ = Describe("Notificator", func() { Context("When notification cannot be fetched from db", func() { fetchNotificationFromDBFail := func(platformID string) { - fakeStorage.GetNotificationReturns(nil, expectedError) + fakeNotificationStorage.GetNotificationReturns(nil, expectedError) ch := queue.Channel() notificationChannel <- &pq.Notification{ Extra: createNotificationPayload(platformID, "some_id"), @@ -514,7 +518,7 @@ var _ = Describe("Notificator", func() { q := registerDefaultPlatform() notification := createNotification("") - fakeStorage.GetNotificationReturns(notification, nil) + fakeNotificationStorage.GetNotificationReturns(notification, nil) notificationChannel <- &pq.Notification{ Extra: createNotificationPayload("", notification.ID), } @@ -526,7 +530,7 @@ var _ = Describe("Notificator", func() { Context("When notification is sent with unregistered platform ID", func() { It("Should call storage once", func() { notification := createNotification(defaultPlatform.ID) - fakeStorage.GetNotificationReturns(notification, nil) + fakeNotificationStorage.GetNotificationReturns(notification, nil) notificationChannel <- &pq.Notification{ Extra: createNotificationPayload("not_registered", "some_id"), } @@ -534,7 +538,7 @@ var _ = Describe("Notificator", func() { Extra: createNotificationPayload(defaultPlatform.ID, notification.ID), } expectReceivedNotification(notification, queue) - Expect(fakeStorage.GetNotificationCallCount()).To(Equal(1)) + Expect(fakeNotificationStorage.GetNotificationCallCount()).To(Equal(1)) }) }) @@ -552,7 +556,7 @@ var _ = Describe("Notificator", func() { Context("When notification is null", func() { It("Should not send notification", func() { notification := createNotification(defaultPlatform.ID) - fakeStorage.GetNotificationReturns(notification, nil) + fakeNotificationStorage.GetNotificationReturns(notification, nil) notificationChannel <- nil notificationChannel <- &pq.Notification{ Extra: createNotificationPayload(defaultPlatform.ID, notification.ID), @@ -576,7 +580,7 @@ var _ = Describe("Notificator", func() { It("Should close notification queue", func() { q := registerDefaultPlatform() notification := createNotification(defaultPlatform.ID) - fakeStorage.GetNotificationReturns(notification, nil) + fakeNotificationStorage.GetNotificationReturns(notification, nil) ch := q.Channel() notificationChannel <- &pq.Notification{ Extra: createNotificationPayload(defaultPlatform.ID, notification.ID), diff --git a/test/ws_notification_test/ws_notification_test.go b/test/ws_notification_test/ws_notification_test.go index 20bd9fe11..a3a7acd8b 100644 --- a/test/ws_notification_test/ws_notification_test.go +++ b/test/ws_notification_test/ws_notification_test.go @@ -235,6 +235,55 @@ var _ = Describe("WS", func() { }) }) + Context("when platform is connected", func() { + It("should switch platform's active status to true", func() { + Expect(platform.Active).To(BeFalse()) + _, _, err := ctx.ConnectWebSocket(platform, queryParams) + Expect(err).ShouldNot(HaveOccurred()) + + idCriteria := query.Criterion{ + LeftOp: "id", + Operator: query.EqualsOperator, + RightOp: []string{platform.ID}, + Type: query.FieldQuery, + } + obj, err := repository.Get(context.TODO(), types.PlatformType, idCriteria) + Expect(err).ShouldNot(HaveOccurred()) + Expect(obj.(*types.Platform).Active).To(BeTrue()) + }) + }) + + Context("when platform disconnects", func() { + It("should switch platform's active status to false", func() { + conn, _, err := ctx.ConnectWebSocket(platform, queryParams) + Expect(err).ShouldNot(HaveOccurred()) + + ctx.CloseWebSocket(conn) + + idCriteria := query.Criterion{ + LeftOp: "id", + Operator: query.EqualsOperator, + RightOp: []string{platform.ID}, + Type: query.FieldQuery, + } + ctx, _ := context.WithTimeout(context.TODO(), 5*time.Second) + ticker := time.NewTicker(500 * time.Millisecond) + for { + select { + case <-ticker.C: + obj, err := repository.Get(context.TODO(), types.PlatformType, idCriteria) + Expect(err).ShouldNot(HaveOccurred()) + p := obj.(*types.Platform) + if p.Active == false && !p.LastActive.IsZero() { + return + } + case <-ctx.Done(): + Fail("Timeout: platform active status not set to false") + } + } + }) + }) + Context("when same platform is connected twice", func() { It("should send same notifications to both", func() { conn, _, err := ctx.ConnectWebSocket(platform, queryParams) From 7ffd05197433e8a36fb4fbfc44b72f0d1b3a3bb4 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 26 Aug 2019 14:38:52 +0300 Subject: [PATCH 21/34] Add tests --- .../healthcheck_controller_test.go | 1 + api/healthcheck/platform_indicator.go | 22 ++++- api/healthcheck/platform_indicator_test.go | 94 +++++++++++++++++++ 3 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 api/healthcheck/platform_indicator_test.go diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index fc353d341..59920d8bd 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package healthcheck import ( diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go index 5cc2351e5..28e21513d 100644 --- a/api/healthcheck/platform_indicator.go +++ b/api/healthcheck/platform_indicator.go @@ -1,3 +1,19 @@ +/* + * Copyright 2018 The Service Manager Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package healthcheck import ( @@ -9,8 +25,6 @@ import ( "github.com/Peripli/service-manager/storage" ) -//TODO: Add Tests - // NewPlatformIndicator returns new health indicator for platforms of given type func NewPlatformIndicator(ctx context.Context, repository storage.Repository, platformType string) health.Indicator { return &platformIndicator{ @@ -33,13 +47,13 @@ func (pi *platformIndicator) Name() string { // Status returns status of the health check func (pi *platformIndicator) Status() (interface{}, error) { - cfCriteria := query.Criterion{ + typeCriteria := query.Criterion{ LeftOp: "type", Operator: query.EqualsOperator, RightOp: []string{pi.platformType}, Type: query.FieldQuery, } - objList, err := pi.repository.List(pi.ctx, types.PlatformType, cfCriteria) + objList, err := pi.repository.List(pi.ctx, types.PlatformType, typeCriteria) if err != nil { return nil, fmt.Errorf("could not fetch platforms health from storage: %v", err) } diff --git a/api/healthcheck/platform_indicator_test.go b/api/healthcheck/platform_indicator_test.go new file mode 100644 index 000000000..c6d3b5d4b --- /dev/null +++ b/api/healthcheck/platform_indicator_test.go @@ -0,0 +1,94 @@ +/* + * Copyright 2018 The Service Manager Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package healthcheck + +import ( + "context" + "errors" + "github.com/Peripli/service-manager/pkg/health" + "github.com/Peripli/service-manager/pkg/types" + storagefakes2 "github.com/Peripli/service-manager/storage/storagefakes" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "time" +) + + +var _ = Describe("Platforms Indicator", func() { + var indicator health.Indicator + var repository *storagefakes2.FakeStorage + var platformType string + var ctx context.Context + var platform *types.Platform + + BeforeEach(func() { + ctx = context.TODO() + repository = &storagefakes2.FakeStorage{} + platformType = "kubernetes" + platform = &types.Platform{ + Name: "test-platform", + Type: platformType, + Active: false, + LastActive: time.Now(), + } + indicator = NewPlatformIndicator(ctx, repository, platformType) + }) + + Context("Name", func() { + It("should contain platform type in it", func() { + Expect(indicator.Name()).Should(ContainSubstring(platformType)) + }) + }) + + Context("There is inactive platforms", func() { + BeforeEach(func() { + objectList := &types.Platforms{[]*types.Platform{platform}} + repository.ListReturns(objectList,nil) + }) + It("should return error", func() { + details, err := indicator.Status() + health := details.(map[string]interface{})[platform.Name].(*health.Health) + Expect(err).Should(HaveOccurred()) + Expect(health.Details["since"]).ShouldNot(BeNil()) + }) + }) + + Context("Storage returns error", func() { + var expectedErr error + BeforeEach(func() { + expectedErr = errors.New("storage err") + repository.ListReturns(nil,expectedErr) + }) + It("should return error", func() { + _, err := indicator.Status() + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedErr.Error())) + }) + }) + + Context("All platforms are active", func() { + BeforeEach(func() { + platform.Active = true + objectList := &types.Platforms{[]*types.Platform{platform}} + repository.ListReturns(objectList, nil) + }) + It("should not return error", func() { + _, err := indicator.Status() + Expect(err).ShouldNot(HaveOccurred()) + }) + }) +}) \ No newline at end of file From 9621fb0175b3c92fe77d0d9b5602efd4036b8c53 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Mon, 26 Aug 2019 14:50:44 +0300 Subject: [PATCH 22/34] Fix formatting --- api/healthcheck/platform_indicator_test.go | 13 ++++++------- .../ws_notification_test.go | 18 +++++++++--------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/api/healthcheck/platform_indicator_test.go b/api/healthcheck/platform_indicator_test.go index c6d3b5d4b..384b5ded4 100644 --- a/api/healthcheck/platform_indicator_test.go +++ b/api/healthcheck/platform_indicator_test.go @@ -27,7 +27,6 @@ import ( "time" ) - var _ = Describe("Platforms Indicator", func() { var indicator health.Indicator var repository *storagefakes2.FakeStorage @@ -40,9 +39,9 @@ var _ = Describe("Platforms Indicator", func() { repository = &storagefakes2.FakeStorage{} platformType = "kubernetes" platform = &types.Platform{ - Name: "test-platform", - Type: platformType, - Active: false, + Name: "test-platform", + Type: platformType, + Active: false, LastActive: time.Now(), } indicator = NewPlatformIndicator(ctx, repository, platformType) @@ -57,7 +56,7 @@ var _ = Describe("Platforms Indicator", func() { Context("There is inactive platforms", func() { BeforeEach(func() { objectList := &types.Platforms{[]*types.Platform{platform}} - repository.ListReturns(objectList,nil) + repository.ListReturns(objectList, nil) }) It("should return error", func() { details, err := indicator.Status() @@ -71,7 +70,7 @@ var _ = Describe("Platforms Indicator", func() { var expectedErr error BeforeEach(func() { expectedErr = errors.New("storage err") - repository.ListReturns(nil,expectedErr) + repository.ListReturns(nil, expectedErr) }) It("should return error", func() { _, err := indicator.Status() @@ -91,4 +90,4 @@ var _ = Describe("Platforms Indicator", func() { Expect(err).ShouldNot(HaveOccurred()) }) }) -}) \ No newline at end of file +}) diff --git a/test/ws_notification_test/ws_notification_test.go b/test/ws_notification_test/ws_notification_test.go index a3a7acd8b..e0b548b81 100644 --- a/test/ws_notification_test/ws_notification_test.go +++ b/test/ws_notification_test/ws_notification_test.go @@ -270,15 +270,15 @@ var _ = Describe("WS", func() { ticker := time.NewTicker(500 * time.Millisecond) for { select { - case <-ticker.C: - obj, err := repository.Get(context.TODO(), types.PlatformType, idCriteria) - Expect(err).ShouldNot(HaveOccurred()) - p := obj.(*types.Platform) - if p.Active == false && !p.LastActive.IsZero() { - return - } - case <-ctx.Done(): - Fail("Timeout: platform active status not set to false") + case <-ticker.C: + obj, err := repository.Get(context.TODO(), types.PlatformType, idCriteria) + Expect(err).ShouldNot(HaveOccurred()) + p := obj.(*types.Platform) + if p.Active == false && !p.LastActive.IsZero() { + return + } + case <-ctx.Done(): + Fail("Timeout: platform active status not set to false") } } }) From a2ba4fe8422ab08bad7eb5a1b2e15b0a7fdba8e1 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Tue, 27 Aug 2019 18:00:28 +0300 Subject: [PATCH 23/34] Hide platform details on unauthorized healthcheck --- api/healthcheck/healthcheck_controller.go | 9 +++- .../healthcheck_controller_test.go | 45 +++++++++++++++---- api/healthcheck/platform_indicator.go | 4 +- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index 9e55f611e..86df69f35 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -17,10 +17,12 @@ package healthcheck import ( + "context" h "github.com/InVisionApp/go-health" "github.com/Peripli/service-manager/pkg/health" "github.com/Peripli/service-manager/pkg/util" "net/http" + "strings" "github.com/Peripli/service-manager/pkg/log" "github.com/Peripli/service-manager/pkg/web" @@ -46,7 +48,7 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { logger := log.C(ctx) logger.Debugf("Performing health check...") healthState, _, _ := c.health.State() - healthResult := c.aggregate(healthState) + healthResult := c.aggregate(ctx, healthState) var status int if healthResult.Status == health.StatusUp { status = http.StatusOK @@ -56,7 +58,7 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { return util.NewJSONResponse(status, healthResult) } -func (c *controller) aggregate(overallState map[string]h.State) *health.Health { +func (c *controller) aggregate(ctx context.Context, overallState map[string]h.State) *health.Health { if len(overallState) == 0 { return health.New().WithStatus(health.StatusUp) } @@ -70,6 +72,9 @@ func (c *controller) aggregate(overallState map[string]h.State) *health.Health { details := make(map[string]interface{}) for name, state := range overallState { state.Status = convertStatus(state.Status) + if strings.Contains(name, PlatformIndicatorSuffix) && !web.IsAuthorized(ctx) { + state.Details = nil + } details[name] = state } return health.New().WithStatus(overallStatus).WithDetails(details) diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index 59920d8bd..99f469cfe 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -17,6 +17,7 @@ package healthcheck import ( + "context" "fmt" h "github.com/InVisionApp/go-health" "github.com/Peripli/service-manager/pkg/health" @@ -61,14 +62,20 @@ var _ = Describe("Healthcheck controller", func() { }) Describe("aggregation", func() { + var ctx context.Context var c *controller var healths map[string]h.State + var platformHealths map[string]h.State var thresholds map[string]int64 BeforeEach(func() { + ctx = context.TODO() healths = map[string]h.State{ - "test1": {Status: "ok"}, - "test2": {Status: "ok"}, + "test1": {Status: "ok", Details: "details"}, + "test2": {Status: "ok", Details: "details"}, + } + platformHealths = map[string]h.State{ + "k8s_platforms": {Status: "ok", Details: "details"}, } thresholds = map[string]int64{ "test1": 3, @@ -82,7 +89,7 @@ var _ = Describe("Healthcheck controller", func() { When("No healths are provided", func() { It("Returns UP", func() { - aggregatedHealth := c.aggregate(nil) + aggregatedHealth := c.aggregate(ctx, nil) Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) }) @@ -91,7 +98,7 @@ var _ = Describe("Healthcheck controller", func() { It("Returns DOWN", func() { healths["test3"] = h.State{Status: "failed", Fatal: true, ContiguousFailures: 4} c.thresholds["test3"] = 3 - aggregatedHealth := c.aggregate(healths) + aggregatedHealth := c.aggregate(ctx, healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusDown)) }) }) @@ -99,7 +106,7 @@ var _ = Describe("Healthcheck controller", func() { When("At least one health is DOWN and is not Fatal", func() { It("Returns UP", func() { healths["test3"] = h.State{Status: "failed", Fatal: false, ContiguousFailures: 4} - aggregatedHealth := c.aggregate(healths) + aggregatedHealth := c.aggregate(ctx, healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) }) @@ -108,27 +115,49 @@ var _ = Describe("Healthcheck controller", func() { It("Returns UP", func() { healths["test3"] = h.State{Status: "failed"} c.thresholds["test3"] = 3 - aggregatedHealth := c.aggregate(healths) + aggregatedHealth := c.aggregate(ctx, healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) }) When("All healths are UP", func() { It("Returns UP", func() { - aggregatedHealth := c.aggregate(healths) + aggregatedHealth := c.aggregate(ctx, healths) Expect(aggregatedHealth.Status).To(Equal(health.StatusUp)) }) }) When("Aggregating healths", func() { It("Includes them as overall details", func() { - aggregatedHealth := c.aggregate(healths) + aggregatedHealth := c.aggregate(ctx, healths) for name, h := range healths { h.Status = convertStatus(h.Status) Expect(aggregatedHealth.Details[name]).To(Equal(h)) } }) }) + + When("Aggregating platforms health as unauthorized user", func() { + It("should strip platform details", func() { + aggregatedHealth := c.aggregate(ctx, platformHealths) + for name, h := range platformHealths { + h.Status = convertStatus(h.Status) + h.Details = nil + Expect(aggregatedHealth.Details[name]).To(Equal(h)) + } + }) + }) + + When("Aggregating platforms health as authorized user", func() { + It("should include all platform details", func() { + ctx = web.ContextWithAuthorization(ctx) + aggregatedHealth := c.aggregate(ctx, platformHealths) + for name, h := range platformHealths { + h.Status = convertStatus(h.Status) + Expect(aggregatedHealth.Details[name]).To(Equal(h)) + } + }) + }) }) }) diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go index 28e21513d..7010c1865 100644 --- a/api/healthcheck/platform_indicator.go +++ b/api/healthcheck/platform_indicator.go @@ -25,6 +25,8 @@ import ( "github.com/Peripli/service-manager/storage" ) +const PlatformIndicatorSuffix = "_platforms" + // NewPlatformIndicator returns new health indicator for platforms of given type func NewPlatformIndicator(ctx context.Context, repository storage.Repository, platformType string) health.Indicator { return &platformIndicator{ @@ -42,7 +44,7 @@ type platformIndicator struct { // Name returns the name of the indicator func (pi *platformIndicator) Name() string { - return pi.platformType + "_platforms" // e.g. cf_platforms, k8s_platforms ... + return pi.platformType + PlatformIndicatorSuffix // e.g. cf_platforms, k8s_platforms ... } // Status returns status of the health check From 89ab1ff3d8f72821c8235f2f01242bb24b17dc3c Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Thu, 29 Aug 2019 10:18:18 +0300 Subject: [PATCH 24/34] Rename migrations --- ...atus.down.sql => 20190829101500_add_platforms_status.down.sql} | 0 ...s_status.up.sql => 20190829101500_add_platforms_status.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename storage/postgres/migrations/{012_add_platforms_status.down.sql => 20190829101500_add_platforms_status.down.sql} (100%) rename storage/postgres/migrations/{012_add_platforms_status.up.sql => 20190829101500_add_platforms_status.up.sql} (100%) diff --git a/storage/postgres/migrations/012_add_platforms_status.down.sql b/storage/postgres/migrations/20190829101500_add_platforms_status.down.sql similarity index 100% rename from storage/postgres/migrations/012_add_platforms_status.down.sql rename to storage/postgres/migrations/20190829101500_add_platforms_status.down.sql diff --git a/storage/postgres/migrations/012_add_platforms_status.up.sql b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql similarity index 100% rename from storage/postgres/migrations/012_add_platforms_status.up.sql rename to storage/postgres/migrations/20190829101500_add_platforms_status.up.sql From b04b750591799fa9df6e224cab24dee4ca27ebd4 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Thu, 29 Aug 2019 10:27:51 +0300 Subject: [PATCH 25/34] Fix test last migration --- storage/postgres/keystore_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/postgres/keystore_test.go b/storage/postgres/keystore_test.go index 93f69fe7f..8a7a13f33 100644 --- a/storage/postgres/keystore_test.go +++ b/storage/postgres/keystore_test.go @@ -59,7 +59,7 @@ var _ = Describe("Secured Storage", func() { mock.ExpectQuery(`SELECT CURRENT_DATABASE()`).WillReturnRows(sqlmock.NewRows([]string{"mock"}).FromCSVString("mock")) mock.ExpectQuery(`SELECT COUNT(1)*`).WillReturnRows(sqlmock.NewRows([]string{"mock"}).FromCSVString("1")) mock.ExpectExec("SELECT pg_advisory_lock*").WithArgs(sqlmock.AnyArg()).WillReturnResult(sqlmock.NewResult(1, 1)) - mock.ExpectQuery(`SELECT version, dirty FROM "schema_migrations" LIMIT 1`).WillReturnRows(sqlmock.NewRows([]string{"version", "dirty"}).FromCSVString("20190816162000,false")) + mock.ExpectQuery(`SELECT version, dirty FROM "schema_migrations" LIMIT 1`).WillReturnRows(sqlmock.NewRows([]string{"version", "dirty"}).FromCSVString("20190829101500,false")) mock.ExpectExec("SELECT pg_advisory_unlock*").WithArgs(sqlmock.AnyArg()).WillReturnResult(sqlmock.NewResult(1, 1)) options := storage.DefaultSettings() options.EncryptionKey = string(envEncryptionKey) From dfe900fb1f9c4ef2feed30c02729152b34f85f72 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Thu, 29 Aug 2019 11:50:55 +0300 Subject: [PATCH 26/34] Add default value to migration --- .../migrations/20190829101500_add_platforms_status.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql index 328cbddfd..f92ca8ef4 100644 --- a/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql +++ b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql @@ -1,6 +1,6 @@ BEGIN; -ALTER TABLE platforms ADD COLUMN active BOOLEAN; +ALTER TABLE platforms ADD COLUMN active BOOLEAN DEFAULT FALSE; ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP; COMMIT; \ No newline at end of file From 3e864fd898666c0e4059bb41a12b809b40b00026 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Thu, 29 Aug 2019 12:21:01 +0300 Subject: [PATCH 27/34] Last Active default value --- .../migrations/20190829101500_add_platforms_status.up.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql index f92ca8ef4..03a51f3e5 100644 --- a/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql +++ b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql @@ -1,6 +1,6 @@ BEGIN; -ALTER TABLE platforms ADD COLUMN active BOOLEAN DEFAULT FALSE; -ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP; +ALTER TABLE platforms ADD COLUMN active boolean NOT NULL DEFAULT '0'; +ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP NOT NULL DEFAULT '1970-01-01 00:00:00+00'; COMMIT; \ No newline at end of file From 46bc9c59e0756a7835716d1fb7c343ce9cfec448 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Fri, 30 Aug 2019 17:39:29 +0300 Subject: [PATCH 28/34] Initial fix health configuration --- api/healthcheck/healthcheck_controller.go | 2 +- api/healthcheck/platform_indicator.go | 4 +- pkg/env/env_test.go | 48 ++++++++++-- pkg/env/helpers.go | 89 ++++++++++++++--------- pkg/health/types.go | 29 +++++++- storage/healthcheck.go | 2 +- 6 files changed, 123 insertions(+), 51 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index 86df69f35..be93306cb 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -72,7 +72,7 @@ func (c *controller) aggregate(ctx context.Context, overallState map[string]h.St details := make(map[string]interface{}) for name, state := range overallState { state.Status = convertStatus(state.Status) - if strings.Contains(name, PlatformIndicatorSuffix) && !web.IsAuthorized(ctx) { + if strings.Contains(name, health.PlatformIndicatorSuffix) && !web.IsAuthorized(ctx) { state.Details = nil } details[name] = state diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go index 7010c1865..fe12c9bb8 100644 --- a/api/healthcheck/platform_indicator.go +++ b/api/healthcheck/platform_indicator.go @@ -25,8 +25,6 @@ import ( "github.com/Peripli/service-manager/storage" ) -const PlatformIndicatorSuffix = "_platforms" - // NewPlatformIndicator returns new health indicator for platforms of given type func NewPlatformIndicator(ctx context.Context, repository storage.Repository, platformType string) health.Indicator { return &platformIndicator{ @@ -44,7 +42,7 @@ type platformIndicator struct { // Name returns the name of the indicator func (pi *platformIndicator) Name() string { - return pi.platformType + PlatformIndicatorSuffix // e.g. cf_platforms, k8s_platforms ... + return pi.platformType + health.PlatformIndicatorSuffix // e.g. cf_platforms, k8s_platforms ... } // Status returns status of the health check diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index f38eb03ef..fa71cb762 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -48,6 +48,7 @@ func TestEnv(t *testing.T) { var _ = Describe("Env", func() { const ( + mapKey = "map" key = "key" description = "desc" flagDefaultValue = "pflagDefaultValue" @@ -67,6 +68,12 @@ var _ = Describe("Env", func() { keyNslice = "nest.nslice" keyNmappedVal = "nest.n_mapped_val" + keyMapNbool = mapKey + "." + keyNbool + keyMapNint = mapKey + "." + keyNint + keyMapNstring = mapKey + "." + keyNstring + keyMapNslice = mapKey + "." + keyNslice + keyMapNmappedVal = mapKey + "." + keyNmappedVal + keyLogFormat = "log.format" keyLogLevel = "log.level" ) @@ -83,6 +90,7 @@ var _ = Describe("Env", func() { WInt int WString string WMappedVal string `mapstructure:"w_mapped_val" structs:"w_mapped_val" yaml:"w_mapped_val"` + WMapNest map[string]Nest Nest Nest Log log.Settings } @@ -123,6 +131,12 @@ var _ = Describe("Env", func() { set.StringSlice(keyNslice, s.Nest.NSlice, description) set.String(keyNmappedVal, s.Nest.NMappedVal, description) + set.Bool(keyMapNbool, s.WMapNest[mapKey].NBool, description) + set.Int(keyMapNint, s.WMapNest[mapKey].NInt, description) + set.String(keyMapNstring, s.WMapNest[mapKey].NString, description) + set.StringSlice(keyMapNslice, s.WMapNest[mapKey].NSlice, description) + set.String(keyMapNmappedVal, s.WMapNest[mapKey].NMappedVal, description) + set.String(keyLogLevel, s.Log.Level, description) set.String(keyLogFormat, s.Log.Format, description) @@ -147,6 +161,11 @@ var _ = Describe("Env", func() { Expect(testFlags.Set(keyNstring, o.Nest.NString)).ShouldNot(HaveOccurred()) Expect(testFlags.Set(keyNmappedVal, o.Nest.NMappedVal)).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keyMapNbool, cast.ToString(o.WMapNest[mapKey].NBool))).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keyMapNint, cast.ToString(o.WMapNest[mapKey].NInt))).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keyMapNstring, o.WMapNest[mapKey].NString)).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keyMapNmappedVal, o.WMapNest[mapKey].NMappedVal)).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keyLogFormat, o.Log.Format)).ShouldNot(HaveOccurred()) Expect(testFlags.Set(keyLogLevel, o.Log.Level)).ShouldNot(HaveOccurred()) } @@ -163,6 +182,12 @@ var _ = Describe("Env", func() { Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", 1), strings.Join(structure.Nest.NSlice, ","))).ShouldNot(HaveOccurred()) Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", 1), structure.Nest.NMappedVal)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", 1), cast.ToString(structure.WMapNest[mapKey].NBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", 1), cast.ToString(structure.WMapNest[mapKey].NInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", 1), structure.WMapNest[mapKey].NString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", 1), strings.Join(structure.WMapNest[mapKey].NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", 1), structure.WMapNest[mapKey].NMappedVal)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", 1), structure.Log.Format)).ShouldNot(HaveOccurred()) Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", 1), structure.Log.Level)).ShouldNot(HaveOccurred()) } @@ -179,6 +204,12 @@ var _ = Describe("Env", func() { Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", 1))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", 1))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", 1))).ShouldNot(HaveOccurred()) @@ -248,6 +279,14 @@ var _ = Describe("Env", func() { BeforeEach(func() { testFlags = env.EmptyFlagSet() + nest := Nest{ + NBool: true, + NInt: 4321, + NString: "nstringval", + NSlice: []string{"nval1", "nval2", "nval3"}, + NMappedVal: "nmappedval", + } + structure = Outer{ WBool: true, WInt: 1234, @@ -257,12 +296,9 @@ var _ = Describe("Env", func() { Level: "error", Format: "text", }, - Nest: Nest{ - NBool: true, - NInt: 4321, - NString: "nstringval", - NSlice: []string{"nval1", "nval2", "nval3"}, - NMappedVal: "nmappedval", + Nest: nest, + WMapNest: map[string]Nest{ + mapKey: nest, }, } }) diff --git a/pkg/env/helpers.go b/pkg/env/helpers.go index 3e6930fb3..0d6d75353 100644 --- a/pkg/env/helpers.go +++ b/pkg/env/helpers.go @@ -89,47 +89,64 @@ func buildDescriptionPaths(root *descriptionTree, path []*descriptionTree) []str } func buildDescriptionTreeWithParameters(value interface{}, tree *descriptionTree, buffer string, result *[]configurationParameter) { - if !structs.IsStruct(value) { - index := strings.LastIndex(buffer, ".") - if index == -1 { - index = 0 - } - key := strings.ToLower(buffer[0:index]) - *result = append(*result, configurationParameter{Name: key, DefaultValue: value}) - tree.Children = nil - return - } - s := structs.New(value) - k := 0 - for _, field := range s.Fields() { - if isValidField(field) { - var name string - if field.Tag("mapstructure") != "" { - name = field.Tag("mapstructure") - } else { - name = field.Name() + v := reflect.ValueOf(value) + if v.Kind() != reflect.Map { + if !structs.IsStruct(value) { + index := strings.LastIndex(buffer, ".") + if index == -1 { + index = 0 } - - if name == "-" || name == ",squash" { - continue + key := strings.ToLower(buffer[0:index]) + *result = append(*result, configurationParameter{Name: key, DefaultValue: value}) + tree.Children = nil + return + } + s := structs.New(value) + k := 0 + for _, field := range s.Fields() { + if isValidField(field) { + var name string + if field.Tag("mapstructure") != "" { + name = field.Tag("mapstructure") + } else { + name = field.Name() + } + + if name == "-" || name == ",squash" { + continue + } + + description := "" + if field.Tag("description") != "" { + description = field.Tag("description") + } + + baseTree := newDescriptionTree(description) + tree.AddNode(baseTree) + buffer += name + "." + buildDescriptionTreeWithParameters(field.Value(), tree.Children[k], buffer, result) + k++ + buffer = buffer[0:strings.LastIndex(buffer, name)] } - - description := "" - if field.Tag("description") != "" { - description = field.Tag("description") + } + } else { + for _, key := range v.MapKeys() { + field := v.MapIndex(key).Interface() + if isValidField(field) { + name := key.String() + buffer += name + "." + buildDescriptionTreeWithParameters(field, tree, buffer, result) + buffer = buffer[0:strings.LastIndex(buffer, name)] } - - baseTree := newDescriptionTree(description) - tree.AddNode(baseTree) - buffer += name + "." - buildDescriptionTreeWithParameters(field.Value(), tree.Children[k], buffer, result) - k++ - buffer = buffer[0:strings.LastIndex(buffer, name)] } } } -func isValidField(field *structs.Field) bool { - kind := field.Kind() - return field.IsExported() && kind != reflect.Interface && kind != reflect.Func +func isValidField(field interface{}) bool { + if fieldStruct, ok := field.(*structs.Field); ok { + kind := fieldStruct.Kind() + return fieldStruct.IsExported() && kind != reflect.Interface && kind != reflect.Func + } + kind := reflect.ValueOf(field).Kind() + return kind != reflect.Interface && kind != reflect.Func } diff --git a/pkg/health/types.go b/pkg/health/types.go index 1fde4b92c..5b1359fd3 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -26,18 +26,39 @@ import ( "time" ) +// StorageIndicatorName is the name of storage indicator +const StorageIndicatorName = "storage" + +// PlatformIndicatorSuffix is the suffix for a platform type indicator +const PlatformIndicatorSuffix = "_platforms" + +// indicatorNames is a list of names of indicators which will be registered with default settings +// as part of default health settings, this will allow binding them as part of environment. +// If an indicator is registered but not specified in this list, it will be configured with +// default settings again, but this defaults could be overridden only via application.yml, +// env variables and pflags won't have any effect. If an indicator is specified in this list +// but later not registered nothing will happen. +var indicatorNames = [...]string{ + StorageIndicatorName, + "kubernetes" + PlatformIndicatorSuffix, + "cloudfoundry" + PlatformIndicatorSuffix, +} + // Settings type to be loaded from the environment type Settings struct { - PlatformTypes []string `mapstructure:"platform_types,omitempty" description:"platform types which health will be measured"` - Indicators map[string]*IndicatorSettings `mapstructure:"indicators,omitempty"` + PlatformTypes []string `mapstructure:"platform_types" description:"platform types for which health will be measured"` + Indicators map[string]*IndicatorSettings `mapstructure:"indicators"` } // DefaultSettings returns default values for health settings func DefaultSettings() *Settings { - emptySettings := make(map[string]*IndicatorSettings) + defaultIndicatorSettings := make(map[string]*IndicatorSettings) + for _, name := range indicatorNames { + defaultIndicatorSettings[name] = DefaultIndicatorSettings() + } return &Settings{ PlatformTypes: make([]string, 0), - Indicators: emptySettings, + Indicators: defaultIndicatorSettings, } } diff --git a/storage/healthcheck.go b/storage/healthcheck.go index d74d07396..9ae7b53fe 100644 --- a/storage/healthcheck.go +++ b/storage/healthcheck.go @@ -45,5 +45,5 @@ type SQLHealthIndicator struct { // Name returns the name of the storage component func (i *SQLHealthIndicator) Name() string { - return "storage" + return health.StorageIndicatorName } From aace011f5563b6b2a97285ca23d09426e7d588dc Mon Sep 17 00:00:00 2001 From: kkabakchiev Date: Sat, 31 Aug 2019 18:20:49 +0300 Subject: [PATCH 29/34] fix failing tests --- pkg/env/env_test.go | 124 +++++++++++++++++++++++--------------------- pkg/env/helpers.go | 24 +++++---- 2 files changed, 77 insertions(+), 71 deletions(-) diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index fa71cb762..4ea3c0dad 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -48,7 +48,7 @@ func TestEnv(t *testing.T) { var _ = Describe("Env", func() { const ( - mapKey = "map" + mapKey = "mapkey" key = "key" description = "desc" flagDefaultValue = "pflagDefaultValue" @@ -68,11 +68,11 @@ var _ = Describe("Env", func() { keyNslice = "nest.nslice" keyNmappedVal = "nest.n_mapped_val" - keyMapNbool = mapKey + "." + keyNbool - keyMapNint = mapKey + "." + keyNint - keyMapNstring = mapKey + "." + keyNstring - keyMapNslice = mapKey + "." + keyNslice - keyMapNmappedVal = mapKey + "." + keyNmappedVal + keyMapNbool = "wmapnest" + "." + mapKey + "." + "nbool" + keyMapNint = "wmapnest" + "." + mapKey + "." + "nint" + keyMapNstring = "wmapnest" + "." + mapKey + "." + "nstring" + keyMapNslice = "wmapnest" + "." + mapKey + "." + "nslice" + keyMapNmappedVal = "wmapnest" + "." + mapKey + "." + "n_mapped_val" keyLogFormat = "log.format" keyLogLevel = "log.level" @@ -101,7 +101,7 @@ var _ = Describe("Env", func() { } var ( - structure Outer + expected Outer cfgFile testFile testFlags *pflag.FlagSet @@ -171,25 +171,25 @@ var _ = Describe("Env", func() { } setEnvVars := func() { - Expect(os.Setenv(strings.ToTitle(keyWbool), cast.ToString(structure.WBool))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.ToTitle(keyWint), cast.ToString(structure.WInt))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.ToTitle(keyWstring), structure.WString)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.ToTitle(keyWmappedVal), structure.WMappedVal)).ShouldNot(HaveOccurred()) - - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", 1), cast.ToString(structure.Nest.NBool))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", 1), cast.ToString(structure.Nest.NInt))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", 1), structure.Nest.NString)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", 1), strings.Join(structure.Nest.NSlice, ","))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", 1), structure.Nest.NMappedVal)).ShouldNot(HaveOccurred()) - - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", 1), cast.ToString(structure.WMapNest[mapKey].NBool))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", 1), cast.ToString(structure.WMapNest[mapKey].NInt))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", 1), structure.WMapNest[mapKey].NString)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", 1), strings.Join(structure.WMapNest[mapKey].NSlice, ","))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", 1), structure.WMapNest[mapKey].NMappedVal)).ShouldNot(HaveOccurred()) - - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", 1), structure.Log.Format)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", 1), structure.Log.Level)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWbool), cast.ToString(expected.WBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWint), cast.ToString(expected.WInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWstring), expected.WString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWmappedVal), expected.WMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", -1), cast.ToString(expected.Nest.NBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", -1), cast.ToString(expected.Nest.NInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", -1), expected.Nest.NString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", -1), strings.Join(expected.Nest.NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", -1), expected.Nest.NMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", -1), cast.ToString(expected.WMapNest[mapKey].NBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", -1), cast.ToString(expected.WMapNest[mapKey].NInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", -1), expected.WMapNest[mapKey].NString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", -1), strings.Join(expected.WMapNest[mapKey].NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", -1), expected.WMapNest[mapKey].NMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", -1), expected.Log.Format)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", -1), expected.Log.Level)).ShouldNot(HaveOccurred()) } cleanUpEnvVars := func() { @@ -198,20 +198,20 @@ var _ = Describe("Env", func() { Expect(os.Unsetenv(strings.ToTitle(keyWstring))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.ToTitle(keyWmappedVal))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", -1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", -1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", 1))).ShouldNot(HaveOccurred()) - Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", 1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", -1))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", -1))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.ToTitle(key))).ShouldNot(HaveOccurred()) } @@ -287,7 +287,7 @@ var _ = Describe("Env", func() { NMappedVal: "nmappedval", } - structure = Outer{ + expected = Outer{ WBool: true, WInt: 1234, WString: "wstringval", @@ -316,19 +316,19 @@ var _ = Describe("Env", func() { ) It("adds viper bindings for the provided flags", func() { - testFlags.AddFlagSet(standardPFlagsSet(structure)) + testFlags.AddFlagSet(standardPFlagsSet(expected)) cfgFile.content = nil verifyEnvCreated() - verifyEnvContainsValues(structure) + verifyEnvContainsValues(expected) }) Context("when SM config file exists", func() { BeforeEach(func() { cfgFile = testFile{ File: env.DefaultConfigFile(), - content: structure, + content: expected, } }) @@ -384,7 +384,7 @@ var _ = Describe("Env", func() { It("reads the file in the environment", func() { verifyEnvCreated() - verifyEnvContainsValues(structure) + verifyEnvContainsValues(expected) }) It("returns an err if config file loading fails", func() { @@ -488,11 +488,11 @@ var _ = Describe("Env", func() { Context("when properties are loaded via standard pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(standardPFlagsSet(structure)) + testFlags.AddFlagSet(standardPFlagsSet(expected)) }) It("returns the default flag value if the flag is not set", func() { - verifyEnvContainsValues(structure) + verifyEnvContainsValues(expected) }) It("returns the flags values if the flags are set", func() { @@ -505,11 +505,11 @@ var _ = Describe("Env", func() { Context("when properties are loaded via generated pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(generatedPFlagsSet(structure)) + testFlags.AddFlagSet(generatedPFlagsSet(expected)) }) It("returns the default flag value if the flag is not set", func() { - verifyEnvContainsValues(structure) + verifyEnvContainsValues(expected) }) It("returns the flags values if the flags are set", func() { @@ -523,14 +523,14 @@ var _ = Describe("Env", func() { BeforeEach(func() { cfgFile = testFile{ File: env.DefaultConfigFile(), - content: structure, + content: expected, } config.AddPFlags(testFlags) verifyEnvCreated() }) It("returns values from the config file", func() { - verifyEnvContainsValues(structure) + verifyEnvContainsValues(expected) }) }) @@ -540,7 +540,7 @@ var _ = Describe("Env", func() { }) It("returns values from environment", func() { - verifyEnvContainsValues(structure) + verifyEnvContainsValues(expected) }) }) @@ -588,9 +588,9 @@ var _ = Describe("Env", func() { It("has highest priority", func() { testFlags.AddFlagSet(singlePFlagSet(key, flagDefaultValue, description)) - os.Setenv(key, envValue) + Expect(os.Setenv(key, envValue)).ToNot(HaveOccurred()) verifyEnvCreated() - testFlags.Set(key, flagValue) + Expect(testFlags.Set(key, flagValue)).ToNot(HaveOccurred()) environment.Set(key, overrideValue) @@ -602,7 +602,11 @@ var _ = Describe("Env", func() { var actual Outer BeforeEach(func() { - actual = Outer{} + actual = Outer{ + WMapNest: map[string]Nest{ + mapKey: {}, + }, + } }) JustBeforeEach(func() { @@ -632,21 +636,21 @@ var _ = Describe("Env", func() { Context("when properties are loaded via standard pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(standardPFlagsSet(structure)) + testFlags.AddFlagSet(standardPFlagsSet(expected)) }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &structure) + verifyUnmarshallingIsCorrect(&actual, &expected) }) }) Context("when properties are loaded via generated pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(generatedPFlagsSet(structure)) + testFlags.AddFlagSet(generatedPFlagsSet(expected)) }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &structure) + verifyUnmarshallingIsCorrect(&actual, &expected) }) }) @@ -654,13 +658,13 @@ var _ = Describe("Env", func() { BeforeEach(func() { cfgFile = testFile{ File: env.DefaultConfigFile(), - content: structure, + content: expected, } config.AddPFlags(testFlags) }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &structure) + verifyUnmarshallingIsCorrect(&actual, &expected) }) }) @@ -670,7 +674,7 @@ var _ = Describe("Env", func() { }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &structure) + verifyUnmarshallingIsCorrect(&actual, &expected) }) }) diff --git a/pkg/env/helpers.go b/pkg/env/helpers.go index 0d6d75353..784be03f6 100644 --- a/pkg/env/helpers.go +++ b/pkg/env/helpers.go @@ -90,7 +90,18 @@ func buildDescriptionPaths(root *descriptionTree, path []*descriptionTree) []str func buildDescriptionTreeWithParameters(value interface{}, tree *descriptionTree, buffer string, result *[]configurationParameter) { v := reflect.ValueOf(value) - if v.Kind() != reflect.Map { + switch v.Kind() { + case reflect.Map: + for _, key := range v.MapKeys() { + field := v.MapIndex(key).Interface() + if isValidField(field) { + name := key.String() + buffer += name + "." + buildDescriptionTreeWithParameters(field, tree, buffer, result) + buffer = buffer[0:strings.LastIndex(buffer, name)] + } + } + default: if !structs.IsStruct(value) { index := strings.LastIndex(buffer, ".") if index == -1 { @@ -129,16 +140,7 @@ func buildDescriptionTreeWithParameters(value interface{}, tree *descriptionTree buffer = buffer[0:strings.LastIndex(buffer, name)] } } - } else { - for _, key := range v.MapKeys() { - field := v.MapIndex(key).Interface() - if isValidField(field) { - name := key.String() - buffer += name + "." - buildDescriptionTreeWithParameters(field, tree, buffer, result) - buffer = buffer[0:strings.LastIndex(buffer, name)] - } - } + } } From c9db83ecfe5e21fc3ba8dca243797b2883c734ba Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 11 Sep 2019 11:30:56 +0300 Subject: [PATCH 30/34] Address PR comments --- api/healthcheck/healthcheck_controller.go | 27 +++++++++---------- .../healthcheck_controller_test.go | 1 + api/healthcheck/platform_indicator.go | 10 +++++-- api/healthcheck/platform_indicator_test.go | 2 +- pkg/health/types.go | 2 +- storage/postgres/notificator.go | 10 ++++--- 6 files changed, 29 insertions(+), 23 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index be93306cb..93cf60a26 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -18,24 +18,22 @@ package healthcheck import ( "context" - h "github.com/InVisionApp/go-health" + gohealth "github.com/InVisionApp/go-health" "github.com/Peripli/service-manager/pkg/health" - "github.com/Peripli/service-manager/pkg/util" - "net/http" - "strings" - "github.com/Peripli/service-manager/pkg/log" + "github.com/Peripli/service-manager/pkg/util" "github.com/Peripli/service-manager/pkg/web" + "net/http" ) // controller platform controller type controller struct { - health h.IHealth + health gohealth.IHealth thresholds map[string]int64 } // NewController returns a new healthcheck controller with the given health and thresholds -func NewController(health h.IHealth, thresholds map[string]int64) web.Controller { +func NewController(health gohealth.IHealth, thresholds map[string]int64) web.Controller { return &controller{ health: health, thresholds: thresholds, @@ -58,25 +56,24 @@ func (c *controller) healthCheck(r *web.Request) (*web.Response, error) { return util.NewJSONResponse(status, healthResult) } -func (c *controller) aggregate(ctx context.Context, overallState map[string]h.State) *health.Health { - if len(overallState) == 0 { +func (c *controller) aggregate(ctx context.Context, healthState map[string]gohealth.State) *health.Health { + if len(healthState) == 0 { return health.New().WithStatus(health.StatusUp) } + + details := make(map[string]interface{}) overallStatus := health.StatusUp - for name, state := range overallState { + for name, state := range healthState { if state.Fatal && state.ContiguousFailures >= c.thresholds[name] { overallStatus = health.StatusDown - break } - } - details := make(map[string]interface{}) - for name, state := range overallState { state.Status = convertStatus(state.Status) - if strings.Contains(name, health.PlatformIndicatorSuffix) && !web.IsAuthorized(ctx) { + if !web.IsAuthorized(ctx) { state.Details = nil } details[name] = state } + return health.New().WithStatus(overallStatus).WithDetails(details) } diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index 99f469cfe..472c088f3 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -132,6 +132,7 @@ var _ = Describe("Healthcheck controller", func() { aggregatedHealth := c.aggregate(ctx, healths) for name, h := range healths { h.Status = convertStatus(h.Status) + h.Details = nil Expect(aggregatedHealth.Details[name]).To(Equal(h)) } }) diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go index fe12c9bb8..73f2df7bc 100644 --- a/api/healthcheck/platform_indicator.go +++ b/api/healthcheck/platform_indicator.go @@ -58,15 +58,21 @@ func (pi *platformIndicator) Status() (interface{}, error) { return nil, fmt.Errorf("could not fetch platforms health from storage: %v", err) } - details := make(map[string]interface{}) + details := make(map[string]*health.Health) + inactivePlatformsCount := 0 for i := 0; i < objList.Len(); i++ { platform := objList.ItemAt(i).(*types.Platform) if platform.Active { details[platform.Name] = health.New().WithStatus(health.StatusUp) } else { details[platform.Name] = health.New().WithStatus(health.StatusDown).WithDetail("since", platform.LastActive) - err = fmt.Errorf("there is inactive %s platforms", pi.platformType) + inactivePlatformsCount++ } } + + if inactivePlatformsCount > 0 { + err = fmt.Errorf("there are %d inactive %s platforms", inactivePlatformsCount, pi.platformType) + } + return details, err } diff --git a/api/healthcheck/platform_indicator_test.go b/api/healthcheck/platform_indicator_test.go index 384b5ded4..cc3a330cd 100644 --- a/api/healthcheck/platform_indicator_test.go +++ b/api/healthcheck/platform_indicator_test.go @@ -60,7 +60,7 @@ var _ = Describe("Platforms Indicator", func() { }) It("should return error", func() { details, err := indicator.Status() - health := details.(map[string]interface{})[platform.Name].(*health.Health) + health := details.(map[string]*health.Health)[platform.Name] Expect(err).Should(HaveOccurred()) Expect(health.Details["since"]).ShouldNot(BeNil()) }) diff --git a/pkg/health/types.go b/pkg/health/types.go index 5b1359fd3..6725b8897 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -91,7 +91,7 @@ func DefaultIndicatorSettings() *IndicatorSettings { // Validate validates indicator settings func (is *IndicatorSettings) Validate() error { if !is.Fatal && is.FailuresThreshold != 0 { - return fmt.Errorf("validate Settings: FailuresThreshold not applicable for non-fatal indicators") + return fmt.Errorf("validate Settings: FailuresThreshold must be 0 for non-fatal indicators") } if is.Fatal && is.FailuresThreshold <= 0 { return fmt.Errorf("validate Settings: FailuresThreshold must be > 0 for fatal indicators") diff --git a/storage/postgres/notificator.go b/storage/postgres/notificator.go index bcc5b1754..3477d3901 100644 --- a/storage/postgres/notificator.go +++ b/storage/postgres/notificator.go @@ -465,10 +465,11 @@ func (c *consumers) Delete(queue storage.NotificationQueue) error { for index, platform := range c.platforms { if platform.ID == platformIDToDelete { c.platforms = append(c.platforms[:index], c.platforms[index+1:]...) - if err := c.updatePlatform(platform.ID, func(p *types.Platform) { + err := c.updatePlatform(platform.ID, func(p *types.Platform) { p.Active = false p.LastActive = time.Now() - }); err != nil { + }) + if err != nil { return err } break @@ -481,9 +482,10 @@ func (c *consumers) Delete(queue storage.NotificationQueue) error { func (c *consumers) Add(platform *types.Platform, queue storage.NotificationQueue) error { if len(c.queues[platform.ID]) == 0 { c.platforms = append(c.platforms, platform) - if err := c.updatePlatform(platform.ID, func(p *types.Platform) { + err := c.updatePlatform(platform.ID, func(p *types.Platform) { p.Active = true - }); err != nil { + }) + if err != nil { return err } } From 11fb13f8831282523dc90950cc369e8050ffa235 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 11 Sep 2019 16:38:05 +0300 Subject: [PATCH 31/34] Refactor platform indicator --- api/healthcheck/platform_indicator.go | 47 ++++++++++--------- api/healthcheck/platform_indicator_test.go | 12 ++--- pkg/health/types.go | 24 ++++++---- pkg/sm/sm.go | 6 +-- ...20190829101500_add_platforms_status.up.sql | 2 +- 5 files changed, 49 insertions(+), 42 deletions(-) diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go index 73f2df7bc..edb899d0f 100644 --- a/api/healthcheck/platform_indicator.go +++ b/api/healthcheck/platform_indicator.go @@ -20,58 +20,61 @@ import ( "context" "fmt" "github.com/Peripli/service-manager/pkg/health" - "github.com/Peripli/service-manager/pkg/query" "github.com/Peripli/service-manager/pkg/types" "github.com/Peripli/service-manager/storage" ) // NewPlatformIndicator returns new health indicator for platforms of given type -func NewPlatformIndicator(ctx context.Context, repository storage.Repository, platformType string) health.Indicator { +func NewPlatformIndicator(ctx context.Context, repository storage.Repository, fatal func(*types.Platform) bool) health.Indicator { + if fatal == nil { + fatal = func(platform *types.Platform) bool { + return true + } + } return &platformIndicator{ - ctx: ctx, - repository: repository, - platformType: platformType, + ctx: ctx, + repository: repository, + fatal: fatal, } } type platformIndicator struct { - repository storage.Repository - ctx context.Context - platformType string + repository storage.Repository + ctx context.Context + fatal func(*types.Platform) bool } // Name returns the name of the indicator func (pi *platformIndicator) Name() string { - return pi.platformType + health.PlatformIndicatorSuffix // e.g. cf_platforms, k8s_platforms ... + return health.PlatformsIndicatorName } // Status returns status of the health check func (pi *platformIndicator) Status() (interface{}, error) { - typeCriteria := query.Criterion{ - LeftOp: "type", - Operator: query.EqualsOperator, - RightOp: []string{pi.platformType}, - Type: query.FieldQuery, - } - objList, err := pi.repository.List(pi.ctx, types.PlatformType, typeCriteria) + objList, err := pi.repository.List(pi.ctx, types.PlatformType) if err != nil { return nil, fmt.Errorf("could not fetch platforms health from storage: %v", err) } + platforms := objList.(*types.Platforms).Platforms details := make(map[string]*health.Health) inactivePlatformsCount := 0 - for i := 0; i < objList.Len(); i++ { - platform := objList.ItemAt(i).(*types.Platform) + for _, platform := range platforms { if platform.Active { - details[platform.Name] = health.New().WithStatus(health.StatusUp) + details[platform.Name] = health.New().WithStatus(health.StatusUp). + WithDetail("type", platform.Type) } else { - details[platform.Name] = health.New().WithStatus(health.StatusDown).WithDetail("since", platform.LastActive) - inactivePlatformsCount++ + details[platform.Name] = health.New().WithStatus(health.StatusDown). + WithDetail("since", platform.LastActive). + WithDetail("type", platform.Type) + if pi.fatal(platform) { + inactivePlatformsCount++ + } } } if inactivePlatformsCount > 0 { - err = fmt.Errorf("there are %d inactive %s platforms", inactivePlatformsCount, pi.platformType) + err = fmt.Errorf("there are %d inactive platforms", inactivePlatformsCount) } return details, err diff --git a/api/healthcheck/platform_indicator_test.go b/api/healthcheck/platform_indicator_test.go index cc3a330cd..94ff4d4ea 100644 --- a/api/healthcheck/platform_indicator_test.go +++ b/api/healthcheck/platform_indicator_test.go @@ -30,30 +30,28 @@ import ( var _ = Describe("Platforms Indicator", func() { var indicator health.Indicator var repository *storagefakes2.FakeStorage - var platformType string var ctx context.Context var platform *types.Platform BeforeEach(func() { ctx = context.TODO() repository = &storagefakes2.FakeStorage{} - platformType = "kubernetes" platform = &types.Platform{ Name: "test-platform", - Type: platformType, + Type: "kubernetes", Active: false, LastActive: time.Now(), } - indicator = NewPlatformIndicator(ctx, repository, platformType) + indicator = NewPlatformIndicator(ctx, repository, nil) }) Context("Name", func() { - It("should contain platform type in it", func() { - Expect(indicator.Name()).Should(ContainSubstring(platformType)) + It("should not be empty", func() { + Expect(indicator.Name()).Should(Equal(health.PlatformsIndicatorName)) }) }) - Context("There is inactive platforms", func() { + Context("There are inactive platforms", func() { BeforeEach(func() { objectList := &types.Platforms{[]*types.Platform{platform}} repository.ListReturns(objectList, nil) diff --git a/pkg/health/types.go b/pkg/health/types.go index 6725b8897..dbf5ac851 100644 --- a/pkg/health/types.go +++ b/pkg/health/types.go @@ -29,8 +29,8 @@ import ( // StorageIndicatorName is the name of storage indicator const StorageIndicatorName = "storage" -// PlatformIndicatorSuffix is the suffix for a platform type indicator -const PlatformIndicatorSuffix = "_platforms" +// PlatformsIndicatorName is the name of platforms indicator +const PlatformsIndicatorName = "platforms" // indicatorNames is a list of names of indicators which will be registered with default settings // as part of default health settings, this will allow binding them as part of environment. @@ -40,14 +40,12 @@ const PlatformIndicatorSuffix = "_platforms" // but later not registered nothing will happen. var indicatorNames = [...]string{ StorageIndicatorName, - "kubernetes" + PlatformIndicatorSuffix, - "cloudfoundry" + PlatformIndicatorSuffix, + PlatformsIndicatorName, } // Settings type to be loaded from the environment type Settings struct { - PlatformTypes []string `mapstructure:"platform_types" description:"platform types for which health will be measured"` - Indicators map[string]*IndicatorSettings `mapstructure:"indicators"` + Indicators map[string]*IndicatorSettings `mapstructure:"indicators"` } // DefaultSettings returns default values for health settings @@ -57,8 +55,7 @@ func DefaultSettings() *Settings { defaultIndicatorSettings[name] = DefaultIndicatorSettings() } return &Settings{ - PlatformTypes: make([]string, 0), - Indicators: defaultIndicatorSettings, + Indicators: defaultIndicatorSettings, } } @@ -192,6 +189,17 @@ type Registry struct { HealthIndicators []Indicator } +// SetIndicator adds or replaces existing indicator with same name in registry +func (r *Registry) SetIndicator(healthIndicator Indicator) { + for i, indicator := range r.HealthIndicators { + if indicator.Name() == healthIndicator.Name() { + r.HealthIndicators[i] = healthIndicator + return + } + } + r.HealthIndicators = append(r.HealthIndicators, healthIndicator) +} + // Configure creates new health using provided settings. func Configure(ctx context.Context, indicators []Indicator, settings *Settings) (*h.Health, map[string]int64, error) { healthz := h.New() diff --git a/pkg/sm/sm.go b/pkg/sm/sm.go index 664ce621c..f13496209 100644 --- a/pkg/sm/sm.go +++ b/pkg/sm/sm.go @@ -133,10 +133,8 @@ func New(ctx context.Context, cancel context.CancelFunc, cfg *config.Settings) ( return nil, fmt.Errorf("error creating storage health indicator: %s", err) } - API.HealthIndicators = append(API.HealthIndicators, storageHealthIndicator) - for _, platformType := range cfg.Health.PlatformTypes { - API.HealthIndicators = append(API.HealthIndicators, healthcheck.NewPlatformIndicator(ctx, interceptableRepository, platformType)) - } + API.SetIndicator(storageHealthIndicator) + API.SetIndicator(healthcheck.NewPlatformIndicator(ctx, interceptableRepository, nil)) notificationCleaner := &storage.NotificationCleaner{ Storage: interceptableRepository, diff --git a/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql index 03a51f3e5..aa3ccbcb4 100644 --- a/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql +++ b/storage/postgres/migrations/20190829101500_add_platforms_status.up.sql @@ -1,6 +1,6 @@ BEGIN; ALTER TABLE platforms ADD COLUMN active boolean NOT NULL DEFAULT '0'; -ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP NOT NULL DEFAULT '1970-01-01 00:00:00+00'; +ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP NOT NULL DEFAULT '0001-01-01 00:00:00+00'; COMMIT; \ No newline at end of file From bf23318d7456f66a6536218e13c2464ddc07684b Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 11 Sep 2019 17:25:22 +0300 Subject: [PATCH 32/34] Strip indicator error on unauthorized request --- api/healthcheck/healthcheck_controller.go | 1 + .../healthcheck_controller_test.go | 29 +++++-------------- api/healthcheck/platform_indicator.go | 10 ++++--- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/api/healthcheck/healthcheck_controller.go b/api/healthcheck/healthcheck_controller.go index 93cf60a26..3e3a09a23 100644 --- a/api/healthcheck/healthcheck_controller.go +++ b/api/healthcheck/healthcheck_controller.go @@ -70,6 +70,7 @@ func (c *controller) aggregate(ctx context.Context, healthState map[string]gohea state.Status = convertStatus(state.Status) if !web.IsAuthorized(ctx) { state.Details = nil + state.Err = "" } details[name] = state } diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index 472c088f3..75c914880 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -65,7 +65,6 @@ var _ = Describe("Healthcheck controller", func() { var ctx context.Context var c *controller var healths map[string]h.State - var platformHealths map[string]h.State var thresholds map[string]int64 BeforeEach(func() { @@ -73,9 +72,7 @@ var _ = Describe("Healthcheck controller", func() { healths = map[string]h.State{ "test1": {Status: "ok", Details: "details"}, "test2": {Status: "ok", Details: "details"}, - } - platformHealths = map[string]h.State{ - "k8s_platforms": {Status: "ok", Details: "details"}, + "failedState": {Status: "failed", Details: "details", Err: "err"}, } thresholds = map[string]int64{ "test1": 3, @@ -127,33 +124,23 @@ var _ = Describe("Healthcheck controller", func() { }) }) - When("Aggregating healths", func() { - It("Includes them as overall details", func() { + When("Aggregating health as unauthorized user", func() { + It("should strip details and error", func() { aggregatedHealth := c.aggregate(ctx, healths) for name, h := range healths { h.Status = convertStatus(h.Status) h.Details = nil + h.Err = "" Expect(aggregatedHealth.Details[name]).To(Equal(h)) } }) }) - When("Aggregating platforms health as unauthorized user", func() { - It("should strip platform details", func() { - aggregatedHealth := c.aggregate(ctx, platformHealths) - for name, h := range platformHealths { - h.Status = convertStatus(h.Status) - h.Details = nil - Expect(aggregatedHealth.Details[name]).To(Equal(h)) - } - }) - }) - - When("Aggregating platforms health as authorized user", func() { - It("should include all platform details", func() { + When("Aggregating health as authorized user", func() { + It("should include all details and errors", func() { ctx = web.ContextWithAuthorization(ctx) - aggregatedHealth := c.aggregate(ctx, platformHealths) - for name, h := range platformHealths { + aggregatedHealth := c.aggregate(ctx, healths) + for name, h := range healths { h.Status = convertStatus(h.Status) Expect(aggregatedHealth.Details[name]).To(Equal(h)) } diff --git a/api/healthcheck/platform_indicator.go b/api/healthcheck/platform_indicator.go index edb899d0f..6688090b8 100644 --- a/api/healthcheck/platform_indicator.go +++ b/api/healthcheck/platform_indicator.go @@ -58,7 +58,8 @@ func (pi *platformIndicator) Status() (interface{}, error) { platforms := objList.(*types.Platforms).Platforms details := make(map[string]*health.Health) - inactivePlatformsCount := 0 + inactivePlatforms := 0 + fatalInactivePlatforms := 0 for _, platform := range platforms { if platform.Active { details[platform.Name] = health.New().WithStatus(health.StatusUp). @@ -67,14 +68,15 @@ func (pi *platformIndicator) Status() (interface{}, error) { details[platform.Name] = health.New().WithStatus(health.StatusDown). WithDetail("since", platform.LastActive). WithDetail("type", platform.Type) + inactivePlatforms++ if pi.fatal(platform) { - inactivePlatformsCount++ + fatalInactivePlatforms++ } } } - if inactivePlatformsCount > 0 { - err = fmt.Errorf("there are %d inactive platforms", inactivePlatformsCount) + if fatalInactivePlatforms > 0 { + err = fmt.Errorf("there are %d inactive platforms %d of them are fatal", inactivePlatforms, fatalInactivePlatforms) } return details, err From cfd8a90956a5e7d1c60aec734ea7217438d82646 Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Wed, 11 Sep 2019 17:25:51 +0300 Subject: [PATCH 33/34] Fix formatting --- api/healthcheck/healthcheck_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/healthcheck/healthcheck_controller_test.go b/api/healthcheck/healthcheck_controller_test.go index 75c914880..826c95934 100644 --- a/api/healthcheck/healthcheck_controller_test.go +++ b/api/healthcheck/healthcheck_controller_test.go @@ -70,8 +70,8 @@ var _ = Describe("Healthcheck controller", func() { BeforeEach(func() { ctx = context.TODO() healths = map[string]h.State{ - "test1": {Status: "ok", Details: "details"}, - "test2": {Status: "ok", Details: "details"}, + "test1": {Status: "ok", Details: "details"}, + "test2": {Status: "ok", Details: "details"}, "failedState": {Status: "failed", Details: "details", Err: "err"}, } thresholds = map[string]int64{ From d2fd94421a1060024dc11b99c0e6b434b880e06a Mon Sep 17 00:00:00 2001 From: Dimitar Petrov Date: Thu, 12 Sep 2019 14:30:17 +0300 Subject: [PATCH 34/34] Fix squash tag handling --- pkg/env/env_test.go | 234 ++++++++++++++++++++++++++++++-------------- pkg/env/helpers.go | 15 ++- 2 files changed, 164 insertions(+), 85 deletions(-) diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 4ea3c0dad..bdbee1634 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -19,12 +19,11 @@ package env_test import ( "context" "fmt" - "github.com/Peripli/service-manager/pkg/log" + "github.com/fatih/structs" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "github.com/sirupsen/logrus" - "io/ioutil" "os" "testing" @@ -35,7 +34,6 @@ import ( "github.com/Peripli/service-manager/config" "github.com/Peripli/service-manager/pkg/env" - "github.com/fatih/structs" "github.com/spf13/cast" "github.com/spf13/pflag" "gopkg.in/yaml.v2" @@ -68,6 +66,12 @@ var _ = Describe("Env", func() { keyNslice = "nest.nslice" keyNmappedVal = "nest.n_mapped_val" + keySquashNbool = "nbool" + keySquashNint = "nint" + keySquashNstring = "nstring" + keySquashNslice = "nslice" + keySquashNmappedVal = "n_mapped_val" + keyMapNbool = "wmapnest" + "." + mapKey + "." + "nbool" keyMapNint = "wmapnest" + "." + mapKey + "." + "nint" keyMapNstring = "wmapnest" + "." + mapKey + "." + "nstring" @@ -92,16 +96,36 @@ var _ = Describe("Env", func() { WMappedVal string `mapstructure:"w_mapped_val" structs:"w_mapped_val" yaml:"w_mapped_val"` WMapNest map[string]Nest Nest Nest + Squash Nest `mapstructure:",squash"` Log log.Settings } + type FlatOuter struct { + WBool bool + WInt int + WString string + WMappedVal string `mapstructure:"w_mapped_val" structs:"w_mapped_val" yaml:"w_mapped_val"` + WMapNest map[string]Nest + Nest Nest + + // Flattened Nest fields due to squash tag + NBool bool + NInt int + NString string + NSlice []string + NMappedVal string `mapstructure:"n_mapped_val" structs:"n_mapped_val" yaml:"n_mapped_val"` + + Log log.Settings + } + type testFile struct { env.File content interface{} } var ( - expected Outer + outer Outer + flatOuter FlatOuter cfgFile testFile testFlags *pflag.FlagSet @@ -125,6 +149,12 @@ var _ = Describe("Env", func() { set.String(keyWstring, s.WString, description) set.String(keyWmappedVal, s.WMappedVal, description) + set.Bool(keySquashNbool, s.Squash.NBool, description) + set.Int(keySquashNint, s.Squash.NInt, description) + set.String(keySquashNstring, s.Squash.NString, description) + set.StringSlice(keySquashNslice, s.Squash.NSlice, description) + set.String(keySquashNmappedVal, s.Squash.NMappedVal, description) + set.Bool(keyNbool, s.Nest.NBool, description) set.Int(keyNint, s.Nest.NInt, description) set.String(keyNstring, s.Nest.NString, description) @@ -156,6 +186,12 @@ var _ = Describe("Env", func() { Expect(testFlags.Set(keyWstring, o.WString)).ShouldNot(HaveOccurred()) Expect(testFlags.Set(keyWmappedVal, o.WMappedVal)).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keySquashNbool, cast.ToString(o.Squash.NBool))).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keySquashNint, cast.ToString(o.Squash.NInt))).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keySquashNstring, o.Squash.NString)).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keySquashNslice, strings.Join(o.Squash.NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keySquashNmappedVal, o.Squash.NMappedVal)).ShouldNot(HaveOccurred()) + Expect(testFlags.Set(keyNbool, cast.ToString(o.Nest.NBool))).ShouldNot(HaveOccurred()) Expect(testFlags.Set(keyNint, cast.ToString(o.Nest.NInt))).ShouldNot(HaveOccurred()) Expect(testFlags.Set(keyNstring, o.Nest.NString)).ShouldNot(HaveOccurred()) @@ -171,25 +207,31 @@ var _ = Describe("Env", func() { } setEnvVars := func() { - Expect(os.Setenv(strings.ToTitle(keyWbool), cast.ToString(expected.WBool))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.ToTitle(keyWint), cast.ToString(expected.WInt))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.ToTitle(keyWstring), expected.WString)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.ToTitle(keyWmappedVal), expected.WMappedVal)).ShouldNot(HaveOccurred()) - - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", -1), cast.ToString(expected.Nest.NBool))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", -1), cast.ToString(expected.Nest.NInt))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", -1), expected.Nest.NString)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", -1), strings.Join(expected.Nest.NSlice, ","))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", -1), expected.Nest.NMappedVal)).ShouldNot(HaveOccurred()) - - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", -1), cast.ToString(expected.WMapNest[mapKey].NBool))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", -1), cast.ToString(expected.WMapNest[mapKey].NInt))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", -1), expected.WMapNest[mapKey].NString)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", -1), strings.Join(expected.WMapNest[mapKey].NSlice, ","))).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", -1), expected.WMapNest[mapKey].NMappedVal)).ShouldNot(HaveOccurred()) - - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", -1), expected.Log.Format)).ShouldNot(HaveOccurred()) - Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", -1), expected.Log.Level)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWbool), cast.ToString(outer.WBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWint), cast.ToString(outer.WInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWstring), outer.WString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keyWmappedVal), outer.WMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.ToTitle(keySquashNbool), cast.ToString(outer.Squash.NBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keySquashNint), cast.ToString(outer.Squash.NInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keySquashNstring), outer.Squash.NString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keySquashNslice), strings.Join(outer.Squash.NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.ToTitle(keySquashNmappedVal), outer.Squash.NMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", -1), cast.ToString(outer.Nest.NBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", -1), cast.ToString(outer.Nest.NInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", -1), outer.Nest.NString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNslice), ".", "_", -1), strings.Join(outer.Nest.NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyNmappedVal), ".", "_", -1), outer.Nest.NMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNbool), ".", "_", -1), cast.ToString(outer.WMapNest[mapKey].NBool))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNint), ".", "_", -1), cast.ToString(outer.WMapNest[mapKey].NInt))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNstring), ".", "_", -1), outer.WMapNest[mapKey].NString)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNslice), ".", "_", -1), strings.Join(outer.WMapNest[mapKey].NSlice, ","))).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyMapNmappedVal), ".", "_", -1), outer.WMapNest[mapKey].NMappedVal)).ShouldNot(HaveOccurred()) + + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogFormat), ".", "_", -1), outer.Log.Format)).ShouldNot(HaveOccurred()) + Expect(os.Setenv(strings.Replace(strings.ToTitle(keyLogLevel), ".", "_", -1), outer.Log.Level)).ShouldNot(HaveOccurred()) } cleanUpEnvVars := func() { @@ -198,6 +240,12 @@ var _ = Describe("Env", func() { Expect(os.Unsetenv(strings.ToTitle(keyWstring))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.ToTitle(keyWmappedVal))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.ToTitle(keySquashNbool))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.ToTitle(keySquashNint))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.ToTitle(keySquashNstring))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.ToTitle(keySquashNslice))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.ToTitle(keySquashNmappedVal))).ShouldNot(HaveOccurred()) + Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNbool), ".", "_", -1))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNint), ".", "_", -1))).ShouldNot(HaveOccurred()) Expect(os.Unsetenv(strings.Replace(strings.ToTitle(keyNstring), ".", "_", -1))).ShouldNot(HaveOccurred()) @@ -245,37 +293,33 @@ var _ = Describe("Env", func() { Expect(createEnv()).ShouldNot(HaveOccurred()) } - verifyEnvContainsValues := func(expected interface{}) { - props := structs.Map(expected) - for key, expectedValue := range props { - switch v := expectedValue.(type) { - case map[string]interface{}: - for nestedKey, nestedExpectedValue := range v { - expectedValue, ok := nestedExpectedValue.([]string) - if ok { - nestedExpectedValue = strings.Join(expectedValue, ",") - } + var verifyValues func(expected map[string]interface{}, prefix string) - envValue := environment.Get(key + "." + nestedKey) - switch actualValue := envValue.(type) { - case []string: - envValue = strings.Join(actualValue, ",") - case []interface{}: - temp := make([]string, len(actualValue)) - for i, v := range actualValue { - temp[i] = fmt.Sprint(v) - } - envValue = strings.Join(temp, ",") - } - - Expect(cast.ToString(envValue)).Should(Equal(cast.ToString(nestedExpectedValue))) + verifyValues = func(fields map[string]interface{}, prefix string) { + for name, value := range fields { + switch v := value.(type) { + case map[string]interface{}: + verifyValues(v, prefix+name+".") + case []string: + switch envVar := environment.Get(prefix + name).(type) { + case string: + Expect(envVar).To(Equal(strings.Join(v, ","))) + case []string, []interface{}: + Expect(fmt.Sprint(envVar)).To(Equal(fmt.Sprint(v))) + default: + Fail(fmt.Sprintf("Expected env value of type []string but got: %T", envVar)) } default: - Expect(cast.ToString(environment.Get(key))).To(Equal(cast.ToString(expectedValue))) + Expect(cast.ToString(environment.Get(prefix+name))).To(Equal(cast.ToString(v)), prefix+name) } } } + verifyEnvContainsValues := func(expected interface{}) { + fields := structs.Map(expected) + verifyValues(fields, "") + } + BeforeEach(func() { testFlags = env.EmptyFlagSet() @@ -287,11 +331,32 @@ var _ = Describe("Env", func() { NMappedVal: "nmappedval", } - expected = Outer{ + outer = Outer{ + WBool: true, + WInt: 1234, + WString: "wstringval", + WMappedVal: "wmappedval", + Squash: nest, + Log: log.Settings{ + Level: "error", + Format: "text", + }, + Nest: nest, + WMapNest: map[string]Nest{ + mapKey: nest, + }, + } + + flatOuter = FlatOuter{ WBool: true, WInt: 1234, WString: "wstringval", WMappedVal: "wmappedval", + NBool: true, + NInt: 4321, + NString: "nstringval", + NSlice: []string{"nval1", "nval2", "nval3"}, + NMappedVal: "nmappedval", Log: log.Settings{ Level: "error", Format: "text", @@ -316,19 +381,19 @@ var _ = Describe("Env", func() { ) It("adds viper bindings for the provided flags", func() { - testFlags.AddFlagSet(standardPFlagsSet(expected)) + testFlags.AddFlagSet(standardPFlagsSet(outer)) cfgFile.content = nil verifyEnvCreated() - verifyEnvContainsValues(expected) + verifyEnvContainsValues(flatOuter) }) Context("when SM config file exists", func() { BeforeEach(func() { cfgFile = testFile{ File: env.DefaultConfigFile(), - content: expected, + content: flatOuter, } }) @@ -384,7 +449,7 @@ var _ = Describe("Env", func() { It("reads the file in the environment", func() { verifyEnvCreated() - verifyEnvContainsValues(expected) + verifyEnvContainsValues(flatOuter) }) It("returns an err if config file loading fails", func() { @@ -406,7 +471,7 @@ var _ = Describe("Env", func() { Expect(log.D().Logger.Out.(*os.File).Name()).ToNot(Equal(newOutput)) f := cfgFile.Location + string(filepath.Separator) + cfgFile.Name + "." + cfgFile.Format - fileContent := cfgFile.content.(Outer) + fileContent := cfgFile.content.(FlatOuter) fileContent.Log.Level = logrus.DebugLevel.String() fileContent.Log.Output = newOutput cfgFile.content = fileContent @@ -461,20 +526,37 @@ var _ = Describe("Env", func() { Describe("Get", func() { var overrideStructure Outer + var overrideStructureOutput FlatOuter BeforeEach(func() { + nest := Nest{ + NBool: false, + NInt: 9999, + NString: "overrideval", + NSlice: []string{"nval1", "nval2", "nval3"}, + NMappedVal: "overrideval", + } + overrideStructure = Outer{ WBool: false, WInt: 8888, WString: "overrideval", WMappedVal: "overrideval", - Nest: Nest{ - NBool: false, - NInt: 9999, - NString: "overrideval", - NSlice: []string{"nval1", "nval2", "nval3"}, - NMappedVal: "overrideval", - }, + Nest: nest, + Squash: nest, + } + + overrideStructureOutput = FlatOuter{ + WBool: false, + WInt: 8888, + WString: "overrideval", + WMappedVal: "overrideval", + Nest: nest, + NBool: false, + NInt: 9999, + NString: "overrideval", + NSlice: []string{"nval1", "nval2", "nval3"}, + NMappedVal: "overrideval", } }) @@ -488,34 +570,34 @@ var _ = Describe("Env", func() { Context("when properties are loaded via standard pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(standardPFlagsSet(expected)) + testFlags.AddFlagSet(standardPFlagsSet(outer)) }) It("returns the default flag value if the flag is not set", func() { - verifyEnvContainsValues(expected) + verifyEnvContainsValues(flatOuter) }) It("returns the flags values if the flags are set", func() { setPFlags(overrideStructure) - verifyEnvContainsValues(overrideStructure) + verifyEnvContainsValues(overrideStructureOutput) }) }) Context("when properties are loaded via generated pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(generatedPFlagsSet(expected)) + testFlags.AddFlagSet(generatedPFlagsSet(outer)) }) It("returns the default flag value if the flag is not set", func() { - verifyEnvContainsValues(expected) + verifyEnvContainsValues(flatOuter) }) It("returns the flags values if the flags are set", func() { setPFlags(overrideStructure) - verifyEnvContainsValues(overrideStructure) + verifyEnvContainsValues(overrideStructureOutput) }) }) @@ -523,14 +605,14 @@ var _ = Describe("Env", func() { BeforeEach(func() { cfgFile = testFile{ File: env.DefaultConfigFile(), - content: expected, + content: flatOuter, } config.AddPFlags(testFlags) verifyEnvCreated() }) It("returns values from the config file", func() { - verifyEnvContainsValues(expected) + verifyEnvContainsValues(flatOuter) }) }) @@ -540,7 +622,7 @@ var _ = Describe("Env", func() { }) It("returns values from environment", func() { - verifyEnvContainsValues(expected) + verifyEnvContainsValues(flatOuter) }) }) @@ -636,21 +718,21 @@ var _ = Describe("Env", func() { Context("when properties are loaded via standard pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(standardPFlagsSet(expected)) + testFlags.AddFlagSet(standardPFlagsSet(outer)) }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &expected) + verifyUnmarshallingIsCorrect(&actual, &outer) }) }) Context("when properties are loaded via generated pflags", func() { BeforeEach(func() { - testFlags.AddFlagSet(generatedPFlagsSet(expected)) + testFlags.AddFlagSet(generatedPFlagsSet(outer)) }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &expected) + verifyUnmarshallingIsCorrect(&actual, &outer) }) }) @@ -658,13 +740,13 @@ var _ = Describe("Env", func() { BeforeEach(func() { cfgFile = testFile{ File: env.DefaultConfigFile(), - content: expected, + content: flatOuter, } config.AddPFlags(testFlags) }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &expected) + verifyUnmarshallingIsCorrect(&actual, &outer) }) }) @@ -674,7 +756,7 @@ var _ = Describe("Env", func() { }) It("unmarshals correctly", func() { - verifyUnmarshallingIsCorrect(&actual, &expected) + verifyUnmarshallingIsCorrect(&actual, &outer) }) }) diff --git a/pkg/env/helpers.go b/pkg/env/helpers.go index 784be03f6..61322aba7 100644 --- a/pkg/env/helpers.go +++ b/pkg/env/helpers.go @@ -113,7 +113,6 @@ func buildDescriptionTreeWithParameters(value interface{}, tree *descriptionTree return } s := structs.New(value) - k := 0 for _, field := range s.Fields() { if isValidField(field) { var name string @@ -122,11 +121,13 @@ func buildDescriptionTreeWithParameters(value interface{}, tree *descriptionTree } else { name = field.Name() } - - if name == "-" || name == ",squash" { + if name == "-" { + continue + } + if name == ",squash" { + buildDescriptionTreeWithParameters(field.Value(), tree, buffer, result) continue } - description := "" if field.Tag("description") != "" { description = field.Tag("description") @@ -134,13 +135,9 @@ func buildDescriptionTreeWithParameters(value interface{}, tree *descriptionTree baseTree := newDescriptionTree(description) tree.AddNode(baseTree) - buffer += name + "." - buildDescriptionTreeWithParameters(field.Value(), tree.Children[k], buffer, result) - k++ - buffer = buffer[0:strings.LastIndex(buffer, name)] + buildDescriptionTreeWithParameters(field.Value(), baseTree, buffer+name+".", result) } } - } }