Upgrade conftest version to 0.23

[msarvar]

Upgrading conftest to 0.23.
It has some UI improvements and also fixes conftest pull that seems to be broken in 0.21. With conftest pull working we can implement remote policies.

[msarvar]

@lkysow can you update the test image to use conftest 0.23? I update the dockerfile

[lkysow]

runatlantis/testing-env                                          20296f9638a4eca04733f60b39e1d3025b9d708a   410434eb0822   4 minutes ago   1.49GB
runatlantis/testing-env                                          latest                                     410434eb0822   4 minutes ago   1.49GB

pushed

[lkysow]

@nishkrishnan @msarvar can you pin to runatlantis/testing-env:20296f9638a4eca04733f60b39e1d3025b9d708a in .circleci/config.yml

Then pushing changes to the testing image won't break existing PRs (from now on)

[msarvar]

@lkysow done

[xarses]

Can you help me understand why --all-namespaces was dropped? We where anticipating this being present

[rgooler]

Why was this pulled into 0.17.0 4 days before the release to stable, with this major of a behavior regression? This straight up breaks anyone organizing their policies into multiple namespaces, with no ability to organize as they please. In addition, this is not necessary for the PR's listed goal of upgrading conftest - nor is it explained at all in the PR.

This should be reverted, and @msarvar can add a config flag to atlantis to toggle that if lyft wants to disable --all-namespaces.

[nishkrishnan]

Sorry about the regression since it was a pre-prod release we took some liberties with backwards incompatible changes in the general release

I'll let @msarvar answer to specifics as to why it was removed and whether we should add it back.

As for a current workaround, you can still have folder separation, your rules just have to all be declared in the same package.

policies
... <set1>
   ... main.rego
... <set2>
   ... main.rego

Why this works for us is because you can have namespaces within your folders that you decide should get pulled into the main package. This makes things like testing easier.

e.g.

/policies/set1/main.rego

package main

import data.infra

....

/policies/set1/infra.rego

package infra


deny[msg] {
...
}

/policies/set1/infra_test.rego

package infra_test

test_deny {
...
}

Based on this example, we wouldn't want to load all namespaces. Also doesn't seem like there is a way to have this nice co-existence of tests and rules if we did want to load all namespaces by default.

[rgooler]

As a counterpoint - the tests could be written with that level of separation, without putting the onus on the end users to manually update the list of policies every time they add a new policy to be checked. :)

I do believe designing for simplified unit tests at the expense of end-user experience is an anti-pattern. There are several ways to approach this in a more user-friendly manner:

• Unit tests could use namespaces to keep each test entirely separate from main
• A command line argument can be added to allow the configuration of the conftest flags at runtime
• Tests can be built with globally unique function names to avoid these issues

Additionally, right now, the config only allows for 'local' policies. However, a fairly reasonable extension of that would be to load policies via pointing to a git repo that would be fetch'd before updating. The current behavior would require that repo full of policies MUST have unique function names everywhere and can only use the main namespace. If the end user wishes to lift this restriction, then they must maintain a fork of atlantis.

[msarvar]

Sorry about reverting the code as @nishkrishnan mentioned it was a pre-release and we took some liberties to make changes. The reason behind the revert was that conftest was not working properly when using a helper library. For instance regula would fail with --all-namespaces flag. To demonstrate the issue try running

conftest pull -p policy/ github.com/fugue/regula/conftest
conftest pull -p policy/regula/lib 'github.com/fugue/regula//lib?ref=v0.7.0'
conftest pull -p policy/regula/rules github.com/fugue/regula/rules

conftest test -p test.json --all-namespaces 

You will receive an error similar to:

Error: running test: query rule: check: query rule: evaluating policy: 1 error occurred: 1:1: rego_type_error: undefined ref: data.fugue.deny_resource_with_message
	data.fugue.deny_resource_with_message
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	have: (any, any) => object<id: any, message: any, provider: any, type: any, valid: boolean>

Removing the --all-namespaces flag just makes it easier to use libraries like regula in your policies. And you're still free to namespace you policies as described in the previous comment.

[msarvar]

@rgooler in the current behavior you are required to have unique functions within the namespaces. You are able to share those namespaces across all of your polices, no? The only thing your main package needs to do is to run deny query. Or am I missing something?

[xarses]

If this is to stay, then the most obvious need is that the documentation get updated so that its clear the package needs to be main. As it currently stands, there is no messaging about namespaces, and the example is implying that you can have whatever namespace you want. https://github.com/runatlantis/atlantis/blame/master/runatlantis.io/docs/policy-checking.md#L58