feat(iam): support accessPolicy parameter for readonly bucket access#3
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an internal helper to map accessPolicy values to IAM actions, validates policies, and uses the computed actions when granting per-bucket IAM access. Tests updated to assert policy-derived actions and to expose the fake filer for IAM inspection. (≤50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @IvanHunters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the IAM capabilities by introducing a configurable 'accessPolicy' parameter for bucket access grants. This allows consumers to request more granular permissions, specifically enabling read-only access to buckets, which improves security and adherence to the principle of least privilege. The change integrates seamlessly with existing functionality while providing robust validation for policy inputs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces support for an accessPolicy parameter, allowing for granular control over bucket access (read-only or read-write). The changes are well-implemented, with clear logic for determining IAM actions based on the policy and comprehensive unit tests covering various scenarios, including invalid policies. The new actionsForPolicy function centralizes the policy logic, improving maintainability. The test suite has been appropriately extended to cover the new functionality.
Add a second BucketAccessClass with -ro suffix and accessPolicy: readonly parameter to both extra and system SeaweedFS chart templates. Ref: seaweedfs/seaweedfs-cosi-driver#3 Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/driver/provisioner_test.go (1)
69-86: Addt.Cleanupto stop the gRPC server
srv.Serve(lis)spawns a goroutine that is never stopped. Each call tonewProv— including one per sub-test inTestDriverGrantBucketAccessPolicy— leaks a server goroutine and its associated listener. This can cause false positives under-raceand inflated resource consumption under-count=N.♻️ Proposed fix
go srv.Serve(lis) +t.Cleanup(func() { srv.Stop() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/driver/provisioner_test.go` around lines 69 - 86, The newProv helper starts a gRPC server (srv) and listener (lis) but never stops them, leaking goroutines; update newProv to register a t.Cleanup that stops the server and closes the listener (e.g. call srv.Stop() or srv.GracefulStop() and lis.Close()) so each test/subtest tears down the grpc.Server and net.Listener; locate the newProv function and add a t.Cleanup(func(){ srv.Stop(); lis.Close() }) (or equivalent) to ensure the provisionerServer/fakeFiler test resources are released.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/driver/provisioner_test.go`:
- Around line 69-86: The newProv helper starts a gRPC server (srv) and listener
(lis) but never stops them, leaking goroutines; update newProv to register a
t.Cleanup that stops the server and closes the listener (e.g. call srv.Stop() or
srv.GracefulStop() and lis.Close()) so each test/subtest tears down the
grpc.Server and net.Listener; locate the newProv function and add a
t.Cleanup(func(){ srv.Stop(); lis.Close() }) (or equivalent) to ensure the
provisionerServer/fakeFiler test resources are released.
|
@IvanHunters thanks, #5 just got merged, could you please rebase? |
IvanHunters
force-pushed
the
feat/access-policy-parameter
branch
from
425c83d to
b69142b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/driver/provisioner.go (1)
197-202:⚠️ Potential issue | 🟠 Major
readonlygrants are silently ineffective when the identity already carries write actions.The action loop only appends missing entries; it never removes existing ones. If
idalready hasWrite:bandTagging:bfrom a prior grant, a subsequentreadonlygrant leaves those actions in place — the identity retains full write access despite the policy. This contradicts the stated contract ofaccessPolicy: readonly.The fix is to replace the per-bucket actions that overlap with the policy's action set instead of accumulating them. A minimal approach: strip any existing
<action>:<bucket>entries that are not in the newactionsslice before appending.🐛 Proposed fix — replace per-bucket actions on re-grant
- for _, a := range actions { - action := fmt.Sprintf("%s:%s", a, bucket) - if !contains(id.Actions, action) { - id.Actions = append(id.Actions, action) - } - } + // Build the desired set for this bucket under the requested policy. + desired := make(map[string]struct{}, len(actions)) + for _, a := range actions { + desired[fmt.Sprintf("%s:%s", a, bucket)] = struct{}{} + } + // Remove any existing per-bucket actions not in the new policy. + kept := id.Actions[:0] + for _, existing := range id.Actions { + parts := strings.SplitN(existing, ":", 2) + if len(parts) == 2 && parts[1] == bucket { + if _, ok := desired[existing]; ok { + kept = append(kept, existing) + } + // else: drop — policy no longer grants this action on this bucket + } else { + kept = append(kept, existing) // different bucket or global action, keep + } + } + // Append any desired actions not already present. + for action := range desired { + if !contains(kept, action) { + kept = append(kept, action) + } + } + id.Actions = kept🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/driver/provisioner.go` around lines 197 - 202, The loop that appends per-bucket actions (using variables actions, bucket and id.Actions) leaves old actions like "Write:<bucket>" when re-granting a readonly policy; instead, first remove from id.Actions any entries matching "<anyAction>:<bucket>" that are not present in the new actions slice, then add the new "<action>:<bucket>" entries as currently done (you can reuse the existing contains helper). Update the code around the actions loop in provisioner.go (where id.Actions is mutated) to filter out stale per-bucket actions before appending so readonly grants replace existing write/tagging entries rather than accumulate.
🧹 Nitpick comments (2)
pkg/driver/provisioner.go (1)
338-349: LGTM — clean policy dispatch.The three-way switch is correct:
readonly→{Read, List},readwrite/""→ full set, anything else →InvalidArgument. The empty-string default aligns with the PR description's "default to readwrite" requirement.One minor nit: the error message on line 347 says
must be "readonly" or "readwrite"but omits that the empty string (i.e., omitting the key entirely) is also accepted. Consider rephrasing to avoid confusion if a consumer receives the error.✏️ Optional wording tweak
- return nil, fmt.Errorf("unsupported accessPolicy %q, must be \"readonly\" or \"readwrite\"", policy) + return nil, fmt.Errorf("unsupported accessPolicy %q; valid values are \"readonly\" and \"readwrite\" (or omit for readwrite default)", policy)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/driver/provisioner.go` around lines 338 - 349, Update the error message in actionsForPolicy to clarify that an empty string (omitting the key) is also accepted; specifically, change the fmt.Errorf call returned in the default case to mention that valid values are "readonly", "readwrite", or the empty string (defaulting to readwrite) so consumers won't be confused when they receive the error.pkg/driver/provisioner_test.go (1)
144-194: Add a re-grant downgrade test case to expose the permission-accumulation bug.The four existing cases only cover fresh grants. The bug in
DriverGrantBucketAccess(actions are never removed on re-grant) is not exercised because every case starts with a cleannewProv. A case that first grantsreadwrite, then re-grantsreadonlyon the same identity, would have caught it.🧪 Suggested additional test case
// After the existing cases, add to TestDriverGrantBucketAccessPolicy: t.Run("downgrade readwrite to readonly", func(t *testing.T) { p, ff := newProv(t) base := &cosispec.DriverGrantBucketAccessRequest{ BucketId: "b", Name: "u", Parameters: map[string]string{"accessPolicy": "readwrite"}, } if _, err := p.DriverGrantBucketAccess(context.Background(), base); err != nil { t.Fatalf("initial grant: %v", err) } req := &cosispec.DriverGrantBucketAccessRequest{ BucketId: "b", Name: "u", Parameters: map[string]string{"accessPolicy": "readonly"}, } if _, err := p.DriverGrantBucketAccess(context.Background(), req); err != nil { t.Fatalf("readonly re-grant: %v", err) } got := iamActions(t, ff, "u") want := []string{"List:b", "Read:b"} if !reflect.DeepEqual(got, want) { t.Errorf("actions after downgrade = %v, want %v", got, want) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/driver/provisioner_test.go` around lines 144 - 194, Add a test that re-grants with lower privileges to exercise the permission-accumulation bug: inside TestDriverGrantBucketAccessPolicy use newProv() to create p and ff, call p.DriverGrantBucketAccess with parameters {"accessPolicy":"readwrite"} for Name "u", then call p.DriverGrantBucketAccess again with {"accessPolicy":"readonly"} for the same Name "u", and finally assert via iamActions(t, ff, "u") that the resulting actions equal only {"List:b","Read:b"} (and fail the test if not); this will reveal if DriverGrantBucketAccess fails to remove actions on downgrade.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/driver/provisioner_test.go`:
- Around line 69-86: newProv currently starts srv.Serve in a goroutine and never
stops it, leaking goroutines and the TCP listener; register a t.Cleanup that
stops the gRPC server and closes the listener (e.g., call srv.GracefulStop() or
srv.Stop() and lis.Close()) so the goroutine running srv.Serve returns and the
socket is released; add this cleanup in newProv (referencing symbols newProv,
srv, lis, provisionerServer, NewProvisionerServer) immediately after starting
the server (or after creating p) to ensure resources are cleaned up when the
test finishes.
---
Outside diff comments:
In `@pkg/driver/provisioner.go`:
- Around line 197-202: The loop that appends per-bucket actions (using variables
actions, bucket and id.Actions) leaves old actions like "Write:<bucket>" when
re-granting a readonly policy; instead, first remove from id.Actions any entries
matching "<anyAction>:<bucket>" that are not present in the new actions slice,
then add the new "<action>:<bucket>" entries as currently done (you can reuse
the existing contains helper). Update the code around the actions loop in
provisioner.go (where id.Actions is mutated) to filter out stale per-bucket
actions before appending so readonly grants replace existing write/tagging
entries rather than accumulate.
---
Nitpick comments:
In `@pkg/driver/provisioner_test.go`:
- Around line 144-194: Add a test that re-grants with lower privileges to
exercise the permission-accumulation bug: inside
TestDriverGrantBucketAccessPolicy use newProv() to create p and ff, call
p.DriverGrantBucketAccess with parameters {"accessPolicy":"readwrite"} for Name
"u", then call p.DriverGrantBucketAccess again with {"accessPolicy":"readonly"}
for the same Name "u", and finally assert via iamActions(t, ff, "u") that the
resulting actions equal only {"List:b","Read:b"} (and fail the test if not);
this will reveal if DriverGrantBucketAccess fails to remove actions on
downgrade.
In `@pkg/driver/provisioner.go`:
- Around line 338-349: Update the error message in actionsForPolicy to clarify
that an empty string (omitting the key) is also accepted; specifically, change
the fmt.Errorf call returned in the default case to mention that valid values
are "readonly", "readwrite", or the empty string (defaulting to readwrite) so
consumers won't be confused when they receive the error.
| func newProv(t *testing.T) (*provisionerServer, *fakeFiler) { | ||
| t.Helper() | ||
|
|
||
| lis, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("listen: %v", err) | ||
| } | ||
| ff := &fakeFiler{} | ||
| srv := grpc.NewServer() | ||
| filer_pb.RegisterSeaweedFilerServer(srv, &fakeFiler{}) | ||
| filer_pb.RegisterSeaweedFilerServer(srv, ff) | ||
| go srv.Serve(lis) | ||
|
|
||
| p, err := NewProvisionerServer("prov", lis.Addr().String(), "", "", grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if err != nil { | ||
| t.Fatalf("init prov: %v", err) | ||
| } | ||
| return p.(*provisionerServer) | ||
| return p.(*provisionerServer), ff | ||
| } |
There was a problem hiding this comment.
Register t.Cleanup to stop the gRPC server and avoid goroutine/socket leaks.
srv.Serve is launched in a goroutine but is never stopped; each call to newProv leaks a goroutine and a TCP listener for the duration of the test binary. Under -count=N or with the race detector this can cause resource-exhaustion flakiness.
🛠️ Proposed fix
go srv.Serve(lis)
+ t.Cleanup(srv.GracefulStop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/driver/provisioner_test.go` around lines 69 - 86, newProv currently
starts srv.Serve in a goroutine and never stops it, leaking goroutines and the
TCP listener; register a t.Cleanup that stops the gRPC server and closes the
listener (e.g., call srv.GracefulStop() or srv.Stop() and lis.Close()) so the
goroutine running srv.Serve returns and the socket is released; add this cleanup
in newProv (referencing symbols newProv, srv, lis, provisionerServer,
NewProvisionerServer) immediately after starting the server (or after creating
p) to ensure resources are cleaned up when the test finishes.
Add actionsForPolicy() to determine IAM actions based on the accessPolicy key in BucketAccessClass.parameters: - "readonly" grants Read and List - "readwrite" or empty (default) grants Read, Write, List, Tagging - invalid values return InvalidArgument error Ref: kubernetes-sigs/container-object-storage-interface#244 Signed-off-by: ohotnikov.ivan <ohotnikov.ivan@e-queo.net>
kvaps
force-pushed
the
feat/access-policy-parameter
branch
from
b69142b to
c2c27ea
Compare
3 participants
What this PR does
Adds support for the
accessPolicykey inBucketAccessClass.parameters,allowing COSI consumers to request read-only bucket access.
"readonly"grantsReadandListactions only"readwrite"(or empty/missing — the default) grantsRead,Write,List,Tagging(preserving existing behavior)InvalidArgumenterrorRef: kubernetes-sigs/container-object-storage-interface#244
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests