Skip to content

fix: SecurityPolicy reference grant #5792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix: SecurityPolicy reference grant #5792

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5afa684
fix: SecurityPolicy reference grant
kkk777-7 Apr 22, 2025
a5272a3
add: release note
kkk777-7 Apr 22, 2025
0033104
update: func name
kkk777-7 Apr 23, 2025
b581e80
Merge branch 'main' into fix-rg-sp
kkk777-7 Apr 23, 2025
25ad110
revert func name
kkk777-7 Apr 24, 2025
4c3842b
update: use processBackendRef to handle route backends
kkk777-7 Apr 24, 2025
6bb7c7d
fix: use not pointer type for extAuth backendRef
kkk777-7 Apr 28, 2025
b6f83df
Add: testcase for ExtAuth
kkk777-7 Apr 28, 2025
d0fbe14
fix: add jwt backendref to backendSecurityPolicyIndexFunc
kkk777-7 Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 88 additions & 71 deletions internal/provider/kubernetes/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,41 +568,17 @@
}
}
}

backendNamespace := gatewayapi.NamespaceDerefOr(backendRef.Namespace, policy.Namespace)
resourceMap.allAssociatedBackendRefs.Insert(gwapiv1.BackendObjectReference{
Group: backendRef.Group,
Kind: backendRef.Kind,
Namespace: gatewayapi.NamespacePtr(backendNamespace),
Name: backendRef.Name,
})

if backendNamespace != policy.Namespace {
from := ObjectKindNamespacedName{
kind: resource.KindSecurityPolicy,
namespace: policy.Namespace,
name: policy.Name,
}
to := ObjectKindNamespacedName{
kind: gatewayapi.KindDerefOr(backendRef.Kind, resource.KindService),
namespace: backendNamespace,
name: string(backendRef.Name),
}
refGrant, err := r.findReferenceGrant(ctx, from, to)
switch {
case err != nil:
r.log.Error(err, "failed to find ReferenceGrant")
case refGrant == nil:
r.log.Info("no matching ReferenceGrants found", "from", from.kind,
"from namespace", from.namespace, "target", to.kind, "target namespace", to.namespace)
default:
if !resourceMap.allAssociatedReferenceGrants.Has(utils.NamespacedName(refGrant).String()) {
resourceMap.allAssociatedReferenceGrants.Insert(utils.NamespacedName(refGrant).String())
resourceTree.ReferenceGrants = append(resourceTree.ReferenceGrants, refGrant)
r.log.Info("added ReferenceGrant to resource map", "namespace", refGrant.Namespace,
"name", refGrant.Name)
}
}
if err := r.processBackendRef(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add a testcase for ExtAuth in TestProcessSecurityPolicyObjectRefs ? TIA

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added by the following commit.
b6f83df

ctx,
resourceMap,
resourceTree,
resource.KindSecurityPolicy,
policy.Namespace,
policy.Name,
*backendRef); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may panic here if backendRef is a nil pointer, can you also add a testcase to verify this? and should not call processBackendRef anymore if backendRef is a nil pointer.

Copy link
Author

@kkk777-7 kkk777-7 Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review!
You're right. I have fixed the logic where the variable is defined before calling processBackendRef.
6bb7c7d

r.log.Error(err,
"failed to process ExtAuth BackendRef for SecurityPolicy",
"policy", policy, "backendRef", *backendRef)

Check warning on line 581 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L571-L581 

Added lines #L571 - L581 were not covered by tests
}
}

Expand All @@ -625,12 +601,78 @@
}); err != nil {
r.log.Error(err, "failed to process LocalJWKS ConfigMap", "policy", policy, "localJWKS", provider.LocalJWKS)
}
} else if provider.RemoteJWKS != nil {
for _, br := range provider.RemoteJWKS.BackendRefs {
if err := r.processBackendRef(
ctx,
resourceMap,
resourceTree,
resource.KindSecurityPolicy,
policy.Namespace,
policy.Name,
br.BackendObjectReference); err != nil {
r.log.Error(err,
"failed to process RemoteJWKS BackendRef for SecurityPolicy",
"policy", policy, "backendRef", br.BackendObjectReference)
}
}
}
}
}
}
}

