plugins/bundle: Support for saving and reading bundles from disk #2702

ashutosh-narkar · 2020-09-16T06:18:07Z

This commit adds support to persist and load bundles from disk.
A new field is introduced in OPA's bundle configuration that can
be optionally set to specify the location on disk to write and read
bundles. This feature will allow OPA to serve policy decisions in scenarios
such as OPA being unable to communicate with the bundle server.

Fixes #2097

Signed-off-by: Ashutosh Narkar anarkar4387@gmail.com

patrick-east · 2020-09-21T18:54:17Z

docs/content/management.md

@@ -97,6 +97,7 @@ bundles:
  authz:
    service: acmecorp
    resource: somedir/bundle.tar.gz
+    persistence_file_location: /tmp/example


Just kind of thinking out-loud here but would it make sense to have a default cache path and then more like a boolean opt-in for keeping local copies for each bundle?

That way we can default to some safe-ish internal thing like /var/lib/opa/ or ~/.opa/ or whatever and someone can turn it on or off by configuring a bundle like:

authz: service: acmecorp resource: somedir/bundle.tar.gz cache: persist: true

(or whatever we want to call it)

Without having to worry about where it goes, more than likely they don't care. We can still allow for customizing it with like:

authz: service: acmecorp resource: somedir/bundle.tar.gz cache: persist: true persistence_file_location: /tmp/example

I like the idea of adding a default persist location. Regarding the persist field, if a user sets persist: false or does not specify it but sets persistence_file_location that would be an error or do we imply persist: true ?

I'd say yes, but to start I don't think we'll need the persistence_file_location.

Removed persistence_file_location and added persist field.

patrick-east · 2020-09-21T19:07:48Z

plugins/bundle/plugin.go

+	bundleDir := filepath.Join(path, name)
+	var backupBundleDir string
+
+	// if a bundle already exists, create a backup and then write the new bundle


We might want to consider adjusting the order for these. Making the backup and restoring on error is good, but if OPA was stopped/crashed/etc mid-way through writing the new one I think we'd end up in trouble (the deferred calls to restore the backup would never get called)

A common strategy for this kind of thing is to write into a temporary file, and only after a successful write with the whole thing we swap it in with a like mv sort of operation on the filesystem so it is atomic. The goal being that you don't allow for a period of time where the file we've persisted is in a partially written state. From a high level I'd imagine the steps to be more like:

Write to filepath.Join(bundleDir, ".bundle.tar.gz.tmp"). or whatever we want to call it, the gist being that its in the same directory and similar name but is a hidden file and somewhat obvious that its a partial or temp file.

If it fails, delete that new/temporary file. No restore needed, the original is still there.

If it succeeds, use os.Rename() to change the tmp file to the "real" path. This will overwrite the old version (same as like mv -f)

Updated the backup/restore logic.

patrick-east · 2020-09-24T18:23:18Z

plugins/bundle/plugin.go

