Repository: Azure/draft
Pull request #255 (2024-03-13 19:06:59)

Changing violations to manifests to not be errors alongside other formatting changes
# 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)
- [ ] 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



radu-matei: The Helm release list request contains:

```proto
// ListReleasesRequest requests a list of releases.
//
// Releases can be retrieved in chunks by setting limit and offset.
//
// Releases can be sorted according to a few pre-determined sort stategies.
message ListReleasesRequest {
	// Limit is the maximum number of releases to be returned.
	int64 limit  = 1;

	// Offset is the last release name that was seen. The next listing
	// operation will start with the name after this one.
	// Example: If list one returns albert, bernie, carl, and sets 'next: dennis'.
	// dennis is the offset. Supplying 'dennis' for the next request should
	// cause the next batch to return a set of results starting with 'dennis'.
	string offset = 2;

	// SortBy is the sort field that the ListReleases server should sort data before returning.
	ListSort.SortBy sort_by = 3;

	// Filter is a regular expression used to filter which releases should be listed.
	//
	// Anything that matches the regexp will be included in the results.
	string filter = 4;

	// SortOrder is the ordering directive used for sorting.
	ListSort.SortOrder sort_order = 5;

	repeated hapi.release.Status.Code status_codes = 6;
	// Namespace is the filter to select releases only from a specific namespace.
	string namespace = 7;
}
```

What should `draft list` request contain, and what should the response be like?
I can model it after the Helm service, but it would be helpful to understand what you have in mind.

Cheers!
radu-matei: Is this still on the backlog and something you want implemented?
Thanks!
bacongobbler: yes!

Patch:
diff --git a/pkg/safeguards/lib/v1.0.0/container-allowed-images/constraint.yaml b/pkg/safeguards/lib/v1.0.0/container-allowed-images/constraint.yaml
index dd757e5..6aba820 100644
--- a/pkg/safeguards/lib/v1.0.0/container-allowed-images/constraint.yaml
+++ b/pkg/safeguards/lib/v1.0.0/container-allowed-images/constraint.yaml
@@ -9,4 +9,4 @@ spec:
         kinds: ["Deployment"]
   parameters:
     imageRegex: "nginx:latest"
-    excludedContainers: ["my-container"]
\ No newline at end of file
+    excludedContainers: []
\ No newline at end of file
diff --git a/pkg/safeguards/lib/v1.0.0/container-enforce-probes/constraint.yaml b/pkg/safeguards/lib/v1.0.0/container-enforce-probes/constraint.yaml
index d950832..927f4c1 100644
--- a/pkg/safeguards/lib/v1.0.0/container-enforce-probes/constraint.yaml
+++ b/pkg/safeguards/lib/v1.0.0/container-enforce-probes/constraint.yaml
@@ -7,7 +7,9 @@ spec:
     kinds:
       - apiGroups: ["apps"]
         kinds: ["Deployment"]
+      - apiGroups: [""]
+        kinds: ["Pod"]
   parameters:
-    enforceProbes : ["my-probe"]
-    excludedContainers: ["my-container"]
-    excludedImages: ["nginx:latest"]
\ No newline at end of file
+    enforceProbes : ["livenessProbe", "readinessProbe"]
+    excludedContainers: []
+    excludedImages: []
diff --git a/pkg/safeguards/lib/v1.0.0/container-enforce-probes/template.yaml b/pkg/safeguards/lib/v1.0.0/container-enforce-probes/template.yaml
index 0f9e9e9..de51ce9 100644
--- a/pkg/safeguards/lib/v1.0.0/container-enforce-probes/template.yaml
+++ b/pkg/safeguards/lib/v1.0.0/container-enforce-probes/template.yaml
@@ -67,6 +67,11 @@ spec:
         input_containers[c] {
           c := input.review.object.spec.template.spec.containers[_]
         }
+        
+        input_containers[c] {
+          c := input.review.object.spec.containers[_]
+        }
+
         input_container_excluded(field) {
           field == input.parameters.excludedContainers[_]
         }
@@ -87,4 +92,4 @@ spec:
               endswith(exclusion, "*")
               prefix := trim_suffix(exclusion, "*")
               startswith(img, prefix)
