Misleading storage_not_found_error when delta bundle uses persistence

[jnethery]

Version Details

• Docker v4.17.0
• Image openpolicyagent/opa:latest-debug

Description

If persist: true is set in the config for a delta bundle, OPA will throw the following error:

storage_not_found_error: <path>: document does not exist.

This seems to be a slightly misleading or perhaps incomplete error, as it doesn't provide much information or feedback to the user.

Steps To Reproduce

1. Add persist: true as a field to the bundle config for a delta bundle in config.yaml.
2. Run OPA, and allow it to attempt to load the bundle.
3. See the storage_not_found_error in the logs.

Expected behavior

I would expect a more specific error, maybe one pointing out that not only is persist: true not available for delta bundles, but is also invalid.

[ashutosh-narkar]

In the downloader we know if the bundle will be persisted to disk. If we can pass that to the bundle reader and add a check during the read operation we should be able to generate a better error message.

[yogisinha]

Can I handle this bug. I am looking to understand OPA and contribute to it

[ashutosh-narkar]

Sure go for it @yogisinha!

[yogisinha]

Ok. thanks. it might take some time as I am new to code.

[anderseknert]

Take your time, @yogisinha! There's a #contributors channel on the OPA Slack if you'd like to get some help while you're working on this. But asking here is fine too.

[yogisinha]

I am able to replicate the error. I did the following.

1. Created a delta bundle
2. Exposed that bundle over http
3. Ran the command : opa_windows_amd64.exe run -s http://localhost:8080/bundle.tar.gz
   Got the following error :

{"addrs":[":8181"],"diagnostic-addrs":[],"level":"info","msg":"Initializing server. OPA is running on a public (0.0.0.0) network interface. Unless you intend to expose OPA outside of the host, binding to the localhost interface (--addr localhost:8181) is recommended. See https://www.openpolicyagent.org/docs/latest/security/#interface-binding","time":"2023-06-08T18:16:13-04:00"} {"level":"info","msg":"Starting bundle loader.","name":"cli1","plugin":"bundle","time":"2023-06-08T18:16:13-04:00"} {"level":"error","msg":"Bundle activation failed: storage_not_found_error: /c: document does not exist","name":"cli1","plugin":"bundle","time":"2023-06-08T18:16:13-04:00"} {"level":"info","msg":"Shutting down...","time":"2023-06-08T18:16:15-04:00"} {"level":"info","msg":"Server shutdown.","time":"2023-06-08T18:16:15-04:00"} {"level":"info","msg":"Stopping bundle loader.","name":"cli1","plugin":"bundle","time":"2023-06-08T18:16:15-04:00"}

Looking into the code now.

[yogisinha]

Hi @ashutosh-narkar and @anderseknert

I think my delta bundle is not correct.
Another take at replicating the issue.

I have 2 files input.json and patch.json in 2 different folders.
folder "complete" has only input.json.
input.json looks like this:
{
"baz": "qux",
"foo": "bar"
}

folder "patch" has only patch.json.
patch.json looks like this:
{
"data": [
{ "op": "replace", "path": "/baz", "value": "boo" }
]
}

From same location I create the complete and patch bundle and a python process is exposing the bundle.tar.gz on localhost:8080

1. I created the complete bundle. Terminal 1 : C:\Users...\Downloads>opa_windows_amd64.exe build -b complete/
2. Started the opa server from another terminal. Note that I am passing persist=false.
   Terminal 2 : C:\Users...\Downloads>opa_windows_amd64.exe run -s --set "services.cli1.url=http://localhost:8080" --set "bundles.cli1.service=cli1" --set "bundles.cli1.resource=/bundle.tar.gz" --set "bundles.cli1.persist=false"

It loaded the bundle successfully. terminal output :

