COSI-40, COSI-52: Revoke Bucket Access API Implementation and Tests #57

anurag4DSB · 2024-12-10T17:23:50Z

Summary

This PR implements the DriverRevokeBucketAccess API, enhances the IAM client, and adds comprehensive tests, including unit and end-to-end coverage.

Key Changes

COSI-40: Remove Prefix from Inline Policies

Inline policy names now match bucket names, simplifying access control.

COSI-40: API Implementation

Fetches bucket parameters for IAM client configuration.
Adds IAM methods to delete users, inline policies, and keys.

COSI-40: Add Unit Tests

Updated IAM mock and added unit tests for IAM client new methods and RevokeBucketAccess.

COSI-52: Add E2E Test

Validates end-to-end flow for revoking bucket access by checking if the IAM user was deleted.

codecov · 2024-12-10T17:25:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.12%. Comparing base (7bc2de2) to head (07a2202).
Report is 8 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
pkg/clients/iam/iam_client.go	`100.00% <100.00%> (+3.15%)`	⬆️
pkg/driver/provisioner_server_impl.go	`97.36% <100.00%> (+0.35%)`	⬆️

... and 1 file with indirect coverage changes

Components	Coverage Δ
🏠 Main Package	`∅ <ø> (∅)`
🚗 Driver Package	`91.40% <100.00%> (+1.01%)`	⬆️
📡 gRPC Factory Package	`81.65% <ø> (ø)`
🔐 IAM Client Package	`100.00% <100.00%> (+3.15%)`	⬆️
🌐 S3 Client Package	`100.00% <ø> (+6.12%)`	⬆️
🔧 Util Package	`100.00% <ø> (ø)`

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   90.69%   93.12%   +2.42%     
==========================================
  Files           9        9              
  Lines         516      611      +95     
==========================================
+ Hits          468      569     +101     
+ Misses         38       36       -2     
+ Partials       10        6       -4

pkg/clients/iam/iam_client.go

jonathan-gramain · 2024-12-13T02:16:59Z

pkg/clients/iam/iam_client.go

@@ -145,3 +152,88 @@ func (client *IAMClient) CreateBucketAccess(ctx context.Context, userName, bucke

 	return accessKeyOutput, nil
 }
