From fc07e836bd5af9d83c6945e061a5d9234033c45e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Sep 2022 12:21:10 -0400 Subject: [PATCH 1/5] improve: add destination table --- internal/server/data/destination.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/server/data/destination.go b/internal/server/data/destination.go index ffe66af5dc..6bd27c4208 100644 --- a/internal/server/data/destination.go +++ b/internal/server/data/destination.go @@ -9,6 +9,24 @@ import ( "github.com/infrahq/infra/uid" ) +type destinationsTable models.Destination + +func (d destinationsTable) Table() string { + return "destinations" +} + +func (d destinationsTable) Columns() []string { + return []string{"connection_ca", "connection_url", "created_at", "deleted_at", "id", "last_seen_at", "name", "organization_id", "resources", "roles", "unique_id", "updated_at", "version"} +} + +func (d destinationsTable) Values() []any { + return []any{d.ConnectionCA, d.ConnectionURL, d.CreatedAt, d.DeletedAt, d.ID, d.LastSeenAt, d.Name, d.OrganizationID, d.Resources, d.Roles, d.UniqueID, d.UpdatedAt, d.Version} +} + +func (d *destinationsTable) ScanFields() []any { + return []any{&d.ConnectionCA, &d.ConnectionURL, &d.CreatedAt, &d.DeletedAt, &d.ID, &d.LastSeenAt, &d.Name, &d.OrganizationID, &d.Resources, &d.Roles, &d.UniqueID, &d.UpdatedAt, &d.Version} +} + func validateDestination(dest *models.Destination) error { if dest.Name == "" { return fmt.Errorf("name is required") From f6c774eff4b9daeb85ad8224c7f5aceff70e3e52 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Sep 2022 12:43:32 -0400 Subject: [PATCH 2/5] improve: remove a sub-select in count query We can get the count directly without the sub-query. This does change the order of results but the only caller of this function does not care about the order because they are emitted as independent metrics. --- internal/server/data/destination.go | 22 ++++++++++---------- internal/server/data/destination_test.go | 26 ++++++++++++++++-------- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/internal/server/data/destination.go b/internal/server/data/destination.go index 6bd27c4208..054a4a0c81 100644 --- a/internal/server/data/destination.go +++ b/internal/server/data/destination.go @@ -74,23 +74,23 @@ func DeleteDestinations(db GormTxn, selector SelectorFunc) error { return internal.ErrNotFound } -type destinationsCount struct { +type DestinationsCount struct { Connected bool Version string Count float64 } -func CountDestinationsByConnectedVersion(tx ReadTxn) ([]destinationsCount, error) { +func CountDestinationsByConnectedVersion(tx ReadTxn) ([]DestinationsCount, error) { timeout := time.Now().Add(-5 * time.Minute) stmt := ` - SELECT *, COUNT(*) AS count - FROM ( - SELECT COALESCE(version, '') AS version, last_seen_at >= ? AS connected - FROM destinations - WHERE deleted_at IS NULL - ) AS d - GROUP BY version, connected` + SELECT COALESCE(version, '') as version, + last_seen_at >= ? as connected, + count(*) + FROM destinations + WHERE deleted_at IS NULL + GROUP BY connected, version + ` rows, err := tx.Query(stmt, timeout) if err != nil { return nil, err @@ -98,9 +98,9 @@ func CountDestinationsByConnectedVersion(tx ReadTxn) ([]destinationsCount, error } defer rows.Close() - var result []destinationsCount + var result []DestinationsCount for rows.Next() { - var item destinationsCount + var item DestinationsCount if err := rows.Scan(&item.Version, &item.Connected, &item.Count); err != nil { return nil, err } diff --git a/internal/server/data/destination_test.go b/internal/server/data/destination_test.go index 2f856d0ff0..46d43e35b0 100644 --- a/internal/server/data/destination_test.go +++ b/internal/server/data/destination_test.go @@ -33,22 +33,30 @@ func TestDestinationSaveCreatedPersists(t *testing.T) { func TestCountDestinationsByConnectedVersion(t *testing.T) { runDBTests(t, func(t *testing.T, db *DB) { - assert.NilError(t, CreateDestination(db, &models.Destination{Name: "1", UniqueID: "1", LastSeenAt: time.Now()})) - assert.NilError(t, CreateDestination(db, &models.Destination{Name: "2", UniqueID: "2", Version: "", LastSeenAt: time.Now().Add(-10 * time.Minute)})) - assert.NilError(t, CreateDestination(db, &models.Destination{Name: "3", UniqueID: "3", Version: "0.1.0", LastSeenAt: time.Now()})) - assert.NilError(t, CreateDestination(db, &models.Destination{Name: "4", UniqueID: "4", Version: "0.1.0"})) - assert.NilError(t, CreateDestination(db, &models.Destination{Name: "5", UniqueID: "5", Version: "0.1.0"})) - + createDestinations(t, db, + &models.Destination{Name: "1", UniqueID: "1", LastSeenAt: time.Now()}, + &models.Destination{Name: "2", UniqueID: "2", Version: "", LastSeenAt: time.Now().Add(-10 * time.Minute)}, + &models.Destination{Name: "3", UniqueID: "3", Version: "0.1.0", LastSeenAt: time.Now()}, + &models.Destination{Name: "4", UniqueID: "4", Version: "0.1.0"}, + &models.Destination{Name: "5", UniqueID: "5", Version: "0.1.0"}, + ) actual, err := CountDestinationsByConnectedVersion(db) assert.NilError(t, err) - expected := []destinationsCount{ + expected := []DestinationsCount{ {Connected: false, Version: "", Count: 1}, - {Connected: true, Version: "", Count: 1}, {Connected: false, Version: "0.1.0", Count: 2}, + {Connected: true, Version: "", Count: 1}, {Connected: true, Version: "0.1.0", Count: 1}, } - assert.DeepEqual(t, actual, expected) }) } + +func createDestinations(t *testing.T, tx GormTxn, destinations ...*models.Destination) { + t.Helper() + for i := range destinations { + err := CreateDestination(tx, destinations[i]) + assert.NilError(t, err, destinations[i].Name) + } +} From 3b5f89c667a37a9275fcbabef840568bd3c55592 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Sep 2022 12:55:05 -0400 Subject: [PATCH 3/5] improve: use SQL for create destination --- internal/server/data/data.go | 4 +- internal/server/data/destination.go | 9 ++-- internal/server/data/destination_test.go | 66 +++++++++++++++++++++++- 3 files changed, 73 insertions(+), 6 deletions(-) diff --git a/internal/server/data/data.go b/internal/server/data/data.go index b52a1d9d63..cce471f724 100644 --- a/internal/server/data/data.go +++ b/internal/server/data/data.go @@ -439,9 +439,9 @@ func handleError(err error) error { "idx_groups_name": "name", "idx_providers_name": "name", "idx_access_keys_name": "name", - "idx_destinations_unique_id": "uniqueId", + "idx_destinations_unique_id": "uniqueID", "idx_access_keys_key_id": "keyId", - "idx_credentials_identity_id": "identityId", + "idx_credentials_identity_id": "identityID", "idx_organizations_domain": "domain", } diff --git a/internal/server/data/destination.go b/internal/server/data/destination.go index 054a4a0c81..ba72291fa7 100644 --- a/internal/server/data/destination.go +++ b/internal/server/data/destination.go @@ -29,16 +29,19 @@ func (d *destinationsTable) ScanFields() []any { func validateDestination(dest *models.Destination) error { if dest.Name == "" { - return fmt.Errorf("name is required") + return fmt.Errorf("Destination.Name is required") + } + if dest.UniqueID == "" { + return fmt.Errorf("Destination.UniqueID is required") } return nil } -func CreateDestination(db GormTxn, destination *models.Destination) error { +func CreateDestination(db WriteTxn, destination *models.Destination) error { if err := validateDestination(destination); err != nil { return err } - return add(db, destination) + return insert(db, (*destinationsTable)(destination)) } func SaveDestination(db GormTxn, destination *models.Destination) error { diff --git a/internal/server/data/destination_test.go b/internal/server/data/destination_test.go index 46d43e35b0..4a480811a0 100644 --- a/internal/server/data/destination_test.go +++ b/internal/server/data/destination_test.go @@ -1,6 +1,7 @@ package data import ( + "errors" "testing" "time" @@ -9,10 +10,73 @@ import ( "github.com/infrahq/infra/internal/server/models" ) +func TestCreateDestination(t *testing.T) { + runDBTests(t, func(t *testing.T, db *DB) { + t.Run("success", func(t *testing.T) { + tx := txnForTestCase(t, db, db.DefaultOrg.ID) + + destination := &models.Destination{ + Name: "kubernetes", + UniqueID: "unique-id", + ConnectionURL: "10.0.0.1:1001", + ConnectionCA: "the-pem-encoded-cert", + LastSeenAt: time.Date(2022, 1, 2, 3, 4, 5, 600, time.UTC), + Version: "0.100.1", + Resources: []string{"res1", "res2"}, + Roles: []string{"role1", "role2"}, + } + + err := CreateDestination(tx, destination) + assert.NilError(t, err) + assert.Assert(t, destination.ID != 0) + + expected := &models.Destination{ + Model: models.Model{ + ID: destination.ID, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }, + OrganizationMember: models.OrganizationMember{OrganizationID: defaultOrganizationID}, + Name: "kubernetes", + UniqueID: "unique-id", + ConnectionURL: "10.0.0.1:1001", + ConnectionCA: "the-pem-encoded-cert", + LastSeenAt: time.Date(2022, 1, 2, 3, 4, 5, 600, time.UTC), + Version: "0.100.1", + Resources: []string{"res1", "res2"}, + Roles: []string{"role1", "role2"}, + } + assert.DeepEqual(t, destination, expected, cmpModel) + }) + t.Run("conflict on uniqueID", func(t *testing.T) { + tx := txnForTestCase(t, db, db.DefaultOrg.ID) + + destination := &models.Destination{ + Name: "kubernetes", + UniqueID: "unique-id", + } + err := CreateDestination(tx, destination) + assert.NilError(t, err) + assert.Assert(t, destination.ID != 0) + + next := &models.Destination{ + Name: "other", + UniqueID: "unique-id", + } + err = CreateDestination(tx, next) + var ucErr UniqueConstraintError + assert.Assert(t, errors.As(err, &ucErr)) + expected := UniqueConstraintError{Table: "destinations", Column: "uniqueID"} + assert.DeepEqual(t, ucErr, expected) + }) + }) +} + func TestDestinationSaveCreatedPersists(t *testing.T) { runDBTests(t, func(t *testing.T, db *DB) { destination := &models.Destination{ - Name: "example-cluster-1", + Name: "example-cluster-1", + UniqueID: "11111", } err := CreateDestination(db, destination) From 20fa80af51831829055283c2b419391e7cd648cf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Sep 2022 13:30:00 -0400 Subject: [PATCH 4/5] improve: use sql for update destination --- internal/access/destination.go | 2 +- internal/server/data/destination.go | 25 ++++++++-- internal/server/data/destination_test.go | 59 ++++++++++++++++++------ 3 files changed, 66 insertions(+), 20 deletions(-) diff --git a/internal/access/destination.go b/internal/access/destination.go index 6545ae9c1b..eb54530207 100644 --- a/internal/access/destination.go +++ b/internal/access/destination.go @@ -24,7 +24,7 @@ func SaveDestination(rCtx RequestContext, destination *models.Destination) error return HandleAuthErr(err, "destination", "update", roles...) } - return data.SaveDestination(rCtx.DBTxn, destination) + return data.UpdateDestination(rCtx.DBTxn, destination) } func GetDestination(c *gin.Context, id uid.ID) (*models.Destination, error) { diff --git a/internal/server/data/destination.go b/internal/server/data/destination.go index ba72291fa7..5fb5da0915 100644 --- a/internal/server/data/destination.go +++ b/internal/server/data/destination.go @@ -27,6 +27,23 @@ func (d *destinationsTable) ScanFields() []any { return []any{&d.ConnectionCA, &d.ConnectionURL, &d.CreatedAt, &d.DeletedAt, &d.ID, &d.LastSeenAt, &d.Name, &d.OrganizationID, &d.Resources, &d.Roles, &d.UniqueID, &d.UpdatedAt, &d.Version} } +// destinationsUpdateTable is used to update the destination. It excludes +// the CreatedAt field, because that field is not part of the input to +// UpdateDestination. +type destinationsUpdateTable models.Destination + +func (d destinationsUpdateTable) Table() string { + return "destinations" +} + +func (d destinationsUpdateTable) Columns() []string { + return []string{"connection_ca", "connection_url", "deleted_at", "id", "last_seen_at", "name", "organization_id", "resources", "roles", "unique_id", "updated_at", "version"} +} + +func (d destinationsUpdateTable) Values() []any { + return []any{d.ConnectionCA, d.ConnectionURL, d.DeletedAt, d.ID, d.LastSeenAt, d.Name, d.OrganizationID, d.Resources, d.Roles, d.UniqueID, d.UpdatedAt, d.Version} +} + func validateDestination(dest *models.Destination) error { if dest.Name == "" { return fmt.Errorf("Destination.Name is required") @@ -37,18 +54,18 @@ func validateDestination(dest *models.Destination) error { return nil } -func CreateDestination(db WriteTxn, destination *models.Destination) error { +func CreateDestination(tx WriteTxn, destination *models.Destination) error { if err := validateDestination(destination); err != nil { return err } - return insert(db, (*destinationsTable)(destination)) + return insert(tx, (*destinationsTable)(destination)) } -func SaveDestination(db GormTxn, destination *models.Destination) error { +func UpdateDestination(tx WriteTxn, destination *models.Destination) error { if err := validateDestination(destination); err != nil { return err } - return save(db, destination) + return update(tx, (*destinationsUpdateTable)(destination)) } func GetDestination(db GormTxn, selectors ...SelectorFunc) (*models.Destination, error) { diff --git a/internal/server/data/destination_test.go b/internal/server/data/destination_test.go index 4a480811a0..9ddb9a3e11 100644 --- a/internal/server/data/destination_test.go +++ b/internal/server/data/destination_test.go @@ -72,26 +72,55 @@ func TestCreateDestination(t *testing.T) { }) } -func TestDestinationSaveCreatedPersists(t *testing.T) { +func TestUpdateDestination(t *testing.T) { runDBTests(t, func(t *testing.T, db *DB) { - destination := &models.Destination{ - Name: "example-cluster-1", - UniqueID: "11111", - } + t.Run("success", func(t *testing.T) { + tx := txnForTestCase(t, db, db.DefaultOrg.ID) - err := CreateDestination(db, destination) - assert.NilError(t, err) - assert.Assert(t, !destination.CreatedAt.IsZero()) + created := time.Date(2022, 1, 2, 3, 4, 5, 600, time.UTC) + orig := &models.Destination{ + Model: models.Model{CreatedAt: created, UpdatedAt: created}, + Name: "example-cluster-1", + UniqueID: "11111", + } + createDestinations(t, tx, orig) - destination.Name = "example-cluster-2" - destination.CreatedAt = time.Time{} + // Unlike other update operations, the passed in destination + // may be constructed entirely by the caller and may not have the + // created, or updated time set. + destination := &models.Destination{ + Model: models.Model{ID: orig.ID}, + Name: "example-cluster-2", + UniqueID: "22222", + ConnectionURL: "dest.internal:10001", + ConnectionCA: "the-pem-encoded-cert", + Resources: []string{"res1", "res3"}, + Roles: []string{"role1"}, + Version: "0.100.2", + } + err := UpdateDestination(tx, destination) + assert.NilError(t, err) - err = SaveDestination(db, destination) - assert.NilError(t, err) + actual, err := GetDestination(tx, ByID(destination.ID)) + assert.NilError(t, err) - destination, err = GetDestination(db, ByID(destination.ID)) - assert.NilError(t, err) - assert.Assert(t, !destination.CreatedAt.IsZero()) + expected := &models.Destination{ + Model: models.Model{ + ID: destination.ID, + CreatedAt: created, + UpdatedAt: time.Now(), + }, + OrganizationMember: models.OrganizationMember{OrganizationID: defaultOrganizationID}, + Name: "example-cluster-2", + UniqueID: "22222", + ConnectionURL: "dest.internal:10001", + ConnectionCA: "the-pem-encoded-cert", + Resources: []string{"res1", "res3"}, + Roles: []string{"role1"}, + Version: "0.100.2", + } + assert.DeepEqual(t, actual, expected, cmpModel) + }) }) } From 7d900b2be2a8c1cad3318eabb7423d71a8fdb12d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 30 Sep 2022 13:39:16 -0400 Subject: [PATCH 5/5] improve: use sql for delete destination Previously we were returning ErrNotFound if the ID did not match any destination, but we don't do that for any other entity, and no tests appear to expect that behaviour. Removing that for now for consistency, but we can re-introduce it by checking the result for the number of updated rows if we want to restore it. --- internal/access/destination.go | 2 +- internal/server/data/destination.go | 24 +++++++----------------- internal/server/data/destination_test.go | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/internal/access/destination.go b/internal/access/destination.go index eb54530207..77961eeeeb 100644 --- a/internal/access/destination.go +++ b/internal/access/destination.go @@ -44,5 +44,5 @@ func DeleteDestination(c *gin.Context, id uid.ID) error { return HandleAuthErr(err, "destination", "delete", models.InfraAdminRole) } - return data.DeleteDestinations(db, data.ByID(id)) + return data.DeleteDestination(db, id) } diff --git a/internal/server/data/destination.go b/internal/server/data/destination.go index 5fb5da0915..3edae58aa7 100644 --- a/internal/server/data/destination.go +++ b/internal/server/data/destination.go @@ -4,7 +4,6 @@ import ( "fmt" "time" - "github.com/infrahq/infra/internal" "github.com/infrahq/infra/internal/server/models" "github.com/infrahq/infra/uid" ) @@ -76,22 +75,13 @@ func ListDestinations(db GormTxn, p *Pagination, selectors ...SelectorFunc) ([]m return list[models.Destination](db, p, selectors...) } -func DeleteDestinations(db GormTxn, selector SelectorFunc) error { - toDelete, err := ListDestinations(db, nil, selector) - if err != nil { - return err - } - - if len(toDelete) > 0 { - ids := make([]uid.ID, 0) - for _, g := range toDelete { - ids = append(ids, g.ID) - } - - return deleteAll[models.Destination](db, ByIDs(ids)) - } - - return internal.ErrNotFound +func DeleteDestination(tx WriteTxn, id uid.ID) error { + stmt := ` + UPDATE destinations SET deleted_at = ? + WHERE id = ? AND organization_id = ? AND deleted_at is null + ` + _, err := tx.Exec(stmt, time.Now(), id, tx.OrganizationID()) + return handleError(err) } type DestinationsCount struct { diff --git a/internal/server/data/destination_test.go b/internal/server/data/destination_test.go index 9ddb9a3e11..59b43ab848 100644 --- a/internal/server/data/destination_test.go +++ b/internal/server/data/destination_test.go @@ -7,6 +7,7 @@ import ( "gotest.tools/v3/assert" + "github.com/infrahq/infra/internal" "github.com/infrahq/infra/internal/server/models" ) @@ -124,6 +125,21 @@ func TestUpdateDestination(t *testing.T) { }) } +func TestDeleteDestination(t *testing.T) { + runDBTests(t, func(t *testing.T, db *DB) { + tx := txnForTestCase(t, db, db.DefaultOrg.ID) + + dest := &models.Destination{Name: "kube", UniqueID: "1111"} + createDestinations(t, tx, dest) + + err := DeleteDestination(tx, dest.ID) + assert.NilError(t, err) + + _, err = GetDestination(tx, ByID(dest.ID)) + assert.ErrorIs(t, err, internal.ErrNotFound) + }) +} + func TestCountDestinationsByConnectedVersion(t *testing.T) { runDBTests(t, func(t *testing.T, db *DB) { createDestinations(t, db,