// processBackendRef adds the referenced BackendRef to the resourceMap for later processBackendRefs.
// If it exists in a different namespace and there is a ReferenceGrant, adds ReferenceGrant to the resourceTree.
func (r *gatewayAPIReconciler) processBackendRef(
ctx context.Context,
resourceMap *resourceMappings,
resourceTree *resource.Resources,
ownerKind string,
ownerNS string,
ownerName string,
backendRef gwapiv1.BackendObjectReference,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would a pointer type be better? in this way, can directly return if backendRef is a nil pointer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for comment, You're right.

In this case, only the mentioned situation above pass pointer type to function.
In most cases, gwapiv1.BackendObjectReference type is passed to function.

e.g. routes.go

gateway/internal/provider/kubernetes/routes.go

Lines 237 to 253 in d0fbe14

for _, backendRef := range rule.BackendRefs {
if err := validateBackendRef(&backendRef.BackendRef); err != nil {
r.log.Error(err, "invalid backendRef")
continue
}
if err := r.processBackendRef(
ctx,
resourceMap,
resourceTree,
resource.KindHTTPRoute,
httpRoute.Namespace,
httpRoute.Name,
backendRef.BackendObjectReference); err != nil {
r.log.Error(err,
"failed to process BackendRef for HTTPRoute",
"httpRoute", httpRoute, "backendRef", backendRef.BackendObjectReference)
}

https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/shared_types.go#L264-L287

so, fixed in the following commit.
6bb7c7d

) error {
backendNamespace := gatewayapi.NamespaceDerefOr(backendRef.Namespace, ownerNS)
resourceMap.allAssociatedBackendRefs.Insert(gwapiv1.BackendObjectReference{
Group: backendRef.Group,
Kind: backendRef.Kind,
Namespace: gatewayapi.NamespacePtr(backendNamespace),
Name: backendRef.Name,
})

if backendNamespace != ownerNS {
from := ObjectKindNamespacedName{
kind: ownerKind,
namespace: ownerNS,
name: ownerName,
}
to := ObjectKindNamespacedName{
kind: gatewayapi.KindDerefOr(backendRef.Kind, resource.KindService),
namespace: backendNamespace,
name: string(backendRef.Name),
}
refGrant, err := r.findReferenceGrant(ctx, from, to)
switch {
case err != nil:
return fmt.Errorf("failed to find ReferenceGrant: %w", err)

Check warning on line 658 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L657-L658 

Added lines #L657 - L658 were not covered by tests
case refGrant == nil:
return fmt.Errorf(
"no matching ReferenceGrants found: from %s/%s to %s/%s",
from.kind, from.namespace, to.kind, to.namespace)
default:
// RefGrant found
if !resourceMap.allAssociatedReferenceGrants.Has(utils.NamespacedName(refGrant).String()) {
resourceMap.allAssociatedReferenceGrants.Insert(utils.NamespacedName(refGrant).String())
resourceTree.ReferenceGrants = append(resourceTree.ReferenceGrants, refGrant)
r.log.Info("added ReferenceGrant to resource map", "namespace", refGrant.Namespace,
"name", refGrant.Name)
}
}
}
return nil
}

// processOIDCHMACSecret adds the OIDC HMAC Secret to the resourceTree.
// The OIDC HMAC Secret is created by the CertGen job and is used by SecurityPolicy
// to configure OAuth2 filters.
Expand Down Expand Up @@ -2123,42 +2165,17 @@
// Add the referenced BackendRefs and ReferenceGrants in ExtAuth to Maps for later processing
for _, ep := range policy.Spec.ExtProc {
for _, br := range ep.BackendRefs {
backendRef := br.BackendObjectReference

backendNamespace := gatewayapi.NamespaceDerefOr(backendRef.Namespace, policy.Namespace)
resourceMap.allAssociatedBackendRefs.Insert(gwapiv1.BackendObjectReference{
Group: backendRef.Group,
Kind: backendRef.Kind,
Namespace: gatewayapi.NamespacePtr(backendNamespace),
Name: backendRef.Name,
})

if backendNamespace != policy.Namespace {
from := ObjectKindNamespacedName{
kind: resource.KindEnvoyExtensionPolicy,
namespace: policy.Namespace,
name: policy.Name,
}
to := ObjectKindNamespacedName{
kind: gatewayapi.KindDerefOr(backendRef.Kind, resource.KindService),
namespace: backendNamespace,
name: string(backendRef.Name),
}
refGrant, err := r.findReferenceGrant(ctx, from, to)
switch {
case err != nil:
r.log.Error(err, "failed to find ReferenceGrant")
case refGrant == nil:
r.log.Info("no matching ReferenceGrants found", "from", from.kind,
"from namespace", from.namespace, "target", to.kind, "target namespace", to.namespace)
default:
if !resourceMap.allAssociatedReferenceGrants.Has(utils.NamespacedName(refGrant).String()) {
resourceMap.allAssociatedReferenceGrants.Insert(utils.NamespacedName(refGrant).String())
resourceTree.ReferenceGrants = append(resourceTree.ReferenceGrants, refGrant)
r.log.Info("added ReferenceGrant to resource map", "namespace", refGrant.Namespace,
"name", refGrant.Name)
}
}
if err := r.processBackendRef(
ctx,
resourceMap,
resourceTree,
resource.KindEnvoyExtensionPolicy,
policy.Namespace,
policy.Name,
br.BackendObjectReference); err != nil {
r.log.Error(err,
"failed to process ExtProc BackendRef for EnvoyExtensionPolicy",
"policy", policy, "backendRef", br.BackendObjectReference)
}
}
}
Expand Down
179 changes: 167 additions & 12 deletions internal/provider/kubernetes/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,27 +420,166 @@ func TestProcessEnvoyExtensionPolicyObjectRefs(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
// Add objects referenced by test cases.
objs := []client.Object{tc.envoyExtensionPolicy, tc.backend, tc.referenceGrant}

// Create the reconciler.
logger := logging.DefaultLogger(os.Stdout, egv1a1.LogLevelInfo)
r := setupFakeReconciler(objs)

ctx := context.Background()
resourceTree := resource.NewResources()
resourceMap := newResourceMapping()

r := &gatewayAPIReconciler{
log: logger,
classController: "some-gateway-class",
err := r.processEnvoyExtensionPolicies(ctx, resourceTree, resourceMap)
require.NoError(t, err)
if tc.shouldBeAdded {
require.Contains(t, resourceTree.ReferenceGrants, tc.referenceGrant)
} else {
require.NotContains(t, resourceTree.ReferenceGrants, tc.referenceGrant)
}
})
}
}

