From 3f6247f155a09810f5a6148f4da898b51ceb8cc2 Mon Sep 17 00:00:00 2001 From: "Stephen Lewis (Burrows)" Date: Thu, 30 Sep 2021 11:52:45 -0700 Subject: [PATCH] Made structured output of validate command always set resource_body to a valid type (#324) Simplified return value for ValidateAssetsFunc to be a slice of violations instead of an AuditResponse. This guarantees that the violations won't end up being a nil slice, which would be serialized to null instead of [] --- cmd/validate.go | 15 +++++++++------ cmd/validate_test.go | 38 +++++++++++++++++--------------------- go.mod | 1 + tfgcv/proto_test.go | 2 +- tfgcv/validate_assets.go | 16 +++++++++------- 5 files changed, 37 insertions(+), 35 deletions(-) diff --git a/cmd/validate.go b/cmd/validate.go index 405b16da1..56df2cff9 100644 --- a/cmd/validate.go +++ b/cmd/validate.go @@ -20,6 +20,7 @@ import ( "os" "github.com/GoogleCloudPlatform/terraform-validator/tfgcv" + "github.com/forseti-security/config-validator/pkg/api/validator" "github.com/golang/protobuf/jsonpb" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -107,36 +108,38 @@ func (o *validateOptions) run(plan string) error { return errors.Wrap(err, "converting tfplan to CAI assets") } - auditResult, err := o.validateAssets(ctx, assets, o.policyPath) + violations, err := o.validateAssets(ctx, assets, o.policyPath) if err != nil { return errors.Wrap(err, "validating: FCV") } if o.rootOptions.useStructuredLogging { msg := "No violations found" - if len(auditResult.Violations) > 0 { + if len(violations) > 0 { msg = "Violations found" } o.rootOptions.outputLogger.Info( msg, - zap.Any("resource_body", auditResult.Violations), + zap.Any("resource_body", violations), ) - if len(auditResult.Violations) > 0 { + if len(violations) > 0 { return errViolations } return nil } // Legacy behavior - if len(auditResult.Violations) > 0 { + if len(violations) > 0 { if o.outputJSON { marshaller := &jsonpb.Marshaler{} + auditResult := &validator.AuditResponse{} + auditResult.Violations = violations if err := marshaller.Marshal(os.Stdout, auditResult); err != nil { return errors.Wrap(err, "marshalling violations to json") } } else { fmt.Print("Found Violations:\n\n") - for _, v := range auditResult.Violations { + for _, v := range violations { fmt.Printf("Constraint %v on resource %v: %v\n\n", v.Constraint, v.Resource, diff --git a/cmd/validate_test.go b/cmd/validate_test.go index bce727008..97d44da00 100644 --- a/cmd/validate_test.go +++ b/cmd/validate_test.go @@ -10,31 +10,27 @@ import ( "github.com/stretchr/testify/assert" ) -func testAuditResponseNoViolations() *validator.AuditResponse { - return &validator.AuditResponse{ - Violations: []*validator.Violation{}, - } +func testNoViolations() []*validator.Violation { + return []*validator.Violation{} } -func MockValidateAssetsNoViolations(ctx context.Context, assets []google.Asset, policyRootPath string) (*validator.AuditResponse, error) { - return testAuditResponseNoViolations(), nil +func MockValidateAssetsNoViolations(ctx context.Context, assets []google.Asset, policyRootPath string) ([]*validator.Violation, error) { + return testNoViolations(), nil } -func testAuditResponseWithViolations() *validator.AuditResponse { - return &validator.AuditResponse{ - Violations: []*validator.Violation{ - &validator.Violation{ - Constraint: "GCPAlwaysViolatesConstraintV1.always_violates_project_match_target", - Resource: "//bigtable.googleapis.com/projects/my-project/instances/tf-instance", - Message: "Constraint GCPAlwaysViolatesConstraintV1.always_violates_project_match_target on resource //bigtable.googleapis.com/projects/my-project/instances/tf-instance", - Severity: "high", - }, +func testWithViolations() []*validator.Violation { + return []*validator.Violation{ + &validator.Violation{ + Constraint: "GCPAlwaysViolatesConstraintV1.always_violates_project_match_target", + Resource: "//bigtable.googleapis.com/projects/my-project/instances/tf-instance", + Message: "Constraint GCPAlwaysViolatesConstraintV1.always_violates_project_match_target on resource //bigtable.googleapis.com/projects/my-project/instances/tf-instance", + Severity: "high", }, } } -func MockValidateAssetsWithViolations(ctx context.Context, assets []google.Asset, policyRootPath string) (*validator.AuditResponse, error) { - return testAuditResponseWithViolations(), nil +func MockValidateAssetsWithViolations(ctx context.Context, assets []google.Asset, policyRootPath string) ([]*validator.Violation, error) { + return testWithViolations(), nil } func TestValidateRun(t *testing.T) { @@ -76,10 +72,10 @@ func TestValidateRun(t *testing.T) { a.Contains(output, "resource_body") a.Len(output["resource_body"], 1) - var expectedAuditResponse map[string]interface{} - expectedAuditResponseJSON, _ := json.Marshal(testAuditResponseWithViolations()) - json.Unmarshal(expectedAuditResponseJSON, &expectedAuditResponse) - a.Equal(expectedAuditResponse["violations"], output["resource_body"]) + var expectedViolations []interface{} + expectedViolationsJSON, _ := json.Marshal(testWithViolations()) + json.Unmarshal(expectedViolationsJSON, &expectedViolations) + a.Equal(expectedViolations, output["resource_body"]) } func TestValidateRunNoViolations(t *testing.T) { diff --git a/go.mod b/go.mod index 3052695b0..036a72cf9 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( go.uber.org/zap v1.19.1 google.golang.org/api v0.56.0 google.golang.org/genproto v0.0.0-20210828152312-66f60bf46e71 + google.golang.org/protobuf v1.27.1 // indirect ) go 1.14 diff --git a/tfgcv/proto_test.go b/tfgcv/proto_test.go index 8d42b92d9..40c44d4a7 100644 --- a/tfgcv/proto_test.go +++ b/tfgcv/proto_test.go @@ -18,8 +18,8 @@ import ( "testing" "github.com/forseti-security/config-validator/pkg/api/validator" - structpb "github.com/golang/protobuf/ptypes/struct" "github.com/golang/protobuf/proto" + structpb "github.com/golang/protobuf/ptypes/struct" "github.com/stretchr/testify/require" "google.golang.org/genproto/googleapis/cloud/asset/v1" ) diff --git a/tfgcv/validate_assets.go b/tfgcv/validate_assets.go index 3be8d8d57..8798c69c0 100644 --- a/tfgcv/validate_assets.go +++ b/tfgcv/validate_assets.go @@ -32,18 +32,18 @@ func BuildVersion() string { return buildVersion } -type ValidateAssetsFunc func(ctx context.Context, assets []google.Asset, policyRootPath string) (*validator.AuditResponse, error) +type ValidateAssetsFunc func(ctx context.Context, assets []google.Asset, policyRootPath string) ([]*validator.Violation, error) // ValidateAssets instantiates GCV and audits CAI assets using "policies" // and "lib" folder under policyRootPath. -func ValidateAssets(ctx context.Context, assets []google.Asset, policyRootPath string) (*validator.AuditResponse, error) { +func ValidateAssets(ctx context.Context, assets []google.Asset, policyRootPath string) ([]*validator.Violation, error) { return ValidateAssetsWithLibrary(ctx, assets, []string{filepath.Join(policyRootPath, "policies")}, filepath.Join(policyRootPath, "lib")) } // ValidateAssetsWithLibrary instantiates GCV and audits CAI assets. -func ValidateAssetsWithLibrary(ctx context.Context, assets []google.Asset, policyPaths []string, policyLibraryDir string) (*validator.AuditResponse, error) { +func ValidateAssetsWithLibrary(ctx context.Context, assets []google.Asset, policyPaths []string, policyLibraryDir string) ([]*validator.Violation, error) { valid, err := gcv.NewValidator(policyPaths, policyLibraryDir) if err != nil { return nil, errors.Wrap(err, "initializing gcv validator") @@ -59,17 +59,19 @@ func ValidateAssetsWithLibrary(ctx context.Context, assets []google.Asset, polic pbSplitAssets := splitAssets(pbAssets) - auditResult := &validator.AuditResponse{} + // Make an empty slice, not a nil slice, so that this + // can be properly serialized to JSON. + violations := []*validator.Violation{} for _, asset := range pbSplitAssets { - violations, err := valid.ReviewAsset(context.Background(), asset) + newViolations, err := valid.ReviewAsset(context.Background(), asset) if err != nil { return nil, errors.Wrapf(err, "reviewing asset %s", asset) } - auditResult.Violations = append(auditResult.Violations, violations...) + violations = append(violations, newViolations...) } - return auditResult, nil + return violations, nil } // splitAssets split assets because for the GCP target Constraint