Skip to content

Commit 21cfda0

Browse files
ivanmatmatiivanmatmati
committed
MINOR: replace positional string params in IsAccessGranted with typed structs
IsAccessGranted previously took seven positional string parameters (fromGroup, fromKind, fromNamespace, toGroup, toKind, toNamespace, toName), which the compiler cannot distinguish — an incorrectly ordered call site is a silent bug waiting to happen. Introduce GrantFrom{Group, Kind, Namespace} and GrantTo{Group, Kind, Namespace, Name}, mirroring the From/To triples of the Gateway API spec. IsAccessGranted now accepts these two structs, making each field explicit and compiler-verified at every call site. Move the key-building helpers onto the types themselves as methods: - GrantFrom.ToKey() From - GrantTo.ToKey() To - GrantTo.wildcardKey() To (name-stripped variant for wildcard grant lookup) Replace string concatenation in those methods with strings.Join. Remove the now-redundant package-level ConvertTo/ConvertFrom functions. Update all call sites: gateway_listener.go, route-rule.go, tls-route-rule.go, and reference_grant_manager_test.go. make removeReferenceGrantWithCheck private The method was only ever called from within the tree package — once by UpsertReferenceGrant (check=false) and once by RemoveReferenceGrant (check=true). Exposing it as a public method leaked an implementation detail and invited callers to pass the bool flag directly instead of going through the two clearly-named entry points. Rename to removeReferenceGrantWithCheck and update all internal call sites and comments accordingly.
1 parent 36754be commit 21cfda0

5 files changed

Lines changed: 84 additions & 58 deletions

File tree

