Skip to content

Commit 31792ee

Browse files
committed
fix: Union derived rw and ro claims
Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
1 parent a98e23a commit 31792ee

3 files changed

Lines changed: 74 additions & 2 deletions

File tree

internal/registrar/claims.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,15 @@
1515
package registrar
1616

1717
import (
18+
"cmp"
1819
"fmt"
20+
"slices"
1921
"sort"
2022

2123
apisv1alpha2 "github.com/kcp-dev/sdk/apis/apis/v1alpha2"
2224
"github.com/kubernetes-sigs/kro/pkg/graph"
2325
"k8s.io/apimachinery/pkg/runtime/schema"
26+
"k8s.io/apimachinery/pkg/util/sets"
2427

2528
kropengine "go.opendefense.cloud/krop-controller/internal/engine"
2629
)
@@ -56,6 +59,33 @@ func DeriveClaims(foreign []schema.GroupResource, verbs []string, identity map[s
5659
return claims
5760
}
5861

62+
// mergeClaims merges multiple claim sets and combines the verbs for matching GR.
63+
func mergeClaims(inputs ...[]apisv1alpha2.PermissionClaim) []apisv1alpha2.PermissionClaim {
64+
var all []apisv1alpha2.PermissionClaim
65+
for _, in := range inputs {
66+
all = append(all, in...)
67+
}
68+
slices.SortStableFunc(all, func(i, j apisv1alpha2.PermissionClaim) int {
69+
if g := cmp.Compare(i.Group, j.Group); g != 0 {
70+
return g
71+
}
72+
73+
return cmp.Compare(i.Resource, j.Resource)
74+
})
75+
76+
out := []apisv1alpha2.PermissionClaim{all[0]}
77+
for _, in := range all {
78+
last := out[len(out)-1]
79+
if last.GroupResource != in.GroupResource {
80+
out = append(out, in)
81+
continue
82+
}
83+
last.Verbs = sets.List(sets.New(append(last.Verbs, in.Verbs...)...))
84+
}
85+
86+
return out
87+
}
88+
5989
// validateClaims rejects any foreign (non-core) permissionClaim whose identity
6090
// could not be resolved. A claim with Group != "" but IdentityHash == "" means the
6191
// owning APIExport isn't bound in the provider workspace, so the emitted claim

internal/registrar/claims_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,48 @@ func TestDeriveClaims_CoreAndForeign(t *testing.T) {
5252
}
5353
}
5454

55+
func TestMergeClaims(t *testing.T) {
56+
gr := func(g, r string) apisv1alpha2.GroupResource {
57+
return apisv1alpha2.GroupResource{Group: g, Resource: r}
58+
}
59+
claim := func(g, r string, verbs ...string) apisv1alpha2.PermissionClaim {
60+
return apisv1alpha2.PermissionClaim{GroupResource: gr(g, r), Verbs: verbs}
61+
}
62+
63+
writable := []apisv1alpha2.PermissionClaim{
64+
claim("b.example", "widgets", "get", "create"),
65+
claim("access.opendefense.cloud", "scopes", "get", "create"),
66+
}
67+
external := []apisv1alpha2.PermissionClaim{
68+
claim("access.opendefense.cloud", "scopes", "get"), // overlaps writable
69+
claim("", "configmaps", "get"),
70+
}
71+
72+
got := mergeClaims(writable, external)
73+
74+
wantGRs := []apisv1alpha2.GroupResource{
75+
gr("", "configmaps"),
76+
gr("access.opendefense.cloud", "scopes"),
77+
gr("b.example", "widgets"),
78+
}
79+
gotGRs := make([]apisv1alpha2.GroupResource, len(got))
80+
for i, c := range got {
81+
gotGRs[i] = c.GroupResource
82+
}
83+
if !reflect.DeepEqual(gotGRs, wantGRs) {
84+
t.Fatalf("order = %+v, want %+v", gotGRs, wantGRs)
85+
}
86+
87+
// Overlapping GR keeps the writable (superset) verbs, not the read-only ones.
88+
for _, c := range got {
89+
if c.Group == "access.opendefense.cloud" && c.Resource == "scopes" {
90+
if !reflect.DeepEqual(c.Verbs, []string{"get", "create"}) {
91+
t.Fatalf("scopes verbs = %v, want writable superset [get create]", c.Verbs)
92+
}
93+
}
94+
}
95+
}
96+
5597
func TestValidateClaims(t *testing.T) {
5698
tests := []struct {
5799
name string

internal/registrar/registrar.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,9 @@ func (r *Registrar) Reconcile(ctx context.Context, req reconcile.Request) (recon
178178
// Consumer-target template nodes get full CRUD claims (the engine writes them);
179179
// consumer-target external refs get read-only claims (the engine only reads them).
180180
writableGRs, externalGRs := ForeignConsumerGRs(g, instanceGR, routing)
181-
claims := append(
181+
claims := mergeClaims(
182182
DeriveClaims(writableGRs, claimVerbs, identity),
183-
DeriveClaims(externalGRs, readOnlyVerbs, identity)...,
183+
DeriveClaims(externalGRs, readOnlyVerbs, identity),
184184
)
185185
// A foreign (non-core) claim with no identityHash would not authorize: the
186186
// owning APIExport isn't bound in the provider workspace yet. Fail the publish

0 commit comments

Comments
 (0)