Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade conftest version to 0.23 #1516

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Upgrade conftest version to 0.23 #1516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
93ce558
Upgraded conftest to 0.23.0 and removed --all-namespaces flag (#62)
msarvar Apr 12, 2021
20296f9
Update e2e tests to use 0.23 conftest
msarvar Apr 19, 2021
945dad8
Fix log function
msarvar Apr 22, 2021
e81a287
Make test client fetch version synchronously.
Apr 22, 2021
641718e
Merge branch 'upgrade_conftest_version' of github.com:lyft/atlantis i…
Apr 22, 2021
4ef7ed9
Update fixtures.
Apr 22, 2021
b0f2617
Merge branch 'master' of github.com:runatlantis/atlantis into upgrade…
msarvar Apr 23, 2021
75edc82
Pinning test image
msarvar Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: 2
jobs:
test:
docker:
- image: runatlantis/testing-env:latest
- image: runatlantis/testing-env:20296f9638a4eca04733f60b39e1d3025b9d708a
steps:
- checkout
- run: make check-fmt
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ RUN AVAILABLE_TERRAFORM_VERSIONS="0.8.8 0.9.11 0.10.8 0.11.14 0.12.30 0.13.6 ${D
done && \
ln -s /usr/local/bin/tf/versions/${DEFAULT_TERRAFORM_VERSION}/terraform /usr/local/bin/terraform

ENV DEFAULT_CONFTEST_VERSION=0.21.0
ENV DEFAULT_CONFTEST_VERSION=0.23.0

RUN AVAILABLE_CONFTEST_VERSIONS="${DEFAULT_CONFTEST_VERSION}" && \
for VERSION in ${AVAILABLE_CONFTEST_VERSIONS}; do \
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile.dev
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FROM runatlantis/atlantis:latest
COPY atlantis /usr/local/bin/atlantis
# TODO: remove this once we get this in the base image
ENV DEFAULT_CONFTEST_VERSION=0.21.0
ENV DEFAULT_CONFTEST_VERSION=0.23.0

WORKDIR /atlantis/src
2 changes: 1 addition & 1 deletion server/events/runtime/policy/conftest_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (c ConftestTestCommandArgs) build() ([]string, error) {
commandArgs = append(commandArgs, a.build()...)
}

commandArgs = append(commandArgs, c.InputFile, "--no-color", "--all-namespaces")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nishkrishnan nishkrishnan Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

commandArgs = append(commandArgs, c.InputFile, "--no-color")

return commandArgs, nil
}
Expand Down
12 changes: 6 additions & 6 deletions server/events/runtime/policy/conftest_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import (

func TestConfTestVersionDownloader(t *testing.T) {

version, _ := version.NewVersion("0.21.0")
version, _ := version.NewVersion("0.23.0")
destPath := "some/path"

fullURL := fmt.Sprintf("https://github.com/open-policy-agent/conftest/releases/download/v0.21.0/conftest_0.21.0_%s_x86_64.tar.gz?checksum=file:https://github.com/open-policy-agent/conftest/releases/download/v0.21.0/checksums.txt", strings.Title(runtime.GOOS))
fullURL := fmt.Sprintf("https://github.com/open-policy-agent/conftest/releases/download/v0.23.0/conftest_0.23.0_%s_x86_64.tar.gz?checksum=file:https://github.com/open-policy-agent/conftest/releases/download/v0.23.0/checksums.txt", strings.Title(runtime.GOOS))

RegisterMockTestingT(t)

Expand Down Expand Up @@ -175,7 +175,7 @@ func TestRun(t *testing.T) {

expectedOutput := "Success"
expectedResult := "Checking plan against the following policies: \n policy1\n policy2\nSuccess"
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "-p", localPolicySetPath2, "/some_workdir/testproj-default.json", "--no-color", "--all-namespaces"}
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "-p", localPolicySetPath2, "/some_workdir/testproj-default.json", "--no-color"}

When(mockResolver.Resolve(policySet1)).ThenReturn(localPolicySetPath1, nil)
When(mockResolver.Resolve(policySet2)).ThenReturn(localPolicySetPath2, nil)
Expand All @@ -196,7 +196,7 @@ func TestRun(t *testing.T) {

expectedOutput := "Success"
expectedResult := "Checking plan against the following policies: \n policy1\nSuccess"
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "/some_workdir/testproj-default.json", "--no-color", "--all-namespaces"}
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "/some_workdir/testproj-default.json", "--no-color"}