‎k8s/gate/tree/gateway_listener.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,9 @@ func (l *Listener) checkCertificateRefs(treeGw *Gateway, gateSecrets map[types.N
280280
continue
281281
}
282282
isAccessGranted := referenceGrantManager.IsAccessGranted(
283-
gatewayv1.GroupName, "Gateway", treeGw.K8sResource.GetNamespace(),
284-
"", "Secret", treeSecret.K8sResource.GetNamespace(), treeSecret.K8sResource.GetName())
283+
GrantFrom{Group: gatewayv1.GroupName, Kind: "Gateway", Namespace: treeGw.K8sResource.GetNamespace()},
284+
GrantTo{Group: "", Kind: "Secret", Namespace: treeSecret.K8sResource.GetNamespace(), Name: treeSecret.K8sResource.GetName()},
285+
)
285286
if !isAccessGranted {
286287
msg := fmt.Sprintf("Secret reference %s/%s is not granted for listener in gateway %s/%s", nsName.Namespace, nsName.Name, treeGw.K8sResource.GetNamespace(), treeGw.K8sResource.GetName())
287288
l.CheckSecret = CheckResult{

‎k8s/gate/tree/reference_grant_manager.go‎

Lines changed: 57 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package tree
22

33
import (
4+
"strings"
5+
46
"github.com/haproxytech/haproxy-unified-gateway/k8s/gate/store"
57
"github.com/haproxytech/haproxy-unified-gateway/k8s/gate/utils"
68
"k8s.io/apimachinery/pkg/types"
@@ -13,6 +15,37 @@ type To string
1315
// From identifies a source resource type as "namespace/group/kind".
1416
type From string
1517

18+
// GrantFrom identifies the source side of a ReferenceGrant check.
19+
type GrantFrom struct {
20+
Group string
21+
Kind string
22+
Namespace string
23+
}
24+
25+
// ToKey converts the descriptor to the internal From key.
26+
func (f GrantFrom) ToKey() From {
27+
return From(strings.Join([]string{f.Namespace, f.Group, f.Kind}, "/"))
28+
}
29+
30+
// GrantTo identifies the target side of a ReferenceGrant check.
31+
type GrantTo struct {
32+
Group string
33+
Kind string
34+
Namespace string
35+
Name string
36+
}
37+
38+
// ToKey converts the descriptor to the internal To key.
39+
// An empty Name represents a wildcard (any resource name).
40+
func (t GrantTo) ToKey() To {
41+
return To(strings.Join([]string{t.Namespace, t.Group, t.Kind, t.Name}, "/"))
42+
}
43+
44+
// wildcardKey returns the To key with an empty name, matching wildcard grants.
45+
func (t GrantTo) wildcardKey() To {
46+
return To(strings.Join([]string{t.Namespace, t.Group, t.Kind, ""}, "/"))
47+
}
48+
1649
// ReferenceGrantNamespacedName is the namespace/name key of a ReferenceGrant.
1750
type ReferenceGrantNamespacedName types.NamespacedName
1851

@@ -33,18 +66,6 @@ type ReferenceGrantManager struct {
3366
toFrom map[To]map[From]struct{}
3467
}
3568

36-
// ConvertTo builds a To key from the target resource's namespace, API group,
37-
// kind, and name. An empty name represents a wildcard (any resource name).
38-
func ConvertTo(namespace string, group string, kind, name string) To {
39-
return To(namespace + "/" + group + "/" + kind + "/" + name)
40-
}
41-
42-
// ConvertFrom builds a From key from the source resource's namespace, API group,
43-
// and kind.
44-
func ConvertFrom(namespace, group, kind string) From {
45-
return From(namespace + "/" + group + "/" + kind)
46-
}
47-
4869
// ConvertReferenceGrantNamespacedName extracts the namespace/name identity of a
4970
// ReferenceGrant from its K8sResource. K8sResource must not be nil.
5071
func ConvertReferenceGrantNamespacedName(referenceGrant ReferenceGrant) ReferenceGrantNamespacedName {
@@ -64,21 +85,18 @@ func NewReferenceGrantManager() *ReferenceGrantManager {
6485
}
6586
}
6687

67-
// IsAccessGranted reports whether a resource of type (fromGroup, fromKind) in
68-
// fromNamespace may reference a resource of type (toGroup, toKind) named toName
69-
// in toNamespace. Same-namespace references are always permitted. Cross-namespace
70-
// access requires a matching entry in toFrom, covering both named grants and
71-
// wildcard grants (empty name).
72-
func (mgr *ReferenceGrantManager) IsAccessGranted(fromGroup, fromKind, fromNamespace,
73-
toGroup, toKind, toNamespace, toName string,
74-
) bool {
88+
// IsAccessGranted reports whether the resource described by from may reference
89+
// the resource described by to. Same-namespace references are always permitted.
90+
// Cross-namespace access requires a matching entry in toFrom, covering both
91+
// named grants and wildcard grants (empty name).
92+
func (mgr *ReferenceGrantManager) IsAccessGranted(from GrantFrom, to GrantTo) bool {
7593
// Same namespace access is always granted
76-
if toNamespace == fromNamespace {
94+
if to.Namespace == from.Namespace {
7795
return true
7896
}
79-
convertedTo := ConvertTo(toNamespace, toGroup, toKind, toName)
80-
convertedToWithoutName := ConvertTo(toNamespace, toGroup, toKind, "")
81-
convertedFrom := ConvertFrom(fromNamespace, fromGroup, fromKind)
97+
convertedTo := to.ToKey()
98+
convertedToWithoutName := to.wildcardKey()
99+
convertedFrom := from.ToKey()
82100
froms := mgr.toFrom[convertedTo]
83101
fromsWithoutName := mgr.toFrom[convertedToWithoutName]
84102
if froms == nil && fromsWithoutName == nil {
@@ -105,20 +123,24 @@ func (mgr *ReferenceGrantManager) UpsertReferenceGrant(referenceGrant ReferenceG
105123

106124
referenceGrantNamespacedName := ConvertReferenceGrantNamespacedName(referenceGrant)
107125
// We remove any previous association
108-
mgr.RemoveReferenceGrantWithCheck(referenceGrant, false)
126+
mgr.removeReferenceGrantWithCheck(referenceGrant, false)
109127
// For each 'To' of the ReferenceGrant
110128
for _, to := range referenceGrant.K8sResource.Spec.To {
111-
convertedTo := ConvertTo(referenceGrant.K8sResource.Namespace,
112-
string(to.Group),
113-
string(to.Kind),
114-
string(utils.PointerDefaultValueIfNil(to.Name)))
129+
convertedTo := (GrantTo{
130+
Namespace: referenceGrant.K8sResource.Namespace,
131+
Group: string(to.Group),
132+
Kind: string(to.Kind),
133+
Name: string(utils.PointerDefaultValueIfNil(to.Name)),
134+
}).ToKey()
115135
// ___________________________
116136
// Update toReferenceGrantFrom
117137
referenceGrantFrom := mgr.toReferenceGrantFrom[convertedTo]
118138
for _, from := range referenceGrant.K8sResource.Spec.From {
119-
convertedFrom := ConvertFrom(string(from.Namespace),
120-
string(from.Group),
121-
string(from.Kind))
139+
convertedFrom := (GrantFrom{
140+
Namespace: string(from.Namespace),
141+
Group: string(from.Group),
142+
Kind: string(from.Kind),
143+
}).ToKey()
122144
// We associate the 'To' with the couple 'ReferenceGrant' and 'From'
123145
if referenceGrantFrom == nil {
124146
// First association so we create the map
@@ -152,12 +174,12 @@ func (mgr *ReferenceGrantManager) UpsertReferenceGrant(referenceGrant ReferenceG
152174
}
153175
}
154176

155-
// RemoveReferenceGrantWithCheck removes all entries for a ReferenceGrant.
177+
// removeReferenceGrantWithCheck removes all entries for a ReferenceGrant.
156178
// When check is true, the function validates that the grant's status is StatusDeleted
157179
// and operates on OldTreeResource (the pre-deletion snapshot). When check is false,
158180
// it operates on the grant as given; this is used by UpsertReferenceGrant to clear
159181
// the previous footprint before reinserting updated rules.
160-
func (mgr *ReferenceGrantManager) RemoveReferenceGrantWithCheck(referenceGrant ReferenceGrant, check bool) {
182+
func (mgr *ReferenceGrantManager) removeReferenceGrantWithCheck(referenceGrant ReferenceGrant, check bool) {
161183
if check && referenceGrant.TreeStatus.Status != store.StatusDeleted {
162184
return
163185
}
@@ -203,9 +225,8 @@ func (mgr *ReferenceGrantManager) RemoveReferenceGrantWithCheck(referenceGrant R
203225
}
204226

205227
// RemoveReferenceGrant removes all entries for a deleted ReferenceGrant.
206-
// It is the public counterpart of RemoveReferenceGrantWithCheck with check=true.
207228
func (mgr *ReferenceGrantManager) RemoveReferenceGrant(referenceGrant ReferenceGrant) {
208-
mgr.RemoveReferenceGrantWithCheck(referenceGrant, true)
229+
mgr.removeReferenceGrantWithCheck(referenceGrant, true)
209230
}
210231

211232
// ComputeToFrom rebuilds the toFrom map from toReferenceGrantFrom. It must be called

‎k8s/gate/tree/reference_grant_manager_test.go‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,11 @@ func TestReferenceGrantManager_WildcardTo(t *testing.T) {
6767
mgr.UpsertReferenceGrant(upserted(k8s))
6868
mgr.ComputeToFrom()
6969

70-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns", "", "Service", "backend-ns", "svc-a"),
70+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc-a"}),
7171
"wildcard grant should permit any named service")
72-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns", "", "Service", "backend-ns", "svc-b"),
72+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc-b"}),
7373
"wildcard grant should permit a different service name")
74-
assert.False(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "other-ns", "", "Service", "backend-ns", "svc-a"),
74+
assert.False(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "other-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc-a"}),
7575
"grant should not cover a route from a different namespace")
7676
}
7777

@@ -99,15 +99,15 @@ func TestReferenceGrantManager_NamedTo(t *testing.T) {
9999
mgr.UpsertReferenceGrant(upserted(k8s))
100100
mgr.ComputeToFrom()
101101

102-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns", "", "Service", "backend-ns", "my-svc"),
102+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "my-svc"}),
103103
"named grant should permit the exact service")
104-
assert.False(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns", "", "Service", "backend-ns", "other-svc"),
104+
assert.False(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "other-svc"}),
105105
"named grant must not permit a different service name")
106106
}
107107

108108
// TestReferenceGrantManager_UpdateWipesPreviousFootprint verifies that upserting a
109109
// modified grant clears the old From entries before inserting the new ones.
110-
// This exercises the RemoveReferenceGrantWithCheck(_, false) call inside Upsert.
110+
// This exercises the removeReferenceGrantWithCheck(_, false) call inside Upsert.
111111
func TestReferenceGrantManager_UpdateWipesPreviousFootprint(t *testing.T) {
112112
mgr := NewReferenceGrantManager()
113113

@@ -129,7 +129,7 @@ func TestReferenceGrantManager_UpdateWipesPreviousFootprint(t *testing.T) {
129129

130130
mgr.UpsertReferenceGrant(upserted(k8sV1))
131131
mgr.ComputeToFrom()
132-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-old", "", "Service", "backend-ns", "svc"))
132+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-old"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc"}))
133133

134134
// Update the grant: only route-ns-new is now authorised.
135135
k8sV2 := &gatewayv1beta1.ReferenceGrant{
@@ -151,9 +151,9 @@ func TestReferenceGrantManager_UpdateWipesPreviousFootprint(t *testing.T) {
151151
mgr.UpsertReferenceGrant(upserted(k8sV2))
152152
mgr.ComputeToFrom()
153153

154-
assert.False(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-old", "", "Service", "backend-ns", "svc"),
154+
assert.False(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-old"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc"}),
155155
"old From must be revoked after update")
156-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-new", "", "Service", "backend-ns", "svc"),
156+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-new"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc"}),
157157
"new From must be allowed after update")
158158
}
159159

