Skip to content

Commit

Permalink
Check for ReferenceGrants from GRPCRoutes to Services (#2337)
Browse files Browse the repository at this point in the history 
Problem: ReferenceGrants that permit GRPCRoutes to reference Services 
in different namespaces are ignored by NGF, effectively preventing all 
cross-namespace routing for GRPC traffic.

Solution: Check for ReferenceGrants from GRPCRoutes to Services 
when validating GRPCRoutes. If a ReferenceGrant exists that permits 
the cross-namespace reference, allow it.
  • Loading branch information
@kate-osborn
kate-osborn authored Aug 7, 2024
1 parent 9840a51 commit 7b5c6ab
Show file tree
Hide file tree
Showing 5 changed files with 266 additions and 65 deletions.
24 changes: 18 additions & 6 deletions internal/mode/static/state/graph/backend_refs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,12 @@ func addBackendRefsToRules(

for refIdx, ref := range rule.RouteBackendRefs {
refPath := field.NewPath("spec").Child("rules").Index(idx).Child("backendRefs").Index(refIdx)
routeNs := route.Source.GetNamespace()

ref, cond := createBackendRef(
ref,
route.Source.GetNamespace(),
refGrantResolver,
routeNs,
refGrantResolver.refAllowedFrom(getRefGrantFromResourceForRoute(route.RouteType, routeNs)),
services,
refPath,
backendTLSPolicies,
Expand Down Expand Up @@ -118,7 +119,7 @@ func addBackendRefsToRules(
func createBackendRef(
ref RouteBackendRef,
sourceNamespace string,
refGrantResolver *referenceGrantResolver,
refGrantResolver func(resource toResource) bool,
services map[types.NamespacedName]*v1.Service,
refPath *field.Path,
backendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy,
Expand Down Expand Up @@ -347,7 +348,7 @@ func verifyIPFamily(npCfg *NginxProxy, svcIPFamily []v1.IPFamily) error {
func validateRouteBackendRef(
ref RouteBackendRef,
routeNs string,
refGrantResolver *referenceGrantResolver,
refGrantResolver func(resource toResource) bool,
path *field.Path,
) (valid bool, cond conditions.Condition) {
// Because all errors cause the same condition but different reasons, we return as soon as we find an error
Expand All @@ -362,7 +363,7 @@ func validateRouteBackendRef(
func validateBackendRef(
ref gatewayv1.BackendRef,
routeNs string,
refGrantResolver *referenceGrantResolver,
refGrantResolver func(toResource toResource) bool,
path *field.Path,
) (valid bool, cond conditions.Condition) {
// Because all errors cause same condition but different reasons, we return as soon as we find an error
Expand All @@ -382,7 +383,7 @@ func validateBackendRef(
if ref.Namespace != nil && string(*ref.Namespace) != routeNs {
refNsName := types.NamespacedName{Namespace: string(*ref.Namespace), Name: string(ref.Name)}

if !refGrantResolver.refAllowed(toService(refNsName), fromHTTPRoute(routeNs)) {
if !refGrantResolver(toService(refNsName)) {
msg := fmt.Sprintf("Backend ref to Service %s not permitted by any ReferenceGrant", refNsName)

return false, staticConds.NewRouteBackendRefRefNotPermitted(msg)
Expand Down Expand Up @@ -428,3 +429,14 @@ func getServicePort(svc *v1.Service, port int32) (v1.ServicePort, error) {

return v1.ServicePort{}, fmt.Errorf("no matching port for Service %s and port %d", svc.Name, port)
}

func getRefGrantFromResourceForRoute(routeType RouteType, routeNs string) fromResource {
switch routeType {
case RouteTypeHTTP:
return fromHTTPRoute(routeNs)
case RouteTypeGRPC:
return fromGRPCRoute(routeNs)
default:
panic(fmt.Errorf("unknown route type %s", routeType))
}
}
124 changes: 69 additions & 55 deletions internal/mode/static/state/graph/backend_refs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,10 @@ import (
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
"sigs.k8s.io/gateway-api/apis/v1alpha2"
"sigs.k8s.io/gateway-api/apis/v1alpha3"
"sigs.k8s.io/gateway-api/apis/v1beta1"

ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
)

Expand Down Expand Up @@ -88,9 +86,9 @@ func TestValidateRouteBackendRef(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
g := NewWithT(t)
resolver := newReferenceGrantResolver(nil)
alwaysTrueRefGrantResolver := func(_ toResource) bool { return true }

valid, cond := validateRouteBackendRef(test.ref, "test", resolver, field.NewPath("test"))
valid, cond := validateRouteBackendRef(test.ref, "test", alwaysTrueRefGrantResolver, field.NewPath("test"))

g.Expect(valid).To(Equal(test.expectedValid))
g.Expect(cond).To(Equal(test.expectedCondition))
Expand All @@ -99,84 +97,57 @@ func TestValidateRouteBackendRef(t *testing.T) {
}

func TestValidateBackendRef(t *testing.T) {
specificRefGrant := &v1beta1.ReferenceGrant{
Spec: v1beta1.ReferenceGrantSpec{
To: []v1beta1.ReferenceGrantTo{
{
Kind: "Service",
Name: helpers.GetPointer[gatewayv1.ObjectName]("service1"),
},
},
From: []v1beta1.ReferenceGrantFrom{
{
Group: gatewayv1.GroupName,
Kind: kinds.HTTPRoute,
Namespace: "test",
},
},
},
}

allInNamespaceRefGrant := specificRefGrant.DeepCopy()
allInNamespaceRefGrant.Spec.To[0].Name = nil
alwaysFalseRefGrantResolver := func(_ toResource) bool { return false }
alwaysTrueRefGrantResolver := func(_ toResource) bool { return true }

tests := []struct {
ref gatewayv1.BackendRef
refGrants map[types.NamespacedName]*v1beta1.ReferenceGrant
refGrantResolver func(resource toResource) bool
expectedCondition conditions.Condition
name string
expectedValid bool
}{
{
name: "normal case",
ref: getNormalRef(),
expectedValid: true,
name: "normal case",
ref: getNormalRef(),
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: true,
},
{
name: "normal case with implicit namespace",
ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef {
backend.Namespace = nil
return backend
}),
expectedValid: true,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: true,
},
{
name: "normal case with implicit kind Service",
ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef {
backend.Kind = nil
return backend
}),
expectedValid: true,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: true,
},
{
name: "normal case with backend ref allowed by specific reference grant",
name: "normal case with backend ref allowed by reference grant",
ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef {
backend.Namespace = helpers.GetPointer[gatewayv1.Namespace]("cross-ns")
return backend
}),
refGrants: map[types.NamespacedName]*v1beta1.ReferenceGrant{
{Namespace: "cross-ns", Name: "rg"}: specificRefGrant,
},
expectedValid: true,
},
{
name: "normal case with backend ref allowed by all-in-namespace reference grant",
ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef {
backend.Namespace = helpers.GetPointer[gatewayv1.Namespace]("cross-ns")
return backend
}),
refGrants: map[types.NamespacedName]*v1beta1.ReferenceGrant{
{Namespace: "cross-ns", Name: "rg"}: allInNamespaceRefGrant,
},
expectedValid: true,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: true,
},
{
name: "invalid group",
ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef {
backend.Group = helpers.GetPointer[gatewayv1.Group]("invalid")
return backend
}),
expectedValid: false,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: false,
expectedCondition: staticConds.NewRouteBackendRefInvalidKind(
`test.group: Unsupported value: "invalid": supported values: "core", ""`,
),
Expand All @@ -187,7 +158,8 @@ func TestValidateBackendRef(t *testing.T) {
backend.Kind = helpers.GetPointer[gatewayv1.Kind]("NotService")
return backend
}),
expectedValid: false,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: false,
expectedCondition: staticConds.NewRouteBackendRefInvalidKind(
`test.kind: Unsupported value: "NotService": supported values: "Service"`,
),
Expand All @@ -198,7 +170,8 @@ func TestValidateBackendRef(t *testing.T) {
backend.Namespace = helpers.GetPointer[gatewayv1.Namespace]("invalid")
return backend
}),
expectedValid: false,
refGrantResolver: alwaysFalseRefGrantResolver,
expectedValid: false,
expectedCondition: staticConds.NewRouteBackendRefRefNotPermitted(
"Backend ref to Service invalid/service1 not permitted by any ReferenceGrant",
),
Expand All @@ -209,7 +182,8 @@ func TestValidateBackendRef(t *testing.T) {
backend.Weight = helpers.GetPointer[int32](-1)
return backend
}),
expectedValid: false,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: false,
expectedCondition: staticConds.NewRouteBackendRefUnsupportedValue(
"test.weight: Invalid value: -1: must be in the range [0, 1000000]",
),
Expand All @@ -220,7 +194,8 @@ func TestValidateBackendRef(t *testing.T) {
backend.Port = nil
return backend
}),
expectedValid: false,
refGrantResolver: alwaysTrueRefGrantResolver,
expectedValid: false,
expectedCondition: staticConds.NewRouteBackendRefUnsupportedValue(
"test.port: Required value: port cannot be nil",
),
Expand All @@ -231,8 +206,7 @@ func TestValidateBackendRef(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
g := NewWithT(t)

resolver := newReferenceGrantResolver(test.refGrants)
valid, cond := validateBackendRef(test.ref, "test", resolver, field.NewPath("test"))
valid, cond := validateBackendRef(test.ref, "test", test.refGrantResolver, field.NewPath("test"))

g.Expect(valid).To(Equal(test.expectedValid))
g.Expect(cond).To(Equal(test.expectedCondition))
Expand Down Expand Up @@ -437,6 +411,7 @@ func TestAddBackendRefsToRulesTest(t *testing.T) {
Name: name,
},
},
RouteType: RouteTypeHTTP,
ParentRefs: sectionNameRefs,
Valid: true,
}
Expand Down Expand Up @@ -1034,7 +1009,7 @@ func TestCreateBackend(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
g := NewWithT(t)

resolver := newReferenceGrantResolver(nil)
alwaysTrueRefGrantResolver := func(_ toResource) bool { return true }

rbr := RouteBackendRef{
test.ref.BackendRef,
Expand All @@ -1043,7 +1018,7 @@ func TestCreateBackend(t *testing.T) {
backend, cond := createBackendRef(
rbr,
sourceNamespace,
resolver,
alwaysTrueRefGrantResolver,
services,
refPath,
policies,
Expand Down Expand Up @@ -1265,3 +1240,42 @@ func TestFindBackendTLSPolicyForService(t *testing.T) {
})
}
}

func TestGetRefGrantFromResourceForRoute(t *testing.T) {
tests := []struct {
name string
routeType RouteType
ns string
expFromResource fromResource
}{
{
name: "HTTPRoute",
routeType: RouteTypeHTTP,
ns: "hr",
expFromResource: fromHTTPRoute("hr"),
},
{
name: "GRPCRoute",
routeType: RouteTypeGRPC,
ns: "gr",
expFromResource: fromGRPCRoute("gr"),
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
g := NewWithT(t)
g.Expect(getRefGrantFromResourceForRoute(test.routeType, test.ns)).To(Equal(test.expFromResource))
})
}
}

func TestGetRefGrantFromResourceForRoute_Panics(t *testing.T) {
g := NewWithT(t)

get := func() {
getRefGrantFromResourceForRoute("unknown", "ns")
}

g.Expect(get).To(Panic())
}
30 changes: 26 additions & 4 deletions internal/mode/static/state/graph/graph_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,9 @@ func TestBuildGraph(t *testing.T) {
},
}

rgService := &v1beta1.ReferenceGrant{
hrToServiceNsRefGrant := &v1beta1.ReferenceGrant{
ObjectMeta: metav1.ObjectMeta{
Name: "rg-service",
Name: "hr-to-service",
Namespace: "service",
},
Spec: v1beta1.ReferenceGrantSpec{
Expand All @@ -327,6 +327,27 @@ func TestBuildGraph(t *testing.T) {
},
}

grToServiceNsRefGrant := &v1beta1.ReferenceGrant{
ObjectMeta: metav1.ObjectMeta{
Name: "gr-to-service",
Namespace: "service",
},
Spec: v1beta1.ReferenceGrantSpec{
From: []v1beta1.ReferenceGrantFrom{
{
Group: gatewayv1.GroupName,
Kind: kinds.GRPCRoute,
Namespace: gatewayv1.Namespace(testNs),
},
},
To: []v1beta1.ReferenceGrantTo{
{
Kind: "Service",
},
},
},
}

proxy := &ngfAPI.NginxProxy{
ObjectMeta: metav1.ObjectMeta{
Name: "nginx-proxy",
Expand Down Expand Up @@ -443,8 +464,9 @@ func TestBuildGraph(t *testing.T) {
client.ObjectKeyFromObject(ns): ns,
},
ReferenceGrants: map[types.NamespacedName]*v1beta1.ReferenceGrant{
client.ObjectKeyFromObject(rgSecret): rgSecret,
client.ObjectKeyFromObject(rgService): rgService,
client.ObjectKeyFromObject(rgSecret): rgSecret,
client.ObjectKeyFromObject(hrToServiceNsRefGrant): hrToServiceNsRefGrant,
client.ObjectKeyFromObject(grToServiceNsRefGrant): grToServiceNsRefGrant,
},
Secrets: map[types.NamespacedName]*v1.Secret{
client.ObjectKeyFromObject(secret): secret,
Expand Down
16 changes: 16 additions & 0 deletions internal/mode/static/state/graph/reference_grant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,14 @@ func fromHTTPRoute(namespace string) fromResource {
}
}

func fromGRPCRoute(namespace string) fromResource {
return fromResource{
group: v1.GroupName,
kind: kinds.GRPCRoute,
namespace: namespace,
}
}

// newReferenceGrantResolver creates a new referenceGrantResolver.
func newReferenceGrantResolver(refGrants map[types.NamespacedName]*v1beta1.ReferenceGrant) *referenceGrantResolver {
allowed := make(map[allowedReference]struct{})
Expand Down Expand Up @@ -137,3 +145,11 @@ func (r *referenceGrantResolver) refAllowed(to toResource, from fromResource) bo

return false
}

// refAllowedFrom returns a closure function that takes a toResource parameter
// and checks if a reference from the specified fromResource to the given toResource is allowed.
func (r *referenceGrantResolver) refAllowedFrom(from fromResource) func(to toResource) bool {
return func(to toResource) bool {
return r.refAllowed(to, from)
}
}
Loading

0 comments on commit 7b5c6ab

Please sign in to comment.