From caa24f0c4e5c7e5dcd30b1eb44d8de76537d10fc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Benjamin=20N=C3=B8rgaard?= <mail@blacksails.dev>
Date: Fri, 23 Jun 2023 14:01:39 +0200
Subject: [PATCH] plugins/discovery: Fix discovery erasing
 `persistence_directory` config
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before this change, if a discovery bundle didn't contain configuration
for `persistence_directory`, this would be deleted from the manager's
configuration. When enabling persistence of the discovery bundle this
doesn't make much sense, as the first discovery bundle would erase the
persistence settings.

This change ensures that discovery never erases `persistence_directory`.

Signed-off-by: Benjamin Nørgaard <mail@blacksails.dev>
---
 docs/content/management-discovery.md |  7 ++++-
 plugins/discovery/discovery_test.go  | 39 ++++++++++++++++++++++++++--
 plugins/plugins.go                   |  5 ++++
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/docs/content/management-discovery.md b/docs/content/management-discovery.md
index 07da970f36..5889bd35b1 100644
--- a/docs/content/management-discovery.md
+++ b/docs/content/management-discovery.md
@@ -300,5 +300,10 @@ the discovery bundle itself is persisted. The discovered configuration that is g
 policies contained in the discovery bundle will **NOT** be persisted.
 
 {{< info >}}
-By default, the discovery bundle is persisted under the current working directory of the OPA process (e.g., `./.opa/bundles/<discovery.name>/bundle.tar.gz`).
+The discovery bundle is persisted at
+`<persistence_directory>/bundles/<discovery.name>/bundle.tar.gz`. By default
+`persistence_directory` is `.opa` in the working directory of the OPA process.
+If `persistence_directory` is changed through discovery this will not affect
+where the discovery plugin will store the discovery bundles, the boot
+configuration will always be used.
 {{< /info >}}
diff --git a/plugins/discovery/discovery_test.go b/plugins/discovery/discovery_test.go
index 8538a9517f..9cd9b41efb 100644
--- a/plugins/discovery/discovery_test.go
+++ b/plugins/discovery/discovery_test.go
@@ -1053,7 +1053,8 @@ func TestReconfigureWithUpdates(t *testing.T) {
 				"key": "some_private_key",
 				"scope": "write"
 			}
-		}
+		},
+		"persistence_directory": "test"
 	}`), "test-id", inmem.New())
 	if err != nil {
 		t.Fatal(err)
@@ -1063,6 +1064,7 @@ func TestReconfigureWithUpdates(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	originalConfig := disco.config
 
 	initialBundle := makeDataBundle(1, `
 		{
@@ -1079,7 +1081,6 @@ func TestReconfigureWithUpdates(t *testing.T) {
 		t.Fatalf("Unexpected error %v", err)
 	}
 
-	originalConfig := disco.config
 	// update the discovery configuration and check
 	// the boot configuration is not overwritten
 	updatedBundle := makeDataBundle(2, `
@@ -1346,6 +1347,40 @@ func TestReconfigureWithUpdates(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Unexpected error %v", err)
 	}
+
+	// check that not setting persistence_directory doesn't remove boot config
+	updatedBundle = makeDataBundle(13, `
+		{
+			"config": {}
+		}
+	`)
+
+	err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
+	if err != nil {
+		t.Fatalf("Unexpected error %v", err)
+	}
+
+	if manager.Config.PersistenceDirectory == nil {
+		t.Fatal("Erased persistence directory configuration")
+	}
+
+	// update persistence directory
+	updatedBundle = makeDataBundle(14, `
+		{
+			"config": {
+				"persistence_directory": "my_bundles"
+			}
+		}
+	`)
+
+	err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
+	if err != nil {
+		t.Fatalf("Unexpected error %v", err)
+	}
+
+	if manager.Config.PersistenceDirectory == nil || *manager.Config.PersistenceDirectory != "my_bundles" {
+		t.Fatal("Did not update persistence directory")
+	}
 }
 
 func TestProcessBundleWithSigning(t *testing.T) {
diff --git a/plugins/plugins.go b/plugins/plugins.go
index 8535bfa0f6..60a8cb16c7 100644
--- a/plugins/plugins.go
+++ b/plugins/plugins.go
@@ -691,6 +691,11 @@ func (m *Manager) Reconfigure(config *config.Config) error {
 		}
 	}
 
+	// don't erase persistence directory
+	if config.PersistenceDirectory == nil {
+		config.PersistenceDirectory = m.Config.PersistenceDirectory
+	}
+
 	m.Config = config
 	m.interQueryBuiltinCacheConfig = interQueryBuiltinCacheConfig
 	for name, client := range services {