r.client = fakeclient.NewClientBuilder().
WithScheme(envoygateway.GetScheme()).
WithObjects(objs...).
WithIndex(&gwapiv1b1.ReferenceGrant{}, targetRefGrantRouteIndex, getReferenceGrantIndexerFunc()).
Build()
func TestProcessSecurityPolicyObjectRefs(t *testing.T) {
testCases := []struct {
name string
securityPolicy *egv1a1.SecurityPolicy
backend *egv1a1.Backend
referenceGrant *gwapiv1b1.ReferenceGrant
shouldBeAdded bool
}{
{
name: "valid security policy with proper ref grant to backend",
securityPolicy: &egv1a1.SecurityPolicy{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "test-policy",
},
Spec: egv1a1.SecurityPolicySpec{
JWT: &egv1a1.JWT{
Providers: []egv1a1.JWTProvider{
{
RemoteJWKS: &egv1a1.RemoteJWKS{
BackendCluster: egv1a1.BackendCluster{
BackendRefs: []egv1a1.BackendRef{
{
BackendObjectReference: gwapiv1.BackendObjectReference{
Namespace: gatewayapi.NamespacePtr("ns-2"),
Name: "test-backend",
Kind: gatewayapi.KindPtr(resource.KindBackend),
Group: gatewayapi.GroupPtr(egv1a1.GroupName),
},
},
},
},
},
},
},
},
},
},
backend: &egv1a1.Backend{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-2",
Name: "test-backend",
},
},
referenceGrant: &gwapiv1b1.ReferenceGrant{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-2",
Name: "test-grant",
},
Spec: gwapiv1b1.ReferenceGrantSpec{
From: []gwapiv1b1.ReferenceGrantFrom{
{
Namespace: gwapiv1.Namespace("ns-1"),
Kind: gwapiv1.Kind(resource.KindSecurityPolicy),
Group: gwapiv1.Group(egv1a1.GroupName),
},
},
To: []gwapiv1b1.ReferenceGrantTo{
{
Name: gatewayapi.ObjectNamePtr("test-backend"),
Kind: gwapiv1.Kind(resource.KindBackend),
Group: gwapiv1.Group(egv1a1.GroupName),
},
},
},
},
shouldBeAdded: true,
},
{
name: "valid security policy with wrong namespace ref grant to backend",
securityPolicy: &egv1a1.SecurityPolicy{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "test-policy",
},
Spec: egv1a1.SecurityPolicySpec{
JWT: &egv1a1.JWT{
Providers: []egv1a1.JWTProvider{
{
RemoteJWKS: &egv1a1.RemoteJWKS{
BackendCluster: egv1a1.BackendCluster{
BackendRefs: []egv1a1.BackendRef{
{
BackendObjectReference: gwapiv1.BackendObjectReference{
Namespace: gatewayapi.NamespacePtr("ns-2"),
Name: "test-backend",
Kind: gatewayapi.KindPtr(resource.KindBackend),
Group: gatewayapi.GroupPtr(egv1a1.GroupName),
},
},
},
},
},
},
},
},
},
},
backend: &egv1a1.Backend{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-2",
Name: "test-backend",
},
},
referenceGrant: &gwapiv1b1.ReferenceGrant{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-2",
Name: "test-grant",
},
Spec: gwapiv1b1.ReferenceGrantSpec{
From: []gwapiv1b1.ReferenceGrantFrom{
{
Namespace: gwapiv1.Namespace("ns-invalid"),
Kind: gwapiv1.Kind(resource.KindSecurityPolicy),
Group: gwapiv1.Group(egv1a1.GroupName),
},
},
To: []gwapiv1b1.ReferenceGrantTo{
{
Name: gatewayapi.ObjectNamePtr("test-backend"),
Kind: gwapiv1.Kind(resource.KindBackend),
Group: gwapiv1.Group(egv1a1.GroupName),
},
},
},
},
shouldBeAdded: false,
},
}