-          }
\ No newline at end of file
+          }
diff --git a/pkg/safeguards/lib/v1.0.0/container-limits/constraint.yaml b/pkg/safeguards/lib/v1.0.0/container-limits/constraint.yaml
index cc6f14e..d84442d 100644
--- a/pkg/safeguards/lib/v1.0.0/container-limits/constraint.yaml
+++ b/pkg/safeguards/lib/v1.0.0/container-limits/constraint.yaml
@@ -10,5 +10,5 @@ spec:
   parameters:
     cpuLimit : "200m"
     memoryLimit: "1Gi"
-    excludedContainers: ["my-container"]
-    excludedImages: ["nginx:latest"]
+    excludedContainers: []
+    excludedImages: []
diff --git a/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/constraint.yaml b/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/constraint.yaml
index 8063e5e..397ec3f 100644
--- a/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/constraint.yaml
+++ b/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/constraint.yaml
@@ -7,5 +7,7 @@ spec:
     kinds:
       - apiGroups: [""]
         kinds: ["Pod"]
+      - apiGroups: ["apps"]
+        kinds: ["Deployment"]
   parameters:
-    excludedImages: ["my-container"]
\ No newline at end of file
+    excludedImages: []
diff --git a/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/template.yaml b/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/template.yaml
index f04da23..1b0fef2 100644
--- a/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/template.yaml
+++ b/pkg/safeguards/lib/v1.0.0/container-restricted-image-pulls/template.yaml
@@ -45,6 +45,18 @@ spec:
           c := input.review.object.spec.ephemeralContainers[_]
         }
 
+        input_containers[c] {
+          c := input.review.object.spec.template.spec.containers[_]
+        }
+
+        input_containers[c] {
+          c := input.review.object.spec.template.spec.initContainers[_]
+        }
+
+        input_containers[c] {
+          c := input.review.object.spec.template.spec.ephemeralContainers[_]
+        }
+
         is_excluded(container) {
             exclude_images := object.get(object.get(input, "parameters", {}), "excludedImages", [])
             img := container.image
diff --git a/pkg/safeguards/safeguards.go b/pkg/safeguards/safeguards.go
index 2298a44..25d1497 100644
--- a/pkg/safeguards/safeguards.go
+++ b/pkg/safeguards/safeguards.go
@@ -3,7 +3,6 @@ package safeguards
 import (
 	"context"
 	"embed"
-	"fmt"
 	api "github.com/open-policy-agent/gatekeeper/v3/apis"
 	log "github.com/sirupsen/logrus"
 
@@ -64,24 +63,26 @@ func ValidateManifests(ctx context.Context, manifestFiles []string) error {
 
 	var violations []string
 	for _, m := range manifestFiles {
+		var tempViolations []string
 		manifests, err := fc.ReadManifests(m)
 		if err != nil {
+			log.Errorf("reading manifests %s", err.Error())
 			return err
 		}
 
 		// validation of deployment manifest with constraints, templates loaded
-		err = validateManifests(ctx, c, manifests)
+		tempViolations, err = validateManifests(ctx, c, manifests)
 		if err != nil {
-			violations = append(violations, err.Error())
+			log.Errorf("validating manifests: %s", err.Error())
+			return err
 		}
+		violations = append(violations, tempViolations...)
 	}
 
 	// returning the full list of violations after each manifest is checked
-	if len(violations) > 0 {
-
-		return fmt.Errorf("violations have occurred: %s", violations)
+	if len(violations) == 0 {
+		log.Printf("No violations found.")
 	}
 
-	log.Printf("No violations found.")
 	return nil
 }
diff --git a/pkg/safeguards/safeguards_constants.go b/pkg/safeguards/safeguards_constants.go
index aed566a..e446b53 100644
--- a/pkg/safeguards/safeguards_constants.go
+++ b/pkg/safeguards/safeguards_constants.go
@@ -32,8 +32,10 @@ var selectedVersion = "v1.0.0"
 // TODO: consider getting this from a text file we can bump
 var supportedVersions = []string{selectedVersion}
 
-const templateFileName = "template.yaml"
-const constraintFileName = "constraint.yaml"
+const (
+	templateFileName   = "template.yaml"
+	constraintFileName = "constraint.yaml"
+)
 
 var safeguards = []Safeguard{
 	{
@@ -51,29 +53,29 @@ var safeguards = []Safeguard{
 		templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_CL, templateFileName),
 		constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_CL, constraintFileName),
 	},
-	//{
-	//	name:           Constraint_CRIP,
-	//	templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_CRIP, templateFileName),
-	//	constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_CRIP, constraintFileName),
-	//},
-	//{
-	//	name:           Constraint_DBPDB,
-	//	templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_DBPDB, templateFileName),
-	//	constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_DBPDB, constraintFileName),
-	//},
-	//{
-	//	name:           Constraint_PEA,
-	//	templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_PEA, templateFileName),
-	//	constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_PEA, constraintFileName),
-	//},
-	//{
-	//	name:           Constraint_RT,
-	//	templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_RT, templateFileName),
-	//	constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_RT, constraintFileName),
-	//},
-	//{
-	//	name:           Constraint_USS,
-	//	templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_USS, templateFileName),
-	//	constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_USS, constraintFileName),
-	//},
+	{
+		name:           Constraint_CRIP,
+		templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_CRIP, templateFileName),
+		constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_CRIP, constraintFileName),
+	},
+	{
+		name:           Constraint_DBPDB,
+		templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_DBPDB, templateFileName),
+		constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_DBPDB, constraintFileName),
+	},
+	{
+		name:           Constraint_PEA,
+		templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_PEA, templateFileName),
+		constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_PEA, constraintFileName),
+	},
+	{
+		name:           Constraint_RT,
+		templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_RT, templateFileName),
+		constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_RT, constraintFileName),
+	},
+	{
+		name:           Constraint_USS,
+		templatePath:   fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_USS, templateFileName),
+		constraintPath: fmt.Sprintf("lib/%s/%s/%s", selectedVersion, Constraint_USS, constraintFileName),
+	},
 }
