From 02c1c1e577b0c05699bf1f867f12e546269c503c Mon Sep 17 00:00:00 2001 From: Bryan Fulton Date: Wed, 27 Apr 2022 16:46:41 +0000 Subject: [PATCH] bundle/status: Include bundle type in status information OPA has support for Delta Bundles. The status object already contains valuable information such as last activation timestamp but does not specify if the bundle was a canonical snapshot or delta. This change updates the bundle.Status object to include the bundle type string: either "snapshot" or "delta". This can be useful for status endpoints to differentiate between the bundle types. Issue: 4477 Signed-off-by: Bryan Fulton --- plugins/bundle/plugin.go | 2 ++ plugins/bundle/plugin_test.go | 12 +++++++++++ plugins/bundle/status.go | 1 + plugins/discovery/discovery.go | 1 + plugins/discovery/discovery_test.go | 33 ++++++++++++++++++++--------- 5 files changed, 39 insertions(+), 10 deletions(-) diff --git a/plugins/bundle/plugin.go b/plugins/bundle/plugin.go index 851654d86b..e8e4037b0e 100644 --- a/plugins/bundle/plugin.go +++ b/plugins/bundle/plugin.go @@ -375,6 +375,7 @@ func (p *Plugin) loadAndActivateBundlesFromDisk(ctx context.Context) { numActivatedBundles := 0 for name, b := range persistedBundles { p.status[name].Metrics = metrics.New() + p.status[name].Type = b.Type() err := p.activate(ctx, name, b) if err != nil { @@ -474,6 +475,7 @@ func (p *Plugin) process(ctx context.Context, name string, u download.Update) { p.status[name].LastSuccessfulRequest = p.status[name].LastRequest if u.Bundle != nil { + p.status[name].Type = u.Bundle.Type() p.status[name].LastSuccessfulDownload = p.status[name].LastSuccessfulRequest p.status[name].Metrics.Timer(metrics.RegoLoadBundles).Start() diff --git a/plugins/bundle/plugin_test.go b/plugins/bundle/plugin_test.go index be3756bde8..ae36ab5df6 100644 --- a/plugins/bundle/plugin_test.go +++ b/plugins/bundle/plugin_test.go @@ -71,6 +71,12 @@ func TestPluginOneShot(t *testing.T) { ensurePluginState(t, plugin, plugins.StateOK) + if status, ok := plugin.status[bundleName]; !ok { + t.Fatalf("Expected to find status for %s, found nil", bundleName) + } else if status.Type != bundle.SnapshotBundleType { + t.Fatalf("expected snapshot bundle but got %v", status.Type) + } + txn := storage.NewTransactionOrDie(ctx, manager.Store) defer manager.Store.Abort(ctx, txn) @@ -262,6 +268,12 @@ func TestPluginOneShotDeltaBundle(t *testing.T) { ensurePluginState(t, plugin, plugins.StateOK) + if status, ok := plugin.status[bundleName]; !ok { + t.Fatalf("Expected to find status for %s, found nil", bundleName) + } else if status.Type != bundle.DeltaBundleType { + t.Fatalf("expected delta bundle but got %v", status.Type) + } + txn := storage.NewTransactionOrDie(ctx, manager.Store) defer manager.Store.Abort(ctx, txn) diff --git a/plugins/bundle/status.go b/plugins/bundle/status.go index f298ab7e13..af9623cdb5 100644 --- a/plugins/bundle/status.go +++ b/plugins/bundle/status.go @@ -26,6 +26,7 @@ type Status struct { Name string `json:"name"` ActiveRevision string `json:"active_revision,omitempty"` LastSuccessfulActivation time.Time `json:"last_successful_activation,omitempty"` + Type string `json:"type,omitempty"` LastSuccessfulDownload time.Time `json:"last_successful_download,omitempty"` LastSuccessfulRequest time.Time `json:"last_successful_request,omitempty"` LastRequest time.Time `json:"last_request,omitempty"` diff --git a/plugins/discovery/discovery.go b/plugins/discovery/discovery.go index 1a1a1792e6..d56cadd8ba 100644 --- a/plugins/discovery/discovery.go +++ b/plugins/discovery/discovery.go @@ -188,6 +188,7 @@ func (c *Discovery) processUpdate(ctx context.Context, u download.Update) { c.status.LastSuccessfulRequest = c.status.LastRequest if u.Bundle != nil { + c.status.Type = u.Bundle.Type() c.status.LastSuccessfulDownload = c.status.LastSuccessfulRequest if err := c.reconfigure(ctx, u); err != nil { diff --git a/plugins/discovery/discovery_test.go b/plugins/discovery/discovery_test.go index 0729b17a59..6e4326f785 100644 --- a/plugins/discovery/discovery_test.go +++ b/plugins/discovery/discovery_test.go @@ -25,11 +25,12 @@ import ( "github.com/open-policy-agent/opa/logging/test" "github.com/open-policy-agent/opa/ast" + "github.com/open-policy-agent/opa/bundle" bundleApi "github.com/open-policy-agent/opa/bundle" "github.com/open-policy-agent/opa/download" "github.com/open-policy-agent/opa/metrics" "github.com/open-policy-agent/opa/plugins" - "github.com/open-policy-agent/opa/plugins/bundle" + bundlePlugin "github.com/open-policy-agent/opa/plugins/bundle" "github.com/open-policy-agent/opa/plugins/logs" "github.com/open-policy-agent/opa/plugins/status" "github.com/open-policy-agent/opa/server" @@ -90,13 +91,13 @@ func TestEvaluateBundle(t *testing.T) { t.Fatal("Expected a bundle configuration") } - var parsedConfig bundle.Config + var parsedConfig bundlePlugin.Config if err := util.Unmarshal(config.Bundle, &parsedConfig); err != nil { t.Fatal("Unexpected error:", err) } - expectedBundleConfig := bundle.Config{ + expectedBundleConfig := bundlePlugin.Config{ Name: "test/bundle1", Service: "example", } @@ -416,6 +417,12 @@ func TestReconfigure(t *testing.T) { disco.oneShot(ctx, download.Update{Bundle: initialBundle}) + if disco.status == nil { + t.Fatal("Expected to find status, found nil") + } else if disco.status.Type != bundle.SnapshotBundleType { + t.Fatalf("expected snapshot bundle but got %v", disco.status.Type) + } + // Verify labels are unchanged exp := map[string]string{"x": "y", "id": "test-id", "version": version.Version} if !reflect.DeepEqual(manager.Labels(), exp) { @@ -453,6 +460,12 @@ func TestReconfigure(t *testing.T) { disco.oneShot(ctx, download.Update{Bundle: updatedBundle}) + if disco.status == nil { + t.Fatal("Expected to find status, found nil") + } else if disco.status.Type != bundle.SnapshotBundleType { + t.Fatalf("expected snapshot bundle but got %v", disco.status.Type) + } + if !reflect.DeepEqual(testPlugin.counts, map[string]int{"start": 1, "reconfig": 1}) { t.Errorf("Expected one plugin start and one reconfig but got %v", testPlugin) } @@ -619,7 +632,7 @@ func TestReconfigureWithUpdates(t *testing.T) { t.Fatalf("Expected two services but got %v\n", len(disco.manager.Services())) } - bPlugin := bundle.Lookup(disco.manager) + bPlugin := bundlePlugin.Lookup(disco.manager) config := bPlugin.Config() expected := "acmecorp" if config.Bundles["authz"].Service != expected { @@ -648,7 +661,7 @@ func TestReconfigureWithUpdates(t *testing.T) { t.Fatalf("Unexpected error %v", err) } - bPlugin = bundle.Lookup(disco.manager) + bPlugin = bundlePlugin.Lookup(disco.manager) config = bPlugin.Config() expectedSvc := "localhost" if config.Bundles["authz"].Service != expectedSvc { @@ -1163,11 +1176,11 @@ bundle: t.Fatalf("Unexpected error: %s", err) } - p := manager.Plugin(bundle.Name) + p := manager.Plugin(bundlePlugin.Name) if p == nil { t.Fatal("Unable to find bundle plugin on manager") } - bp := p.(*bundle.Plugin) + bp := p.(*bundlePlugin.Plugin) // make sure the older style `bundle` config takes precedence if bp.Config().Name != "bundle-classic" { @@ -1200,11 +1213,11 @@ bundles: t.Fatalf("Unexpected error: %s", err) } - p := manager.Plugin(bundle.Name) + p := manager.Plugin(bundlePlugin.Name) if p == nil { t.Fatal("Unable to find bundle plugin on manager") } - bp := p.(*bundle.Plugin) + bp := p.(*bundlePlugin.Plugin) if len(bp.Config().Bundles) != 1 { t.Fatal("Expected a single bundle configured") @@ -1742,7 +1755,7 @@ func (t *testFixture) loop(ctx context.Context) { close(done) case done := <-t.bundleTrigger: - if p, ok := t.manager.Plugin(bundle.Name).(plugins.Triggerable); ok { + if p, ok := t.manager.Plugin(bundlePlugin.Name).(plugins.Triggerable); ok { _ = p.Trigger(ctx) } close(done)