Make checkSARs idempotent

The previous logic would return randomly sorted results.

Signed-off-by: Darren Shepherd <darren@acorn.io>

pkg/apis/internal.acorn.io/v1/appspec.go

@@ -509,6 +509,16 @@ func sortEquals(left, right []string) bool {
 	return slices.Equal(left, right)
 }
 
+func combineSlice(left, right []string) (result []string) {
+	result = append(result, left...)
+	for _, x := range right {
+		if !slices.Contains(left, x) {
+			result = append(result, x)
+		}
+	}
+	return
+}
+
 func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) {
 	var (
 		singleDiff bool
@@ -519,47 +529,47 @@ func canCombine(left, right PolicyRule) (combined PolicyRule, ok bool) {
 		if singleDiff {
 			return combined, false
 		}
-		combined.Verbs = append(combined.Verbs, right.Verbs...)
+		combined.Verbs = combineSlice(combined.Verbs, right.Verbs)
 		singleDiff = true
 	}
 
 	if !sortEquals(combined.APIGroups, right.APIGroups) {
 		if singleDiff {
 			return combined, false
 		}
-		combined.APIGroups = append(combined.APIGroups, right.APIGroups...)
+		combined.APIGroups = combineSlice(combined.APIGroups, right.APIGroups)
 		singleDiff = true
 	}
 
 	if !sortEquals(combined.Resources, right.Resources) {
 		if singleDiff {
 			return combined, false
 		}
-		combined.Resources = append(combined.Resources, right.Resources...)
+		combined.Resources = combineSlice(combined.Resources, right.Resources)
 		singleDiff = true
 	}
 
 	if !sortEquals(combined.ResourceNames, right.ResourceNames) {
 		if singleDiff {
 			return combined, false
 		}
-		combined.ResourceNames = append(combined.ResourceNames, right.ResourceNames...)
+		combined.ResourceNames = combineSlice(combined.ResourceNames, right.ResourceNames)
 		singleDiff = true
 	}
 
 	if !sortEquals(combined.NonResourceURLs, right.NonResourceURLs) {
 		if singleDiff {
 			return combined, false
 		}
-		combined.NonResourceURLs = append(combined.NonResourceURLs, right.NonResourceURLs...)
+		combined.NonResourceURLs = combineSlice(combined.NonResourceURLs, right.NonResourceURLs)
 		singleDiff = true
 	}
 
 	if !sortEquals(combined.Scopes, right.Scopes) {
 		if singleDiff {
 			return combined, false
 		}
-		combined.Scopes = append(combined.Scopes, right.Scopes...)
+		combined.Scopes = combineSlice(combined.Scopes, right.Scopes)
 		singleDiff = true
 	}
 

pkg/apis/internal.acorn.io/v1/appspec_test.go

@@ -57,6 +57,35 @@ func TestSimplify(t *testing.T) {
 		},
 	}))
 
+	autogold.Expect(Permissions{Rules: []PolicyRule{
+		{PolicyRule: rbacv1.PolicyRule{
+			Verbs:     []string{"a", "b"},
+			Resources: []string{"x", "y", "z"},
+		}},
+	}}).Equal(t, Simplify(Permissions{
+		Rules: []PolicyRule{
+			{
+				PolicyRule: rbacv1.PolicyRule{
+					Verbs:     []string{"a"},
+					Resources: []string{"x", "y"},
+				},
+			},
+			{
+				PolicyRule: rbacv1.PolicyRule{
+					Verbs:     []string{"b"},
+					Resources: []string{"x", "y"},
+				},
+			},
+
+			{
+				PolicyRule: rbacv1.PolicyRule{
+					Verbs:     []string{"a", "b"},
+					Resources: []string{"x", "y", "z"},
+				},
+			},
+		},
+	}))
+
 	autogold.Expect(Permissions{Rules: []PolicyRule{
 		{PolicyRule: rbacv1.PolicyRule{
 			Verbs: []string{"a", "b"},

pkg/server/registry/apigroups/acorn/apps/validator.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -299,17 +300,29 @@ func (s *Validator) checkRemoteAccess(ctx context.Context, namespace, image stri
 	return nil
 }
 
+func sarToPolicyRules(sars []sarRequest) []v1.PolicyRule {
+	result := make([]v1.PolicyRule, 0, len(sars))
+	sort.Slice(sars, func(i, j int) bool {
+		return sars[i].Order < sars[j].Order
+	})
+	for _, sar := range sars {
+		result = append(result, sar.Rule)
+	}
+	return result
+}
+
 func (s *Validator) checkSARs(ctx context.Context, sars []sarRequest) (granted, rejected []v1.Permissions, _ error) {
 	var (
 		lock        sync.Mutex
-		grantedMap  = map[string][]v1.PolicyRule{}
-		rejectedMap = map[string][]v1.PolicyRule{}
+		grantedMap  = map[string][]sarRequest{}
+		rejectedMap = map[string][]sarRequest{}
 	)
 
 	eg, ctx := errgroup.WithContext(ctx)
 	eg.SetLimit(5)
-	for _, sar := range sars {
+	for i, sar := range sars {
 		sar := sar
+		sar.Order = i
 		eg.Go(func() error {
 			ok, err := s.check(ctx, &sar.SAR)
 			if err != nil {
@@ -318,9 +331,9 @@ func (s *Validator) checkSARs(ctx context.Context, sars []sarRequest) (granted,
 			lock.Lock()
 			defer lock.Unlock()
 			if ok {
-				grantedMap[sar.ServiceName] = append(grantedMap[sar.ServiceName], sar.Rule)
+				grantedMap[sar.ServiceName] = append(grantedMap[sar.ServiceName], sar)
 			} else {
-				rejectedMap[sar.ServiceName] = append(rejectedMap[sar.ServiceName], sar.Rule)
+				rejectedMap[sar.ServiceName] = append(rejectedMap[sar.ServiceName], sar)
 			}
 			return nil
 		})
@@ -333,14 +346,14 @@ func (s *Validator) checkSARs(ctx context.Context, sars []sarRequest) (granted,
 	for _, key := range typed.SortedKeys(grantedMap) {
 		granted = append(granted, v1.Permissions{
 			ServiceName: key,
-			Rules:       grantedMap[key],
+			Rules:       sarToPolicyRules(grantedMap[key]),
 		})
 	}
 
 	for _, key := range typed.SortedKeys(rejectedMap) {
 		rejected = append(rejected, v1.Permissions{
 			ServiceName: key,
-			Rules:       rejectedMap[key],
+			Rules:       sarToPolicyRules(rejectedMap[key]),
 		})
 	}
 
@@ -359,6 +372,7 @@ type sarRequest struct {
 	SAR         authv1.SubjectAccessReview
 	ServiceName string
 	Rule        v1.PolicyRule
+	Order       int
 }
 
 func (s *Validator) getSARNonResourceRole(sar *authv1.SubjectAccessReview, serviceName string, rule v1.PolicyRule) (result []sarRequest, _ error) {