backend: k8cache: Fix named resource authorization#5719
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harrshita123 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
the
cncf-cla: yes
There was a problem hiding this comment.
Pull request overview
This PR fixes cache authorization for proxied Kubernetes API requests by correctly parsing API paths for named resources and populating SelfSubjectAccessReview ResourceAttributes with the proper resource, name, namespace, API group/version, and subresource—avoiding false “forbidden” responses when caching is enabled.
Changes:
- Added structured parsing of proxied Kubernetes API paths to extract group/version/namespace/resource/name/subresource.
- Updated SSAR creation to use fully populated
authorizationv1.ResourceAttributesinstead of just the last path segment. - Added regression tests for named resources (including subresources) and namespace paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/pkg/k8cache/authorization.go | Introduces API path parsing and builds SSAR ResourceAttributes from parsed components. |
| backend/pkg/k8cache/authorization_test.go | Adds exported-level tests covering named resource paths for GetKindAndVerb. |
| backend/pkg/k8cache/authorization_internal_test.go | Adds white-box tests for getResourceAttributes on named resources/subresources. |
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.
harrshita123
force-pushed
the
issue-5718-cache-authorization
branch
2 times, most recently
from
b3bb5aa to
780471d
Compare
@illume |
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
I noticed the GitHub CI backend test job is failing. Can you please fix the failing tests? You can run cd backend && go test ./... locally to see the errors.
How to run the backend tests
Run cd backend && go test ./... to see all failures. Fix the failing tests and commit the result.
harrshita123
force-pushed
the
issue-5718-cache-authorization
branch
from
780471d to
5dd8da3
Compare
Parse proxied Kubernetes API paths by structure before building cache authorization SelfSubjectAccessReviews. This keeps named object requests from using the object name as the resource and preserves namespace, name, version, group, and subresource attributes.
harrshita123
force-pushed
the
issue-5718-cache-authorization
branch
from
5dd8da3 to
61e321b
Compare
@illume |
| switch parts[0] { | ||
| case "api": | ||
| request.version = parts[1] | ||
| resourceIndex = 2 | ||
| case "apis": | ||
| if len(parts) < 4 { | ||
| return apiResourceRequest{}, false | ||
| } | ||
|
|
||
| request.group = parts[1] | ||
| request.version = parts[2] | ||
| resourceIndex = 3 | ||
| default: | ||
| return apiResourceRequest{}, false | ||
| } |
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Would you mind addressing the open Copilot review comments? Please mark each comment as resolved after addressing it.
Summary
Fixes cache authorization for named Kubernetes resource requests. The cache middleware now builds SelfSubjectAccessReview requests with the Kubernetes resource type, such as pods, instead of incorrectly using the object name, such as nginx.
Related Issue
Fixes #5718
Changes
Steps to Test
Screenshots
Not applicable. Backend-only change.
Notes for the Reviewer
This only changes cache authorization path parsing. The intent is to avoid false forbidden responses when response caching is enabled for named Kubernetes resource requests.