Repository: Azure/draft
Pull request #252 (2024-03-08 18:12:26)

Bugfixing error manifests passing without violations / rehaul of ReadManifests to not use fs.FS object
# Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)
Feature # (details)

## Type of change

Please delete options that are not relevant.

- [x] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. Is it a  breaking change which will impact consuming tool(s).

- TestRunValidate
- Constraint Unit tests


# Checklist:

- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged and published in downstream modules



bacongobbler: dropped an issue at azure-cli to see if they know any good debugging tips: https://github.com/Azure/azure-cli/issues/4223
mboersma: Yep, that looks like the same issue. Hopefully deis/jenkins-node#21 fixed it for draft too.
bacongobbler: Just [waiting on a job](https://ci.deis.io/job/Azure/job/draft/job/master/125/console) I kicked off on master to see if it's been fixed by that ticket. Thank you for indirectly fixing this, @mboersma! 😀 
bacongobbler: Yep, this fixed the issue.

Patch:
diff --git a/cmd/validate.go b/cmd/validate.go
index 2431c75..5f175b6 100644
--- a/cmd/validate.go
+++ b/cmd/validate.go
@@ -3,11 +3,13 @@ package cmd
 import (
 	"context"
 	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+
 	"github.com/Azure/draft/pkg/safeguards"
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
-	"io/fs"
-	"os"
 )
 
 type validateCmd struct {
@@ -52,19 +54,20 @@ func isDirectory(path string) (bool, error) {
 }
 
 // getManifests uses fs.WalkDir to retrieve a list of the manifest files within the given manifest path
-func getManifests(f fs.FS, path string) ([]string, error) {
-	var manifests []string
+func getManifestFiles(p string) ([]string, error) {
+	var manifestFiles []string
 
-	err := fs.WalkDir(f, path, func(path string, d fs.DirEntry, err error) error {
+	err := filepath.Walk(p, func(filepath string, info fs.FileInfo, err error) error {
 		if err != nil {
-			return fmt.Errorf("error walking path %s with error: %w", path, err)
+			return fmt.Errorf("error walking path %s with error: %w", filepath, err)
 		}
 
-		if !d.IsDir() {
-			log.Debugf("%s is not a directory, appending to manifests", path)
-			manifests = append(manifests, path)
+		if !info.IsDir() && info.Name() != "" {
+			log.Debugf("%s is not a directory, appending to manifestFiles", info.Name())
+
+			manifestFiles = append(manifestFiles, filepath)
 		} else {
-			log.Debugf("%s is a directory, skipping...", path)
+			log.Debugf("%s is a directory, skipping...", info.Name())
 		}
 
 		return nil
@@ -73,7 +76,7 @@ func getManifests(f fs.FS, path string) ([]string, error) {
 		return nil, fmt.Errorf("could not walk directory: %w", err)
 	}
 
-	return manifests, nil
+	return manifestFiles, nil
 }
 
 // run is our entry point to ValidateManifests
@@ -85,18 +88,17 @@ func (vc *validateCmd) run() error {
 	ctx := context.Background()
 	isDir, err := isDirectory(vc.manifestPath)
 	if err != nil {
-		return fmt.Errorf("could not determine if given path is a directory: %w", err)
+		return fmt.Errorf("not a valid file or directory: %w", err)
 	}
 
-	var manifests []string
-	var manifestFS = os.DirFS(vc.manifestPath)
+	var manifestFiles []string
 	if isDir {
-		manifests, err = getManifests(manifestFS, vc.manifestPath)
+		manifestFiles, err = getManifestFiles(vc.manifestPath)
 		if err != nil {
 			return err
 		}
 	} else {
-		manifests = append(manifests, vc.manifestPath)
+		manifestFiles = append(manifestFiles, vc.manifestPath)
 	}
 
 	if err != nil {
@@ -104,7 +106,7 @@ func (vc *validateCmd) run() error {
 	}
 
 	log.Debugf("validating manifests")
-	err = safeguards.ValidateManifests(ctx, manifestFS, manifests)
+	err = safeguards.ValidateManifests(ctx, manifestFiles)
 	if err != nil {
 		log.Errorf("validating safeguards: %s", err.Error())
 		return err
diff --git a/pkg/safeguards/safeguards.go b/pkg/safeguards/safeguards.go
index 4ddb02b..2298a44 100644
--- a/pkg/safeguards/safeguards.go
+++ b/pkg/safeguards/safeguards.go
@@ -4,9 +4,8 @@ import (
 	"context"
 	"embed"
 	"fmt"
-	"io/fs"
-
 	api "github.com/open-policy-agent/gatekeeper/v3/apis"
+	log "github.com/sirupsen/logrus"
 
 	"k8s.io/apimachinery/pkg/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
@@ -36,7 +35,7 @@ func init() {
 
 // ValidateManifests is what will be called by `draft validate` to validate the user's manifests
 // against each safeguards constraint
-func ValidateManifests(ctx context.Context, manifestFS fs.FS, manifests []string) error {
+func ValidateManifests(ctx context.Context, manifestFiles []string) error {
 	// constraint client instantiation
 	c, err := getConstraintClient()
 	if err != nil {
@@ -64,14 +63,14 @@ func ValidateManifests(ctx context.Context, manifestFS fs.FS, manifests []string
 	}
 
 	var violations []string
-	for _, m := range manifests {
-		manifest, err := fc.ReadManifest(m, manifestFS)
+	for _, m := range manifestFiles {
+		manifests, err := fc.ReadManifests(m)
 		if err != nil {
 			return err
 		}
 
 		// validation of deployment manifest with constraints, templates loaded
-		err = validateManifest(ctx, c, manifest)
+		err = validateManifests(ctx, c, manifests)
 		if err != nil {
 			violations = append(violations, err.Error())
 		}
@@ -79,8 +78,10 @@ func ValidateManifests(ctx context.Context, manifestFS fs.FS, manifests []string
 
 	// returning the full list of violations after each manifest is checked
 	if len(violations) > 0 {
+
 		return fmt.Errorf("violations have occurred: %s", violations)
 	}
 
+	log.Printf("No violations found.")
 	return nil
 }
diff --git a/pkg/safeguards/safeguards_helpers.go b/pkg/safeguards/safeguards_helpers.go
index 3f365d9..343f8f4 100644
--- a/pkg/safeguards/safeguards_helpers.go
+++ b/pkg/safeguards/safeguards_helpers.go
@@ -1,15 +1,16 @@
 package safeguards
 
 import (
+	"bufio"
 	"context"
 	"fmt"
-	"io/fs"
-
 	constraintclient "github.com/open-policy-agent/frameworks/constraint/pkg/client"
 	"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/rego"
 	"github.com/open-policy-agent/frameworks/constraint/pkg/core/templates"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/gator/reader"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/target"
+	log "github.com/sirupsen/logrus"
+	"os"
 
 	"golang.org/x/mod/semver"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -44,14 +45,19 @@ func updateSafeguardPaths() {
 }
 
 // methods for retrieval of manifest, constraint templates, and constraints
-func (fc FileCrawler) ReadManifest(path string, manifestFS fs.FS) (*unstructured.Unstructured, error) {
-	// if david prefers not having this in binary, we can change the fs object later
-	deployment, err := reader.ReadObject(manifestFS, path)
+func (fc FileCrawler) ReadManifests(path string) ([]*unstructured.Unstructured, error) {
+	file, err := os.Open(path)
+	if err != nil {
+		return nil, fmt.Errorf("opening file %q: %w", path, err)
+	}
+	defer file.Close()
+
+	manifests, err := reader.ReadK8sResources(bufio.NewReader(file))
 	if err != nil {
-		return nil, fmt.Errorf("could not read manifest: %w", err)
+		return nil, fmt.Errorf("reading file %q: %w", path, err)
 	}
 
-	return deployment, nil
+	return manifests, nil
 }
 
 func (fc FileCrawler) ReadConstraintTemplates() ([]*templates.ConstraintTemplate, error) {
@@ -60,7 +66,7 @@ func (fc FileCrawler) ReadConstraintTemplates() ([]*templates.ConstraintTemplate
 	for _, sg := range fc.Safeguards {
 		ct, err := reader.ReadTemplate(s, fc.constraintFS, sg.templatePath)
 		if err != nil {
-			return nil, fmt.Errorf("could not read template: %w", err)
+			return nil, fmt.Errorf("could not read template: %w\n", err)
 		}
 		constraintTemplates = append(constraintTemplates, ct)
 	}
@@ -152,19 +158,22 @@ func loadConstraints(ctx context.Context, c *constraintclient.Client, constraint
 }
 
 // does validation on manifest based on loaded constraint templates, constraints
-func validateManifest(ctx context.Context, c *constraintclient.Client, manifest *unstructured.Unstructured) error {
+func validateManifests(ctx context.Context, c *constraintclient.Client, manifests []*unstructured.Unstructured) error {
 	// Review makes sure the provided object satisfies all stored constraints.
 	// On error, the responses return value will still be populated so that
 	// partial results can be analyzed.
-	res, err := c.Review(ctx, manifest)
-	if err != nil {
-		return fmt.Errorf("could not review manifest: %w", err)
-	}
+	for _, manifest := range manifests {
+		log.Printf("Reviewing %s...", manifest.GetName())
+		res, err := c.Review(ctx, manifest)
+		if err != nil {
+			return fmt.Errorf("could not review manifests: %w", err)
+		}
 
-	for _, v := range res.ByTarget {
-		for _, result := range v.Results {
-			if result.Msg != "" {
-				return fmt.Errorf("manifest error: %s", result.Msg)
+		for _, v := range res.ByTarget {
+			for _, result := range v.Results {
+				if result.Msg != "" {
+					return fmt.Errorf("manifest error: %s", result.Msg)
+				}
 			}
 		}
 	}


Test patch:
diff --git a/cmd/validate_test.go b/cmd/validate_test.go
index fe0079c..af3a6e8 100644
--- a/cmd/validate_test.go
+++ b/cmd/validate_test.go
@@ -4,16 +4,13 @@ import (
 	"context"
 	"os"
 	"path"
+	"path/filepath"
 	"testing"
 
 	"github.com/Azure/draft/pkg/safeguards"
 	"github.com/stretchr/testify/assert"
 )
 
-var wd, _ = os.Getwd()
-var testFS = os.DirFS(path.Join(wd, "../pkg/safeguards/tests"))
-var emptyFS = os.DirFS("")
-
 // TestIsDirectory tests the isDirectory function for proper returns
 func TestIsDirectory(t *testing.T) {
 	testWd, _ := os.Getwd()
@@ -38,38 +35,38 @@ func TestIsDirectory(t *testing.T) {
 func TestRunValidate(t *testing.T) {
 	ctx := context.TODO()
 	manifestPathEmpty := ""
-	manifestPathDirectorySuccess := "all/success"
-	manifestPathDirectoryError := "all/error"
-	manifestPathFileSuccess := "all/success/all-success-manifest.yaml"
-	manifestPathFileError := "all/error/all-error-manifest.yaml"
-	var manifests []string
+	manifestPathDirectorySuccess, _ := filepath.Abs("../pkg/safeguards/tests/all/success")
+	manifestPathDirectoryError, _ := filepath.Abs("../pkg/safeguards/tests/all/error")
+	manifestPathFileSuccess, _ := filepath.Abs("../pkg/safeguards/tests/all/success/all-success-manifest.yaml")
+	manifestPathFileError, _ := filepath.Abs("../pkg/safeguards/tests/all/error/all-error-manifest.yaml")
+	var manifestFiles []string
 
 	// Scenario 1: empty manifest path should error
-	manifests = append(manifests, manifestPathEmpty)
-	err := safeguards.ValidateManifests(ctx, emptyFS, manifests)
+	manifestFiles = append(manifestFiles, manifestPathEmpty)
+	err := safeguards.ValidateManifests(ctx, manifestFiles)
 	assert.NotNil(t, err)
 
-	// Scenario 2a: manifest path leads to a directory of manifests - expect success
-	manifests, err = getManifests(testFS, manifestPathDirectorySuccess)
+	// Scenario 2a: manifest path leads to a directory of manifestFiles - expect success
+	manifestFiles, err = getManifestFiles(manifestPathDirectorySuccess)
 	assert.Nil(t, err)
-	err = safeguards.ValidateManifests(ctx, testFS, manifests)
+	err = safeguards.ValidateManifests(ctx, manifestFiles)
 	assert.Nil(t, err)
 
-	// Scenario 2b: manifest path leads to a directory of manifests - expect failure
-	manifests, err = getManifests(testFS, manifestPathDirectoryError)
+	// Scenario 2b: manifest path leads to a directory of manifestFiles - expect failure
+	manifestFiles, err = getManifestFiles(manifestPathDirectoryError)
 	assert.Nil(t, err)
-	err = safeguards.ValidateManifests(ctx, testFS, manifests)
+	err = safeguards.ValidateManifests(ctx, manifestFiles)
 	assert.NotNil(t, err)
 
 	// Scenario 3a: manifest path leads to one manifest file - expect success
-	manifests = []string{}
-	manifests = append(manifests, manifestPathFileSuccess)
-	err = safeguards.ValidateManifests(ctx, testFS, manifests)
+	manifestFiles = []string{}
+	manifestFiles = append(manifestFiles, manifestPathFileSuccess)
+	err = safeguards.ValidateManifests(ctx, manifestFiles)
 	assert.Nil(t, err)
 
 	// Scenario 3b: manifest path leads to one manifest file - expect failure
-	manifests = []string{}
-	manifests = append(manifests, manifestPathFileError)
-	err = safeguards.ValidateManifests(ctx, testFS, manifests)
+	manifestFiles = []string{}
+	manifestFiles = append(manifestFiles, manifestPathFileError)
+	err = safeguards.ValidateManifests(ctx, manifestFiles)
 	assert.NotNil(t, err)
 }
diff --git a/pkg/safeguards/safeguards_test_helpers.go b/pkg/safeguards/safeguards_test_helpers.go
index 02b55cc..c719b6c 100644
--- a/pkg/safeguards/safeguards_test_helpers.go
+++ b/pkg/safeguards/safeguards_test_helpers.go
@@ -2,34 +2,30 @@ package safeguards
 
 import (
 	"context"
-	"embed"
 	"testing"
 
 	constraintclient "github.com/open-policy-agent/frameworks/constraint/pkg/client"
 	"github.com/stretchr/testify/assert"
 )
 
-//go:embed tests
-var testFS embed.FS
-
 func validateTestManifests_Error(ctx context.Context, t *testing.T, c *constraintclient.Client, testFc FileCrawler, testManifestPaths []string) {
 	for _, path := range testManifestPaths {
-		errManifest, err := testFc.ReadManifest(path, testFS)
+		errManifests, err := testFc.ReadManifests(path)
 		assert.Nil(t, err)
 
 		// error case - should throw error
-		err = validateManifest(ctx, c, errManifest)
+		err = validateManifests(ctx, c, errManifests)
 		assert.NotNil(t, err)
 	}
 }
 
 func validateTestManifests_Success(ctx context.Context, t *testing.T, c *constraintclient.Client, testFc FileCrawler, testManifestPaths []string) {
 	for _, path := range testManifestPaths {
-		successManifest, err := testFc.ReadManifest(path, testFS)
+		successManifests, err := testFc.ReadManifests(path)
 		assert.Nil(t, err)
 
 		// success case - should not throw error
-		err = validateManifest(ctx, c, successManifest)
+		err = validateManifests(ctx, c, successManifests)
 		assert.Nil(t, err)
 	}
 }
diff --git a/pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml b/pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml
index 2e04317..abcd64e 100644
--- a/pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml
+++ b/pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml
@@ -5,7 +5,6 @@ metadata:
   labels:
     app: my-app
     environment: production
-    testLabel3: randomlabel
 spec:
   replicas: 3
   selector:
diff --git a/pkg/safeguards/tests/all/error/all-error-manifest.yaml b/pkg/safeguards/tests/all/error/all-error-manifest.yaml
index 2e04317..abcd64e 100644
--- a/pkg/safeguards/tests/all/error/all-error-manifest.yaml
+++ b/pkg/safeguards/tests/all/error/all-error-manifest.yaml
@@ -5,7 +5,6 @@ metadata:
   labels:
     app: my-app
     environment: production
-    testLabel3: randomlabel
 spec:
   replicas: 3
   selector:
diff --git a/pkg/safeguards/tests/all/success/all-success-manifest-copy.yaml b/pkg/safeguards/tests/all/success/all-success-manifest-copy.yaml
index b6f9e8b..dd82bfa 100644
--- a/pkg/safeguards/tests/all/success/all-success-manifest-copy.yaml
+++ b/pkg/safeguards/tests/all/success/all-success-manifest-copy.yaml
@@ -5,7 +5,6 @@ metadata:
   labels:
     app: my-app
     environment: production
-    testLabel3: randomlabel
 spec:
   replicas: 3
   selector:
diff --git a/pkg/safeguards/tests/all/success/all-success-manifest.yaml b/pkg/safeguards/tests/all/success/all-success-manifest.yaml
index b6f9e8b..dd82bfa 100644
--- a/pkg/safeguards/tests/all/success/all-success-manifest.yaml
+++ b/pkg/safeguards/tests/all/success/all-success-manifest.yaml
@@ -5,7 +5,6 @@ metadata:
   labels:
     app: my-app
     environment: production
-    testLabel3: randomlabel
 spec:
   replicas: 3
   selector:
diff --git a/pkg/safeguards/tests/container-allowed-images/CAI-error-manifest.yaml b/pkg/safeguards/tests/container-allowed-images/CAI-error-manifest.yaml
index 90b29f2..797d537 100644
--- a/pkg/safeguards/tests/container-allowed-images/CAI-error-manifest.yaml
+++ b/pkg/safeguards/tests/container-allowed-images/CAI-error-manifest.yaml
@@ -2,12 +2,18 @@ apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: cai-error-manifest
+  labels:
+    app: my-app
+    environment: production
 spec:
   replicas: 3
   selector:
     matchLabels:
       app: my-app
   template:
+    metadata:
+      labels:
+        app: my-app
     spec:
       containers:
         - name: badcontainer
diff --git a/pkg/safeguards/tests/container-allowed-images/CAI-success-manifest.yaml b/pkg/safeguards/tests/container-allowed-images/CAI-success-manifest.yaml
index 7a36573..3d6dcb9 100644
--- a/pkg/safeguards/tests/container-allowed-images/CAI-success-manifest.yaml
+++ b/pkg/safeguards/tests/container-allowed-images/CAI-success-manifest.yaml
@@ -2,12 +2,18 @@ apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: cai-success-manifest
+  labels:
+    app: my-app
+    environment: production
 spec:
   replicas: 3
   selector:
     matchLabels:
       app: my-app
   template:
+    metadata:
+      labels:
+        app: my-app
     spec:
       containers:
         - name: my-container