Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Platforms connection in health-check #325

Merged
merged 36 commits into from
Sep 12, 2019
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
4139ea3
Async health check
DimitarPetrov Jun 11, 2019
144ea91
Tests adaptation
DimitarPetrov Jun 11, 2019
61157b4
Make health configurable externally
DimitarPetrov Jun 17, 2019
9f62a81
Install health later
DimitarPetrov Jun 17, 2019
81ff665
Export ConvertStatus function
DimitarPetrov Jun 17, 2019
115e2a4
Fix health settings validation
DimitarPetrov Jun 17, 2019
37f9bc4
Attach logger to health
DimitarPetrov Jun 18, 2019
4f21ed9
Address PR comments
DimitarPetrov Jun 18, 2019
421d88f
Configuration per indicator and refactoring
DimitarPetrov Jun 24, 2019
30ab52e
Add health status listener
DimitarPetrov Jun 25, 2019
d25a87d
Minor tweaks
DimitarPetrov Jun 25, 2019
794c3b0
Fix indicator interval type
DimitarPetrov Jul 8, 2019
40fb02b
Fix tests
DimitarPetrov Jul 8, 2019
37c03dd
Remove unused import
DimitarPetrov Aug 20, 2019
f138807
Extract indicator configuration and address PR comments
DimitarPetrov Aug 20, 2019
ce4d4bb
Add error to panic
DimitarPetrov Aug 20, 2019
3a34381
Address PR comments
DimitarPetrov Aug 21, 2019
91eb2a9
minor fix
DimitarPetrov Aug 21, 2019
31f86b7
Add active status to platform
DimitarPetrov Aug 21, 2019
0ac6fb5
Platforms connection in health
DimitarPetrov Aug 22, 2019
17a1bf7
Merge branch 'master' into platform-health
DimitarPetrov Aug 23, 2019
7ffd051
Add tests
DimitarPetrov Aug 26, 2019
9621fb0
Fix formatting
DimitarPetrov Aug 26, 2019
743afcf
Merge branch 'master' into platform-health
DimitarPetrov Aug 27, 2019
a2ba4fe
Hide platform details on unauthorized healthcheck
DimitarPetrov Aug 27, 2019
89ab1ff
Rename migrations
DimitarPetrov Aug 29, 2019
b04b750
Fix test last migration
DimitarPetrov Aug 29, 2019
dfe900f
Add default value to migration
DimitarPetrov Aug 29, 2019
3e864fd
Last Active default value
DimitarPetrov Aug 29, 2019
46bc9c5
Initial fix health configuration
DimitarPetrov Aug 30, 2019
aace011
fix failing tests
KirilKabakchiev Aug 31, 2019
c9db83e
Address PR comments
DimitarPetrov Sep 11, 2019
11fb13f
Refactor platform indicator
DimitarPetrov Sep 11, 2019
bf23318
Strip indicator error on unauthorized request
DimitarPetrov Sep 11, 2019
cfd8a90
Fix formatting
DimitarPetrov Sep 11, 2019
d2fd944
Fix squash tag handling
DimitarPetrov Sep 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions api/healthcheck/healthcheck_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
package healthcheck

import (
"context"
h "github.com/InVisionApp/go-health"
Copy link
Contributor

Choose a reason for hiding this comment

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

h alias is too short for such a wide scope

"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"
Expand All @@ -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
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

"overall" suggests the state is already aggregated, better rename it

if len(overallState) == 0 {
return health.New().WithStatus(health.StatusUp)
}
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

why loop a second time over overallState? can't we do it in one loop?

state.Status = convertStatus(state.Status)
Copy link
Contributor

Choose a reason for hiding this comment

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

why remap the status?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we decide to adopt library’s status “ok” and “failed” this change should be propagated everywhere since all the components check UP to determine if it is started correctly.

if strings.Contains(name, health.PlatformIndicatorSuffix) && !web.IsAuthorized(ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid parsing strings as it is error prone, better store this data in a structured way

Copy link
Contributor

Choose a reason for hiding this comment

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

why not remove the details for all the health indicators in case the request is not authorised?

state.Details = nil
}
details[name] = state
}
return health.New().WithStatus(overallStatus).WithDetails(details)
Expand Down
46 changes: 38 additions & 8 deletions api/healthcheck/healthcheck_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package healthcheck

import (
"context"
"fmt"
h "github.com/InVisionApp/go-health"
"github.com/Peripli/service-manager/pkg/health"
Expand Down Expand Up @@ -60,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,
Expand All @@ -81,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))
})
})
Expand All @@ -90,15 +98,15 @@ 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))
})
})

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))
})
})
Expand All @@ -107,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))
}
})
})
})
})

Expand Down
72 changes: 72 additions & 0 deletions api/healthcheck/platform_indicator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* 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"
"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 {
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 + health.PlatformIndicatorSuffix // e.g. cf_platforms, k8s_platforms ...
}

// 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)
if err != nil {
return nil, fmt.Errorf("could not fetch platforms health from storage: %v", err)
}

details := make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

map[string]*health.Health

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Better include the total number of inactive platforms, e.g. "there are %d inactive %s platforms"

}
}
return details, err
}
93 changes: 93 additions & 0 deletions api/healthcheck/platform_indicator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* 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())
})
})
})
Loading