diff --git a/pkg/safeguards/safeguards_helpers.go b/pkg/safeguards/safeguards_helpers.go
index 343f8f4..6cb1453 100644
--- a/pkg/safeguards/safeguards_helpers.go
+++ b/pkg/safeguards/safeguards_helpers.go
@@ -4,13 +4,14 @@ import (
 	"bufio"
 	"context"
 	"fmt"
+	"os"
+
 	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"
@@ -158,25 +159,29 @@ func loadConstraints(ctx context.Context, c *constraintclient.Client, constraint
 }
 
 // does validation on manifest based on loaded constraint templates, constraints
-func validateManifests(ctx context.Context, c *constraintclient.Client, manifests []*unstructured.Unstructured) error {
+func validateManifests(ctx context.Context, c *constraintclient.Client, manifests []*unstructured.Unstructured) ([]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)
 		if err != nil {
-			return fmt.Errorf("could not review manifests: %w", err)
+			return violations, fmt.Errorf("could not review manifests: %w", err)
 		}
 
 		for _, v := range res.ByTarget {
+			log.Printf("Found %d errors", len(v.Results))
 			for _, result := range v.Results {
 				if result.Msg != "" {
-					return fmt.Errorf("manifest error: %s", result.Msg)
+					log.Printf("violation: %s", result.Msg)
+					violations = append(violations, result.Msg)
 				}
 			}
 		}
+		fmt.Println("")
 	}
 
-	return nil
+	return violations, nil
 }


Test patch:
diff --git a/pkg/safeguards/safeguards_test_helpers.go b/pkg/safeguards/safeguards_test_helpers.go
index c719b6c..f7ad448 100644
--- a/pkg/safeguards/safeguards_test_helpers.go
+++ b/pkg/safeguards/safeguards_test_helpers.go
@@ -14,8 +14,9 @@ func validateTestManifests_Error(ctx context.Context, t *testing.T, c *constrain
 		assert.Nil(t, err)
 
 		// error case - should throw error
-		err = validateManifests(ctx, c, errManifests)
+		violations, err := validateManifests(ctx, c, errManifests)
 		assert.NotNil(t, err)
+		assert.NotNil(t, violations)
 	}
 }
 
@@ -25,7 +26,8 @@ func validateTestManifests_Success(ctx context.Context, t *testing.T, c *constra
 		assert.Nil(t, err)
 
 		// success case - should not throw error
-		err = validateManifests(ctx, c, successManifests)
+		violations, err := validateManifests(ctx, c, successManifests)
 		assert.Nil(t, err)
+		assert.Nil(t, violations)
 	}
 }