+
+// RevokeBucketAccess is a helper that revokes bucket access by orchestrating individual steps to delete the user, inline policy, and access keys.
+func (client *IAMClient) RevokeBucketAccess(ctx context.Context, userName, bucketName string) error {


So I assume there is a 1-1 mapping between users and buckets, in which case it looks correct to me.

There is a 1 to many mapping for buckets and users.
1 bucket can have multiple IAM users accessing it.

But for each access indeed we have 1 IAM user

pkg/driver/provisioner_server_impl.go

jonathan-gramain · 2024-12-13T02:35:53Z

pkg/clients/iam/iam_client_test.go

+			mockIAM = &mock.MockIAMClient{}
+		})
+
+		It("should fail on non-existent user gracefully", func(ctx SpecContext) {


The test says that it should fail "gracefully" but doesn't seem to check the gracefulness of the error (i.e. it tests the same way than the next test).

Thank you for your feedback — it helped me catch a bug. I’ve added the change along with tests: if an access key is deleted between listing and deletion, we now handle it by returning a “not found” error and continuing with the list.

Additionally, I’ve created a ticket for next week to review the graceful handling of all errors, and I’ve added it to the next sprint: S3C-9439. I have more instances like this.

jonathan-gramain · 2024-12-13T02:37:33Z

pkg/clients/iam/iam_client_test.go

+			client.IAMService = mockIAM
+
+			err := client.RevokeBucketAccess(ctx, "test-user", "test-bucket")
+			Expect(err).To(BeNil())


It would be good to check the side-effects of the function i.e. that it actually deleted stuff, although I don't know if this test suite is the good place where it can be done. It also applies to other tests around for function calls that have (or shouldn't have) side-effects.

One way I'm thinking about is attempting to use an access key that was revoked should not be possible, and conversely, using the access key that shouldn't have been revoked should still work. But that may be best done in E2E tests.

We rely on Vault to handle the revocation of access keys, ensuring that deleted keys cannot be used. As such, this should not be tested in this particular test suite. Instead, the E2E tests verify the deletion of the IAM user, which falls under the scope of the COSI driver’s responsibility.

While I considered testing the deletion of the Kubernetes secret containing access, the responsibility for managing that lies with the COSI controller, not the COSI driver.

Relying on contracts/promises from other services.

anurag4DSB · 2024-12-13T11:19:07Z

Addressed review comments, and added more tests for noSuchEntity scenarios.

We do not need a prefix for inline policy, as bucket object is linked to access at kubernetes level, we keep inline policy name as bucket name

- Gets parameters from the bucket object to get config for IAM client - Adds methods to IAM client to delete user, inline policy and keys - If inline policy doesn't exist, logs a warning and continues

- updated IAM mock with new IAM API methods - added unit tests for RevokeBucketAccess IAM API methods - removed the unimplemented test for RevokeBucketAccess

anurag4DSB changed the title ~~COSI-40: remove-prefix-from-IAM-user-inline-policy~~ COSI-40: Revoke Bucket Access API (delete IAM user, inline policy and keys) Dec 10, 2024

anurag4DSB changed the title ~~COSI-40: Revoke Bucket Access API (delete IAM user, inline policy and keys)~~ COSI-40: Revoke Bucket Access API Implementation and Tests Dec 10, 2024

anurag4DSB added the work in progress label Dec 11, 2024

anurag4DSB force-pushed the feature/COSI-40-DriverRevokeBucketAccess-api branch 6 times, most recently from ba8f6a1 to e1e12fc Compare December 11, 2024 11:36

anurag4DSB removed the work in progress label Dec 11, 2024

anurag4DSB marked this pull request as ready for review December 11, 2024 11:41

anurag4DSB mentioned this pull request Dec 11, 2024

COSI-40: revoke access api #53

Closed

Base automatically changed from improvement/COSI-72-unit-tests-improvements to main December 11, 2024 13:19

anurag4DSB commented Dec 11, 2024

View reviewed changes

pkg/clients/iam/iam_client.go Outdated Show resolved Hide resolved

anurag4DSB force-pushed the feature/COSI-40-DriverRevokeBucketAccess-api branch from e1e12fc to cac48a6 Compare December 11, 2024 19:57

anurag4DSB changed the title ~~COSI-40: Revoke Bucket Access API Implementation and Tests~~ COSI-40, COSI-52: Revoke Bucket Access API Implementation and Tests Dec 12, 2024

BourgoisMickael approved these changes Dec 12, 2024

View reviewed changes

pkg/clients/iam/iam_client.go Show resolved Hide resolved

jonathan-gramain approved these changes Dec 13, 2024

View reviewed changes

anurag4DSB force-pushed the feature/COSI-40-DriverRevokeBucketAccess-api branch from 8b65b98 to 6e9393f Compare December 13, 2024 11:14

COSI-40: remove-prefix-from-IAM-user-inline-policy

d7ce460

We do not need a prefix for inline policy, as bucket object is linked to access at kubernetes level, we keep inline policy name as bucket name

anurag4DSB force-pushed the feature/COSI-40-DriverRevokeBucketAccess-api branch from 6e9393f to e27a1d2 Compare December 13, 2024 11:19

anurag4DSB added 6 commits December 13, 2024 12:22

COSI-40: revoke-bucket-access-api-implementation

c74c59c

- Gets parameters from the bucket object to get config for IAM client - Adds methods to IAM client to delete user, inline policy and keys - If inline policy doesn't exist, logs a warning and continues

COSI-40: add-unit-tests-for-revoke-bucket-access

02e2288

- updated IAM mock with new IAM API methods - added unit tests for RevokeBucketAccess IAM API methods - removed the unimplemented test for RevokeBucketAccess

COSI-40: add-test-for-debug-log-in-aws-config

e0b042d

COSI-52: e2e-test-for-revoke-bucket-access

01beda8

COSI-40: ignore-failure-if-access-key-deleted-in-flight

32f4d3f

COSI-40: add-graceful tests

07a2202

anurag4DSB force-pushed the feature/COSI-40-DriverRevokeBucketAccess-api branch from e27a1d2 to 07a2202 Compare December 13, 2024 11:22

anurag4DSB merged commit daa6db6 into main Dec 13, 2024
8 checks passed

anurag4DSB deleted the feature/COSI-40-DriverRevokeBucketAccess-api branch December 13, 2024 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COSI-40, COSI-52: Revoke Bucket Access API Implementation and Tests #57

COSI-40, COSI-52: Revoke Bucket Access API Implementation and Tests #57

anurag4DSB commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

jonathan-gramain Dec 13, 2024

anurag4DSB Dec 13, 2024

jonathan-gramain Dec 13, 2024

anurag4DSB Dec 13, 2024 •

edited

Loading

jonathan-gramain Dec 13, 2024

anurag4DSB Dec 13, 2024

anurag4DSB Dec 13, 2024

anurag4DSB commented Dec 13, 2024

COSI-40, COSI-52: Revoke Bucket Access API Implementation and Tests #57

COSI-40, COSI-52: Revoke Bucket Access API Implementation and Tests #57

Conversation

anurag4DSB commented Dec 10, 2024 • edited Loading

Summary

Key Changes

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

jonathan-gramain Dec 13, 2024

Choose a reason for hiding this comment

anurag4DSB Dec 13, 2024

Choose a reason for hiding this comment

jonathan-gramain Dec 13, 2024

Choose a reason for hiding this comment

anurag4DSB Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

jonathan-gramain Dec 13, 2024

Choose a reason for hiding this comment

anurag4DSB Dec 13, 2024

Choose a reason for hiding this comment

anurag4DSB Dec 13, 2024

Choose a reason for hiding this comment

anurag4DSB commented Dec 13, 2024

anurag4DSB commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

anurag4DSB Dec 13, 2024 •

edited

Loading