{"addrs":[":8181"],"diagnostic-addrs":[],"level":"info","msg":"Initializing server. OPA is running on a public (0.0.0.0) network interface. Unless you intend to expose OPA outside of the host, binding to the localhost interface (--addr localhost:8181) is recommended. See https://www.openpolicyagent.org/docs/latest/security/#interface-binding","time":"2023-06-09T08:20:34-04:00"}
{"level":"info","msg":"Starting bundle loader.","name":"cli1","plugin":"bundle","time":"2023-06-09T08:20:34-04:00"}
{"level":"info","msg":"Bundle loaded and activated successfully.","name":"cli1","plugin":"bundle","time":"2023-06-09T08:20:34-04:00"}

3. I created the patch bundle. Terminal 1 : C:\Users...\Downloads>opa_windows_amd64.exe build -b patch/

After some time, the Terminal 2 output looks like this:

{"addrs":[":8181"],"diagnostic-addrs":[],"level":"info","msg":"Initializing server. OPA is running on a public (0.0.0.0) network interface. Unless you intend to expose OPA outside of the host, binding to the localhost interface (--addr localhost:8181) is recommended. See https://www.openpolicyagent.org/docs/latest/security/#interface-binding","time":"2023-06-09T08:20:34-04:00"}
{"level":"info","msg":"Starting bundle loader.","name":"cli1","plugin":"bundle","time":"2023-06-09T08:20:34-04:00"}
{"level":"info","msg":"Bundle loaded and activated successfully.","name":"cli1","plugin":"bundle","time":"2023-06-09T08:20:34-04:00"}
{"level":"error","msg":"Bundle activation failed: storage_not_found_error: /baz: document does not exist","name":"cli1","plugin":"bundle","time":"2023-06-09T08:21:50-04:00"}

So even persist=false is passed, its throwing the same "storage not found error". Am I doing something wrong here ?
Is my patch.json file correct ?
Please help.

[ashutosh-narkar]

@yogisinha try a patch document with a upsert operation. See this for more info.

As far as implementation goes, one option is to add a check like below here before we attempt to activate the bundle. Alternatively we can also add a check in the bundle reader as mentioned here.

if u.Bundle.Type() == bundle.DeltaBundleType && p.persistBundle(name) {
        errMsg := "Persisting delta bundle to disk is not supported"
        p.log(name).Error(errMsg)
        p.status[name].SetError(fmt.Errorf(errMsg))
	if !p.stopped {
		etag := p.etags[name]
		p.downloaders[name].SetCache(etag)
	}
	return
}

[yogisinha]

Ok. thanks @ashutosh-narkar I will try upsert. So there is no issue in steps what I explained before ?

Yes, I was thinking to put the code in bundle reader as there is already some checks for delta bundle there in Read() method. I can add the check there.

[ashutosh-narkar]

If you simply load a delta bundle and set persist:true in the config, you should be able to repro this.

[yogisinha]

@ashutosh-narkar thanks for your help. I appreciate it.

I am passing persist=true in command line through --set options as explained in OPA doc and it says that
opa run -s http://someserver/bundle.tar.gz sets the persist = true by default.

Here is my finding about delta bundle.
I tried upsert and it always succeeds whether persist is true or false.
replace and remove always fails with "storage not found error" whether persist is true or false.
So I see no behavior difference when its true or false.

do you see any difference in behavior when you use different values for persist property for delta bundle?

I put your code where you suggested (before call to activate() inside process method) and ran the code with persist=false and loading of delta bundle still failed. The error is coming in downstream calls which starts in from acitvate() method

this is the call chain I could find
acitvate -> bundle.Activate(opts) ->activateBundles(opts) -> activateDeltaBundles(opts, deltaBundles) -> applyPatches(opts.Ctx, opts.Store, opts.Txn, b.Patch.Data) -> store.Write(ctx, txn, op, path, pat.Value) (inmem.go) -> underlying.Write(op, path, *val) (txn.go) -> newUpdate(txn.db.data, op, path, 0, value) -> newUpdateObject(data, op, path, idx, value)

In newUpdateObject, its returning the error. (return nil, errors.NewNotFoundError(path))
I printed the data in newUpdateObject and its like following. There is no entry with data from snapshot bundle which I previously loaded successfully.
map[system:map[bundles:map[cli1:map[etag: manifest:map[revision: roots:[]]]]]]

