Merge pull request #336 from viccuad/fix-result-response

fix: Record errored AdmissionReview.Response as errors and not failures
kubewarden · Aug 1, 2024 · 3c6ed1d · 3c6ed1d
2 parents 5138bbf + 998eddd
commit 3c6ed1d
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 21 deletions.
diff --git a/internal/report/report.go b/internal/report/report.go
@@ -141,7 +141,9 @@ func newPolicyReportResult(policy policiesv1.Policy, admissionReview *admissionv
 	// optional. If the policy returns "allowed" to the admissionReview,
 	// the Result field is not checked by Kubernetes.
 	// https://pkg.go.dev/k8s.io/[email protected]/admission/v1#AdmissionResponse
-	if !errored && admissionReview.Response.Result != nil {
+	if admissionReview.Response != nil && admissionReview.Response.Result != nil {
+		// Mesage contains the human-readable error message if Response.Result.Code == 500
+		// or the reason why the policy returned a failure
 		message = admissionReview.Response.Result.Message
 	}
 

diff --git a/internal/report/report_test.go b/internal/report/report_test.go
@@ -116,11 +116,11 @@ func TestNewPolicyReportResult(t *testing.T) {
 	now := metav1.Timestamp{Seconds: time.Now().Unix()}
 
 	tests := []struct {
-		name           string
-		policy         policiesv1.Policy
-		amissionReview *admissionv1.AdmissionReview
-		errored        bool
-		expectedResult *wgpolicy.PolicyReportResult
+		name            string
+		policy          policiesv1.Policy
+		admissionReview *admissionv1.AdmissionReview
+		errored         bool
+		expectedResult  *wgpolicy.PolicyReportResult
 	}{
 		{
 			name: "Validating policy, allowed response",
@@ -139,7 +139,7 @@ func TestNewPolicyReportResult(t *testing.T) {
 					},
 				},
 			},
-			amissionReview: &admissionv1.AdmissionReview{
+			admissionReview: &admissionv1.AdmissionReview{
 				Response: &admissionv1.AdmissionResponse{
 					Allowed: true,
 					Result:  nil,
@@ -181,7 +181,7 @@ func TestNewPolicyReportResult(t *testing.T) {
 					},
 				},
 			},
-			amissionReview: &admissionv1.AdmissionReview{
+			admissionReview: &admissionv1.AdmissionReview{
 				Response: &admissionv1.AdmissionResponse{
 					Allowed: false,
 					Result:  &metav1.Status{Message: "The request was rejected"},
@@ -225,8 +225,16 @@ func TestNewPolicyReportResult(t *testing.T) {
 					},
 				},
 			},
-			amissionReview: nil,
-			errored:        true,
+			admissionReview: &admissionv1.AdmissionReview{
+				Response: &admissionv1.AdmissionResponse{
+					Allowed: true,
+					Result: &metav1.Status{
+						Message: "The server is on vacation",
+						Code:    500,
+					},
+				},
+			},
+			errored: true,
 			expectedResult: &wgpolicy.PolicyReportResult{
 				Source:          policyReportSource,
 				Policy:          "namespaced-policy-namespace-policy-name",
@@ -235,7 +243,7 @@ func TestNewPolicyReportResult(t *testing.T) {
 				Timestamp:       now,
 				Scored:          true,
 				SubjectSelector: &metav1.LabelSelector{},
-				Description:     "",
+				Description:     "The server is on vacation",
 				Properties: map[string]string{
 					propertyPolicyUID:             "policy-uid",
 					propertyPolicyResourceVersion: "1",
@@ -249,7 +257,7 @@ func TestNewPolicyReportResult(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			result := newPolicyReportResult(test.policy, test.amissionReview, test.errored, now)
+			result := newPolicyReportResult(test.policy, test.admissionReview, test.errored, now)
 			assert.Equal(t, test.expectedResult, result)
 		})
 	}

diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go
@@ -342,17 +342,32 @@ func (s *Scanner) auditResource(ctx context.Context, policies []*policies.Policy
 
 			admissionReviewRequest := newAdmissionReview(resource)
 			admissionReviewResponse, responseErr := s.sendAdmissionReviewToPolicyServer(ctx, url, admissionReviewRequest)
+			errored := false
 
-			errored := responseErr != nil
-			if errored {
+			if responseErr != nil {
+				errored = true
 				// log responseErr, will end in PolicyReportResult too
 				log.Error().Err(responseErr).Dict("response", zerolog.Dict().
 					Str("admissionRequest-name", admissionReviewRequest.Request.Name).
 					Str("policy", policy.GetName()).
 					Str("resource", resource.GetName()),
 				).
 					Msg("error sending AdmissionReview to PolicyServer")
-			} else {
+			}
+
+			if admissionReviewResponse.Response.Result != nil &&
+				admissionReviewResponse.Response.Result.Code == 500 {
+				errored = true
+				// log Result.Message, will end in PolicyReportResult too
+				log.Error().Err(errors.New(admissionReviewResponse.Response.Result.Message)).Dict("response", zerolog.Dict().
+					Str("admissionRequest-name", admissionReviewRequest.Request.Name).
+					Str("policy", policy.GetName()).
+					Str("resource", resource.GetName()),
+				).
+					Msg("error evaluating Policy in PolicyServer")
+			}
+
+			if !errored {
 				log.Debug().Dict("response", zerolog.Dict().
 					Str("uid", string(admissionReviewResponse.Response.UID)).
 					Str("policy", policy.GetName()).
@@ -399,6 +414,12 @@ func (s *Scanner) auditResource(ctx context.Context, policies []*policies.Policy
 }
 
 func (s *Scanner) auditClusterResource(ctx context.Context, policies []*policies.Policy, resource unstructured.Unstructured, runUID string) {
+	log.Info().
+		Str("resource", resource.GetName()).
+		Dict("dict", zerolog.Dict().
+			Int("policies-to-evaluate", len(policies)),
+		).Msg("audit clusterwide resource")
+
 	clusterPolicyReport := report.NewClusterPolicyReport(runUID, resource)
 	for _, p := range policies {
 		url := p.PolicyServer
@@ -415,22 +436,37 @@ func (s *Scanner) auditClusterResource(ctx context.Context, policies []*policies
 
 		admissionReviewRequest := newAdmissionReview(resource)
 		admissionReviewResponse, responseErr := s.sendAdmissionReviewToPolicyServer(ctx, url, admissionReviewRequest)
+		errored := false
 
-		errored := responseErr != nil
-		if errored {
+		if responseErr != nil {
+			errored = true
 			// log error, will end in ClusterPolicyReportResult too
 			log.Error().Err(responseErr).Dict("response", zerolog.Dict().
 				Str("admissionRequest name", admissionReviewRequest.Request.Name).
 				Str("policy", policy.GetName()).
 				Str("resource", resource.GetName()),
 			).
 				Msg("error sending AdmissionReview to PolicyServer")
-		} else {
+		}
+
+		if admissionReviewResponse.Response.Result != nil &&
+			admissionReviewResponse.Response.Result.Code == 500 {
+			errored = true
+			// log Result.Message, will end in PolicyReportResult too
+			log.Error().Err(errors.New(admissionReviewResponse.Response.Result.Message)).Dict("response", zerolog.Dict().
+				Str("admissionRequest-name", admissionReviewRequest.Request.Name).
+				Str("policy", policy.GetName()).
+				Str("resource", resource.GetName()),
+			).
+				Msg("error evaluating Policy in PolicyServer")
+		}
+
+		if !errored {
 			log.Debug().Dict("response", zerolog.Dict().
 				Str("uid", string(admissionReviewResponse.Response.UID)).
-				Bool("allowed", admissionReviewResponse.Response.Allowed).
 				Str("policy", policy.GetName()).
-				Str("resource", resource.GetName()),
+				Str("resource", resource.GetName()).
+				Bool("allowed", admissionReviewResponse.Response.Allowed),
 			).
 				Msg("audit review response")
 		}
@@ -483,7 +519,10 @@ func (s *Scanner) sendAdmissionReviewToPolicyServer(ctx context.Context, url *ur
 		return nil, err
 	}
 
-	req, _ := http.NewRequestWithContext(ctx, http.MethodPost, url.String(), bytes.NewBuffer(payload))
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, url.String(), bytes.NewBuffer(payload))
+	if err != nil {
+		return nil, err
+	}
 	req.Header.Add("Content-Type", "application/json")
 
 	res, err := s.httpClient.Do(req)