diff --git a/pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml b/pkg/safeguards/tests/all/error/all-error-manifest-1.yaml
similarity index 85%
rename from pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml
rename to pkg/safeguards/tests/all/error/all-error-manifest-1.yaml
index abcd64e..156e79f 100644
--- a/pkg/safeguards/tests/all/error/all-error-manifest-copy.yaml
+++ b/pkg/safeguards/tests/all/error/all-error-manifest-1.yaml
@@ -1,7 +1,7 @@
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: my-deployment
+  name: my-deployment-1
   labels:
     app: my-app
     environment: production
diff --git a/pkg/safeguards/tests/all/error/all-error-manifest.yaml b/pkg/safeguards/tests/all/error/all-error-manifest-2.yaml
similarity index 85%
rename from pkg/safeguards/tests/all/error/all-error-manifest.yaml
rename to pkg/safeguards/tests/all/error/all-error-manifest-2.yaml
index abcd64e..9428d5b 100644
--- a/pkg/safeguards/tests/all/error/all-error-manifest.yaml
+++ b/pkg/safeguards/tests/all/error/all-error-manifest-2.yaml
@@ -1,7 +1,7 @@
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: my-deployment
+  name: my-deployment-2
   labels:
     app: my-app
     environment: production
diff --git a/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml b/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml
new file mode 100644
index 0000000..6698c03
--- /dev/null
+++ b/pkg/safeguards/tests/all/success/all-success-manifest-1.yaml
@@ -0,0 +1,48 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: my-deployment-1
+  labels:
+    app: my-app
+    environment: production
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      app: my-app
+  template:
+    metadata:
+      labels:
+        app: my-app
+    spec:
+      affinity:
+        podAntiAffinity:
+          requiredDuringSchedulingIgnoredDuringExecution:
+            - labelSelector:
+                matchExpressions:
+                  - key: app
+                    operator: In
+                    values:
+                      - store
+              topologyKey: "kubernetes.io/hostname"
+      containers:
+      - name: my-container
+        image: nginx:latest
+        livenessProbe:
+          httpGet:
+            path: /healthz
+            port: 8080
+            httpHeaders:
+            - 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/all/success/all-success-manifest-2.yaml b/pkg/safeguards/tests/all/success/all-success-manifest-2.yaml
new file mode 100644
index 0000000..db7a287
--- /dev/null
+++ b/pkg/safeguards/tests/all/success/all-success-manifest-2.yaml
@@ -0,0 +1,48 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: my-deployment-2
+  labels:
+    app: my-app
+    environment: production
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      app: my-app
+  template:
+    metadata:
+      labels:
+        app: my-app
+    spec:
+      affinity:
+        podAntiAffinity:
+          requiredDuringSchedulingIgnoredDuringExecution:
+            - labelSelector:
+                matchExpressions:
+                  - key: app
+                    operator: In
+                    values:
+                      - store
+              topologyKey: "kubernetes.io/hostname"
+      containers:
+      - name: my-container
+        image: nginx:latest
+        livenessProbe:
+          httpGet:
+            path: /healthz
+            port: 8080
+            httpHeaders:
+            - 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/all/success/all-success-manifest-copy.yaml b/pkg/safeguards/tests/all/success/all-success-manifest-copy.yaml
deleted file mode 100644
index dd82bfa..0000000
--- a/pkg/safeguards/tests/all/success/all-success-manifest-copy.yaml
+++ /dev/null
@@ -1,20 +0,0 @@
-apiVersion: apps/v1
-kind: Deployment
-metadata:
-  name: my-deployment
-  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
-          image: nginx:latest
\ No newline at end of file
diff --git a/pkg/safeguards/tests/all/success/all-success-manifest.yaml b/pkg/safeguards/tests/all/success/all-success-manifest.yaml
deleted file mode 100644
index dd82bfa..0000000
--- a/pkg/safeguards/tests/all/success/all-success-manifest.yaml
+++ /dev/null
@@ -1,20 +0,0 @@
-apiVersion: apps/v1
-kind: Deployment
-metadata:
-  name: my-deployment
-  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
-          image: nginx:latest
\ No newline at end of file