Verify --data differs from documented and opa behaviour

[mykter]

The docs say that data in a folder "exceptions/a.json" will be exposed as "data.exceptions".

The example, which is the tested behaviour, shows that it is actually available directly under "data".

# check that the original test works
~/tmp/conftest/examples/data(master)$ conftest verify --data exclusions/services.yaml 

1 test, 1 passed, 0 warnings, 0 failures, 0 exceptions

# move the data file, which should change its import path
~/tmp/conftest/examples/data(master)$ mv exclusions/services.yaml a.yaml
# yet the test still works
~/tmp/conftest/examples/data(master)$ conftest verify --data a.yaml

1 test, 1 passed, 0 warnings, 0 failures, 0 exceptions

I think the test logic is inverted - it is checking that running conftest test gives an error, but it should be checking that it doesn't:

~/tmp/conftest(master)$ conftest test -p examples/data/policy -d examples/data/exclusions examples/data/service.yaml
FAIL - examples/data/service.yaml - main - Cannot expose one of the following ports on a LoadBalancer [22]

1 test, 0 passed, 0 warnings, 1 failure, 0 exceptions

Am I right in thinking that opa test --bundle <path> should be equivalent to conftest verify --policy <path> --data <path> ? My tests are working using this opa command at any rate!

[jpreese]

@mykter I think this has to do with 22 not being in quotes. Once the quotes are added, the result fails:

test_services_not_denied {
  deny["Cannot expose one of the following ports on a LoadBalancer [22]"] with input as {"kind": "Service", "metadata": { "name": "sample" }, "spec": { "type": "LoadBalancer", "ports": [{ "port":  "22" }]}}
}

The test does indeed need to be updated

[mykter]

(just in case it got lost in the discussion of the test: the important part of this report is the incorrect behaviour of verify --data)

[jpreese]

I added #536 to hopefully clarify the behavior. Looks like the docs on the website showcase the current behavior: https://www.conftest.dev/options/#-data

At the moment the folder itself doesn't come into play, it's more on the structure of the data. If this differs from OPA behavior it would probably be worth looking into. I personally haven't used the --bundle flag from the OPA binary that much.

Do you have an example OPA and Conftest command that differs so I can compare the behavior? Everytime I try and add multiple bundles I'm seeing a root conflict error.

[mykter]

Here's a fairly minimal example of the difference in behaviour of opa's bundles vs conftest's --data.

$ cat a/dir/data.json
{"v":1}

$ cat a/p.rego

package main

import data.dir
deny[msg] {
        not dir.v == 1
        msg := "couldn't load data via dir"
}

import data.v
deny[msg] {
        not v == 1
        msg := "couldn't load data via v"
}

$ conftest test --policy=a --data=a <(echo 1)
FAIL - /dev/fd/63 - main - couldn't load data via dir

2 tests, 1 passed, 0 warnings, 1 failure, 0 exceptions

$  opa -b a eval data.main.deny
{
  "result": [
    {
      "expressions": [
        {
          "value": [
            "couldn't load data via v"
          ],
          "text": "data.main.deny",
          "location": {
            "row": 1,
            "col": 1
          }
        }
      ]
    }
  ]
}

[jpreese]

To represent what you're trying to do in Conftest, it would be: conftest test a/dir/data.json -p a

Typically you'll pass in the configuration that you're trying to test (data.json) and tell conftest the policy directory. With opa eval you're telling OPA to evaluate data.main.deny which conftest does by default.

[mykter]

Ah sorry I wasn't clear, the data.json is meant to be an example of static data that the policy refers to, as opposed to the input document. I just didn't refer to "input" or have an input doc to conftest for simplicity. The intent was to show that in conftest the first rule matches and the second doesn't, with opa it's the other way round.

[jpreese]

Yeah I see, the --data for Conftest will take the data files passed in and make them accessible based on their keys. So in this case, {"v":1} would be accessible from data.v not data.dir.