+	// if a bundle already exists, write the new bundle to a temporary file and if successful, replace
+	// the old bundle with the new one
+	if _, err := os.Stat(filepath.Join(bundleDir, "bundle.tar.gz")); err == nil {
+		err := saveCurrentBundleToDisk(bundleDir, ".bundle.tar.gz.tmp", b)


We should always write to the temp file and do the rename to avoid a case where OPA has written part of it, stopped, restarted, and tries to read the partial file.

patrick-east

Looking good, just a few minor comments 👍

One thing that occurs to me is that the plugin status looks like it would only be set to "ready" after it downloads the bundle versus loading the one from disk. Was that something that was discussed previously?

patrick-east · 2020-09-29T19:12:27Z

plugins/bundle/plugin.go

+			if p.status[name].Metrics == nil {
+				p.status[name].Metrics = metrics.New()
+			}


I think we would want to overwrite the metrics here with a new one, even if (for whatever reason) its non-nil. I think the steps in process() try and merge it with the stuff from the download update, but it is still sort of 1:1 for a metric object and an attempt to load a bundle.

patrick-east · 2020-09-29T19:20:52Z

plugins/bundle/plugin.go

+			p.logDebug(name, "Persisting bundle to disk in progress.")
+
+			path, err := getDefaultBundlePersistPath()


nit: I might have missed it below, but we might want to include the path in at least one of the debug logs for saving the bundle to help with troubleshooting.

Updated a debug log to include bundle persistence path on disk.

patrick-east · 2020-09-29T19:24:01Z

plugins/bundle/plugin.go

+	if _, err := os.Stat(filepath.Join(bundleDir, "bundle.tar.gz")); err == nil {
+		err := saveCurrentBundleToDisk(bundleDir, ".bundle.tar.gz.tmp", b)
+		if err != nil {
+			return os.Remove(filepath.Join(bundleDir, ".bundle.tar.gz.tmp"))
+		}
+	} else {
+		err := saveCurrentBundleToDisk(bundleDir, ".bundle.tar.gz.tmp", b)
+		if err != nil {
+			os.Remove(filepath.Join(bundleDir, ".bundle.tar.gz.tmp"))
+			return err
+		}
+	}


I think we can remove the if/else here, both do the same thing, right?

There's one difference: If you try to save a bundle to disk when there isn't one already, we return the error that occurred while saving it. OTOH, if there is an exiting bundle and the new one could not be saved, we return the error from removing the temp file.

Ahhh ok, I see it now. Looking at where the error is used, would we want the bundle status to go into an error state if the temp file cleanup failed? Seems like maybe a log message or something to warn about it, but otherwise the bundle that was activated is still ok, right?

I guess thinking about it more we should be careful to at least log any of the errors (from save, cleanup, etc), especially if they don't affect the bundle status. Future us reading some bug report with a log will probably be thankful.

Added a warning log to capture errors that don't affect bundle status.

patrick-east · 2020-09-29T19:37:33Z

plugins/bundle/plugin_test.go

+	b := getTestBundle(t)
+
+	srcDir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+
+	defer os.RemoveAll(srcDir)
+
+	err = saveBundleToDisk(srcDir, "foo", &b)
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}


I might be missing something, but what is this first bundle used for in the test? Would it make sense to split this up into separate test cases? (seems like there is maybe more than one thing being tested)

Split the test into 2 cases.

patrick-east · 2020-09-29T19:44:13Z

plugins/bundle/plugin_test.go

+	ctx := context.Background()
+	manager := getTestManager()
+
+	dir, err := getDefaultBundlePersistPath()


We might want to provide some mechanism to override the default path, even if only for tests, to avoid having the unit tests clutter the working dir (the deferred cleanup only works if the function finishes, crashing in the middle or getting killed by the user can leave stuff behind). Would be ideal if the unit tests were always using temp directories.

Added a mechanism for tests to override the default bundle persistence path.

patrick-east · 2020-09-30T19:38:02Z

plugins/bundle/plugin.go

+	if _, err := os.Stat(filepath.Join(bundleDir, "bundle.tar.gz")); err == nil {
+		err := saveCurrentBundleToDisk(bundleDir, ".bundle.tar.gz.tmp", b)
+		if err != nil {
+			p.logWarn(name, "Failed to save new bundle to disk: %v", err)
+			err = os.Remove(filepath.Join(bundleDir, ".bundle.tar.gz.tmp"))
+			if err != nil {
+				p.logWarn(name, "Failed to remove temp file ('.bundle.tar.gz.tmp'): %v", err)
+			}
+			return nil
+		}
+	} else {
+		err := saveCurrentBundleToDisk(bundleDir, ".bundle.tar.gz.tmp", b)
+		if err != nil {
+			rErr := os.Remove(filepath.Join(bundleDir, ".bundle.tar.gz.tmp"))
+			if rErr != nil {
+				p.logWarn(name, "Failed to remove temp file ('.bundle.tar.gz.tmp'): %v", rErr)
+			}
+			return err
+		}
+	}


nit: Can refactor this to reduce duplicate code like:

tmpFile := filepath.Join(bundleDir, ".bundle.tar.gz.tmp") bundleFile := filepath.Join(bundleDir, "bundle.tar.gz") saveErr := saveCurrentBundleToDisk(bundleDir, ".bundle.tar.gz.tmp", b) if saveErr != nil { p.logWarn(name, "Failed to save new bundle to disk: %v", saveErr) if err := os.Remove(tmpFile); err != nil { p.logWarn(name, "Failed to remove temp file ('%s'): %v", tmpFile, err) } if _, err := os.Stat(bundleFile); err == nil { p.logWarn(name, "Older version of activated bundle persisted, ignoring error") return nil } return saveErr } return os.Rename(tmpFile, bundleFile)

(note there is a log message when we ignore the error too)

Refactored the code as per your suggestion.

patrick-east

Changes LGTM. Theres one refactor suggestion but nothing blocking, feel free to merge when ready.

This commit adds support to persist and load bundles from disk. A new field is introduced in OPA's bundle configuration that can be optionally set to enable OPA to write and read bundles from disk. This feature will allow OPA to serve policy decisions in scenarios such as OPA being unable to communicate with the bundle server. Fixes open-policy-agent#2097 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

ashutosh-narkar force-pushed the bundle-disk-support branch from a54b87d to a33a0b2 Compare September 16, 2020 06:38

patrick-east reviewed Sep 21, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-disk-support branch from a33a0b2 to fb0181a Compare September 24, 2020 00:10

patrick-east reviewed Sep 24, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-disk-support branch from fb0181a to 5328679 Compare September 28, 2020 23:35

patrick-east reviewed Sep 29, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-disk-support branch 3 times, most recently from 7f8e246 to 1b87df3 Compare September 30, 2020 18:12

patrick-east reviewed Sep 30, 2020

View reviewed changes

patrick-east previously approved these changes Sep 30, 2020

View reviewed changes

ashutosh-narkar dismissed patrick-east’s stale review via b4a876f September 30, 2020 20:39

ashutosh-narkar force-pushed the bundle-disk-support branch from 1b87df3 to b4a876f Compare September 30, 2020 20:39

patrick-east previously approved these changes Sep 30, 2020

View reviewed changes

ashutosh-narkar dismissed patrick-east’s stale review via f49fa71 September 30, 2020 20:47

ashutosh-narkar force-pushed the bundle-disk-support branch 2 times, most recently from f49fa71 to 01684ab Compare September 30, 2020 21:03

ashutosh-narkar force-pushed the bundle-disk-support branch from 01684ab to a933011 Compare September 30, 2020 22:00

ashutosh-narkar merged commit 5a79a45 into open-policy-agent:master Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins/bundle: Support for saving and reading bundles from disk #2702

plugins/bundle: Support for saving and reading bundles from disk #2702

ashutosh-narkar commented Sep 16, 2020

patrick-east Sep 21, 2020

ashutosh-narkar Sep 23, 2020

tsandall Sep 24, 2020

ashutosh-narkar Sep 28, 2020

patrick-east Sep 21, 2020

ashutosh-narkar Sep 24, 2020

patrick-east Sep 24, 2020

ashutosh-narkar Sep 28, 2020

patrick-east left a comment

patrick-east Sep 29, 2020

ashutosh-narkar Sep 29, 2020

patrick-east Sep 29, 2020

ashutosh-narkar Sep 29, 2020

patrick-east Sep 29, 2020

ashutosh-narkar Sep 29, 2020

patrick-east Sep 29, 2020

ashutosh-narkar Sep 30, 2020

patrick-east Sep 29, 2020

ashutosh-narkar Sep 29, 2020

patrick-east Sep 29, 2020

ashutosh-narkar Sep 29, 2020

patrick-east Sep 30, 2020 •

edited

Loading

ashutosh-narkar Sep 30, 2020

patrick-east left a comment

		p.logDebug(name, "Persisting bundle to disk in progress.")

		path, err := getDefaultBundlePersistPath()

plugins/bundle: Support for saving and reading bundles from disk #2702

plugins/bundle: Support for saving and reading bundles from disk #2702

Conversation

ashutosh-narkar commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-east left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-east Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-east left a comment

Choose a reason for hiding this comment

patrick-east Sep 30, 2020 •

edited

Loading