COSI-25, COSI-32: Implement Delete Bucket API, Add Tests, and Enhance CI #61

anurag4DSB · 2024-12-11T21:46:25Z

Note for reviewers: Reviewing commit by commit is easier, as we have a commit story.

Summary

This PR implements the DeleteBucket API and adds comprehensive tests and CI improvements:

Changes:

COSI-25: Implement Delete Bucket API:

Deletes buckets only if they can be deleted with a simple operation (e.g., fails if objects exist).
Ensures safe and fail-proof deletion handling.

COSI-25: Add Unit Tests:

Added unit tests for successful deletion, error propagation, and edge cases.

COSI-32: Add e2e Tests:

Integrated e2e tests for DeleteBucket into the CI pipeline to validate end-to-end functionality.

COSI-32: Ignore Cleanup on Failure:

Skips cleanup steps after test failures for better error handling in CI.

COSI-25: Refactor Tests:

Simplified test setup with helper functions and improved readability and maintainability.

Impact:
• Improves DeleteBucket reliability and test coverage.
• Enhances CI workflows and test maintainability.

Tickets: COSI-25, COSI-32

codecov · 2024-12-11T21:53:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.43%. Comparing base (daa6db6) to head (4bb5259).
Report is 7 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%> (ø)`
pkg/clients/s3/s3_client.go	`100.00% <100.00%> (ø)`
pkg/driver/provisioner_server_impl.go	`97.61% <100.00%> (+0.25%)`	⬆️

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

@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
+ Coverage   93.12%   93.43%   +0.31%     
==========================================
  Files           9        9              
  Lines         611      640      +29     
==========================================
+ Hits          569      598      +29     
  Misses         36       36              
  Partials        6        6

BourgoisMickael · 2024-12-12T21:22:13Z

.github/scripts/e2e_tests.sh

+for ((i=1; i<=$ATTEMPTS; i++)); do
+  BUCKET_HEAD_RESULT=$(aws --endpoint-url "$S3_ENDPOINT" s3api head-bucket --bucket "$BUCKET_TO_BE_DELETED" 2>&1 || true)
+
+  if [[ "$BUCKET_HEAD_RESULT" == *"Not Found"* ]]; then
+    log_and_run echo "Bucket with name '$BUCKET_TO_BE_DELETED' not found. Bucket deletion successful."
+    break
+  else
+    log_and_run echo "Attempt $i: Bucket with name '$BUCKET_TO_BE_DELETED' still exists. Retrying in $DELAY seconds..."
+    sleep $DELAY
+  fi
+done
+
+if [[ "$BUCKET_HEAD_RESULT" != *"Not Found"* ]]; then
+  log_and_run echo "Bucket with name '$BUCKET_TO_BE_DELETED' was not deleted after $ATTEMPTS attempts."
+  exit 1
+fi


The cli has a command to wait for a condition with retries: aws s3api wait ...

https://awscli.amazonaws.com/v2/documentation/api/latest/reference/s3api/wait/bucket-not-exists.html

And you can configure the retries with max_attempts and retry_mode: https://docs.aws.amazon.com/cli/latest/topic/config-vars.html#retry-configuration

Thank you for the suggestion.

Unfortunately, the list-buckets API does not support this functionality. Since we only have the prefix Bucket_CLASS_NAME and not the exact bucket name, it cannot be used in this case.

--retry and --delay: These options are not natively supported by the aws s3api list-buckets command. You would need to implement a custom retry logic, as demonstrated in the script above.

aws s3api wait: This command is ideal for waiting on conditions such as bucket existence. However, it requires an exact bucket name and does not work with prefixes.

That being said, I have added support for retry logic in other cases, such as checking if a bucket is being deleted (when we know the exact bucket name) and verifying a user is being deleted (when we know the user’s name).

Commit for bucket delete check

Commit for user delete check

Actually had to change it a bit more, so used the AWS variables instead of --retry and --delay as its not supported for every API.

BUCKET_HEAD_RESULT=$(log_and_run AWS_MAX_ATTEMPTS=$ATTEMPTS AWS_RETRY_DELAY=$DELAY aws --endpoint-url "$S3_ENDPOINT" s3api head`-bucket --bucket "$BUCKET_TO_BE_DELETED" --profile iam 2>&1 || true)

Apologies for multiple comments, had some CI issues
FInally adding the passing commit link here for reference: 7ff62b3

BourgoisMickael · 2024-12-12T21:23:02Z

cosi-examples/bucketclaim-deletion-policy.yaml

+spec:
+  bucketClassName: delete-bucket-class
+  protocols:
+  - s3


Suggested change

- s3

- s3

indent

its up for debate, I have usually seen (most common) without indent when using -.
I would prefer to follow the industry norm here, unless we have a specific standard or reason.

Technically both are correct though.

pkg/clients/s3/s3_client.go

pkg/driver/provisioner_server_impl.go

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

pkg/driver/provisioner_server_impl.go

+		klog.ErrorS(err, "Failed to get bucket object from kubernetes", "bucketName", bucketName)
+		return nil, status.Error(codes.Internal, "failed to get bucket object from kubernetes")


