Added unit tests and E2E tests #418

ankush-ntx · 2025-05-05T06:44:38Z

Summary:

This PR introduces a testing framework for the Nutanix COSI driver, including both unit tests and end-to-end (E2E) tests. In addition, several minor bugs discovered during test development have been addressed and resolved.

Changes:

Unit Tests:

Covers core driver logic, IAM client ops, S3 client ops and provisioner methods.
Mocks for S3 client and IAM client to isolate functionality.

E2E Tests:

Tests all the workflows completely with edge cases.
Including: Create/Delete buckets, Grant/Revoke user access
Tests can be run on both a real cluster or a local cluster using Objects Triton as S3 backend objects store.

CI/CD Pipeline:

Updated GitHub pipeline to include running unit tests when a PR is created.

Deployment Changes:

PC_SECRET now only contains username and password credentials for the Prism Central in the previous format.
PC_ENDPOINT instead should have the full endpoint of the Prism Central.

Fixes:

Fixed Deleting a Bucket object with a missing bucket fails #378: Bucket deletion succeeds when bucket is missing.
Added a few more relevant error statements.
Minor fixes in bucket policy methods.

How to run tests?

Unit Tests:
Execute the following to run unit tests:

go test ./...

To generate the coverage report and an HTML page to view the report:

go test -coverprofile=coverage.out ./...
go tool cover -html=coverage.out -o coverage.html

Open the HTML file in any web browser to view the coverage of each file.

E2E Tests:
Execute the following to run E2E tests:

sh scripts/setup_test_env.sh [flags]

Options:
-o, --oss_endpoint ENDPOINT    Nutanix Object Store instance endpoint, eg. "http://10.51.142.82:80".
-i, --pc_endpoint ENDPOINT     Prism Central endpoint, eg. "https://10.51.142.82:9440".
-u, --pc_user USERNAME         Prism Central username. [default = admin]
-p, --pc_pass PASSWORD         Prism Central password.
-a, --access_key KEY           Admin IAM Access key to be used for Nutanix Objects.
-s, --secret_key KEY           Admin IAM Secret key to be used for Nutanix Objects.
-n, --namespace NAMESAPCE      Cluster namespace for the COSI deployment [default = cosi]

Check the README.md for more info.

anirudhbansal95

Thanks Ankush. Reviewed the unit tests and the associated code. Yet to review the end to end tests.

anirudhbansal95 · 2025-10-16T06:41:13Z

pkg/admin/user.go

-		return fmt.Errorf("%s", delete_resp.Status)
+	if delete_resp.StatusCode == 404 {
+		return nil
+	} else if delete_resp.StatusCode != 204 {


nit: else if can be removed from line 165 since we are returning in line 164

anirudhbansal95 · 2025-10-16T06:50:34Z

pkg/admin/user_test.go

+
+	t.Run("TestCreateUser_MissingUsername", func(t *testing.T) {
+		api := &admin.API{}
+		_, err := api.CreateUser(ctx, "", mockDisplayName)


Currently we allow the display name to be empty, though not the user name. Is this the expected behaviour?

anirudhbansal95 · 2025-10-16T07:05:32Z

pkg/admin/util_test.go

+		api := admin.API{
+			AccountName: "test",
+		}
+		accountName := api.AccountName


Maybe use api.GetAccountName() here and in the test below

anirudhbansal95 · 2025-10-16T07:15:40Z

pkg/driver/provisioner.go

 		klog.ErrorS(err, "failed to delete user")
 	}
+
+	klog.InfoS("Successfully revoked access of user", "userName", req.GetAccountId(), "bucketName", req.GetBucketId())


Do we also need to update the bucket policy here to remove the user from it, like we updated the bucket policy to grant user access in the previous case?

anirudhbansal95 · 2025-10-16T07:24:18Z

pkg/driver/provisioner.go

 	klog.InfoS("Deleting user", "id", req.GetAccountId())

-	err := s.ntnxIamClient.RemoveUser(ctx, req.GetAccountId())
+	err := s.NtnxIamClient.RemoveUser(ctx, req.GetAccountId())


Does ignoring the error mean this is only a best-effort API to revoke user access from bucket, since it returns nil error even if there was an IAM issue for example? Is the caller fine with this behaviour?

anirudhbansal95 · 2025-10-16T07:42:46Z

pkg/util/s3client/policy_test.go

+				foundSID1 = true
+				assert.Equal(t, "Deny", string(ps.Effect))
+			case "sid2":
+				foundSID2 = true


Maybe also check that sid2 and sid3 have "Allow" effect

anirudhbansal95 · 2025-10-16T07:46:53Z

pkg/util/s3client/policy_test.go

+		ps.Allows()
+		assert.Equal(t, s3client.Effect("Allow"), ps.Effect)
+		ps.Effect = "Other"
+		ps.Allows()


nit: Maybe add a comment that ps.Allows() doesn't modify an existing effect, if set.

anirudhbansal95 · 2025-10-16T07:55:22Z

pkg/util/s3client/s3-handlers_test.go

+		s := &s3client.S3Agent{Client: mock}
+		content, err := s.GetObjectInBucket("missing-bucket", "missing-key")
+		assert.Error(t, err)
+		assert.Equal(t, "ERROR_ OBJECT NOT FOUND", content)


Is this a specific format in which the object not found error should be returned? Or can it be something like: ERROR_OBJECT_NOT_FOUND or some other string?

anirudhbansal95

Thanks Ankush. Took a look at the end to end tests. They look good in general with a few comments.

anirudhbansal95 · 2025-10-29T08:58:19Z

tests/e2e/helpers/helpers.go

+			return err
+		}
+
+		if bucket.Status.BucketReady == false {


nit: if !bucket.Status.BucketReady

anirudhbansal95 · 2025-10-29T09:02:00Z

tests/e2e/helpers/helpers.go

+			return err
+		}
+
+		if bucket.Status.BucketReady == true {


nit: if bucket.Status.BucketReady

anirudhbansal95 · 2025-10-29T11:56:59Z

tests/e2e/revoke_suite_test.go

+			Expect(err).ToNot(HaveOccurred())
+
+			// By("Checking if Bucket is created in the Objectstore backend")
+			err = s3Client.WaitUntilBucketExists(&s3.HeadBucketInput{Bucket: &failBucket.Name})


Do we need to call this function in other tests as well?

ankush-ntx added 3 commits May 4, 2025 12:53

Added unit and e2e tests

9592578

Added example of PC endpoint in values.yaml

504dc02

Updated github pipeline to include unit tests

6f130ab

ankush-ntx requested review from jimmidyson and manoj-nutanix May 5, 2025 14:35

anirudhbansal95 reviewed Oct 16, 2025

View reviewed changes

anirudhbansal95 reviewed Oct 29, 2025

View reviewed changes

Added unit tests and E2E tests #418

Are you sure you want to change the base?

Added unit tests and E2E tests #418

Uh oh!

Conversation

ankush-ntx commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changes:

Fixes:

How to run tests?

Uh oh!

anirudhbansal95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anirudhbansal95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ankush-ntx commented May 5, 2025 •

edited

Loading