When(mockResolver.Resolve(policySet1)).ThenReturn(localPolicySetPath1, nil)
When(mockResolver.Resolve(policySet2)).ThenReturn("", errors.New("err"))
Expand All @@ -214,7 +214,7 @@ func TestRun(t *testing.T) {
t.Run("error resolving both policy sources", func(t *testing.T) {

expectedResult := "Success"
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "/some_workdir/testproj-default.json", "--no-color", "--all-namespaces"}
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "/some_workdir/testproj-default.json", "--no-color"}

When(mockResolver.Resolve(policySet1)).ThenReturn("", errors.New("err"))
When(mockResolver.Resolve(policySet2)).ThenReturn("", errors.New("err"))
Expand All @@ -232,7 +232,7 @@ func TestRun(t *testing.T) {
t.Run("error running cmd", func(t *testing.T) {
expectedOutput := "FAIL - /some_workdir/testproj-default.json - failure"
expectedResult := "Checking plan against the following policies: \n policy1\n policy2\nFAIL - <redacted plan file> - failure"
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "-p", localPolicySetPath2, "/some_workdir/testproj-default.json", "--no-color", "--all-namespaces"}
expectedArgs := []string{executablePath, "test", "-p", localPolicySetPath1, "-p", localPolicySetPath2, "/some_workdir/testproj-default.json", "--no-color"}

When(mockResolver.Resolve(policySet1)).ThenReturn(localPolicySetPath1, nil)
When(mockResolver.Resolve(policySet2)).ThenReturn(localPolicySetPath2, nil)
Expand Down
86 changes: 74 additions & 12 deletions server/events/terraform/terraform_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,8 @@ type Downloader interface {
// => 0.11.10
var versionRegex = regexp.MustCompile("Terraform v(.*?)(\\s.*)?\n")

// NewClient constructs a terraform client.
// tfeToken is an optional terraform enterprise token.
// defaultVersionStr is an optional default terraform version to use unless
// a specific version is set.
// defaultVersionFlagName is the name of the flag that sets the default terraform
// version.
// tfDownloader is used to download terraform versions.
// Will asynchronously download the required version if it doesn't exist already.
func NewClient(
// NewClientWithDefaultVersion creates a new terraform client and pre-fetches the default version
func NewClientWithDefaultVersion(
log logging.SimpleLogging,
binDir string,
cacheDir string,
Expand All @@ -106,7 +99,9 @@ func NewClient(
defaultVersionFlagName string,
tfDownloadURL string,
tfDownloader Downloader,
usePluginCache bool) (*DefaultClient, error) {
usePluginCache bool,
fetchAsync bool,
) (*DefaultClient, error) {
var finalDefaultVersion *version.Version
var localVersion *version.Version
versions := make(map[string]string)
Expand Down Expand Up @@ -135,7 +130,7 @@ func NewClient(
return nil, err
}
finalDefaultVersion = defaultVersion
go func() {
ensureVersionFunc := func() {
// Since ensureVersion might end up downloading terraform,
// we call it asynchronously so as to not delay server startup.
versionsLock.Lock()
Expand All @@ -144,7 +139,13 @@ func NewClient(
if err != nil {
log.Err("could not download terraform %s: %s", defaultVersion.String(), err)
}
}()
}

if fetchAsync {
go ensureVersionFunc()
} else {
ensureVersionFunc()
}
}

// If tfeToken is set, we try to create a ~/.terraformrc file.
Expand All @@ -168,6 +169,67 @@ func NewClient(
versions: versions,
usePluginCache: usePluginCache,
}, nil

}

func NewTestClient(
log logging.SimpleLogging,
binDir string,
cacheDir string,
tfeToken string,
tfeHostname string,
defaultVersionStr string,
defaultVersionFlagName string,
tfDownloadURL string,
tfDownloader Downloader,
usePluginCache bool) (*DefaultClient, error) {
return NewClientWithDefaultVersion(
log,
binDir,
cacheDir,
tfeToken,
tfeHostname,
defaultVersionStr,
defaultVersionFlagName,
tfDownloadURL,
tfDownloader,
usePluginCache,
false,
)
}

// NewClient constructs a terraform client.
// tfeToken is an optional terraform enterprise token.
// defaultVersionStr is an optional default terraform version to use unless
// a specific version is set.
// defaultVersionFlagName is the name of the flag that sets the default terraform
// version.
// tfDownloader is used to download terraform versions.
// Will asynchronously download the required version if it doesn't exist already.
func NewClient(
log logging.SimpleLogging,
binDir string,
cacheDir string,
tfeToken string,
tfeHostname string,
defaultVersionStr string,
defaultVersionFlagName string,
tfDownloadURL string,
tfDownloader Downloader,
usePluginCache bool) (*DefaultClient, error) {
return NewClientWithDefaultVersion(
log,
binDir,
cacheDir,
tfeToken,
tfeHostname,
defaultVersionStr,
defaultVersionFlagName,
tfDownloadURL,
tfDownloader,
usePluginCache,
true,
)
}

// Version returns the default version of Terraform we use if no other version
Expand Down
2 changes: 1 addition & 1 deletion server/events/terraform/terraform_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func TestEnsureVersion_downloaded(t *testing.T) {

mockDownloader := mocks.NewMockDownloader()

c, err := terraform.NewClient(logger, binDir, cacheDir, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, mockDownloader, true)
c, err := terraform.NewTestClient(logger, binDir, cacheDir, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, mockDownloader, true)
Ok(t, err)

Equals(t, "0.11.10", c.DefaultVersion().String())
Expand Down
16 changes: 9 additions & 7 deletions server/events_controller_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
. "github.com/runatlantis/atlantis/testing"
)

const ConftestVersion = "0.23.0"

var applyLocker locking.ApplyLocker
var userConfig server.UserConfig

Expand All @@ -56,7 +58,7 @@ type LocalConftestCache struct {
}

func (m *LocalConftestCache) Get(key *version.Version) (string, error) {
return exec.LookPath("conftest0.21.0")
return exec.LookPath(fmt.Sprintf("conftest%s", ConftestVersion))
}

func TestGitHubWorkflow(t *testing.T) {
Expand Down Expand Up @@ -634,7 +636,7 @@ func setupE2E(t *testing.T, repoDir string) (server.EventsController, *vcsmocks.

if userConfig.EnablePolicyChecksFlag {
// need this to be set or we'll fail the policy check step
os.Setenv(policy.DefaultConftestVersionEnvKey, "0.21.0")
os.Setenv(policy.DefaultConftestVersionEnvKey, "0.23.0")
}

// Mocks.
Expand Down Expand Up @@ -725,7 +727,7 @@ func setupE2E(t *testing.T, repoDir string) (server.EventsController, *vcsmocks.

Ok(t, err)

conftestVersion, _ := version.NewVersion("0.21.0")
conftestVersion, _ := version.NewVersion(ConftestVersion)

conftextExec := policy.NewConfTestExecutorWorkflow(logger, binDir, &NoopTFDownloader{})

Expand Down Expand Up @@ -1077,11 +1079,11 @@ func mkSubDirs(t *testing.T) (string, string, string, func()) {
return tmp, binDir, cachedir, cleanup
}

// Will fail test if conftest isn't in path and isn't version >= 0.21.0
// Will fail test if conftest isn't in path and isn't version >= 0.23.0
func ensureRunningConftest(t *testing.T) {
localPath, err := exec.LookPath("conftest0.21.0")
localPath, err := exec.LookPath(fmt.Sprintf("conftest%s", ConftestVersion))
if err != nil {
t.Log("conftest >= 0.21 must be installed to run this test")
t.Logf("conftest >= %s must be installed to run this test", ConftestVersion)
t.FailNow()
}
versionOutBytes, err := exec.Command(localPath, "--version").Output() // #nosec
Expand All @@ -1097,7 +1099,7 @@ func ensureRunningConftest(t *testing.T) {
}
localVersion, err := version.NewVersion(match[1])
Ok(t, err)
minVersion, err := version.NewVersion("0.21.0")
minVersion, err := version.NewVersion(ConftestVersion)
Ok(t, err)
if localVersion.LessThan(minVersion) {
t.Logf("must have contest version >= %s, you have %s", minVersion, localVersion)
Expand Down
2 changes: 1 addition & 1 deletion server/testfixtures/test-repos/policy-checks-apply-reqs/exp-output-auto-policy-check.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Ran Policy Check for dir: `.` workspace: `default`
exit status 1
Checking plan against the following policies:
test_policy
FAIL - <redacted plan file> - WARNING: Null Resource creation is prohibited.
FAIL - <redacted plan file> - main - WARNING: Null Resource creation is prohibited.

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

Expand Down
2 changes: 1 addition & 1 deletion server/testfixtures/test-repos/policy-checks-diff-owner/exp-output-auto-policy-check.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Ran Policy Check for dir: `.` workspace: `default`
exit status 1
Checking plan against the following policies:
test_policy
FAIL - <redacted plan file> - WARNING: Null Resource creation is prohibited.
FAIL - <redacted plan file> - main - WARNING: Null Resource creation is prohibited.

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

Expand Down
2 changes: 1 addition & 1 deletion server/testfixtures/test-repos/policy-checks-multi-projects/exp-output-auto-policy-check.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Checking plan against the following policies:
exit status 1
Checking plan against the following policies:
test_policy
FAIL - <redacted plan file> - WARNING: Forbidden Resource creation is prohibited.
FAIL - <redacted plan file> - main - WARNING: Forbidden Resource creation is prohibited.

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

Expand Down
2 changes: 1 addition & 1 deletion server/testfixtures/test-repos/policy-checks/exp-output-auto-policy-check.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Ran Policy Check for dir: `.` workspace: `default`
exit status 1
Checking plan against the following policies:
test_policy
FAIL - <redacted plan file> - WARNING: Null Resource creation is prohibited.
FAIL - <redacted plan file> - main - WARNING: Null Resource creation is prohibited.

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

Expand Down
2 changes: 1 addition & 1 deletion testing/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ RUN curl -LOks https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/ter
rm terraform_${TERRAFORM_VERSION}_linux_amd64.zip

# Install conftest
ENV DEFAULT_CONFTEST_VERSION=0.21.0
ENV DEFAULT_CONFTEST_VERSION=0.23.0

RUN AVAILABLE_CONFTEST_VERSIONS="${DEFAULT_CONFTEST_VERSION}" && \
for VERSION in ${AVAILABLE_CONFTEST_VERSIONS}; do \
Expand Down