for i := range testCases {
tc := testCases[i]
// Run the test cases.
t.Run(tc.name, func(t *testing.T) {
// Add objects referenced by test cases.
objs := []client.Object{tc.securityPolicy, tc.backend, tc.referenceGrant}
r := setupFakeReconciler(objs)

ctx := context.Background()
resourceTree := resource.NewResources()
resourceMap := newResourceMapping()

err := r.processEnvoyExtensionPolicies(ctx, resourceTree, resourceMap)
err := r.processSecurityPolicies(ctx, resourceTree, resourceMap)
require.NoError(t, err)
if tc.shouldBeAdded {
require.Contains(t, resourceTree.ReferenceGrants, tc.referenceGrant)
Expand All @@ -450,3 +589,19 @@ func TestProcessEnvoyExtensionPolicyObjectRefs(t *testing.T) {
})
}
}

func setupFakeReconciler(objs []client.Object) *gatewayAPIReconciler {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: about naming here, prefer setupReferenceGrantReconciler, since it's just for ReferenceGrant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for review, I've renamed setupReferenceGrantReconciler.

logger := logging.DefaultLogger(os.Stdout, egv1a1.LogLevelInfo)

r := &gatewayAPIReconciler{
log: logger,
classController: "some-gateway-class",
}

r.client = fakeclient.NewClientBuilder().
WithScheme(envoygateway.GetScheme()).
WithObjects(objs...).
WithIndex(&gwapiv1b1.ReferenceGrant{}, targetRefGrantRouteIndex, getReferenceGrantIndexerFunc()).
Build()
return r
}
1 change: 1 addition & 0 deletions release-notes/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ bug fixes: |
Fix an issue that SamplingFraction was not working.
Fix kubernetes resources not being deleted when the customized name used.
Do not treat essential resource like namespace as the missing resource while loading from file.
Fix reference grant from SecurityPolicy to referenced remoteJWKS backend not respected.

# Enhancements that improve performance.
performance improvements: |
Expand Down