fix: SecurityPolicy reference grant #5792

kkk777-7 · 2025-04-22T16:53:54Z

What this PR does / why we need it:
Fix reference grant from SecurityPolicy to referenced remoteJWKS backend not respected.

Which issue(s) this PR fixes:

Fixes #5743

Release Notes: Yes

Signed-off-by: kkk777-7 <[email protected]>

internal/provider/kubernetes/controller.go

codecov · 2025-04-23T00:26:25Z

Codecov Report

Attention: Patch coverage is 68.45238% with 53 lines in your changes missing coverage. Please review.

Project coverage is 65.72%. Comparing base (096cb8d) to head (d0fbe14).
Report is 51 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/provider/kubernetes/routes.go	63.63%	18 Missing and 6 partials ⚠️
internal/provider/kubernetes/controller.go	77.92%	17 Missing ⚠️
internal/provider/kubernetes/indexers.go	52.00%	9 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5792      +/-   ##
==========================================
+ Coverage   65.19%   65.72%   +0.52%     
==========================================
  Files         214      220       +6     
  Lines       34321    35101     +780     
==========================================
+ Hits        22377    23070     +693     
- Misses      10591    10611      +20     
- Partials     1353     1420      +67

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kkk777-7 · 2025-04-23T02:16:32Z

/retest

zhaohuabing

LGTM thanks!

Signed-off-by: kkk777-7 <[email protected]>

zhaohuabing · 2025-04-24T00:31:32Z

@kkk777-7 Sorry for the extra churn, but could we reuse the processNonRouteBackendRef for handling route backends in the routes.go as well?

If yes, it should be renamed back to processBackendRef.

Signed-off-by: kkk777-7 <[email protected]>

kkk777-7 · 2025-04-24T01:36:56Z

@zhaohuabing
I have overlooked routes.go, thanks for letting me know!
You are right, so I reverted the function name.

Would it be better to replace routes.go in this PR?

zhaohuabing · 2025-04-24T01:43:16Z

@zhaohuabing I have overlooked routes.go, thanks for letting me know! You are right, so I reverted the function name.

Would it be better to replace routes.go in this PR?

Yes, please also use processBackendRef to handle route backends in this PR. Thanks!

Signed-off-by: kkk777-7 <[email protected]>

kkk777-7 · 2025-04-24T05:20:21Z

@zhaohuabing
I’ve updated it to use processBackendRef to handle the route backends.
When you get a chance, please review it.

zhaohuabing

LGTM thanks!

shawnh2 · 2025-04-26T02:39:46Z

internal/provider/kubernetes/controller.go

-							"name", refGrant.Name)
-					}
-				}
+			if err := r.processBackendRef(


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

Added by the following commit.
b6f83df

shawnh2 · 2025-04-26T02:44:49Z

internal/provider/kubernetes/controller.go

+				resource.KindSecurityPolicy,
+				policy.Namespace,
+				policy.Name,
+				*backendRef); err != nil {


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.

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

shawnh2 · 2025-04-26T02:46:27Z

internal/provider/kubernetes/controller.go

+	ownerKind string,
+	ownerNS string,
+	ownerName string,
+	backendRef gwapiv1.BackendObjectReference,


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

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

shawnh2 · 2025-04-26T02:48:05Z

internal/provider/kubernetes/controller_test.go

@@ -450,3 +589,19 @@ func TestProcessEnvoyExtensionPolicyObjectRefs(t *testing.T) {
 		})
 	}
 }