@@ -186,16 +186,16 @@ func TestReferenceGrantManager_DeleteMultiToNoLeftovers(t *testing.T) {
186186

187187
assert.Empty(t, mgr.toReferenceGrantFrom, "ToReferenceGrantFrom must be empty after delete")
188188
assert.Empty(t, mgr.referenceGrantsTo, "ReferenceGrantsTo must be empty after delete")
189-
assert.False(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns", "", "Service", "backend-ns", "svc-a"))
190-
assert.False(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns", "", "Service", "backend-ns", "svc-b"))
189+
assert.False(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc-a"}))
190+
assert.False(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "svc-b"}))
191191
}
192192

193193
// TestReferenceGrantManager_SameNamespaceAlwaysGranted verifies that same-namespace
194194
// references are always allowed regardless of whether any grant exists.
195195
func TestReferenceGrantManager_SameNamespaceAlwaysGranted(t *testing.T) {
196196
mgr := NewReferenceGrantManager()
197197

198-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "ns", "", "Service", "ns", "svc"),
198+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "ns"}, GrantTo{Kind: "Service", Namespace: "ns", Name: "svc"}),
199199
"same-namespace access must be permitted with no grants at all")
200200
}
201201

@@ -231,15 +231,15 @@ func TestReferenceGrantManager_TwoGrantsSameTo_DeleteOneKeepsOther(t *testing.T)
231231
mgr.UpsertReferenceGrant(upserted(k8sB))
232232
mgr.ComputeToFrom()
233233

