From 5c61c3ccdcf3c4255b63bdb3708e664bd562e1dd Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Wed, 14 Sep 2022 17:17:39 +0200 Subject: [PATCH] Fix flaky TestGetObjectWithOutdatedDisks (#15687) On occasion this test fails: ``` 2022-09-12T17:22:44.6562737Z === RUN TestGetObjectWithOutdatedDisks 2022-09-12T17:22:44.6563751Z erasure-object_test.go:1214: Test 2: Expected data to have md5sum = `c946b71bb69c07daf25470742c967e7c`, found `7d16d23f07072af1a809707ba101ae07` 2 ``` Theory: Both objects are written with the same timestamp due to lower timer resolution on Windows. This results in secondary resolution, which is deterministic, but random. Solution: Instead of hacking in a wait we request the specific version we want. Should still keep the test relevant. Bonus: Remote action dependency for vulncheck --- .github/workflows/vulncheck.yml | 17 ++++++++++++----- cmd/admin-handlers-users_test.go | 16 +++++++++++++++- cmd/data-update-tracker_test.go | 6 ++++-- cmd/erasure-object_test.go | 4 ++-- 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/.github/workflows/vulncheck.yml b/.github/workflows/vulncheck.yml index 09578cfb1..69f07dca2 100644 --- a/.github/workflows/vulncheck.yml +++ b/.github/workflows/vulncheck.yml @@ -2,8 +2,10 @@ name: VulnCheck on: pull_request: branches: - - master - + - master + push: + branches: + - master jobs: vulncheck: name: Analysis @@ -11,9 +13,14 @@ jobs: steps: - name: Check out code into the Go module directory uses: actions/checkout@v3 - - uses: actions/setup-go@v3 + - name: Set up Go + uses: actions/setup-go@v3 with: go-version: 1.19.x check-latest: true - - name: Check for vulnerabilities - uses: kmulvey/govulncheck-action@v1.0.0 + - name: Get official govulncheck + run: go install golang.org/x/vuln/cmd/govulncheck@latest + shell: bash + - name: Run govulncheck + run: govulncheck ./... + shell: bash diff --git a/cmd/admin-handlers-users_test.go b/cmd/admin-handlers-users_test.go index 23a711878..5bbea2e64 100644 --- a/cmd/admin-handlers-users_test.go +++ b/cmd/admin-handlers-users_test.go @@ -34,7 +34,7 @@ import ( "time" "github.com/minio/madmin-go" - minio "github.com/minio/minio-go/v7" + "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" cr "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/s3utils" @@ -1141,6 +1141,7 @@ func (s *TestSuiteIAM) TestAccMgmtPlugin(c *check) { } func (c *check) mustCreateIAMUser(ctx context.Context, admClnt *madmin.AdminClient) madmin.Credentials { + c.Helper() randUser := mustGetUUID() randPass := mustGetUUID() err := admClnt.AddUser(ctx, randUser, randPass) @@ -1154,6 +1155,7 @@ func (c *check) mustCreateIAMUser(ctx context.Context, admClnt *madmin.AdminClie } func (c *check) mustGetIAMUserInfo(ctx context.Context, admClnt *madmin.AdminClient, accessKey string) madmin.UserInfo { + c.Helper() ui, err := admClnt.GetUserInfo(ctx, accessKey) if err != nil { c.Fatalf("should be able to get user info: %v", err) @@ -1162,6 +1164,7 @@ func (c *check) mustGetIAMUserInfo(ctx context.Context, admClnt *madmin.AdminCli } func (c *check) mustNotCreateIAMUser(ctx context.Context, admClnt *madmin.AdminClient) { + c.Helper() randUser := mustGetUUID() randPass := mustGetUUID() err := admClnt.AddUser(ctx, randUser, randPass) @@ -1171,6 +1174,7 @@ func (c *check) mustNotCreateIAMUser(ctx context.Context, admClnt *madmin.AdminC } func (c *check) mustCreateSvcAccount(ctx context.Context, tgtUser string, admClnt *madmin.AdminClient) madmin.Credentials { + c.Helper() cr, err := admClnt.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ TargetUser: tgtUser, }) @@ -1181,6 +1185,7 @@ func (c *check) mustCreateSvcAccount(ctx context.Context, tgtUser string, admCln } func (c *check) mustNotCreateSvcAccount(ctx context.Context, tgtUser string, admClnt *madmin.AdminClient) { + c.Helper() _, err := admClnt.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ TargetUser: tgtUser, }) @@ -1190,6 +1195,7 @@ func (c *check) mustNotCreateSvcAccount(ctx context.Context, tgtUser string, adm } func (c *check) mustNotListObjects(ctx context.Context, client *minio.Client, bucket string) { + c.Helper() res := client.ListObjects(ctx, bucket, minio.ListObjectsOptions{}) v, ok := <-res if !ok || v.Err == nil { @@ -1198,6 +1204,7 @@ func (c *check) mustNotListObjects(ctx context.Context, client *minio.Client, bu } func (c *check) mustListObjects(ctx context.Context, client *minio.Client, bucket string) { + c.Helper() res := client.ListObjects(ctx, bucket, minio.ListObjectsOptions{}) v, ok := <-res if ok && v.Err != nil { @@ -1206,6 +1213,7 @@ func (c *check) mustListObjects(ctx context.Context, client *minio.Client, bucke } func (c *check) mustListBuckets(ctx context.Context, client *minio.Client) { + c.Helper() _, err := client.ListBuckets(ctx) if err != nil { c.Fatalf("user was unable to list buckets: %v", err) @@ -1213,6 +1221,7 @@ func (c *check) mustListBuckets(ctx context.Context, client *minio.Client) { } func (c *check) mustNotUpload(ctx context.Context, client *minio.Client, bucket string) { + c.Helper() _, err := client.PutObject(ctx, bucket, "some-object", bytes.NewBuffer([]byte("stuff")), 5, minio.PutObjectOptions{}) if e, ok := err.(minio.ErrorResponse); ok { if e.Code == "AccessDenied" { @@ -1228,6 +1237,7 @@ func (c *check) assertSvcAccS3Access(ctx context.Context, s *TestSuiteIAM, cr ma } func (c *check) assertSvcAccAppearsInListing(ctx context.Context, madmClient *madmin.AdminClient, parentAK, svcAK string) { + c.Helper() listResp, err := madmClient.ListServiceAccounts(ctx, parentAK) if err != nil { c.Fatalf("unable to list svc accounts: %v", err) @@ -1253,6 +1263,7 @@ func (c *check) assertSvcAccInfoQueryable(ctx context.Context, madmClient *madmi // bucket. It creates a session policy that restricts listing on the bucket and // then enables it again in a session policy update call. func (c *check) assertSvcAccSessionPolicyUpdate(ctx context.Context, s *TestSuiteIAM, madmClient *madmin.AdminClient, accessKey, bucket string) { + c.Helper() svcAK, svcSK := mustGenerateCredentials(c) // This policy does not allow listing objects. @@ -1308,6 +1319,7 @@ func (c *check) assertSvcAccSessionPolicyUpdate(ctx context.Context, s *TestSuit } func (c *check) assertSvcAccSecretKeyAndStatusUpdate(ctx context.Context, s *TestSuiteIAM, madmClient *madmin.AdminClient, accessKey, bucket string) { + c.Helper() svcAK, svcSK := mustGenerateCredentials(c) cr, err := madmClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ TargetUser: accessKey, @@ -1344,6 +1356,7 @@ func (c *check) assertSvcAccSecretKeyAndStatusUpdate(ctx context.Context, s *Tes } func (c *check) assertSvcAccDeletion(ctx context.Context, s *TestSuiteIAM, madmClient *madmin.AdminClient, accessKey, bucket string) { + c.Helper() svcAK, svcSK := mustGenerateCredentials(c) cr, err := madmClient.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ TargetUser: accessKey, @@ -1364,6 +1377,7 @@ func (c *check) assertSvcAccDeletion(ctx context.Context, s *TestSuiteIAM, madmC } func mustGenerateCredentials(c *check) (string, string) { + c.Helper() ak, sk, err := auth.GenerateCredentials() if err != nil { c.Fatalf("unable to generate credentials: %v", err) diff --git a/cmd/data-update-tracker_test.go b/cmd/data-update-tracker_test.go index 2763ab53a..d07bad1d9 100644 --- a/cmd/data-update-tracker_test.go +++ b/cmd/data-update-tracker_test.go @@ -200,11 +200,13 @@ func TestDataUpdateTracker(t *testing.T) { } ctx, cancel = context.WithCancel(context.Background()) - defer cancel() - // Reload... dut = newDataUpdateTracker() dut.start(ctx, tmpDir) + defer func() { + cancel() + <-dut.saveExited + }() if dut.current() != 2 { t.Fatal("current idx after load not preserved. want 2, got:", dut.current()) diff --git a/cmd/erasure-object_test.go b/cmd/erasure-object_test.go index a5737f79c..3b62fdf81 100644 --- a/cmd/erasure-object_test.go +++ b/cmd/erasure-object_test.go @@ -1183,7 +1183,7 @@ func TestGetObjectWithOutdatedDisks(t *testing.T) { return disks } sets.erasureDisksMu.Unlock() - _, err = z.PutObject(ctx, testCase.bucket, testCase.object, mustGetPutObjReader(t, bytes.NewReader(testCase.content), int64(len(testCase.content)), "", ""), + got, err := z.PutObject(ctx, testCase.bucket, testCase.object, mustGetPutObjReader(t, bytes.NewReader(testCase.content), int64(len(testCase.content)), "", ""), ObjectOptions{Versioned: testCase.versioned}) if err != nil { t.Fatalf("Test %d: Failed to upload the final object: %v", i+1, err) @@ -1193,7 +1193,7 @@ func TestGetObjectWithOutdatedDisks(t *testing.T) { sets.erasureDisksMu.Lock() xl.getDisks = func() []StorageAPI { return origErasureDisks } sets.erasureDisksMu.Unlock() - gr, err := z.GetObjectNInfo(ctx, testCase.bucket, testCase.object, nil, nil, readLock, ObjectOptions{}) + gr, err := z.GetObjectNInfo(ctx, testCase.bucket, testCase.object, nil, nil, readLock, ObjectOptions{VersionID: got.VersionID}) if err != nil { t.Fatalf("Expected GetObject to succeed, but failed with %v", err) }