+
+func setupFakeReconciler(objs []client.Object) *gatewayAPIReconciler {


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

thanks for review, I've renamed setupReferenceGrantReconciler.

Signed-off-by: kkk777-7 <[email protected]>

kkk777-7 · 2025-04-28T10:52:53Z

@zhaohuabing @shawnh2
Sorry, I missed the following fix earlier, so I have added it now. Please review it when you have a moment.
d0fbe14

kkk777-7 added 2 commits April 23, 2025 01:36

fix: SecurityPolicy reference grant

5afa684

Signed-off-by: kkk777-7 <[email protected]>

add: release note

a5272a3

Signed-off-by: kkk777-7 <[email protected]>

kkk777-7 requested a review from a team as a code owner April 22, 2025 16:53

arkodg requested review from zhaohuabing and guydc April 22, 2025 16:55

kkk777-7 commented Apr 22, 2025

View reviewed changes

internal/provider/kubernetes/controller.go Show resolved Hide resolved

zhaohuabing previously approved these changes Apr 23, 2025

View reviewed changes

kkk777-7 added 2 commits April 23, 2025 22:07

update: func name

0033104

Signed-off-by: kkk777-7 <[email protected]>

Merge branch 'main' into fix-rg-sp

b581e80

Signed-off-by: kkk777-7 <[email protected]>

kkk777-7 dismissed zhaohuabing’s stale review via b581e80 April 23, 2025 13:18

revert func name

25ad110

Signed-off-by: kkk777-7 <[email protected]>

update: use processBackendRef to handle route backends

4c3842b

Signed-off-by: kkk777-7 <[email protected]>

zhaohuabing previously approved these changes Apr 24, 2025

View reviewed changes

shawnh2 reviewed Apr 26, 2025

View reviewed changes

kkk777-7 added 3 commits April 28, 2025 19:14

fix: use not pointer type for extAuth backendRef

6bb7c7d

Signed-off-by: kkk777-7 <[email protected]>

Add: testcase for ExtAuth

b6f83df

Signed-off-by: kkk777-7 <[email protected]>

fix: add jwt backendref to backendSecurityPolicyIndexFunc

d0fbe14

Signed-off-by: kkk777-7 <[email protected]>

kkk777-7 dismissed zhaohuabing’s stale review via d0fbe14 April 28, 2025 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SecurityPolicy reference grant #5792

fix: SecurityPolicy reference grant #5792

kkk777-7 commented Apr 22, 2025

codecov bot commented Apr 23, 2025 •

edited

Loading

kkk777-7 commented Apr 23, 2025

zhaohuabing left a comment

zhaohuabing commented Apr 24, 2025

kkk777-7 commented Apr 24, 2025

zhaohuabing commented Apr 24, 2025

kkk777-7 commented Apr 24, 2025

zhaohuabing left a comment

shawnh2 Apr 26, 2025

kkk777-7 Apr 28, 2025

shawnh2 Apr 26, 2025

kkk777-7 Apr 28, 2025 •

edited

Loading

shawnh2 Apr 26, 2025

kkk777-7 Apr 28, 2025

shawnh2 Apr 26, 2025

kkk777-7 Apr 28, 2025

kkk777-7 commented Apr 28, 2025

	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)
	}

fix: SecurityPolicy reference grant #5792

Are you sure you want to change the base?

fix: SecurityPolicy reference grant #5792

Conversation

kkk777-7 commented Apr 22, 2025

codecov bot commented Apr 23, 2025 • edited Loading

Codecov Report

kkk777-7 commented Apr 23, 2025

zhaohuabing left a comment

Choose a reason for hiding this comment

zhaohuabing commented Apr 24, 2025

kkk777-7 commented Apr 24, 2025

zhaohuabing commented Apr 24, 2025

kkk777-7 commented Apr 24, 2025

zhaohuabing left a comment

Choose a reason for hiding this comment

shawnh2 Apr 26, 2025

Choose a reason for hiding this comment

kkk777-7 Apr 28, 2025

Choose a reason for hiding this comment

shawnh2 Apr 26, 2025

Choose a reason for hiding this comment

kkk777-7 Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

shawnh2 Apr 26, 2025

Choose a reason for hiding this comment

kkk777-7 Apr 28, 2025

Choose a reason for hiding this comment

shawnh2 Apr 26, 2025

Choose a reason for hiding this comment

kkk777-7 Apr 28, 2025

Choose a reason for hiding this comment

kkk777-7 commented Apr 28, 2025

codecov bot commented Apr 23, 2025 •

edited

Loading

kkk777-7 Apr 28, 2025 •

edited

Loading