Can you help in creating a valid delta bundle ? and how you are running it ?
If I were to use config file, where should I put it ? in the same folder where delta bundle is ?

[yogisinha]

Hi @ashutosh-narkar, @anderseknert , I found my issue. data.json file inside the snapshot bundle I created was empty. So OPA was starting with no data, so it makes sense to throw the error in all the case which I was getting before. I was using the file name input.json when I was creating the snapshot bundle. I had to use file name as data.json.

But now with persist=true, I can successfully load the delta bundle. there is no issue.
If the path given in your delta bundle is not found in OPA memory then it throws that error and I think that is right behavior because it could not find the path to make the update. I don't think its related to persist property.

For example , I did this 3 steps.

1. Loaded a valid snapshot bundle -- produced the successful msg
2. loaded a delta bundle for replace operation where I put the invalid path -- produced the error because I passed the invalid path
3. loaded a delta bundle for same replace operation where I put the valid path -- produced the successful msg
   You can see the output:

{"level":"info","msg":"Bundle loaded and activated successfully.","name":"cli1","plugin":"bundle","time":"2023-06-10T09:44:14-04:00"}
{"level":"error","msg":"Bundle activation failed: storage_not_found_error: /test: document does not exist","name":"cli1","plugin":"bundle","time":"2023-06-10T09:45:57-04:00"}
{"level":"info","msg":"Bundle loaded and activated successfully.","name":"cli1","plugin":"bundle","time":"2023-06-10T09:47:03-04:00"}

I tried with both true and false values for persist property and is able to load the delta bundle.

The only logic I see related to persist field is in download method of download.go where it populates the raw field of downloaderResponse.
And that raw field is used in process method of plugin.go. In saveBundleToDisk method it uses the raw field and that statement is already guarded by ( if u.Bundle.Type() == bundle.SnapshotBundleType )

May be we can change the error msg a little bit more clear like : path_not_found_error: "path in delta bundle" does not exist in snapshot bundle
Your thoughts ?

[yogisinha]

HI @ashutosh-narkar @anderseknert Can you look at my findings and let me know if its still a bug or we just need more clear error msg?

[jnethery]

Hey @yogisinha

But now with persist=true, I can successfully load the delta bundle. there is no issue.

The problem is that the path_not_found_error is thrown when persist is set to true in the config regardless of whether or not the patch path is actually valid. I'm not sure how it's working according to your quote here

[yogisinha]

@jnethery So you are saying if you do persist=false, you are not getting any error even if the path in delta bundle is invalid ? I think it would throw the error if the path is invalid, even if persist=false.

What I am saying is that even if you set persist=false, if the path is invalid in delta bundle, it would throw the same error. So its not depending on persist value really. Can you please try loading a delta bundle with persist=false and pass the invalid path?

[jnethery]

@yogisinha If my path was invalid, then the error message would technically be correct, just for the wrong reason :)

It does depend on the persist value, because the issue is that delta bundles do not persist, so the setting is invalid. The error should say something to this effect, rather than saying the path is unfound, which is not true.

[yogisinha]

@jnethery So basically we don't want to even load the delta bundle and try to update it if the persist value is true ? So basically its a validation logic we are trying to put, right ?

[jnethery]

@yogisinha It already does not load; the solution here is simply to make the error message more clear.

I suggest following the advice of @ashutosh-narkar

#5959 (comment)

[yogisinha]

@jnethery So finally we are doing this

1. if we are loading delta bundle and persist is true, we should throw clearer error msg like " "persist" property is passed true in config. Persisting delta bundle to disk is not supported"
2. If we are loading delta bundle and persist is false and path is valid / invalid - No change in behavior.

[jnethery]

@yogisinha Sounds correct to me

[yogisinha]

@ashutosh-narkar @anderseknert I have created the PR for this issue. Please review and let me know any feedback.
#6006