Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

server: Fix panic in /health?bundle=true #1704

Merged
merged 1 commit into from
Aug 30, 2019
Merged

server: Fix panic in /health?bundle=true #1704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ type Server struct {
runtime *ast.Term
httpListeners []httpListener
bundleStatuses map[string]*bundlePlugin.Status
bundleStatusMtx *sync.RWMutex
bundleStatusMtx sync.RWMutex
metrics Metrics
}

Expand Down Expand Up @@ -176,7 +176,6 @@ func (s *Server) Init(ctx context.Context) (*Server, error) {

bp := bundlePlugin.Lookup(s.manager)
if bp != nil {
s.bundleStatusMtx = new(sync.RWMutex)

// initialize statuses to empty defaults for server /health check
s.bundleStatuses = map[string]*bundlePlugin.Status{}
Expand Down
38 changes: 25 additions & 13 deletions server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"reflect"
"sort"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -81,7 +80,6 @@ func TestUnversionedGetHealthCheckBundleActivationSingle(t *testing.T) {
// Initialize the server as if a bundle plugin was
// configured on the manager.
f.server.manager.Register(pluginBundle.Name, &pluginBundle.Plugin{})
f.server.bundleStatusMtx = new(sync.RWMutex)
f.server.bundleStatuses = map[string]*pluginBundle.Status{
bundleName: &pluginBundle.Status{Name: bundleName},
}
Expand All @@ -106,6 +104,31 @@ func TestUnversionedGetHealthCheckBundleActivationSingle(t *testing.T) {
}
}

func TestUnversionedGetHealthCheckBundleActivationSingleLegacy(t *testing.T) {

// Initialize the server as if there is no bundle plugin

f := newFixture(t)

ctx := context.Background()

err := storage.Txn(ctx, f.server.store, storage.WriteParams, func(txn storage.Transaction) error {
Copy link
Member

Choose a reason for hiding this comment

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

What does the health check report before this transaction? Do we have a test that covers that? I'd expect it to be unhealthy if ?bundle is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it actually returns healthy because there is no known bundle. This test is somewhat synthetic to trigger the panic as it starts the server up without a bundle then fakes adding ones manifest in the store.

Bundles loaded via the data paths are processed when the runtime starts up and before the server ever starts serving so it isn't possible to call the health check. In practice the end result is that calls to the health check fail because the server isn't running yet, and opa would exit with an error initializing the runtime.

return bundle.LegacyWriteManifestToStore(ctx, f.server.store, txn, bundle.Manifest{
Revision: "a",
})
})

if err != nil {
t.Fatalf("Unexpected error: %s", err)
}

// The heath check should now respond as healthy
req := newReqUnversioned(http.MethodGet, "/health?bundle=true", "")
if err := f.executeRequest(req, 200, `{}`); err != nil {
t.Fatal(err)
}
}

func TestUnversionedGetHealthCheckBundleActivationMulti(t *testing.T) {

f := newFixture(t)
Expand All @@ -118,7 +141,6 @@ func TestUnversionedGetHealthCheckBundleActivationMulti(t *testing.T) {
"b3": {Service: "s3", Resource: "bundle.tar.gz"},
}}, f.server.manager)
f.server.manager.Register(pluginBundle.Name, bp)
f.server.bundleStatusMtx = new(sync.RWMutex)
f.server.bundleStatuses = map[string]*pluginBundle.Status{
"b1": {Name: "b1"},
"b2": {Name: "b2"},
Expand Down Expand Up @@ -187,10 +209,6 @@ func TestInitWithBundlePlugin(t *testing.T) {
t.Error("server.hasBundle should be true")
}

if server.bundleStatusMtx == nil {
t.Error("server.bundleStatusMtx should be initialized")
}

isActivated := server.bundlesActivated()
if isActivated {
t.Error("bundle should not be initialized to activated status")
Expand Down Expand Up @@ -225,10 +243,6 @@ func TestInitWithBundlePluginMultiBundle(t *testing.T) {
t.Error("server.hasBundle should be true")
}

if server.bundleStatusMtx == nil {
t.Error("server.bundleStatusMtx should be initialized")
}

isActivated := server.bundlesActivated()
if isActivated {
t.Error("bundle should not be initialized to activated")
Expand Down Expand Up @@ -1662,7 +1676,6 @@ func TestDataProvenanceSingleBundle(t *testing.T) {
// Initialize as if a bundle plugin is running
bp := pluginBundle.New(&pluginBundle.Config{Name: "b1"}, f.server.manager)
f.server.manager.Register(pluginBundle.Name, bp)
f.server.bundleStatusMtx = new(sync.RWMutex)
f.server.bundleStatuses = map[string]*pluginBundle.Status{
"b1": {Name: "b1"},
}
Expand Down Expand Up @@ -1774,7 +1787,6 @@ func TestDataProvenanceMultiBundle(t *testing.T) {
}}, f.server.manager)
f.server.manager.Register(pluginBundle.Name, bp)

f.server.bundleStatusMtx = new(sync.RWMutex)
f.server.bundleStatuses = map[string]*pluginBundle.Status{
"b1": {Name: "b1"},
"b2": {Name: "b2"},
Expand Down