fix: use namespace-scoped watching to avoid cluster-wide LIST permiss… #31610
Conversation
|
@mohsenmottaghi would you be able to write tests for this? |
Sure @mattfarina , I've put together a few tests for different scenarios, like deploying across multiple namespaces, working with limited namespace access, and combining namespace-scoped and cluster-scoped resources. |
464d558 to
0fb7cd7
Compare
|
@mattfarina, Are these tests sufficient and appropriate, or should I consider changing them? |
There was a problem hiding this comment.
Pull request overview
This PR fixes permission issues in Helm's wait and rollback functionality by explicitly setting RESTScopeStrategy to RESTScopeNamespace in the wait() and waitForDelete() functions. This ensures namespace-scoped resources are watched per-namespace instead of cluster-wide, allowing service accounts with only namespace-scoped permissions to successfully use Helm wait operations.
Key Changes:
- Modified
wait()andwaitForDelete()to use namespace-scoped watching strategy - Added comprehensive test coverage with a mock restricted RBAC client
- Tests verify proper behavior across multiple namespaces and with restricted permissions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/kube/statuswait.go | Updated wait() and waitForDelete() to explicitly set RESTScopeStrategy: watcher.RESTScopeNamespace in watcher options |
| pkg/kube/statuswait_test.go | Added extensive test coverage including: multi-namespace tests, restricted RBAC client mock, tests for Wait/WaitForDelete/WatchUntilReady with namespace-scoped and cluster-scoped resources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/kube/statuswait_test.go
Outdated
| go func() { | ||
| time.Sleep(timeUntilDelete) | ||
| err := fakeClient.Tracker().Delete(gvr, u.GetNamespace(), u.GetName()) |
There was a problem hiding this comment.
The goroutine captures the loop variables u and gvr which may cause a race condition or incorrect behavior. The variables should be passed as parameters to the goroutine or copied to local variables before the goroutine starts to ensure each goroutine operates on the correct values.
| go func() { | |
| time.Sleep(timeUntilDelete) | |
| err := fakeClient.Tracker().Delete(gvr, u.GetNamespace(), u.GetName()) | |
| uCopy := u | |
| gvrCopy := gvr | |
| go func() { | |
| time.Sleep(timeUntilDelete) | |
| err := fakeClient.Tracker().Delete(gvrCopy, uCopy.GetNamespace(), uCopy.GetName()) |
There was a problem hiding this comment.
I changed all goroutines to pass loop variables as parameters to the anonymous function, ensuring each goroutine gets its own copy
601a6d6 to
0fb7cd7
Compare
…ions Signed-off-by: Mohsen Mottaghi <[email protected]>
Adding some tests for multi namespace deployment, simulate restrcited rbac access and mixed namespace scope and cluster scope resources Signed-off-by: Mohsen Mottaghi <[email protected]>
add missing coverage for the positive case where cluster-scoped resources (like ClusterRole or Namespace) should work correctly Signed-off-by: Mohsen Mottaghi <[email protected]>
… tests Signed-off-by: Mohsen Mottaghi <[email protected]>
0fb7cd7 to
d6b35ce
Compare
Thanks @TerryHowe , I updated the branch with the latest version from the main branch and also tried to resolve Copilot's code review suggestions. |
TerryHowe
left a comment
There was a problem hiding this comment.
/lgtm
Tests pass for me.
gjenkins8
left a comment
There was a problem hiding this comment.
A chart can create objects in any namespace (and can even create namespaces themselves).
Watching a specific namespace will miss objects created in other namespaces?
Ah, the change here is telling the watch library to only watch namespaces associated with the objects (if I have understood correctly) |
What this PR does / why we need it:
This change explicitly sets
RESTScopeStrategytoRESTScopeNamespacein bothwait()andwaitForDelete()functions. This ensures:waitandrollback-on-failurefunctionalityFixes permission errors like:
I think that maybe some issues, like ref #31526, are related to this problem.
If applicable:
docs neededlabel should be applied if so)