If the desire is to have it indexed based on the directory, I'd need to think through it some more as I haven't seen many questions or issues come up about conftest's data parameter.

[jpreese]

@anderseknert are you familiar with OPA and using bundles? There does appear to be a difference with how Conftest and OPA handle external data, but I'm not familiar with them enough to know if the difference is warranted or if making these behaviors consistent is something we should do.

[anderseknert]

Yeah, OPA will load any JSON/YAML data (obviously not policies as those have a package defined) on the (relative) path under the which it was read. The only difference I know of in behavior as far as --bundle is concerned here is that only files named data.json/yaml will be considered in that case.

Whether it makes sense to preserve this behavior for conftest verify you're probably better equipped to tell, but I helped (or well, half-failed to help) another user with this exact issue on Slack the other day, so either the behavior should change or the docs should.

[jpreese]

Thanks! The documentation has been updated to reflect reality. I'll play around with --data and --bundle and see if it makes sense to change. The fear being that I imagine it would be a breaking change, and want to make sure we're actually solving a problem and not a symptom of poor docs 😅

[anderseknert]

Yeah, I guess a sensible middle ground could be to keep current behavoir while introducing the --bundle flag for conftest too, and have that act exactly as it does for OPA? There are already plans/WIP to push more for bundle based policy and data packaging in the OPA docs and so on, so it would make it easier for user to jump seamlessly between the two tools.

[jpreese]

Yeah, I guess a sensible middle ground could be to keep current behavoir while introducing the --bundle flag for conftest too, and have that act exactly as it does for OPA? There are already plans/WIP to push more for bundle based policy and data packaging in the OPA docs and so on, so it would make it easier for user to jump seamlessly between the two tools.

That's honestly not a bad idea either. Love it.

Then come v1.0.0 we could consider removing --data if --bundle does everything we want it to.

[jpreese]

@mykter @anderseknert I just noticed that OPA also has a --data flag. Using the above example, if you use OPA's --data flag the example is the same as Conftest. i.e.

echo {} | conftest test --policy=a --data=a -

opa -d a/dir eval data.main.deny # -b also works here as well.. whats the difference between bundle and data?

So that behavior seems consistent. The dir is slightly different, because Conftest traverses all children.

That said, if the above finding is correct, this issue then is more about Conftest supporting --bundle as well as --data

[xarses]

I've just ran into this and am perplexed. I've spent a bunch of time wrapping my head around how opa loads data files based on the relative path passed and where the file was found walking towards it. Now that my head is wrapped around it, I came to find that conftest just ignores that and loads them directly in data. The main problem being is that in order to validate/develop in opa, and then move to conftest for usage, I have to rewrite my imports to account for each tool loading differently.

I have a policy directory that looks like

├── policy
│   ├── escalation
│   │   ├── infra
│   │   │   ├── escalate_changes.rego
│   │   │   ├── escalate_changes_mock.json
│   │   │   └── test_escalate_changes.rego
│   │   └── security
│   ├── standard
│   │   ├── aws_iam.rego
│   │   ├── aws_iam_test.rego
│   │   ├── aws_security_groups.rego
│   │   └── aws_security_groups_test.rego
│   └── tools.rego

opa test policy gives escalate_changes_mock.json the path of data.escalation.infra where conftest verify -d policy assigns the data directly in data

This means the tests that use the data mock have to be changed depending on if the person is working currently in opa or conftest

#import data.escalation.infra.escalate_changes_mock as mock
# conftest
import data.escalate_changes_mock as mock

is now at the top of my test file to accommodate for this. If I remember I can also force opa to shorten the path but then the command becomes quite hard to remember opa test policy/tools.rego policy/escalation/infra and doesn't run the all of the tests unless each folder is explicitly passed

[joaosilva15]

Any advice on this? We would prefer this path-based data structure instead of playing around with our data and merging it into a single file.

[anderseknert]

Reusing the bundle loader from OPA should not be too complicated, I think. If you want to give it a shot :)