234-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-a", "", "Service", "backend-ns", "shared-svc"))
235-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-b", "", "Service", "backend-ns", "shared-svc"))
234+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-a"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "shared-svc"}))
235+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-b"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "shared-svc"}))
236236

237237
// Delete grant-a only.
238238
mgr.RemoveReferenceGrant(deleted(k8sA))
239239
mgr.ComputeToFrom()
240240

241-
assert.False(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-a", "", "Service", "backend-ns", "shared-svc"),
241+
assert.False(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-a"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "shared-svc"}),
242242
"route-ns-a must lose access after its grant is deleted")
243-
assert.True(t, mgr.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", "route-ns-b", "", "Service", "backend-ns", "shared-svc"),
243+
assert.True(t, mgr.IsAccessGranted(GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: "route-ns-b"}, GrantTo{Kind: "Service", Namespace: "backend-ns", Name: "shared-svc"}),
244244
"route-ns-b must retain access via its own grant")
245245
}

‎k8s/gate/tree/route-rule.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,10 @@ func (r *HTTPRouteRule) checkBackendRef(httpRoute *HTTPRoute, controllerStore Co
6666
// 3- Check if a ReferenceGrant is needed and if so is it valid
6767
backendNs := getNamespace(backendRef.BackendObjectReference.Namespace, httpRoute.K8sResource.Namespace)
6868
accessGranted := backendNs == httpRoute.K8sResource.Namespace ||
69-
referenceGrantManager.IsAccessGranted(gatewayv1.GroupName, "HTTPRoute", httpRoute.K8sResource.Namespace,
70-
"", "Service", backendNs, string(backendRef.BackendObjectReference.Name))
69+
referenceGrantManager.IsAccessGranted(
70+
GrantFrom{Group: gatewayv1.GroupName, Kind: "HTTPRoute", Namespace: httpRoute.K8sResource.Namespace},
71+
GrantTo{Group: "", Kind: "Service", Namespace: backendNs, Name: string(backendRef.BackendObjectReference.Name)},
72+
)
7173
if !accessGranted {
7274
cond := rc.ConditionKORefNotPermitted(utils.BackendObjectReferenceToKey(backendRef.BackendObjectReference))
7375
r.CheckBackendRef.Set(backendRef.BackendObjectReference, CheckResult{

‎k8s/gate/tree/tls-route-rule.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,10 @@ func (r *TLSRouteRule) checkBackendRef(tlsRoute *TLSRoute, controllerStore Contr
6262
// 3- Check if a ReferenceGrant is needed and if so is it valid
6363
backendNs := getNamespace(backendRef.BackendObjectReference.Namespace, tlsRoute.K8sResource.Namespace)
6464
accessGranted := backendNs == tlsRoute.K8sResource.Namespace ||
65-
referenceGrantManager.IsAccessGranted(gatewayv1.GroupName, "TLSRoute", tlsRoute.K8sResource.Namespace,
66-
"", "Service", backendNs, string(backendRef.BackendObjectReference.Name))
65+
referenceGrantManager.IsAccessGranted(
66+
GrantFrom{Group: gatewayv1.GroupName, Kind: "TLSRoute", Namespace: tlsRoute.K8sResource.Namespace},
67+
GrantTo{Group: "", Kind: "Service", Namespace: backendNs, Name: string(backendRef.BackendObjectReference.Name)},
68+
)
6769
if !accessGranted {
6870
cond := rc.ConditionKORefNotPermitted(utils.BackendObjectReferenceToKey(backendRef.BackendObjectReference))
6971
r.CheckBackendRef.Set(backendRef.BackendObjectReference, CheckResult{

0 commit comments

Comments
 (0)