From 1b11b9d335acc0b74f6d9cedc7d226575d87e381 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Mon, 16 Jan 2023 16:01:35 +0200 Subject: [PATCH] fix(repodb): make table reset syncronous for consistent testing (#1111) Signed-off-by: Laurentiu Niculae (cherry picked from commit bea0eabcaa8c8af7a4df52a043998336c49a08d3) (cherry picked from commit 0f02d625331987126326a28731f14d3139061adc) fix(repodb): use dynamodb sdk functions for waiting on tables to be created or deleted Signed-off-by: Andrei Aaron fix(ci): make sure the dynamodb tables used in testing have unique names There were cases of failures when tests were run for the entire repodb package which did not reproduce when tests were run for each sub-folder individually This change should make sure there are no collisions between the names used in concurrent tests. Signed-off-by: Andrei Aaron Signed-off-by: Andrei Aaron Co-authored-by: Laurentiu Niculae --- errors/errors.go | 1 + .../dynamodb-wrapper/dynamo_internal_test.go | 102 ++++++++++++++++++ .../repodb/dynamodb-wrapper/dynamo_test.go | 63 +++++++---- .../repodb/dynamodb-wrapper/dynamo_wrapper.go | 58 ++++++++-- pkg/meta/repodb/repodb_test.go | 16 ++- 5 files changed, 204 insertions(+), 36 deletions(-) create mode 100644 pkg/meta/repodb/dynamodb-wrapper/dynamo_internal_test.go diff --git a/errors/errors.go b/errors/errors.go index fcab3604..faf56d12 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -76,4 +76,5 @@ var ( ErrLimitIsNegative = errors.New("pageturner: limit has negative value") ErrOffsetIsNegative = errors.New("pageturner: offset has negative value") ErrSortCriteriaNotSupported = errors.New("pageturner: the sort criteria is not supported") + ErrTimeout = errors.New("operation timeout") ) diff --git a/pkg/meta/repodb/dynamodb-wrapper/dynamo_internal_test.go b/pkg/meta/repodb/dynamodb-wrapper/dynamo_internal_test.go new file mode 100644 index 00000000..4f4cc44b --- /dev/null +++ b/pkg/meta/repodb/dynamodb-wrapper/dynamo_internal_test.go @@ -0,0 +1,102 @@ +package dynamo + +import ( + "context" + "os" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/service/dynamodb" + guuid "github.com/gofrs/uuid" + "github.com/rs/zerolog" + . "github.com/smartystreets/goconvey/convey" + + "zotregistry.io/zot/pkg/log" //nolint:go-staticcheck + "zotregistry.io/zot/pkg/meta/repodb/version" +) + +func TestWrapperErrors(t *testing.T) { + const ( + endpoint = "http://localhost:4566" + region = "us-east-2" + ) + + uuid, err := guuid.NewV4() + if err != nil { + panic(err) + } + + repoMetaTablename := "RepoMetadataTable" + uuid.String() + manifestDataTablename := "ManifestDataTable" + uuid.String() + versionTablename := "Version" + uuid.String() + + Convey("Create table errors", t, func() { + badEndpoint := endpoint + "1" + + customResolver := aws.EndpointResolverWithOptionsFunc( + func(service, region string, options ...interface{}) (aws.Endpoint, error) { + return aws.Endpoint{ + PartitionID: "aws", + URL: badEndpoint, + SigningRegion: region, + }, nil + }, + ) + + cfg, err := config.LoadDefaultConfig(context.Background(), config.WithRegion(region), + config.WithEndpointResolverWithOptions(customResolver)) + So(err, ShouldBeNil) + + dynamoWrapper := DBWrapper{ + Client: dynamodb.NewFromConfig(cfg), + RepoMetaTablename: repoMetaTablename, + ManifestDataTablename: manifestDataTablename, + VersionTablename: versionTablename, + Patches: version.GetDynamoDBPatches(), + Log: log.Logger{Logger: zerolog.New(os.Stdout)}, + } + + // The table creation should fail as the endpoint is not configured correctly + err = dynamoWrapper.createRepoMetaTable() + So(err, ShouldNotBeNil) + + err = dynamoWrapper.createManifestDataTable() + So(err, ShouldNotBeNil) + + err = dynamoWrapper.createVersionTable() + So(err, ShouldNotBeNil) + }) + + Convey("Delete table errors", t, func() { + customResolver := aws.EndpointResolverWithOptionsFunc( + func(service, region string, options ...interface{}) (aws.Endpoint, error) { + return aws.Endpoint{ + PartitionID: "aws", + URL: endpoint, + SigningRegion: region, + }, nil + }, + ) + + cfg, err := config.LoadDefaultConfig(context.Background(), config.WithRegion(region), + config.WithEndpointResolverWithOptions(customResolver)) + So(err, ShouldBeNil) + + dynamoWrapper := DBWrapper{ + Client: dynamodb.NewFromConfig(cfg), + RepoMetaTablename: repoMetaTablename, + ManifestDataTablename: manifestDataTablename, + VersionTablename: versionTablename, + Patches: version.GetDynamoDBPatches(), + Log: log.Logger{Logger: zerolog.New(os.Stdout)}, + } + + // The tables were not created so delete calls fail, but dynamoWrapper should not error + err = dynamoWrapper.deleteRepoMetaTable() + So(err, ShouldBeNil) + + err = dynamoWrapper.deleteManifestDataTable() + So(err, ShouldBeNil) + }) +} diff --git a/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go b/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go index 2f7fe636..451f441a 100644 --- a/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go +++ b/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue" "github.com/aws/aws-sdk-go-v2/service/dynamodb" "github.com/aws/aws-sdk-go-v2/service/dynamodb/types" + guuid "github.com/gofrs/uuid" "github.com/rs/zerolog" . "github.com/smartystreets/goconvey/convey" @@ -27,13 +28,22 @@ func TestIterator(t *testing.T) { region = "us-east-2" ) + uuid, err := guuid.NewV4() + if err != nil { + panic(err) + } + + repoMetaTablename := "RepoMetadataTable" + uuid.String() + manifestDataTablename := "ManifestDataTable" + uuid.String() + versionTablename := "Version" + uuid.String() + Convey("TestIterator", t, func() { dynamoWrapper, err := dynamo.NewDynamoDBWrapper(dynamoParams.DBDriverParameters{ Endpoint: endpoint, Region: region, - RepoMetaTablename: "RepoMetadataTable", - ManifestDataTablename: "ManifestDataTable", - VersionTablename: "Version", + RepoMetaTablename: repoMetaTablename, + ManifestDataTablename: manifestDataTablename, + VersionTablename: versionTablename, }) So(err, ShouldBeNil) @@ -51,7 +61,7 @@ func TestIterator(t *testing.T) { repoMetaAttributeIterator := iterator.NewBaseDynamoAttributesIterator( dynamoWrapper.Client, - "RepoMetadataTable", + repoMetaTablename, "RepoMetadata", 1, log.Logger{Logger: zerolog.New(os.Stdout)}, @@ -109,15 +119,24 @@ func TestWrapperErrors(t *testing.T) { region = "us-east-2" ) + uuid, err := guuid.NewV4() + if err != nil { + panic(err) + } + + repoMetaTablename := "RepoMetadataTable" + uuid.String() + manifestDataTablename := "ManifestDataTable" + uuid.String() + versionTablename := "Version" + uuid.String() + ctx := context.Background() Convey("Errors", t, func() { dynamoWrapper, err := dynamo.NewDynamoDBWrapper(dynamoParams.DBDriverParameters{ //nolint:contextcheck Endpoint: endpoint, Region: region, - RepoMetaTablename: "RepoMetadataTable", - ManifestDataTablename: "ManifestDataTable", - VersionTablename: "Version", + RepoMetaTablename: repoMetaTablename, + ManifestDataTablename: manifestDataTablename, + VersionTablename: versionTablename, }) So(err, ShouldBeNil) @@ -139,7 +158,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("GetManifestData unmarshal error", func() { - err := setBadManifestData(dynamoWrapper.Client, "dig") + err := setBadManifestData(dynamoWrapper.Client, manifestDataTablename, "dig") So(err, ShouldBeNil) _, err = dynamoWrapper.GetManifestData("dig") @@ -147,7 +166,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("SetManifestMeta GetRepoMeta error", func() { - err := setBadRepoMeta(dynamoWrapper.Client, "repo1") + err := setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo1") So(err, ShouldBeNil) err = dynamoWrapper.SetManifestMeta("repo1", "dig", repodb.ManifestMetadata{}) @@ -174,7 +193,7 @@ func TestWrapperErrors(t *testing.T) { err := dynamoWrapper.SetManifestData("dig", repodb.ManifestData{}) So(err, ShouldBeNil) - err = setBadRepoMeta(dynamoWrapper.Client, "repo") + err = setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") So(err, ShouldBeNil) _, err = dynamoWrapper.GetManifestMeta("repo", "dig") @@ -200,7 +219,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("DeleteRepoTag unmarshal error", func() { - err = setBadRepoMeta(dynamoWrapper.Client, "repo") + err = setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") So(err, ShouldBeNil) err = dynamoWrapper.DeleteRepoTag("repo", "tag") @@ -216,7 +235,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("GetRepoMeta unmarshal error", func() { - err = setBadRepoMeta(dynamoWrapper.Client, "repo") + err = setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") So(err, ShouldBeNil) _, err = dynamoWrapper.GetRepoMeta("repo") @@ -276,7 +295,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("DeleteSignature sigDigest.SignatureManifestDigest != sigMeta.SignatureDigest true", func() { - err := setRepoMeta(dynamoWrapper.Client, repodb.RepoMetadata{ + err := setRepoMeta(dynamoWrapper.Client, repoMetaTablename, repodb.RepoMetadata{ Name: "repo", Signatures: map[string]repodb.ManifestSignatures{ "tag1": { @@ -297,7 +316,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("GetMultipleRepoMeta unmarshal error", func() { - err = setBadRepoMeta(dynamoWrapper.Client, "repo") //nolint:contextcheck + err = setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") //nolint:contextcheck So(err, ShouldBeNil) _, err = dynamoWrapper.GetMultipleRepoMeta(ctx, func(repoMeta repodb.RepoMetadata) bool { return true }, @@ -307,7 +326,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("SearchRepos repoMeta unmarshal error", func() { - err = setBadRepoMeta(dynamoWrapper.Client, "repo") //nolint:contextcheck + err = setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") //nolint:contextcheck So(err, ShouldBeNil) _, _, err = dynamoWrapper.SearchRepos(ctx, "", repodb.Filter{}, repodb.PageInput{}) @@ -340,7 +359,7 @@ func TestWrapperErrors(t *testing.T) { }) Convey("SearchTags repoMeta unmarshal error", func() { - err = setBadRepoMeta(dynamoWrapper.Client, "repo") //nolint:contextcheck + err = setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") //nolint:contextcheck So(err, ShouldBeNil) _, _, err = dynamoWrapper.SearchTags(ctx, "repo:", repodb.Filter{}, repodb.PageInput{}) @@ -377,7 +396,7 @@ func TestWrapperErrors(t *testing.T) { }) } -func setBadManifestData(client *dynamodb.Client, digest string) error { +func setBadManifestData(client *dynamodb.Client, manifestDataTableName, digest string) error { mdAttributeValue, err := attributevalue.Marshal("string") if err != nil { return err @@ -395,14 +414,14 @@ func setBadManifestData(client *dynamodb.Client, digest string) error { Value: digest, }, }, - TableName: aws.String("ManifestDataTable"), + TableName: aws.String(manifestDataTableName), UpdateExpression: aws.String("SET #MD = :ManifestData"), }) return err } -func setBadRepoMeta(client *dynamodb.Client, repoName string) error { +func setBadRepoMeta(client *dynamodb.Client, repoMetadataTableName, repoName string) error { repoAttributeValue, err := attributevalue.Marshal("string") if err != nil { return err @@ -420,14 +439,14 @@ func setBadRepoMeta(client *dynamodb.Client, repoName string) error { Value: repoName, }, }, - TableName: aws.String("RepoMetadataTable"), + TableName: aws.String(repoMetadataTableName), UpdateExpression: aws.String("SET #RM = :RepoMetadata"), }) return err } -func setRepoMeta(client *dynamodb.Client, repoMeta repodb.RepoMetadata) error { +func setRepoMeta(client *dynamodb.Client, repoMetadataTableName string, repoMeta repodb.RepoMetadata) error { repoAttributeValue, err := attributevalue.Marshal(repoMeta) if err != nil { return err @@ -445,7 +464,7 @@ func setRepoMeta(client *dynamodb.Client, repoMeta repodb.RepoMetadata) error { Value: repoMeta.Name, }, }, - TableName: aws.String("RepoMetadataTable"), + TableName: aws.String(repoMetadataTableName), UpdateExpression: aws.String("SET #RM = :RepoMetadata"), }) diff --git a/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go b/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go index f529c4eb..2f8fa0a3 100644 --- a/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go +++ b/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go @@ -833,11 +833,11 @@ func (dwr DBWrapper) createRepoMetaTable() error { BillingMode: types.BillingModePayPerRequest, }) - if err != nil && strings.Contains(err.Error(), "Table already exists") { - return nil + if err != nil && !strings.Contains(err.Error(), "Table already exists") { + return err } - return err + return dwr.waitTableToBeCreated(dwr.RepoMetaTablename) } func (dwr DBWrapper) deleteRepoMetaTable() error { @@ -845,7 +845,11 @@ func (dwr DBWrapper) deleteRepoMetaTable() error { TableName: aws.String(dwr.RepoMetaTablename), }) - return err + if temp := new(types.ResourceNotFoundException); errors.As(err, &temp) { + return nil + } + + return dwr.waitTableToBeDeleted(dwr.RepoMetaTablename) } func (dwr DBWrapper) ResetRepoMetaTable() error { @@ -857,6 +861,26 @@ func (dwr DBWrapper) ResetRepoMetaTable() error { return dwr.createRepoMetaTable() } +func (dwr DBWrapper) waitTableToBeCreated(tableName string) error { + const maxWaitTime = 20 * time.Second + + waiter := dynamodb.NewTableExistsWaiter(dwr.Client) + + return waiter.Wait(context.Background(), &dynamodb.DescribeTableInput{ + TableName: &tableName, + }, maxWaitTime) +} + +func (dwr DBWrapper) waitTableToBeDeleted(tableName string) error { + const maxWaitTime = 20 * time.Second + + waiter := dynamodb.NewTableNotExistsWaiter(dwr.Client) + + return waiter.Wait(context.Background(), &dynamodb.DescribeTableInput{ + TableName: &tableName, + }, maxWaitTime) +} + func (dwr DBWrapper) createManifestDataTable() error { _, err := dwr.Client.CreateTable(context.Background(), &dynamodb.CreateTableInput{ TableName: aws.String(dwr.ManifestDataTablename), @@ -875,11 +899,11 @@ func (dwr DBWrapper) createManifestDataTable() error { BillingMode: types.BillingModePayPerRequest, }) - if err != nil && strings.Contains(err.Error(), "Table already exists") { - return nil + if err != nil && !strings.Contains(err.Error(), "Table already exists") { + return err } - return err + return dwr.waitTableToBeCreated(dwr.ManifestDataTablename) } func (dwr *DBWrapper) createVersionTable() error { @@ -899,9 +923,17 @@ func (dwr *DBWrapper) createVersionTable() error { }, BillingMode: types.BillingModePayPerRequest, }) + if err != nil { + if strings.Contains(err.Error(), "Table already exists") { + return nil + } - if err != nil && strings.Contains(err.Error(), "Table already exists") { - return nil + return err + } + + err = dwr.waitTableToBeCreated(dwr.VersionTablename) + if err != nil { + return err } if err == nil { @@ -931,7 +963,7 @@ func (dwr *DBWrapper) createVersionTable() error { } } - return err + return nil } func (dwr *DBWrapper) getDBVersion() (string, error) { @@ -964,7 +996,11 @@ func (dwr DBWrapper) deleteManifestDataTable() error { TableName: aws.String(dwr.ManifestDataTablename), }) - return err + if temp := new(types.ResourceNotFoundException); errors.As(err, &temp) { + return nil + } + + return dwr.waitTableToBeDeleted(dwr.ManifestDataTablename) } func (dwr DBWrapper) ResetManifestDataTable() error { diff --git a/pkg/meta/repodb/repodb_test.go b/pkg/meta/repodb/repodb_test.go index 461e69f5..0c3e337d 100644 --- a/pkg/meta/repodb/repodb_test.go +++ b/pkg/meta/repodb/repodb_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + guuid "github.com/gofrs/uuid" "github.com/opencontainers/go-digest" "github.com/opencontainers/image-spec/specs-go" ispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -65,12 +66,21 @@ func TestBoltDBWrapper(t *testing.T) { func TestDynamoDBWrapper(t *testing.T) { skipIt(t) + uuid, err := guuid.NewV4() + if err != nil { + panic(err) + } + + repoMetaTablename := "RepoMetadataTable" + uuid.String() + manifestDataTablename := "ManifestDataTable" + uuid.String() + versionTablename := "Version" + uuid.String() + Convey("DynamoDB Wrapper", t, func() { dynamoDBDriverParams := dynamoParams.DBDriverParameters{ Endpoint: os.Getenv("DYNAMODBMOCK_ENDPOINT"), - RepoMetaTablename: "RepoMetadataTable", - ManifestDataTablename: "ManifestDataTable", - VersionTablename: "Version", + RepoMetaTablename: repoMetaTablename, + ManifestDataTablename: manifestDataTablename, + VersionTablename: versionTablename, Region: "us-east-2", }