If it is a recurring pattern to both log and return an error, it could benefit from a small helper that uses the same message and argument list for the log and the error, so by using a variable argument list, you could do here for example:

return loggedError(err, "failed to get ...", "bucketName", bucketName)

Not focusing on logs too much right now, as we will change the logging mechanism next sprint. We will use OTel SDK for logs, which will probably remove all these logs

I added your comment in this ticket

.github/scripts/e2e_tests.sh

- 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

anurag4DSB · 2024-12-13T11:49:24Z

rebased to main and pushed, didn't change anything yet
Now I am gonna address review comments

- Only delete bucket if it can be deleted by a simple delete operation. - If the bucket deletion operation gives an error for any reason, the bucket will not be deleted. Example: If objects are present in the bucket.

This commit refactors the test file by: - Extracting common test data and parameters into helper functions - Reducing repetitive code in BeforeEach/AfterEach blocks - Using local variables or dereferencing pointers to avoid addressability errors for string constants - Improving readability and maintainability of the tests - used AWS CLI delay and retries to check user and bucket deletion

anurag4DSB · 2024-12-13T13:39:04Z

Update: Rebased, and squashed commits
Also changed the refactor commit message to add the context of using AWS CLI retries and delays instead of for loop to check user and bucket deletion. We can use them in these cases as we know the respective bucket and user name, when we know the prefix we still use a for loop.

I will merge the PR.

anurag4DSB force-pushed the feature/COSI-25-delete-bucket-api branch from 9c7bce3 to 4652178 Compare December 11, 2024 21:46

anurag4DSB force-pushed the feature/COSI-25-delete-bucket-api branch 6 times, most recently from a2113ff to e45538a Compare December 12, 2024 08:34

anurag4DSB changed the title ~~COSI-25: implement-delete-bucket-API~~ COSI-25, COSI-32: Implement Delete Bucket API, Add Tests, and Enhance CI Dec 12, 2024

anurag4DSB marked this pull request as ready for review December 12, 2024 08:40

BourgoisMickael reviewed Dec 12, 2024

View reviewed changes

BourgoisMickael approved these changes Dec 12, 2024

View reviewed changes

jonathan-gramain approved these changes Dec 13, 2024

View reviewed changes

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

Base automatically changed from feature/COSI-40-DriverRevokeBucketAccess-api to main December 13, 2024 11:39

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

582513b

- 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

anurag4DSB force-pushed the feature/COSI-25-delete-bucket-api branch from e45538a to bc73aba Compare December 13, 2024 11:49

anurag4DSB force-pushed the feature/COSI-25-delete-bucket-api branch from 103fd74 to 7ff62b3 Compare December 13, 2024 13:34

anurag4DSB added 5 commits December 13, 2024 14:37

COSI-25: implement-delete-bucket-API

fdcf5da

- Only delete bucket if it can be deleted by a simple delete operation. - If the bucket deletion operation gives an error for any reason, the bucket will not be deleted. Example: If objects are present in the bucket.

COSI-25: add-unit-tests

d0a0725

COSI-32: add-e2e-tests-for-delete-bucket

f8de327

COSI-32: ignore-cleanup-step-on-failure

10f4962

anurag4DSB force-pushed the feature/COSI-25-delete-bucket-api branch from 7ff62b3 to 4bb5259 Compare December 13, 2024 13:37

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

anurag4DSB deleted the feature/COSI-25-delete-bucket-api branch December 13, 2024 14:00

anurag4DSB mentioned this pull request Dec 17, 2024

COSI-35: Improve Logging Clarity and Accuracy for Object Storage and IAM Operations #67

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COSI-25, COSI-32: Implement Delete Bucket API, Add Tests, and Enhance CI #61

COSI-25, COSI-32: Implement Delete Bucket API, Add Tests, and Enhance CI #61

anurag4DSB commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

BourgoisMickael Dec 12, 2024

anurag4DSB Dec 13, 2024 •

edited

Loading

anurag4DSB Dec 13, 2024

anurag4DSB Dec 13, 2024

BourgoisMickael Dec 12, 2024

anurag4DSB Dec 13, 2024

jonathan-gramain Dec 13, 2024

anurag4DSB Dec 13, 2024

anurag4DSB commented Dec 13, 2024

anurag4DSB commented Dec 13, 2024

		klog.ErrorS(err, "Failed to get bucket object from kubernetes", "bucketName", bucketName)
		return nil, status.Error(codes.Internal, "failed to get bucket object from kubernetes")

COSI-25, COSI-32: Implement Delete Bucket API, Add Tests, and Enhance CI #61

COSI-25, COSI-32: Implement Delete Bucket API, Add Tests, and Enhance CI #61

Conversation

anurag4DSB commented Dec 11, 2024 • edited Loading

Summary

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

BourgoisMickael Dec 12, 2024

Choose a reason for hiding this comment

anurag4DSB Dec 13, 2024 • edited Loading

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

BourgoisMickael Dec 12, 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

Choose a reason for hiding this comment

anurag4DSB commented Dec 13, 2024

anurag4DSB commented Dec 13, 2024

anurag4DSB commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

anurag4DSB Dec 13, 2024 •

edited

Loading