Repository: Azure/draft
Pull request #257 (2024-03-14 21:11:31)

refactor out logging from validate package
remove logging from the safeguards package


bacongobbler: this makes sense for Helm, but not necessarily for Draft since this is intended as a constantly rolling-forward dev tool.

Patch:
diff --git a/Dockerfile b/Dockerfile
index 53a539f..8d10e5c 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -2,14 +2,15 @@ FROM golang:1.21-alpine
 
 WORKDIR /draft
 
-RUN apk add build-base
+RUN apk add gcc musl-dev python3-dev libffi-dev openssl-dev cargo make
 RUN apk add py3-pip
-RUN apk add python3-dev libffi-dev openssl-dev cargo
 
-RUN python3 -m venv .venv
-ENV VIRTUAL_ENV .venv
-ENV PATH .venv/bin:$PATH
-RUN pip install azure-cli
+RUN python3 -m venv az-cli-env 
+RUN az-cli-env/bin/pip install --upgrade pip
+RUN az-cli-env/bin/pip install --upgrade azure-cli
+RUN az-cli-env/bin/az --version
+
+ENV PATH "$PATH:/draft/az-cli-env/bin"
 
 RUN apk add github-cli
 
diff --git a/cmd/validate.go b/cmd/validate.go
index 5f175b6..0ff087f 100644
--- a/cmd/validate.go
+++ b/cmd/validate.go
@@ -29,7 +29,7 @@ func newValidateCmd() *cobra.Command {
 		Short: "Validates manifests against AKS best practices",
 		Long:  `This command validates manifests against several AKS best practices.`,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			if err := vc.run(); err != nil {
+			if err := vc.run(cmd); err != nil {
 				return err
 			}
 			return nil
@@ -79,8 +79,8 @@ func getManifestFiles(p string) ([]string, error) {
 	return manifestFiles, nil
 }
 
-// run is our entry point to ValidateManifests
-func (vc *validateCmd) run() error {
+// run is our entry point to GetManifestViolations
+func (vc *validateCmd) run(c *cobra.Command) error {
 	if vc.manifestPath == "" {
 		return fmt.Errorf("path to the manifests cannot be empty")
 	}
@@ -106,11 +106,32 @@ func (vc *validateCmd) run() error {
 	}
 
 	log.Debugf("validating manifests")
-	err = safeguards.ValidateManifests(ctx, manifestFiles)
+	manifestViolations, err := safeguards.GetManifestViolations(ctx, manifestFiles)
 	if err != nil {
 		log.Errorf("validating safeguards: %s", err.Error())
 		return err
 	}
 
+	for _, v := range manifestViolations {
+		log.Printf("Analyzing %s for violations", v.Name)
+		manifestViolationsFound := false
+		// returning the full list of violations after each manifest is checked
+		for file, violations := range v.ObjectViolations {
+			log.Printf("  %s:", file)
+			for _, violation := range violations {
+				log.Printf("    %s", violation)
+				manifestViolationsFound = true
+			}
+		}
+		if !manifestViolationsFound {
+			log.Printf("    no violations found.")
+		}
+	}
+
+	if len(manifestViolations) > 0 {
+		c.SilenceUsage = true
+		return fmt.Errorf("violations found in manifests")
+	}
+
 	return nil
 }
diff --git a/pkg/safeguards/safeguards.go b/pkg/safeguards/safeguards.go
index 25d1497..ec5f7f0 100644
--- a/pkg/safeguards/safeguards.go
+++ b/pkg/safeguards/safeguards.go
@@ -3,6 +3,7 @@ package safeguards
 import (
 	"context"
 	"embed"
+
 	api "github.com/open-policy-agent/gatekeeper/v3/apis"
 	log "github.com/sirupsen/logrus"
 
@@ -32,57 +33,62 @@ 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, manifestFiles []string) error {
+type ManifestViolation struct {
+	Name             string              // the name of the manifest
+	ObjectViolations map[string][]string // a map of string object names to slice of string objectViolations
+}
+
+// GetManifestViolations takes in a list of manifest files and returns a slice of ManifestViolation structs
+func GetManifestViolations(ctx context.Context, manifestFiles []string) ([]ManifestViolation, error) {
+	var manifestViolations = make([]ManifestViolation, 0)
+
 	// constraint client instantiation
 	c, err := getConstraintClient()
 	if err != nil {
-		return err
+		return manifestViolations, err
 	}
 
 	// retrieval of templates, constraints, and deployment
 	constraintTemplates, err := fc.ReadConstraintTemplates()
 	if err != nil {
-		return err
+		return manifestViolations, err
 	}
 	constraints, err := fc.ReadConstraints()
 	if err != nil {
-		return err
+		return manifestViolations, err
 	}
 
 	// loading of templates, constraints into constraint client
 	err = loadConstraintTemplates(ctx, c, constraintTemplates)
 	if err != nil {
-		return err
+		return manifestViolations, err
 	}
 	err = loadConstraints(ctx, c, constraints)
 	if err != nil {
-		return err
+		return manifestViolations, err
 	}
 
-	var violations []string
 	for _, m := range manifestFiles {
-		var tempViolations []string
-		manifests, err := fc.ReadManifests(m)
+		var objectViolations map[string][]string
+		objects, err := fc.ReadManifests(m) // read all the objects stored in a single file
 		if err != nil {
-			log.Errorf("reading manifests %s", err.Error())
-			return err
+			log.Errorf("reading objects %s", err.Error())
+			return manifestViolations, err
 		}
 
 		// validation of deployment manifest with constraints, templates loaded
-		tempViolations, err = validateManifests(ctx, c, manifests)
+		objectViolations, err = getObjectViolations(ctx, c, objects)
 		if err != nil {
-			log.Errorf("validating manifests: %s", err.Error())
-			return err
+			log.Errorf("validating objects: %s", err.Error())
+			return manifestViolations, err
+		}
+		if len(objectViolations) > 0 {
+			manifestViolations = append(manifestViolations, ManifestViolation{
+				Name:             m,
+				ObjectViolations: objectViolations,
+			})
 		}
-		violations = append(violations, tempViolations...)
-	}
-
-	// returning the full list of violations after each manifest is checked
-	if len(violations) == 0 {
-		log.Printf("No violations found.")
 	}
 
-	return nil
+	return manifestViolations, nil
 }
diff --git a/pkg/safeguards/safeguards_helpers.go b/pkg/safeguards/safeguards_helpers.go
index 6cb1453..342d2a0 100644
--- a/pkg/safeguards/safeguards_helpers.go
+++ b/pkg/safeguards/safeguards_helpers.go
@@ -158,29 +158,33 @@ func loadConstraints(ctx context.Context, c *constraintclient.Client, constraint
 	return nil
 }
 
-// does validation on manifest based on loaded constraint templates, constraints
-func validateManifests(ctx context.Context, c *constraintclient.Client, manifests []*unstructured.Unstructured) ([]string, error) {
+// getObjectViolations executes validation on manifests based on loaded constraint templates and returns a map of manifest name to list of objectViolations
+func getObjectViolations(ctx context.Context, c *constraintclient.Client, objects []*unstructured.Unstructured) (map[string][]string, 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.
-	var violations []string
-	for _, manifest := range manifests {
-		log.Printf("Reviewing %s...", manifest.GetName())
-		res, err := c.Review(ctx, manifest)
+
+	var violations = make(map[string][]string) // map of object name to slice of objectViolations
+
+	for _, o := range objects {
+		objectViolations := []string{}
+		log.Debugf("Reviewing %s...", o.GetName())
+		res, err := c.Review(ctx, o)
 		if err != nil {
-			return violations, fmt.Errorf("could not review manifests: %w", err)
+			return violations, fmt.Errorf("could not review objects: %w", err)
 		}
 
 		for _, v := range res.ByTarget {
-			log.Printf("Found %d errors", len(v.Results))
 			for _, result := range v.Results {
 				if result.Msg != "" {
-					log.Printf("violation: %s", result.Msg)
-					violations = append(violations, result.Msg)
+					objectViolations = append(objectViolations, result.Msg)
 				}
 			}
 		}
-		fmt.Println("")
+
+		if len(objectViolations) > 0 {
+			violations[o.GetName()] = objectViolations
+		}
 	}
 
 	return violations, nil


Test patch:
diff --git a/cmd/validate_test.go b/cmd/validate_test.go
index af3a6e8..5ed22b4 100644
--- a/cmd/validate_test.go
+++ b/cmd/validate_test.go
@@ -37,36 +37,40 @@ func TestRunValidate(t *testing.T) {
 	manifestPathEmpty := ""
 	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")
+	manifestPathFileSuccess, _ := filepath.Abs("../pkg/safeguards/tests/all/success/all-success-manifest-1.yaml")
+	manifestPathFileError, _ := filepath.Abs("../pkg/safeguards/tests/all/error/all-error-manifest-1.yaml")
 	var manifestFiles []string
 
 	// Scenario 1: empty manifest path should error
 	manifestFiles = append(manifestFiles, manifestPathEmpty)
-	err := safeguards.ValidateManifests(ctx, manifestFiles)
+	_, err := safeguards.GetManifestViolations(ctx, manifestFiles)
 	assert.NotNil(t, err)
 
 	// Scenario 2a: manifest path leads to a directory of manifestFiles - expect success
 	manifestFiles, err = getManifestFiles(manifestPathDirectorySuccess)
 	assert.Nil(t, err)
-	err = safeguards.ValidateManifests(ctx, manifestFiles)
+	v, err := safeguards.GetManifestViolations(ctx, manifestFiles)
 	assert.Nil(t, err)
+	assert.Equal(t, len(v), 0)
 
 	// Scenario 2b: manifest path leads to a directory of manifestFiles - expect failure
 	manifestFiles, err = getManifestFiles(manifestPathDirectoryError)
 	assert.Nil(t, err)
-	err = safeguards.ValidateManifests(ctx, manifestFiles)
-	assert.NotNil(t, err)
+	v, err = safeguards.GetManifestViolations(ctx, manifestFiles)
+	assert.Nil(t, err)
+	assert.Greater(t, len(v), 0)
 
 	// Scenario 3a: manifest path leads to one manifest file - expect success
 	manifestFiles = []string{}
 	manifestFiles = append(manifestFiles, manifestPathFileSuccess)
-	err = safeguards.ValidateManifests(ctx, manifestFiles)
+	v, err = safeguards.GetManifestViolations(ctx, manifestFiles)
 	assert.Nil(t, err)
+	assert.Equal(t, len(v), 0)
 
 	// Scenario 3b: manifest path leads to one manifest file - expect failure
 	manifestFiles = []string{}
 	manifestFiles = append(manifestFiles, manifestPathFileError)
-	err = safeguards.ValidateManifests(ctx, manifestFiles)
-	assert.NotNil(t, err)
+	v, err = safeguards.GetManifestViolations(ctx, manifestFiles)
+	assert.Nil(t, err)
+	assert.Greater(t, len(v), 0)
 }
diff --git a/pkg/safeguards/safeguards_test.go b/pkg/safeguards/safeguards_test.go
index 3362ba3..5d6aecf 100644
--- a/pkg/safeguards/safeguards_test.go
+++ b/pkg/safeguards/safeguards_test.go
@@ -42,8 +42,8 @@ func TestValidateDeployment_ContainerAllowedImages(t *testing.T) {
 	assert.Nil(t, err)
 
 	// validating deployment manifests
-	validateTestManifests_Error(ctx, t, c, testFc, testManifest_CAI.ErrorPaths)
-	validateTestManifests_Success(ctx, t, c, testFc, testManifest_CAI.SuccessPaths)
+	validateTestManifestsFail(ctx, t, c, testFc, testManifest_CAI.ErrorPaths)
+	validateTestManifestsSuccess(ctx, t, c, testFc, testManifest_CAI.SuccessPaths)
 }
 
 // TODO: rich description here
@@ -65,8 +65,8 @@ func TestValidateDeployment_ContainerEnforceProbes(t *testing.T) {
 	assert.Nil(t, err)
 
 	// validating deployment manifests
-	validateTestManifests_Error(ctx, t, c, testFc, testManifest_CEP.ErrorPaths)
-	validateTestManifests_Success(ctx, t, c, testFc, testManifest_CEP.SuccessPaths)
+	validateTestManifestsFail(ctx, t, c, testFc, testManifest_CEP.ErrorPaths)
+	validateTestManifestsSuccess(ctx, t, c, testFc, testManifest_CEP.SuccessPaths)
 }
 
 // // TODO: rich description here
@@ -88,8 +88,8 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 	assert.Nil(t, err)
 
 	// validating deployment manifests
-	validateTestManifests_Error(ctx, t, c, testFc, testManifest_CL.ErrorPaths)
-	validateTestManifests_Success(ctx, t, c, testFc, testManifest_CL.SuccessPaths)
+	validateTestManifestsFail(ctx, t, c, testFc, testManifest_CL.ErrorPaths)
+	validateTestManifestsSuccess(ctx, t, c, testFc, testManifest_CL.SuccessPaths)
 }
 
 //// TODO: rich description here
@@ -111,7 +111,7 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 //	assert.Nil(t, err)
 //
 //	// validating deployment manifests
-//	validateTestManifests_Error(ctx, t, c, testFc, testManifest_CRIP.ErrorPaths)
+//	validateTestManifests_Fail(ctx, t, c, testFc, testManifest_CRIP.ErrorPaths)
 //	validateTestManifests_Success(ctx, t, c, testFc, testManifest_CRIP.SuccessPaths)
 //}
 //
@@ -134,7 +134,7 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 //	assert.Nil(t, err)
 //
 //	// validating deployment manifests
-//	validateTestManifests_Error(ctx, t, c, testFc, testManifest_DBPDB.ErrorPaths)
+//	validateTestManifests_Fail(ctx, t, c, testFc, testManifest_DBPDB.ErrorPaths)
 //	validateTestManifests_Success(ctx, t, c, testFc, testManifest_DBPDB.SuccessPaths)
 //}
 //
@@ -157,7 +157,7 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 //	assert.Nil(t, err)
 //
 //	// validating deployment manifests
-//	validateTestManifests_Error(ctx, t, c, testFc, testManifest_PEA.ErrorPaths)
+//	validateTestManifests_Fail(ctx, t, c, testFc, testManifest_PEA.ErrorPaths)
 //	validateTestManifests_Success(ctx, t, c, testFc, testManifest_PEA.SuccessPaths)
 //}
 //
@@ -180,7 +180,7 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 //	assert.Nil(t, err)
 //
 //	// validating deployment manifests
-//	validateTestManifests_Error(ctx, t, c, testFc, testManifest_RT.ErrorPaths)
+//	validateTestManifests_Fail(ctx, t, c, testFc, testManifest_RT.ErrorPaths)
 //	validateTestManifests_Success(ctx, t, c, testFc, testManifest_RT.SuccessPaths)
 //}
 //
@@ -203,7 +203,7 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 //	assert.Nil(t, err)
 //
 //	// validating deployment manifests
-//	validateTestManifests_Error(ctx, t, c, testFc, testManifest_USS.ErrorPaths)
+//	validateTestManifests_Fail(ctx, t, c, testFc, testManifest_USS.ErrorPaths)
 //	validateTestManifests_Success(ctx, t, c, testFc, testManifest_USS.SuccessPaths)
 //}
 //
@@ -234,6 +234,6 @@ func TestValidateDeployment_ContainerLimits(t *testing.T) {
 //	assert.Nil(t, err)
 //
 //	// validating deployment manifests
-//	validateTestManifests_Error(ctx, t, c, testFc, testManifest_all.ErrorPaths)
+//	validateTestManifests_Fail(ctx, t, c, testFc, testManifest_all.ErrorPaths)
 //	validateTestManifests_Success(ctx, t, c, testFc, testManifest_all.SuccessPaths)
 //}
diff --git a/pkg/safeguards/safeguards_test_helpers.go b/pkg/safeguards/safeguards_test_helpers.go
index f7ad448..3f255f7 100644
--- a/pkg/safeguards/safeguards_test_helpers.go
+++ b/pkg/safeguards/safeguards_test_helpers.go
@@ -8,26 +8,26 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func validateTestManifests_Error(ctx context.Context, t *testing.T, c *constraintclient.Client, testFc FileCrawler, testManifestPaths []string) {
+func validateTestManifestsFail(ctx context.Context, t *testing.T, c *constraintclient.Client, testFc FileCrawler, testManifestPaths []string) {
 	for _, path := range testManifestPaths {
 		errManifests, err := testFc.ReadManifests(path)
 		assert.Nil(t, err)
 
 		// error case - should throw error
-		violations, err := validateManifests(ctx, c, errManifests)
-		assert.NotNil(t, err)
-		assert.NotNil(t, violations)
+		violations, err := getObjectViolations(ctx, c, errManifests)
+		assert.Nil(t, err)
+		assert.Greater(t, len(violations), 0)
 	}
 }
 
-func validateTestManifests_Success(ctx context.Context, t *testing.T, c *constraintclient.Client, testFc FileCrawler, testManifestPaths []string) {
+func validateTestManifestsSuccess(ctx context.Context, t *testing.T, c *constraintclient.Client, testFc FileCrawler, testManifestPaths []string) {
 	for _, path := range testManifestPaths {
 		successManifests, err := testFc.ReadManifests(path)
 		assert.Nil(t, err)
 
 		// success case - should not throw error
-		violations, err := validateManifests(ctx, c, successManifests)
+		violations, err := getObjectViolations(ctx, c, successManifests)
 		assert.Nil(t, err)
-		assert.Nil(t, violations)
+		assert.Equal(t, 0, len(violations))
 	}
 }
diff --git a/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml b/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml
index 6698c03..6c5843d 100644
--- a/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml
+++ b/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml
@@ -28,6 +28,10 @@ spec:
       containers:
       - name: my-container
         image: nginx:latest
+        resources:
+        limits:
+          cpu: "200m"
+          memory: "1Gi"
         livenessProbe:
           httpGet:
             path: /healthz
diff --git a/pkg/safeguards/tests/all/success/all-success-manifest-2.yaml b/pkg/safeguards/tests/all/success/all-success-manifest-2.yaml
index db7a287..8aa4771 100644
--- a/pkg/safeguards/tests/all/success/all-success-manifest-2.yaml
+++ b/pkg/safeguards/tests/all/success/all-success-manifest-2.yaml
@@ -28,6 +28,10 @@ spec:
       containers:
       - name: my-container
         image: nginx:latest
+        resources:
+        limits:
+          cpu: "200m"
+          memory: "1Gi"
         livenessProbe:
           httpGet:
             path: /healthz
diff --git a/pkg/safeguards/tests/container-enforce-probes/CEP-success-manifest.yaml b/pkg/safeguards/tests/container-enforce-probes/CEP-success-manifest.yaml
index 1df2f37..f61019b 100644
--- a/pkg/safeguards/tests/container-enforce-probes/CEP-success-manifest.yaml
+++ b/pkg/safeguards/tests/container-enforce-probes/CEP-success-manifest.yaml
@@ -24,7 +24,16 @@ spec:
               path: /healthz
               port: 8080
               httpHeaders:
-                - name: my-probe
-                  value: awesome
+              - name: my-liveness-probe
+                value: awesome
+            initialDelaySeconds: 3
+            periodSeconds: 3
+          readinessProbe:
+            httpGet:
+              path: /healthz
+              port: 8080
+              httpHeaders:
+              - name: my-readiness-probe
+                value: awesome
             initialDelaySeconds: 3
             periodSeconds: 3
\ No newline at end of file
diff --git a/pkg/safeguards/tests/container-limits/CL-success-manifest.yaml b/pkg/safeguards/tests/container-limits/CL-success-manifest.yaml
index 1791d81..439fc0f 100644
--- a/pkg/safeguards/tests/container-limits/CL-success-manifest.yaml
+++ b/pkg/safeguards/tests/container-limits/CL-success-manifest.yaml
@@ -6,3 +6,7 @@ spec:
   containers:
     - name: my-container
       image: nginx:latest
+      resources:
+        limits:
+          cpu: "200m"
+          memory: "1Gi"