Merge pull request #312 from viccuad/skip-errored

fix: Skip policies with unknown GVR or policy-server URL

internal/policies/client.go

@@ -34,8 +34,10 @@ type Policies struct {
 	PoliciesByGVR map[schema.GroupVersionResource][]*Policy
 	// PolicyNum represents the number of policies
 	PolicyNum int
-	// SkippedNum represents the number of skipped policies
+	// SkippedNum represents the number of skipped policies that don't match audit constraints
 	SkippedNum int
+	// ErroredNum represents the number of errored policies. These policies may be misconfigured
+	ErroredNum int
 }
 
 // Policy represents a policy and the URL of the policy server where it is running
@@ -171,10 +173,12 @@ func (f *Client) getAdmissionPolicies(ctx context.Context, namespace string) ([]
 
 // groupPoliciesByGVRAndLabelSelectorg groups policies by GVR.
 // If namespaced is true, it will skip cluster-wide resources, otherwise it will skip namespaced resources.
+// If the policy targets an unknown GVR or the policy server URL cannot be constructed, the policy will be counted as errored.
 func (f *Client) groupPoliciesByGVR(ctx context.Context, policies []policiesv1.Policy, namespaced bool) (*Policies, error) { //nolint:funlen
 	policiesByGVR := make(map[schema.GroupVersionResource][]*Policy)
 	auditablePolicies := map[string]struct{}{}
 	skippedPolicies := map[string]struct{}{}
+	erroredPolicies := map[string]struct{}{}
 
 	for _, policy := range policies {
 		rules := filterWildcardRules(policy.GetRules())
@@ -201,14 +205,17 @@ func (f *Client) groupPoliciesByGVR(ctx context.Context, policies []policiesv1.P
 
 		groupVersionResources, err := f.getGroupVersionResources(rules, namespaced)
 		if err != nil {
-			return nil, err
+			erroredPolicies[policy.GetUniqueName()] = struct{}{}
+			log.Error().Err(err).Str("policy", policy.GetUniqueName()).Msg("failed to obtain unknown GroupVersion resources. The policy may be misconfigured, skipping as error...")
+			continue
 		}
+
 		if len(groupVersionResources) == 0 {
 			log.
 				Debug().
 				Str("policy", policy.GetUniqueName()).
 				Bool("namespaced", namespaced).
-				Msg("the policy does not target resources with the selected scope")
+				Msg("the policy does not target resources within the selected scope")
 
 			continue
 		}
@@ -229,11 +236,12 @@ func (f *Client) groupPoliciesByGVR(ctx context.Context, policies []policiesv1.P
 
 		url, err := f.getPolicyServerURLRunningPolicy(ctx, policy)
 		if err != nil {
-			return nil, err
+			erroredPolicies[policy.GetUniqueName()] = struct{}{}
+			log.Error().Err(err).Str("policy", policy.GetUniqueName()).Msg("failed to obtain matching policy-server URL, skipping as error...")
+			continue
 		}
 
 		auditablePolicies[policy.GetUniqueName()] = struct{}{}
-
 		policy := &Policy{
 			Policy:       policy,
 			PolicyServer: url,
@@ -248,6 +256,7 @@ func (f *Client) groupPoliciesByGVR(ctx context.Context, policies []policiesv1.P
 		PoliciesByGVR: policiesByGVR,
 		PolicyNum:     len(auditablePolicies),
 		SkippedNum:    len(skippedPolicies),
+		ErroredNum:    len(erroredPolicies),
 	}, nil
 }
 

internal/policies/client_test.go

@@ -173,6 +173,18 @@ func TestGetPoliciesForANamespace(t *testing.T) {
 		}).
 		Build()
 
+	// an AdmissionPolicy with unknown GVR, it should be errored and skipped
+	admissionPolicy5 := testutils.
+		NewAdmissionPolicyFactory().
+		Name("policy9").
+		Namespace("test").
+		Rule(admissionregistrationv1.Rule{
+			APIGroups:   []string{"apps"},
+			APIVersions: []string{"v1"},
+			Resources:   []string{"foo"},
+		}).
+		Build()
+
 	client, err := testutils.NewFakeClient(
 		namespace,
 		policyServer,
@@ -185,6 +197,7 @@ func TestGetPoliciesForANamespace(t *testing.T) {
 		admissionPolicy2,
 		admissionPolicy3,
 		admissionPolicy4,
+		admissionPolicy5,
 	)
 	require.NoError(t, err)
 
@@ -227,6 +240,7 @@ func TestGetPoliciesForANamespace(t *testing.T) {
 		},
 		PolicyNum:  2,
 		SkippedNum: 3,
+		ErroredNum: 1,
 	}
 
 	assert.Equal(t, expectedPolicies, policies)
@@ -339,6 +353,17 @@ func TestGetClusterWidePolicies(t *testing.T) {
 		Namespace("test").
 		Build()
 
+	// a ClusterAdmissionPolicy targeting unknown GVR, it should be errored and skipped
+	clusterAdmissionPolicy7 := testutils.
+		NewClusterAdmissionPolicyFactory().
+		Name("policy8").
+		Rule(admissionregistrationv1.Rule{
+			APIGroups:   []string{""},
+			APIVersions: []string{"v1"},
+			Resources:   []string{"foo"},
+		}).
+		Build()
+
 	client, err := testutils.NewFakeClient(
 		namespace,
 		policyServer,
@@ -350,6 +375,7 @@ func TestGetClusterWidePolicies(t *testing.T) {
 		clusterAdmissionPolicy5,
 		clusterAdmissionPolicy6,
 		admissionPolicy1,
+		clusterAdmissionPolicy7,
 	)
 	require.NoError(t, err)
 
@@ -382,6 +408,7 @@ func TestGetClusterWidePolicies(t *testing.T) {
 		},
 		PolicyNum:  3,
 		SkippedNum: 2,
+		ErroredNum: 1,
 	}
 
 	assert.Equal(t, expectedPolicies, policies)

internal/scanner/scanner.go

@@ -142,19 +142,22 @@ func (s *Scanner) ScanNamespace(ctx context.Context, nsName, runUID string) erro
 	}
 	policies, err := s.policiesClient.GetPoliciesForANamespace(ctx, nsName)
 	if err != nil {
+		log.Error().Err(err).Str("namespace", nsName).Msg("failed to obtain auditable policies")
 		return err
 	}
+
 	log.Info().
 		Str("namespace", nsName).
 		Dict("dict", zerolog.Dict().
 			Int("policies-to-evaluate", policies.PolicyNum).
-			Int("policies-skipped", policies.SkippedNum),
+			Int("policies-skipped", policies.SkippedNum).
+			Int("policies-errored", policies.ErroredNum),
 		).Msg("policy count")
 
 	for gvr, policies := range policies.PoliciesByGVR {
 		pager, err := s.k8sClient.GetResources(gvr, nsName)
 		if err != nil {
-			return err
+			log.Error().Err(err).Str("gvr", gvr.String()).Str("ns", nsName).Msg("failed to get resources")
 		}
 
 		err = pager.EachListItem(ctx, metav1.ListOptions{}, func(obj runtime.Object) error {
@@ -252,6 +255,7 @@ func (s *Scanner) ScanClusterWideResources(ctx context.Context, runUID string) e
 		Dict("dict", zerolog.Dict().
 			Int("policies-to-evaluate", policies.PolicyNum).
 			Int("policies-skipped", policies.SkippedNum).
+			Int("policies-errored", policies.ErroredNum).
 			Int("parallel-resources-audits", s.parallelResourcesAudits),
 		).Msg("cluster admission policies count")
 

internal/scanner/scanner_test.go

@@ -211,6 +211,20 @@ func TestScanAllNamespaces(t *testing.T) {
 		}).
 		Status(policiesv1.PolicyStatusActive).
 		Build()
+
+	// an AdmissionPolicy targeting an unknown GVR, should error and be skipped
+	admissionPolicyUnknownGVR := testutils.
+		NewAdmissionPolicyFactory().
+		Name("policy6").
+		Namespace("namespace1").
+		Rule(admissionregistrationv1.Rule{
+			APIGroups:   []string{"apps"},
+			APIVersions: []string{"v1"},
+			Resources:   []string{"pods"},
+		}).
+		Status(policiesv1.PolicyStatusActive).
+		Build()
+
 	// add a policy report that should be deleted by the scanner
 	oldPolicyReportRunUID := uuid.New().String()
 	oldPolicyReport := testutils.NewPolicyReportFactory().
@@ -245,6 +259,7 @@ func TestScanAllNamespaces(t *testing.T) {
 		admissionPolicy2,
 		admissionPolicy3,
 		admissionPolicy4,
+		admissionPolicyUnknownGVR,
 		clusterAdmissionPolicy,
 		oldPolicyReport,
 	)
@@ -381,6 +396,19 @@ func TestScanClusterWideResources(t *testing.T) {
 		}).
 		Status(policiesv1.PolicyStatusActive).
 		Build()
+
+	// a ClusterAdmissionPolicy targeting an unknown GVR, should error and be skipped
+	clusterAdmissionPolicyUnknownGVR := testutils.
+		NewClusterAdmissionPolicyFactory().
+		Name("policy4").
+		Rule(admissionregistrationv1.Rule{
+			APIGroups:   []string{""},
+			APIVersions: []string{"v1"},
+			Resources:   []string{"foo"},
+		}).
+		Status(policiesv1.PolicyStatusActive).
+		Build()
+
 	// add a policy report that should be deleted by the scanner
 	oldClusterPolicyReportRunUID := uuid.New().String()
 	oldClusterPolicyReport := testutils.NewClusterPolicyReportFactory().
@@ -407,6 +435,7 @@ func TestScanClusterWideResources(t *testing.T) {
 		clusterAdmissionPolicy1,
 		clusterAdmissionPolicy2,
 		clusterAdmissionPolicy3,
+		clusterAdmissionPolicyUnknownGVR,
 		oldClusterPolicyReport,
 	)
 	require.NoError(t, err)