From fcd7abc886f188c671a2b290ab82b895e55ec166 Mon Sep 17 00:00:00 2001 From: slonka Date: Mon, 27 Mar 2023 13:52:14 +0200 Subject: [PATCH 01/28] feat(persistance): prefix plugin and store with pq Signed-off-by: slonka --- pkg/plugins/resources/postgres/{plugin.go => pq_plugin.go} | 0 pkg/plugins/resources/postgres/{store.go => pq_store.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename pkg/plugins/resources/postgres/{plugin.go => pq_plugin.go} (100%) rename pkg/plugins/resources/postgres/{store.go => pq_store.go} (100%) diff --git a/pkg/plugins/resources/postgres/plugin.go b/pkg/plugins/resources/postgres/pq_plugin.go similarity index 100% rename from pkg/plugins/resources/postgres/plugin.go rename to pkg/plugins/resources/postgres/pq_plugin.go diff --git a/pkg/plugins/resources/postgres/store.go b/pkg/plugins/resources/postgres/pq_store.go similarity index 100% rename from pkg/plugins/resources/postgres/store.go rename to pkg/plugins/resources/postgres/pq_store.go From 5f5faab62570ebb16ca71a93911d5ebaab8e41e9 Mon Sep 17 00:00:00 2001 From: slonka Date: Mon, 27 Mar 2023 14:11:59 +0200 Subject: [PATCH 02/28] feat(persistance): compiles Signed-off-by: slonka --- pkg/core/plugins/registry.go | 1 + pkg/plugins/resources/postgres/consts.go | 3 + pkg/plugins/resources/postgres/pgx_plugin.go | 48 ++ pkg/plugins/resources/postgres/pgx_store.go | 386 ++++++++++ pkg/plugins/resources/postgres/pq_plugin.go | 14 +- pkg/plugins/resources/postgres/pq_store.go | 46 +- .../resources/postgres/resource_matadata.go | 41 ++ .../resources/postgres/store_template_test.go | 15 +- pkg/test/store/owner_test_template.go | 291 ++++---- pkg/test/store/store_test_template.go | 689 +++++++++--------- 10 files changed, 995 insertions(+), 539 deletions(-) create mode 100644 pkg/plugins/resources/postgres/consts.go create mode 100644 pkg/plugins/resources/postgres/pgx_plugin.go create mode 100644 pkg/plugins/resources/postgres/pgx_store.go create mode 100644 pkg/plugins/resources/postgres/resource_matadata.go diff --git a/pkg/core/plugins/registry.go b/pkg/core/plugins/registry.go index f8e2c01f3ece..f513b8b47fc8 100644 --- a/pkg/core/plugins/registry.go +++ b/pkg/core/plugins/registry.go @@ -26,6 +26,7 @@ const ( Universal PluginName = "universal" Memory PluginName = "memory" Postgres PluginName = "postgres" + Pgx PluginName = "pgx" CaBuiltin PluginName = "builtin" CaProvided PluginName = "provided" diff --git a/pkg/plugins/resources/postgres/consts.go b/pkg/plugins/resources/postgres/consts.go new file mode 100644 index 000000000000..8b1b7dfa3ad0 --- /dev/null +++ b/pkg/plugins/resources/postgres/consts.go @@ -0,0 +1,3 @@ +package postgres + +const duplicateKeyErrorMsg = "duplicate key value violates unique constraint" diff --git a/pkg/plugins/resources/postgres/pgx_plugin.go b/pkg/plugins/resources/postgres/pgx_plugin.go new file mode 100644 index 000000000000..7dff00e13de2 --- /dev/null +++ b/pkg/plugins/resources/postgres/pgx_plugin.go @@ -0,0 +1,48 @@ +package postgres + +import ( + "errors" + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" + "github.com/kumahq/kuma/pkg/core" + core_plugins "github.com/kumahq/kuma/pkg/core/plugins" + core_store "github.com/kumahq/kuma/pkg/core/resources/store" + "github.com/kumahq/kuma/pkg/core/runtime/component" + "github.com/kumahq/kuma/pkg/events" + postgres_events "github.com/kumahq/kuma/pkg/plugins/resources/postgres/events" +) + +var _ core_plugins.ResourceStorePlugin = &pgxPlugin{} + +type pgxPlugin struct{} + +func init() { + core_plugins.Register(core_plugins.Pgx, &pgxPlugin{}) +} + +func (p *pgxPlugin) NewResourceStore(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_store.ResourceStore, error) { + cfg, ok := config.(*postgres.PostgresStoreConfig) + if !ok { + return nil, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") + } + migrated, err := isDbMigrated(*cfg) + if err != nil { + return nil, err + } + if !migrated { + return nil, errors.New(`database is not migrated. Run "kuma-cp migrate up" to update database to the newest schema`) + } + return NewPgxStore(pc.Metrics(), *cfg) +} + +func (p *pgxPlugin) Migrate(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_plugins.DbVersion, error) { + cfg, ok := config.(*postgres.PostgresStoreConfig) + if !ok { + return 0, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") + } + return MigrateDb(*cfg) +} + +func (p *pgxPlugin) EventListener(pc core_plugins.PluginContext, out events.Emitter) error { + postgresListener := postgres_events.NewListener(*pc.Config().Store.Postgres, out) + return pc.ComponentManager().Add(component.NewResilientComponent(core.Log.WithName("postgres-event-listener-component"), postgresListener)) +} diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go new file mode 100644 index 000000000000..8e53a182a34e --- /dev/null +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -0,0 +1,386 @@ +package postgres + +import ( + "context" + "database/sql" + "fmt" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + "strconv" + "strings" + "time" + + _ "github.com/jackc/pgx/v5/stdlib" + _ "github.com/lib/pq" + "github.com/pkg/errors" + "github.com/prometheus/client_golang/prometheus" + + config "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" + core_model "github.com/kumahq/kuma/pkg/core/resources/model" + "github.com/kumahq/kuma/pkg/core/resources/store" + core_metrics "github.com/kumahq/kuma/pkg/metrics" +) + + +type pgxResourceStore struct { + pool *pgxpool.Pool +} + +var _ store.ResourceStore = &pgxResourceStore{} + +func NewPgxStore(metrics core_metrics.Metrics, config config.PostgresStoreConfig) (store.ResourceStore, error) { + pool, err := connect(config) + if err != nil { + return nil, err + } + + if err := registerMetrics(metrics, pool); err != nil { + return nil, errors.Wrapf(err, "could not register DB metrics") + } + + return &pgxResourceStore{ + pool: pool, + }, nil +} + +func connect(config config.PostgresStoreConfig) (*pgxpool.Pool, error) { + connectionString, err := config.ConnectionString() + if err != nil { + return nil, err + } + pgxConfig, err := pgxpool.ParseConfig(connectionString) + + if err != nil { + return nil, err + } + + return pgxpool.NewWithConfig(context.Background(), pgxConfig) +} + +func (r *pgxResourceStore) Create(ctx context.Context, resource core_model.Resource, fs ...store.CreateOptionsFunc) error { + opts := store.NewCreateOptions(fs...) + + bytes, err := core_model.ToJSON(resource.GetSpec()) + if err != nil { + return errors.Wrap(err, "failed to convert spec to json") + } + + var ownerName *string + var ownerMesh *string + var ownerType *string + + if opts.Owner != nil { + ptr := func(s string) *string { return &s } + ownerName = ptr(opts.Owner.GetMeta().GetName()) + ownerMesh = ptr(opts.Owner.GetMeta().GetMesh()) + ownerType = ptr(string(opts.Owner.Descriptor().Name)) + } + + version := 0 + statement := `INSERT INTO resources VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10);` + _, err = r.pool.Exec(ctx, statement, opts.Name, opts.Mesh, resource.Descriptor().Name, version, string(bytes), + opts.CreationTime.UTC(), opts.CreationTime.UTC(), ownerName, ownerMesh, ownerType) + if err != nil { + if strings.Contains(err.Error(), duplicateKeyErrorMsg) { + return store.ErrorResourceAlreadyExists(resource.Descriptor().Name, opts.Name, opts.Mesh) + } + return errors.Wrapf(err, "failed to execute query: %s", statement) + } + + resource.SetMeta(&resourceMetaObject{ + Name: opts.Name, + Mesh: opts.Mesh, + Version: strconv.Itoa(version), + CreationTime: opts.CreationTime, + ModificationTime: opts.CreationTime, + }) + return nil +} + +func (r *pgxResourceStore) Update(ctx context.Context, resource core_model.Resource, fs ...store.UpdateOptionsFunc) error { + bytes, err := core_model.ToJSON(resource.GetSpec()) + if err != nil { + return err + } + + opts := store.NewUpdateOptions(fs...) + + version, err := strconv.Atoi(resource.GetMeta().GetVersion()) + newVersion := version + 1 + if err != nil { + return errors.Wrap(err, "failed to convert meta version to int") + } + statement := `UPDATE resources SET spec=$1, version=$2, modification_time=$3 WHERE name=$4 AND mesh=$5 AND type=$6 AND version=$7;` + result, err := r.pool.Exec( + ctx, + statement, + string(bytes), + newVersion, + opts.ModificationTime.UTC(), + resource.GetMeta().GetName(), + resource.GetMeta().GetMesh(), + resource.Descriptor().Name, + version, + ) + if err != nil { + return errors.Wrapf(err, "failed to execute query %s", statement) + } + if rows := result.RowsAffected(); rows != 1 { // error ignored, postgres supports RowsAffected() + return store.ErrorResourceConflict(resource.Descriptor().Name, resource.GetMeta().GetName(), resource.GetMeta().GetMesh()) + } + + // update resource's meta with new version + resource.SetMeta(&resourceMetaObject{ + Name: resource.GetMeta().GetName(), + Mesh: resource.GetMeta().GetMesh(), + Version: strconv.Itoa(newVersion), + ModificationTime: opts.ModificationTime, + }) + + return nil +} + +func (r *pgxResourceStore) Delete(ctx context.Context, resource core_model.Resource, fs ...store.DeleteOptionsFunc) error { + opts := store.NewDeleteOptions(fs...) + + statement := `DELETE FROM resources WHERE name=$1 AND type=$2 AND mesh=$3` + result, err := r.pool.Exec(ctx, statement, opts.Name, resource.Descriptor().Name, opts.Mesh) + if err != nil { + return errors.Wrapf(err, "failed to execute query: %s", statement) + } + if rows := result.RowsAffected(); rows == 0 { // error ignored, postgres supports RowsAffected() + return store.ErrorResourceNotFound(resource.Descriptor().Name, opts.Name, opts.Mesh) + } + + return nil +} + +func (r *pgxResourceStore) Get(ctx context.Context, resource core_model.Resource, fs ...store.GetOptionsFunc) error { + opts := store.NewGetOptions(fs...) + + statement := `SELECT spec, version, creation_time, modification_time FROM resources WHERE name=$1 AND mesh=$2 AND type=$3;` + row := r.pool.QueryRow(ctx, statement, opts.Name, opts.Mesh, resource.Descriptor().Name) + + var spec string + var version int + var creationTime, modificationTime time.Time + err := row.Scan(&spec, &version, &creationTime, &modificationTime) + if err == sql.ErrNoRows { + return store.ErrorResourceNotFound(resource.Descriptor().Name, opts.Name, opts.Mesh) + } + if err != nil { + return errors.Wrapf(err, "failed to execute query: %s", statement) + } + + if err := core_model.FromJSON([]byte(spec), resource.GetSpec()); err != nil { + return errors.Wrap(err, "failed to convert json to spec") + } + + meta := &resourceMetaObject{ + Name: opts.Name, + Mesh: opts.Mesh, + Version: strconv.Itoa(version), + CreationTime: creationTime.Local(), + ModificationTime: modificationTime.Local(), + } + resource.SetMeta(meta) + + if opts.Version != "" && resource.GetMeta().GetVersion() != opts.Version { + return store.ErrorResourcePreconditionFailed(resource.Descriptor().Name, opts.Name, opts.Mesh) + } + return nil +} + +func (r *pgxResourceStore) List(ctx context.Context, resources core_model.ResourceList, args ...store.ListOptionsFunc) error { + opts := store.NewListOptions(args...) + + statement := `SELECT name, mesh, spec, version, creation_time, modification_time FROM resources WHERE type=$1` + var statementArgs []interface{} + statementArgs = append(statementArgs, resources.GetItemType()) + argsIndex := 1 + if opts.Mesh != "" { + argsIndex++ + statement += fmt.Sprintf(" AND mesh=$%d", argsIndex) + statementArgs = append(statementArgs, opts.Mesh) + } + if opts.NameContains != "" { + argsIndex++ + statement += fmt.Sprintf(" AND name LIKE $%d", argsIndex) + statementArgs = append(statementArgs, "%"+opts.NameContains+"%") + } + statement += " ORDER BY name, mesh" + + rows, err := r.pool.Query(ctx, statement, statementArgs...) + if err != nil { + return errors.Wrapf(err, "failed to execute query: %s", statement) + } + defer rows.Close() + + total := 0 + for rows.Next() { + item, err := rowToItem(resources, rows) + if err != nil { + return err + } + if err := resources.AddItem(item); err != nil { + return err + } + total++ + } + + resources.GetPagination().SetTotal(uint32(total)) + return nil +} + +func rowToItem(resources core_model.ResourceList, rows pgx.Rows) (core_model.Resource, error) { + var name, mesh, spec string + var version int + var creationTime, modificationTime time.Time + if err := rows.Scan(&name, &mesh, &spec, &version, &creationTime, &modificationTime); err != nil { + return nil, errors.Wrap(err, "failed to retrieve elements from query") + } + + item := resources.NewItem() + if err := core_model.FromJSON([]byte(spec), item.GetSpec()); err != nil { + return nil, errors.Wrap(err, "failed to convert json to spec") + } + + meta := &resourceMetaObject{ + Name: name, + Mesh: mesh, + Version: strconv.Itoa(version), + CreationTime: creationTime.Local(), + ModificationTime: modificationTime.Local(), + } + item.SetMeta(meta) + + return item, nil +} + +func (r *pgxResourceStore) Close() error { + r.pool.Close() + return nil +} + +func registerMetrics(metrics core_metrics.Metrics, pool *pgxpool.Pool) error { + postgresCurrentConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connections", + Help: "Current number of postgres store connections", + ConstLabels: map[string]string{ + "type": "open_connections", + }, + }, func() float64 { + return float64(pool.Stat().TotalConns()) + }) + + postgresInUseConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connections", + Help: "Current number of postgres store connections", + ConstLabels: map[string]string{ + "type": "in_use", + }, + }, func() float64 { + return float64(pool.Stat().AcquiredConns()) + }) + + postgresIdleConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connections", + Help: "Current number of postgres store connections", + ConstLabels: map[string]string{ + "type": "idle", + }, + }, func() float64 { + return float64(pool.Stat().IdleConns()) + }) + + postgresMaxOpenConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connections_max", + Help: "Max postgres store open connections", + }, func() float64 { + return float64(pool.Stat().MaxConns()) + }) + + postgresWaitConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connection_wait_count", + Help: "Current waiting postgres store connections", + }, func() float64 { + return float64(pool.Stat().EmptyAcquireCount()) + }) + + postgresWaitConnectionDurationMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connection_wait_duration", + Help: "Time Blocked waiting for new connection in seconds", + }, func() float64 { + return pool.Stat().AcquireDuration().Seconds() + }) + + postgresMaxIdleClosedConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connection_closed", + Help: "Current number of closed postgres store connections", + ConstLabels: map[string]string{ + "type": "max_idle_conns", + }, + }, func() float64 { + return float64(pool.Stat().MaxIdleDestroyCount()) + }) + + postgresMaxLifeTimeClosedConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connection_closed", + Help: "Current number of closed postgres store connections", + ConstLabels: map[string]string{ + "type": "conn_max_life_time", + }, + }, func() float64 { + return float64(pool.Stat().MaxLifetimeDestroyCount()) + }) + + postgresSuccessfulAcquireCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connection_acquire", + Help: "Cumulative count of successful acquires from the pool", + ConstLabels: map[string]string{ + "type": "successful", + }, + }, func() float64 { + return float64(pool.Stat().AcquireCount()) + }) + + postgresCanceledAcquireCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connection_acquire", + Help: "Cumulative count of acquires from the pool that were canceled by a context", + ConstLabels: map[string]string{ + "type": "canceled", + }, + }, func() float64 { + return float64(pool.Stat().CanceledAcquireCount()) + }) + + postgresConstructingConnectionsCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connections", + Help: "Number of connections with construction in progress in the pool", + ConstLabels: map[string]string{ + "type": "constructing", + }, + }, func() float64 { + return float64(pool.Stat().ConstructingConns()) + }) + + postgresNewConnectionsCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + Name: "store_postgres_connections", + Help: "Cumulative count of new connections opened", + ConstLabels: map[string]string{ + "type": "constructing", + }, + }, func() float64 { + return float64(pool.Stat().NewConnsCount()) + }) + + if err := metrics. + BulkRegister(postgresCurrentConnectionMetric, postgresInUseConnectionMetric, postgresIdleConnectionMetric, + postgresMaxOpenConnectionMetric, postgresWaitConnectionMetric, postgresWaitConnectionDurationMetric, + postgresMaxIdleClosedConnectionMetric, postgresMaxLifeTimeClosedConnectionMetric, postgresSuccessfulAcquireCountMetric, + postgresCanceledAcquireCountMetric, postgresConstructingConnectionsCountMetric, postgresNewConnectionsCountMetric, + ); err != nil { + return err + } + return nil +} diff --git a/pkg/plugins/resources/postgres/pq_plugin.go b/pkg/plugins/resources/postgres/pq_plugin.go index b8f66b26dc99..233b8886013c 100644 --- a/pkg/plugins/resources/postgres/pq_plugin.go +++ b/pkg/plugins/resources/postgres/pq_plugin.go @@ -12,15 +12,15 @@ import ( postgres_events "github.com/kumahq/kuma/pkg/plugins/resources/postgres/events" ) -var _ core_plugins.ResourceStorePlugin = &plugin{} +var _ core_plugins.ResourceStorePlugin = &pqPlugin{} -type plugin struct{} +type pqPlugin struct{} func init() { - core_plugins.Register(core_plugins.Postgres, &plugin{}) + core_plugins.Register(core_plugins.Postgres, &pqPlugin{}) } -func (p *plugin) NewResourceStore(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_store.ResourceStore, error) { +func (p *pqPlugin) NewResourceStore(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_store.ResourceStore, error) { cfg, ok := config.(*postgres.PostgresStoreConfig) if !ok { return nil, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") @@ -32,10 +32,10 @@ func (p *plugin) NewResourceStore(pc core_plugins.PluginContext, config core_plu if !migrated { return nil, errors.New(`database is not migrated. Run "kuma-cp migrate up" to update database to the newest schema`) } - return NewStore(pc.Metrics(), *cfg) + return NewPqStore(pc.Metrics(), *cfg) } -func (p *plugin) Migrate(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_plugins.DbVersion, error) { +func (p *pqPlugin) Migrate(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_plugins.DbVersion, error) { cfg, ok := config.(*postgres.PostgresStoreConfig) if !ok { return 0, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") @@ -43,7 +43,7 @@ func (p *plugin) Migrate(pc core_plugins.PluginContext, config core_plugins.Plug return MigrateDb(*cfg) } -func (p *plugin) EventListener(pc core_plugins.PluginContext, out events.Emitter) error { +func (p *pqPlugin) EventListener(pc core_plugins.PluginContext, out events.Emitter) error { postgresListener := postgres_events.NewListener(*pc.Config().Store.Postgres, out) return pc.ComponentManager().Add(component.NewResilientComponent(core.Log.WithName("postgres-event-listener-component"), postgresListener)) } diff --git a/pkg/plugins/resources/postgres/pq_store.go b/pkg/plugins/resources/postgres/pq_store.go index 8ac40c687c25..410c46322e85 100644 --- a/pkg/plugins/resources/postgres/pq_store.go +++ b/pkg/plugins/resources/postgres/pq_store.go @@ -20,21 +20,19 @@ import ( common_postgres "github.com/kumahq/kuma/pkg/plugins/common/postgres" ) -const duplicateKeyErrorMsg = "duplicate key value violates unique constraint" - type postgresResourceStore struct { db *sql.DB } var _ store.ResourceStore = &postgresResourceStore{} -func NewStore(metrics core_metrics.Metrics, config config.PostgresStoreConfig) (store.ResourceStore, error) { +func NewPqStore(metrics core_metrics.Metrics, config config.PostgresStoreConfig) (store.ResourceStore, error) { db, err := common_postgres.ConnectToDb(config) if err != nil { return nil, err } - if err := registerMetrics(metrics, db); err != nil { + if err := registerPqMetrics(metrics, db); err != nil { return nil, errors.Wrapf(err, "could not register DB metrics") } @@ -203,7 +201,7 @@ func (r *postgresResourceStore) List(_ context.Context, resources core_model.Res total := 0 for rows.Next() { - item, err := rowToItem(resources, rows) + item, err := pqRowToItem(resources, rows) if err != nil { return err } @@ -217,7 +215,7 @@ func (r *postgresResourceStore) List(_ context.Context, resources core_model.Res return nil } -func rowToItem(resources core_model.ResourceList, rows *sql.Rows) (core_model.Resource, error) { +func pqRowToItem(resources core_model.ResourceList, rows *sql.Rows) (core_model.Resource, error) { var name, mesh, spec string var version int var creationTime, modificationTime time.Time @@ -246,41 +244,7 @@ func (r *postgresResourceStore) Close() error { return r.db.Close() } -type resourceMetaObject struct { - Name string - Version string - Mesh string - CreationTime time.Time - ModificationTime time.Time -} - -var _ core_model.ResourceMeta = &resourceMetaObject{} - -func (r *resourceMetaObject) GetName() string { - return r.Name -} - -func (r *resourceMetaObject) GetNameExtensions() core_model.ResourceNameExtensions { - return core_model.ResourceNameExtensionsUnsupported -} - -func (r *resourceMetaObject) GetVersion() string { - return r.Version -} - -func (r *resourceMetaObject) GetMesh() string { - return r.Mesh -} - -func (r *resourceMetaObject) GetCreationTime() time.Time { - return r.CreationTime -} - -func (r *resourceMetaObject) GetModificationTime() time.Time { - return r.ModificationTime -} - -func registerMetrics(metrics core_metrics.Metrics, db *sql.DB) error { +func registerPqMetrics(metrics core_metrics.Metrics, db *sql.DB) error { postgresCurrentConnectionMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ Name: "store_postgres_connections", Help: "Current number of postgres store connections", diff --git a/pkg/plugins/resources/postgres/resource_matadata.go b/pkg/plugins/resources/postgres/resource_matadata.go new file mode 100644 index 000000000000..d5b3399a3815 --- /dev/null +++ b/pkg/plugins/resources/postgres/resource_matadata.go @@ -0,0 +1,41 @@ +package postgres + +import ( + core_model "github.com/kumahq/kuma/pkg/core/resources/model" + "time" +) + +type resourceMetaObject struct { + Name string + Version string + Mesh string + CreationTime time.Time + ModificationTime time.Time +} + +var _ core_model.ResourceMeta = &resourceMetaObject{} + +func (r *resourceMetaObject) GetName() string { + return r.Name +} + +func (r *resourceMetaObject) GetNameExtensions() core_model.ResourceNameExtensions { + return core_model.ResourceNameExtensionsUnsupported +} + +func (r *resourceMetaObject) GetVersion() string { + return r.Version +} + +func (r *resourceMetaObject) GetMesh() string { + return r.Mesh +} + +func (r *resourceMetaObject) GetCreationTime() time.Time { + return r.CreationTime +} + +func (r *resourceMetaObject) GetModificationTime() time.Time { + return r.ModificationTime +} + diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 20eebfd60f66..61ab5b3dd30e 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -11,7 +11,7 @@ import ( ) var _ = Describe("PostgresStore template", func() { - createStore := func() store.ResourceStore { + createStore := func(storeName string) store.ResourceStore { cfg, err := c.Config(test_postgres.WithRandomDb) Expect(err).ToNot(HaveOccurred()) @@ -21,12 +21,19 @@ var _ = Describe("PostgresStore template", func() { _, err = MigrateDb(*cfg) Expect(err).ToNot(HaveOccurred()) - pStore, err := NewStore(metrics, *cfg) + var pStore store.ResourceStore + if storeName == "pgx" { + pStore, err = NewPgxStore(metrics, *cfg) + } else { + pStore, err = NewPqStore(metrics, *cfg) + } Expect(err).ToNot(HaveOccurred()) return pStore } - test_store.ExecuteStoreTests(createStore) - test_store.ExecuteOwnerTests(createStore) + test_store.ExecuteStoreTests(createStore, "pgx") + test_store.ExecuteOwnerTests(createStore, "pgx") + test_store.ExecuteStoreTests(createStore, "pq") + test_store.ExecuteOwnerTests(createStore, "pq") }) diff --git a/pkg/test/store/owner_test_template.go b/pkg/test/store/owner_test_template.go index 5937723e23f8..e2fe23a70ddd 100644 --- a/pkg/test/store/owner_test_template.go +++ b/pkg/test/store/owner_test_template.go @@ -15,13 +15,14 @@ import ( ) func ExecuteOwnerTests( - createStore func() store.ResourceStore, + createStore func(name string) store.ResourceStore, + storeName string, ) { const mesh = "default-mesh" var s store.ClosableResourceStore BeforeEach(func() { - s = store.NewStrictResourceStore(createStore()) + s = store.NewStrictResourceStore(createStore(storeName)) }) AfterEach(func() { @@ -29,81 +30,15 @@ func ExecuteOwnerTests( Expect(err).ToNot(HaveOccurred()) }) - It("should delete resource when its owner is deleted", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - name := "resource-1" - trRes := core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, - }, - }, - } - err = s.Create(context.Background(), &trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) - - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - // then - actual := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) - }) - - It("should delete resource when its owner is deleted after owner update", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - name := "resource-1" - trRes := core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, - }, - }, - } - err = s.Create(context.Background(), &trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) - - // when owner is updated - Expect(s.Update(context.Background(), meshRes)).To(Succeed()) - - // and only then deleted - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - // then - actual := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) - }) - - It("should delete several resources when their owner is deleted", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + Context("Store: " + storeName, func() { + It("should delete resource when its owner is deleted", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - for i := 0; i < 10; i++ { - tr := core_mesh.TrafficRouteResource{ + name := "resource-1" + trRes := core_mesh.TrafficRouteResource{ Spec: &v1alpha1.TrafficRoute{ Conf: &v1alpha1.TrafficRoute_Conf{ Destination: map[string]string{ @@ -112,37 +47,30 @@ func ExecuteOwnerTests( }, }, } - err = s.Create(context.Background(), &tr, - store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + err = s.Create(context.Background(), &trRes, + store.CreateByKey(name, mesh), store.CreatedAt(time.Now()), store.CreateWithOwner(meshRes)) Expect(err).ToNot(HaveOccurred()) - } - actual := core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(HaveLen(10)) - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual = core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(BeEmpty()) - }) + // then + actual := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) + }) - It("should delete owners chain", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete resource when its owner is deleted after owner update", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - var prev model.Resource = meshRes - for i := 0; i < 10; i++ { - curr := &core_mesh.TrafficRouteResource{ + name := "resource-1" + trRes := core_mesh.TrafficRouteResource{ Spec: &v1alpha1.TrafficRoute{ Conf: &v1alpha1.TrafficRoute_Conf{ Destination: map[string]string{ @@ -151,62 +79,137 @@ func ExecuteOwnerTests( }, }, } - err := s.Create(context.Background(), curr, - store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + err = s.Create(context.Background(), &trRes, + store.CreateByKey(name, mesh), store.CreatedAt(time.Now()), - store.CreateWithOwner(prev)) + store.CreateWithOwner(meshRes)) Expect(err).ToNot(HaveOccurred()) - prev = curr - } - actual := core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(HaveLen(10)) + // when owner is updated + Expect(s.Update(context.Background(), meshRes)).To(Succeed()) - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // and only then deleted + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual = core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(BeEmpty()) - }) + // then + actual := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) + }) - It("should delete a parent after children is deleted", func() { - // given - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete several resources when their owner is deleted", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - name := "resource-1" - trRes := &core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", + for i := 0; i < 10; i++ { + tr := core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, + }, + }, + } + err = s.Create(context.Background(), &tr, + store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) + } + actual := core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(HaveLen(10)) + + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + // then + actual = core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(BeEmpty()) + }) + + It("should delete owners chain", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + var prev model.Resource = meshRes + for i := 0; i < 10; i++ { + curr := &core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, + }, + }, + } + err := s.Create(context.Background(), curr, + store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(prev)) + Expect(err).ToNot(HaveOccurred()) + prev = curr + } + + actual := core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(HaveLen(10)) + + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + // then + actual = core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(BeEmpty()) + }) + + It("should delete a parent after children is deleted", func() { + // given + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + name := "resource-1" + trRes := &core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, }, }, - }, - } - err = s.Create(context.Background(), trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) + } + err = s.Create(context.Background(), trRes, + store.CreateByKey(name, mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) - // when children is deleted - err = s.Delete(context.Background(), core_mesh.NewTrafficRouteResource(), store.DeleteByKey(name, mesh)) + // when children is deleted + err = s.Delete(context.Background(), core_mesh.NewTrafficRouteResource(), store.DeleteByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // when parent is deleted - err = s.Delete(context.Background(), core_mesh.NewMeshResource(), store.DeleteByKey(mesh, model.NoMesh)) + // when parent is deleted + err = s.Delete(context.Background(), core_mesh.NewMeshResource(), store.DeleteByKey(mesh, model.NoMesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) + }) }) } diff --git a/pkg/test/store/store_test_template.go b/pkg/test/store/store_test_template.go index 12e25887950a..80270601a030 100644 --- a/pkg/test/store/store_test_template.go +++ b/pkg/test/store/store_test_template.go @@ -17,13 +17,14 @@ import ( ) func ExecuteStoreTests( - createStore func() store.ResourceStore, + createStore func(name string) store.ResourceStore, + storeName string, ) { const mesh = "default-mesh" var s store.ClosableResourceStore BeforeEach(func() { - s = store.NewStrictResourceStore(store.NewPaginationStore(createStore())) + s = store.NewStrictResourceStore(store.NewPaginationStore(createStore(storeName))) }) AfterEach(func() { @@ -56,434 +57,436 @@ func ExecuteStoreTests( return &res } - Describe("Create()", func() { - It("should create a new resource", func() { - // given - name := "resource1.demo" + Context("Store: "+storeName, func() { + Describe("Create()", func() { + It("should create a new resource", func() { + // given + name := "resource1.demo" - // when - created := createResource(name) + // when + created := createResource(name) - // when retrieve created object - resource := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when retrieve created object + resource := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // and it has same data - Expect(resource.Meta.GetName()).To(Equal(name)) - Expect(resource.Meta.GetMesh()).To(Equal(mesh)) - Expect(resource.Meta.GetVersion()).ToNot(BeEmpty()) - Expect(resource.Meta.GetCreationTime().Unix()).ToNot(Equal(0)) - Expect(resource.Meta.GetCreationTime()).To(Equal(resource.Meta.GetModificationTime())) - Expect(resource.Spec).To(MatchProto(created.Spec)) - }) + // and it has same data + Expect(resource.Meta.GetName()).To(Equal(name)) + Expect(resource.Meta.GetMesh()).To(Equal(mesh)) + Expect(resource.Meta.GetVersion()).ToNot(BeEmpty()) + Expect(resource.Meta.GetCreationTime().Unix()).ToNot(Equal(0)) + Expect(resource.Meta.GetCreationTime()).To(Equal(resource.Meta.GetModificationTime())) + Expect(resource.Spec).To(MatchProto(created.Spec)) + }) - It("should not create a duplicate record", func() { - // given - name := "duplicated-record.demo" - resource := createResource(name) + It("should not create a duplicate record", func() { + // given + name := "duplicated-record.demo" + resource := createResource(name) - // when try to create another one with same name - resource.SetMeta(nil) - err := s.Create(context.Background(), resource, store.CreateByKey(name, mesh)) + // when try to create another one with same name + resource.SetMeta(nil) + err := s.Create(context.Background(), resource, store.CreateByKey(name, mesh)) - // then - Expect(err).To(MatchError(store.ErrorResourceAlreadyExists(resource.Descriptor().Name, name, mesh))) + // then + Expect(err).To(MatchError(store.ErrorResourceAlreadyExists(resource.Descriptor().Name, name, mesh))) + }) }) - }) - - Describe("Update()", func() { - It("should return an error if resource is not found", func() { - // given - name := "to-be-updated.demo" - resource := createResource(name) - - // when delete resource - err := s.Delete( - context.Background(), - resource, - store.DeleteByKey(resource.Meta.GetName(), mesh), - ) - // then - Expect(err).ToNot(HaveOccurred()) + Describe("Update()", func() { + It("should return an error if resource is not found", func() { + // given + name := "to-be-updated.demo" + resource := createResource(name) - // when trying to update nonexistent resource - err = s.Update(context.Background(), resource) + // when delete resource + err := s.Delete( + context.Background(), + resource, + store.DeleteByKey(resource.Meta.GetName(), mesh), + ) - // then - Expect(err).To(MatchError(store.ErrorResourceConflict(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).ToNot(HaveOccurred()) - It("should update an existing resource", func() { - // given a resources in storage - name := "to-be-updated.demo" - resource := createResource(name) - modificationTime := time.Now().Add(time.Second) - versionBeforeUpdate := resource.Meta.GetVersion() + // when trying to update nonexistent resource + err = s.Update(context.Background(), resource) - // when - resource.Spec.Conf.Destination["path"] = "new-path" - err := s.Update(context.Background(), resource, store.ModifiedAt(modificationTime)) + // then + Expect(err).To(MatchError(store.ErrorResourceConflict(resource.Descriptor().Name, name, mesh))) + }) - // then - Expect(err).ToNot(HaveOccurred()) + It("should update an existing resource", func() { + // given a resources in storage + name := "to-be-updated.demo" + resource := createResource(name) + modificationTime := time.Now().Add(time.Second) + versionBeforeUpdate := resource.Meta.GetVersion() - // and meta is updated (version and modification time) - Expect(resource.Meta.GetVersion()).ToNot(Equal(versionBeforeUpdate)) - if reflect.TypeOf(createStore()) != reflect.TypeOf(&resources_k8s.KubernetesStore{}) { - Expect(resource.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) - } + // when + resource.Spec.Conf.Destination["path"] = "new-path" + err := s.Update(context.Background(), resource, store.ModifiedAt(modificationTime)) - // when retrieve the resource - res := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), res, store.GetByKey(name, mesh)) + // then + Expect(err).ToNot(HaveOccurred()) - // then - Expect(err).ToNot(HaveOccurred()) + // and meta is updated (version and modification time) + Expect(resource.Meta.GetVersion()).ToNot(Equal(versionBeforeUpdate)) + if reflect.TypeOf(createStore(storeName)) != reflect.TypeOf(&resources_k8s.KubernetesStore{}) { + Expect(resource.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) + } - // and - Expect(res.Spec.Conf.Destination["path"]).To(Equal("new-path")) - - // and modification time is updated - // on K8S modification time is always the creation time, because there is no data for modification time - if reflect.TypeOf(createStore()) == reflect.TypeOf(&resources_k8s.KubernetesStore{}) { - Expect(res.Meta.GetModificationTime()).To(Equal(res.Meta.GetCreationTime())) - } else { - Expect(res.Meta.GetModificationTime()).ToNot(Equal(res.Meta.GetCreationTime())) - Expect(res.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) - } - }) + // when retrieve the resource + res := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), res, store.GetByKey(name, mesh)) - // todo(jakubdyszkiewicz) write tests for optimistic locking - }) + // then + Expect(err).ToNot(HaveOccurred()) - Describe("Delete()", func() { - It("should throw an error if resource is not found", func() { - // given - name := "non-existent-name.demo" - resource := core_mesh.NewTrafficRouteResource() + // and + Expect(res.Spec.Conf.Destination["path"]).To(Equal("new-path")) - // when - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) + // and modification time is updated + // on K8S modification time is always the creation time, because there is no data for modification time + if reflect.TypeOf(createStore(storeName)) == reflect.TypeOf(&resources_k8s.KubernetesStore{}) { + Expect(res.Meta.GetModificationTime()).To(Equal(res.Meta.GetCreationTime())) + } else { + Expect(res.Meta.GetModificationTime()).ToNot(Equal(res.Meta.GetCreationTime())) + Expect(res.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) + } + }) - // then - Expect(err).To(HaveOccurred()) - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + // todo(jakubdyszkiewicz) write tests for optimistic locking }) - It("should not delete resource from another mesh", func() { - // given - name := "tr-1.demo" - resource := createResource(name) + Describe("Delete()", func() { + It("should throw an error if resource is not found", func() { + // given + name := "non-existent-name.demo" + resource := core_mesh.NewTrafficRouteResource() - // when - resource.SetMeta(nil) // otherwise the validation from strict client fires that mesh is different - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, "different-mesh")) + // when + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) - // then - Expect(err).To(HaveOccurred()) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) + // then + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - // and when getting the given resource - getResource := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), getResource, store.GetByKey(name, mesh)) + It("should not delete resource from another mesh", func() { + // given + name := "tr-1.demo" + resource := createResource(name) - // then resource still exists - Expect(err).ToNot(HaveOccurred()) - }) + // when + resource.SetMeta(nil) // otherwise the validation from strict client fires that mesh is different + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, "different-mesh")) - It("should delete an existing resource", func() { - // given a resources in storage - name := "to-be-deleted.demo" - createResource(name) + // then + Expect(err).To(HaveOccurred()) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) - // when - resource := core_mesh.NewTrafficRouteResource() - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) + // and when getting the given resource + getResource := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), getResource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then resource still exists + Expect(err).ToNot(HaveOccurred()) + }) - // when query for deleted resource - resource = core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + It("should delete an existing resource", func() { + // given a resources in storage + name := "to-be-deleted.demo" + createResource(name) - // then resource cannot be found - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) - }) + // when + resource := core_mesh.NewTrafficRouteResource() + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) - Describe("Get()", func() { - It("should return an error if resource is not found", func() { - // given - name := "non-existing-resource.demo" - resource := core_mesh.NewTrafficRouteResource() + // then + Expect(err).ToNot(HaveOccurred()) - // when - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when query for deleted resource + resource = core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).To(MatchError(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + // then resource cannot be found + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) }) - It("should return an error if resource is not found in given mesh", func() { - // given a resources in mesh "mesh" - name := "existing-resource.demo" - mesh := "different-mesh" - createResource(name) + Describe("Get()", func() { + It("should return an error if resource is not found", func() { + // given + name := "non-existing-resource.demo" + resource := core_mesh.NewTrafficRouteResource() - // when - resource := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).To(MatchError(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - It("should return an existing resource", func() { - // given a resources in storage - name := "get-existing-resource.demo" - createdResource := createResource(name) + It("should return an error if resource is not found in given mesh", func() { + // given a resources in mesh "mesh" + name := "existing-resource.demo" + mesh := "different-mesh" + createResource(name) - // when - res := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), res, store.GetByKey(name, mesh)) + // when + resource := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - // and - Expect(res.Meta.GetName()).To(Equal(name)) - Expect(res.Meta.GetVersion()).ToNot(BeEmpty()) - Expect(res.Spec).To(MatchProto(createdResource.Spec)) - }) + It("should return an existing resource", func() { + // given a resources in storage + name := "get-existing-resource.demo" + createdResource := createResource(name) - It("should get resource by version", func() { - // given - name := "existing-resource.demo" - res := createResource(name) + // when + res := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), res, store.GetByKey(name, mesh)) - // when trying to retrieve resource with proper version - err := s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion(res.GetMeta().GetVersion())) + // then + Expect(err).ToNot(HaveOccurred()) - // then resource is found - Expect(err).ToNot(HaveOccurred()) + // and + Expect(res.Meta.GetName()).To(Equal(name)) + Expect(res.Meta.GetVersion()).ToNot(BeEmpty()) + Expect(res.Spec).To(MatchProto(createdResource.Spec)) + }) - // when trying to retrieve resource with different version - err = s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion("9999999")) + It("should get resource by version", func() { + // given + name := "existing-resource.demo" + res := createResource(name) - // then resource precondition failed error occurred - Expect(store.IsResourcePreconditionFailed(err)).To(BeTrue()) - }) - }) + // when trying to retrieve resource with proper version + err := s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion(res.GetMeta().GetVersion())) - Describe("List()", func() { - It("should return an empty list if there are no matching resources", func() { - // given - list := core_mesh.TrafficRouteResourceList{} + // then resource is found + Expect(err).ToNot(HaveOccurred()) - // when - err := s.List(context.Background(), &list, store.ListByMesh(mesh)) + // when trying to retrieve resource with different version + err = s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion("9999999")) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(0))) - // and - Expect(list.Items).To(HaveLen(0)) + // then resource precondition failed error occurred + Expect(store.IsResourcePreconditionFailed(err)).To(BeTrue()) + }) }) - It("should return a list of resources", func() { - // given two resources - createResource("res-1.demo") - createResource("res-2.demo") + Describe("List()", func() { + It("should return an empty list if there are no matching resources", func() { + // given + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list, store.ListByMesh(mesh)) - // when - err := s.List(context.Background(), &list) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(0))) + // and + Expect(list.Items).To(HaveLen(0)) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(HaveLen(2)) - // and - names := []string{list.Items[0].Meta.GetName(), list.Items[1].Meta.GetName()} - Expect(names).To(ConsistOf("res-1.demo", "res-2.demo")) - Expect(list.Items[0].Meta.GetMesh()).To(Equal(mesh)) - Expect(list.Items[0].Spec.Conf.Destination["path"]).To(Equal("demo")) - Expect(list.Items[1].Meta.GetMesh()).To(Equal(mesh)) - Expect(list.Items[1].Spec.Conf.Destination["path"]).To(Equal("demo")) - }) + It("should return a list of resources", func() { + // given two resources + createResource("res-1.demo") + createResource("res-2.demo") - It("should not return a list of resources in different mesh", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list) - // when - err := s.List(context.Background(), &list, store.ListByMesh("different-mesh")) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(HaveLen(2)) + // and + names := []string{list.Items[0].Meta.GetName(), list.Items[1].Meta.GetName()} + Expect(names).To(ConsistOf("res-1.demo", "res-2.demo")) + Expect(list.Items[0].Meta.GetMesh()).To(Equal(mesh)) + Expect(list.Items[0].Spec.Conf.Destination["path"]).To(Equal("demo")) + Expect(list.Items[1].Meta.GetMesh()).To(Equal(mesh)) + Expect(list.Items[1].Spec.Conf.Destination["path"]).To(Equal("demo")) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(0))) - // and - Expect(list.Items).To(HaveLen(0)) - }) + It("should not return a list of resources in different mesh", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") - It("should return a list of resources with prefix from all meshes", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") - createResource("list-mes-1.demo") + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list, store.ListByMesh("different-mesh")) - // when - err := s.List(context.Background(), &list, store.ListByNameContains("list-res")) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(0))) + // and + Expect(list.Items).To(HaveLen(0)) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { - var res []string - for _, v := range itms { - res = append(res, v.GetMeta().GetName()) - } - return res - }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) - }) + It("should return a list of resources with prefix from all meshes", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") + createResource("list-mes-1.demo") - It("should return a list of resources with prefix from the specific mesh", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") - createResource("list-mes-1.demo") + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list, store.ListByNameContains("list-res")) - // when - err := s.List(context.Background(), &list, store.ListByNameContains("list-res"), store.ListByMesh(mesh)) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { + var res []string + for _, v := range itms { + res = append(res, v.GetMeta().GetName()) + } + return res + }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { - var res []string - for _, v := range itms { - res = append(res, v.GetMeta().GetName()) - } - return res - }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) - }) + It("should return a list of resources with prefix from the specific mesh", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") + createResource("list-mes-1.demo") - Describe("Pagination", func() { - It("should list all resources using pagination", func() { - // given - offset := "" - pageSize := 2 - numOfResources := 5 - resourceNames := map[string]bool{} - - // setup create resources - for i := 0; i < numOfResources; i++ { - createResource(fmt.Sprintf("res-%d.demo", i)) - } + list := core_mesh.TrafficRouteResourceList{} + + // when + err := s.List(context.Background(), &list, store.ListByNameContains("list-res"), store.ListByMesh(mesh)) + + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { + var res []string + for _, v := range itms { + res = append(res, v.GetMeta().GetName()) + } + return res + }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) + }) - // when list first two pages with 2 elements - for i := 1; i <= 2; i++ { + Describe("Pagination", func() { + It("should list all resources using pagination", func() { + // given + offset := "" + pageSize := 2 + numOfResources := 5 + resourceNames := map[string]bool{} + + // setup create resources + for i := 0; i < numOfResources; i++ { + createResource(fmt.Sprintf("res-%d.demo", i)) + } + + // when list first two pages with 2 elements + for i := 1; i <= 2; i++ { + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) + + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).ToNot(BeEmpty()) + Expect(list.Items).To(HaveLen(2)) + + resourceNames[list.Items[0].GetMeta().GetName()] = true + resourceNames[list.Items[1].GetMeta().GetName()] = true + offset = list.Pagination.NextOffset + } + + // when list third page with 1 element (less than page size) list := core_mesh.TrafficRouteResourceList{} err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) + // then Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).ToNot(BeEmpty()) - Expect(list.Items).To(HaveLen(2)) - + Expect(list.Pagination.Total).To(Equal(uint32(numOfResources))) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + Expect(list.Items).To(HaveLen(1)) resourceNames[list.Items[0].GetMeta().GetName()] = true - resourceNames[list.Items[1].GetMeta().GetName()] = true - offset = list.Pagination.NextOffset - } - // when list third page with 1 element (less than page size) - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) - - // then - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.Total).To(Equal(uint32(numOfResources))) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - Expect(list.Items).To(HaveLen(1)) - resourceNames[list.Items[0].GetMeta().GetName()] = true - - // and all elements were retrieved - Expect(resourceNames).To(HaveLen(numOfResources)) - for i := 0; i < numOfResources; i++ { - Expect(resourceNames).To(HaveKey(fmt.Sprintf("res-%d.demo", i))) - } - }) + // and all elements were retrieved + Expect(resourceNames).To(HaveLen(numOfResources)) + for i := 0; i < numOfResources; i++ { + Expect(resourceNames).To(HaveKey(fmt.Sprintf("res-%d.demo", i))) + } + }) - It("next offset should be null when queried collection with less elements than page has", func() { - // setup - createResource("res-1.demo") + It("next offset should be null when queried collection with less elements than page has", func() { + // setup + createResource("res-1.demo") - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(5, "")) + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(5, "")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(1))) - Expect(list.Items).To(HaveLen(1)) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // then + Expect(list.Pagination.Total).To(Equal(uint32(1))) + Expect(list.Items).To(HaveLen(1)) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - It("next offset should be null when queried about size equals to elements available", func() { - // setup - createResource("res-1.demo") + It("next offset should be null when queried about size equals to elements available", func() { + // setup + createResource("res-1.demo") - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(1, "")) + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(1, "")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(1))) - Expect(list.Items).To(HaveLen(1)) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // then + Expect(list.Pagination.Total).To(Equal(uint32(1))) + Expect(list.Items).To(HaveLen(1)) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - It("next offset should be null when queried empty collection", func() { - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "")) + It("next offset should be null when queried empty collection", func() { + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(0))) - Expect(list.Items).To(BeEmpty()) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // then + Expect(list.Pagination.Total).To(Equal(uint32(0))) + Expect(list.Items).To(BeEmpty()) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - It("next offset should return error when query with invalid offset", func() { - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "123invalidOffset")) + It("next offset should return error when query with invalid offset", func() { + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "123invalidOffset")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(0))) - Expect(err).To(Equal(store.ErrorInvalidOffset)) + // then + Expect(list.Pagination.Total).To(Equal(uint32(0))) + Expect(err).To(Equal(store.ErrorInvalidOffset)) + }) }) }) }) From e2d54a9d54ea56020a3dbc4b811fe4ae6b547ae3 Mon Sep 17 00:00:00 2001 From: slonka Date: Mon, 27 Mar 2023 14:30:18 +0200 Subject: [PATCH 03/28] feat(persistance): wire in pgx tests Signed-off-by: slonka --- pkg/plugins/resources/postgres/listener_test.go | 17 +++++++++++------ pkg/plugins/resources/postgres/pgx_store.go | 12 ++++++------ .../resources/postgres/store_template_test.go | 14 +++++++++----- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/pkg/plugins/resources/postgres/listener_test.go b/pkg/plugins/resources/postgres/listener_test.go index 194b26fe0f88..3c4cceb87d7d 100644 --- a/pkg/plugins/resources/postgres/listener_test.go +++ b/pkg/plugins/resources/postgres/listener_test.go @@ -40,7 +40,7 @@ var _ = Describe("Events", func() { defer close(eventBusStopCh) defer close(listenerErrCh) listener := setupListeners(cfg, driverName, listenerErrCh, listenerStopCh) - go triggerNotifications(cfg, storeErrCh) + go triggerNotifications(cfg, driverName, storeErrCh) // when event, err := listener.Recv(eventBusStopCh) @@ -67,7 +67,7 @@ var _ = Describe("Events", func() { listenerStopCh, listenerErrCh, eventBusStopCh, storeErrCh := setupChannels() defer close(eventBusStopCh) listener := setupListeners(cfg, driverName, listenerErrCh, listenerStopCh) - go triggerNotifications(cfg, storeErrCh) + go triggerNotifications(cfg, driverName, storeErrCh) // when event, err := listener.Recv(eventBusStopCh) @@ -110,10 +110,15 @@ func setupChannels() (chan struct{}, chan error, chan struct{}, chan error) { return listenerStopCh, listenerErrCh, eventBusStopCh, storeErrCh } -func setupStore(cfg postgres_config.PostgresStoreConfig) store.ResourceStore { +func setupStore(cfg postgres_config.PostgresStoreConfig, driverName string) store.ResourceStore { metrics, err := core_metrics.NewMetrics("Standalone") Expect(err).ToNot(HaveOccurred()) - pStore, err := NewStore(metrics, cfg) + var pStore store.ResourceStore + if driverName == "pgx" { + pStore, err = NewPgxStore(metrics, cfg) + } else { + pStore, err = NewPqStore(metrics, cfg) + } Expect(err).ToNot(HaveOccurred()) return pStore } @@ -131,8 +136,8 @@ func setupListeners(cfg postgres_config.PostgresStoreConfig, driverName string, return listener } -func triggerNotifications(cfg postgres_config.PostgresStoreConfig, storeErrCh chan error) { - pStore := setupStore(cfg) +func triggerNotifications(cfg postgres_config.PostgresStoreConfig, driverName string, storeErrCh chan error) { + pStore := setupStore(cfg, driverName) defer GinkgoRecover() for i := 0; !channels.IsClosed(storeErrCh); i++ { err := pStore.Create(context.Background(), mesh.NewMeshResource(), store.CreateByKey(fmt.Sprintf("mesh-%d", i), "")) diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index 8e53a182a34e..f654484b22e8 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -49,7 +49,7 @@ func connect(config config.PostgresStoreConfig) (*pgxpool.Pool, error) { return nil, err } pgxConfig, err := pgxpool.ParseConfig(connectionString) - + if err != nil { return nil, err } @@ -336,7 +336,7 @@ func registerMetrics(metrics core_metrics.Metrics, pool *pgxpool.Pool) error { postgresSuccessfulAcquireCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ Name: "store_postgres_connection_acquire", - Help: "Cumulative count of successful acquires from the pool", + Help: "Cumulative count of acquires from the pool", ConstLabels: map[string]string{ "type": "successful", }, @@ -346,7 +346,7 @@ func registerMetrics(metrics core_metrics.Metrics, pool *pgxpool.Pool) error { postgresCanceledAcquireCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ Name: "store_postgres_connection_acquire", - Help: "Cumulative count of acquires from the pool that were canceled by a context", + Help: "Cumulative count of acquires from the pool", ConstLabels: map[string]string{ "type": "canceled", }, @@ -356,7 +356,7 @@ func registerMetrics(metrics core_metrics.Metrics, pool *pgxpool.Pool) error { postgresConstructingConnectionsCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ Name: "store_postgres_connections", - Help: "Number of connections with construction in progress in the pool", + Help: "Current number of postgres store connections", ConstLabels: map[string]string{ "type": "constructing", }, @@ -366,9 +366,9 @@ func registerMetrics(metrics core_metrics.Metrics, pool *pgxpool.Pool) error { postgresNewConnectionsCountMetric := prometheus.NewGaugeFunc(prometheus.GaugeOpts{ Name: "store_postgres_connections", - Help: "Cumulative count of new connections opened", + Help: "Current number of postgres store connections", ConstLabels: map[string]string{ - "type": "constructing", + "type": "new", }, }, func() float64 { return float64(pool.Stat().NewConnsCount()) diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 61ab5b3dd30e..3370db3d4a0f 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -15,7 +15,10 @@ var _ = Describe("PostgresStore template", func() { cfg, err := c.Config(test_postgres.WithRandomDb) Expect(err).ToNot(HaveOccurred()) - metrics, err := core_metrics.NewMetrics("Standalone") + pqMetrics, err := core_metrics.NewMetrics("Standalone") + Expect(err).ToNot(HaveOccurred()) + + pgxMetrics, err := core_metrics.NewMetrics("Standalone") Expect(err).ToNot(HaveOccurred()) _, err = MigrateDb(*cfg) @@ -23,17 +26,18 @@ var _ = Describe("PostgresStore template", func() { var pStore store.ResourceStore if storeName == "pgx" { - pStore, err = NewPgxStore(metrics, *cfg) + pStore, err = NewPgxStore(pqMetrics, *cfg) } else { - pStore, err = NewPqStore(metrics, *cfg) + pStore, err = NewPqStore(pgxMetrics, *cfg) } Expect(err).ToNot(HaveOccurred()) return pStore } - test_store.ExecuteStoreTests(createStore, "pgx") - test_store.ExecuteOwnerTests(createStore, "pgx") + //pgx tests failing + //test_store.ExecuteStoreTests(createStore, "pgx") + //test_store.ExecuteOwnerTests(createStore, "pgx") test_store.ExecuteStoreTests(createStore, "pq") test_store.ExecuteOwnerTests(createStore, "pq") }) From 5f14b54682ace8e76d2e141c29a7f6695ae72a01 Mon Sep 17 00:00:00 2001 From: slonka Date: Mon, 27 Mar 2023 14:41:01 +0200 Subject: [PATCH 04/28] feat(persistance): wire in pgx store for store operations Signed-off-by: slonka --- pkg/config/core/resources/store/config.go | 1 + pkg/config/plugins/resources/postgres/config.go | 2 +- pkg/core/bootstrap/bootstrap.go | 10 +++++++++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/config/core/resources/store/config.go b/pkg/config/core/resources/store/config.go index 162223a17e22..222a24f69baa 100644 --- a/pkg/config/core/resources/store/config.go +++ b/pkg/config/core/resources/store/config.go @@ -18,6 +18,7 @@ type StoreType = string const ( KubernetesStore StoreType = "kubernetes" PostgresStore StoreType = "postgres" + PgxStore StoreType = "pgx" MemoryStore StoreType = "memory" ) diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index c7d58e6c6a3c..6eb52a0a9852 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -188,7 +188,7 @@ func DefaultPostgresStoreConfig() *PostgresStoreConfig { User: "kuma", Password: "kuma", DbName: "kuma", - DriverName: "pgx", + DriverName: DriverNamePgx, ConnectionTimeout: 5, MaxOpenConnections: 50, // 0 for unlimited MaxIdleConnections: 50, // 0 for unlimited diff --git a/pkg/core/bootstrap/bootstrap.go b/pkg/core/bootstrap/bootstrap.go index 24bc982765e1..a74b0c454e40 100644 --- a/pkg/core/bootstrap/bootstrap.go +++ b/pkg/core/bootstrap/bootstrap.go @@ -2,6 +2,7 @@ package bootstrap import ( "context" + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" "net" "github.com/pkg/errors" @@ -257,7 +258,14 @@ func initializeResourceStore(cfg kuma_cp.Config, builder *core_runtime.Builder) pluginName = core_plugins.Memory pluginConfig = nil case store.PostgresStore: - pluginName = core_plugins.Postgres + switch cfg.Store.Postgres.DriverName { + case postgres.DriverNamePgx: + pluginName = core_plugins.Pgx + case postgres.DriverNamePq: + pluginName = core_plugins.Postgres + default: + return errors.Errorf("unknown driver name %s", cfg.Store.Postgres.DriverName) + } pluginConfig = cfg.Store.Postgres default: return errors.Errorf("unknown store type %s", cfg.Store.Type) From 72bbcfa914c0c10540924c1ea482da6d3a9176f9 Mon Sep 17 00:00:00 2001 From: slonka Date: Tue, 28 Mar 2023 08:37:58 +0200 Subject: [PATCH 05/28] feat(persistance): fixed update test Signed-off-by: slonka --- pkg/core/bootstrap/bootstrap.go | 3 +- pkg/plugins/resources/postgres/pgx_plugin.go | 1 + pkg/plugins/resources/postgres/pgx_store.go | 14 ++++------ .../resources/postgres/resource_matadata.go | 28 +++++++++---------- .../resources/postgres/store_template_test.go | 5 ++-- pkg/test/store/owner_test_template.go | 2 +- tools/postgres/Dockerfile | 5 ++-- 7 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/core/bootstrap/bootstrap.go b/pkg/core/bootstrap/bootstrap.go index a74b0c454e40..a270ad699b27 100644 --- a/pkg/core/bootstrap/bootstrap.go +++ b/pkg/core/bootstrap/bootstrap.go @@ -2,9 +2,9 @@ package bootstrap import ( "context" - "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" "net" + "github.com/pkg/errors" mesh_proto "github.com/kumahq/kuma/api/mesh/v1alpha1" @@ -12,6 +12,7 @@ import ( kuma_cp "github.com/kumahq/kuma/pkg/config/app/kuma-cp" config_core "github.com/kumahq/kuma/pkg/config/core" "github.com/kumahq/kuma/pkg/config/core/resources/store" + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" "github.com/kumahq/kuma/pkg/core" config_manager "github.com/kumahq/kuma/pkg/core/config/manager" "github.com/kumahq/kuma/pkg/core/datasource" diff --git a/pkg/plugins/resources/postgres/pgx_plugin.go b/pkg/plugins/resources/postgres/pgx_plugin.go index 7dff00e13de2..075485afb7e6 100644 --- a/pkg/plugins/resources/postgres/pgx_plugin.go +++ b/pkg/plugins/resources/postgres/pgx_plugin.go @@ -2,6 +2,7 @@ package postgres import ( "errors" + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" "github.com/kumahq/kuma/pkg/core" core_plugins "github.com/kumahq/kuma/pkg/core/plugins" diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index f654484b22e8..08ca6b3fe740 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -2,14 +2,14 @@ package postgres import ( "context" - "database/sql" "fmt" - "github.com/jackc/pgx/v5" - "github.com/jackc/pgx/v5/pgxpool" "strconv" "strings" "time" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + _ "github.com/jackc/pgx/v5/stdlib" _ "github.com/lib/pq" "github.com/pkg/errors" @@ -21,7 +21,6 @@ import ( core_metrics "github.com/kumahq/kuma/pkg/metrics" ) - type pgxResourceStore struct { pool *pgxpool.Pool } @@ -49,11 +48,10 @@ func connect(config config.PostgresStoreConfig) (*pgxpool.Pool, error) { return nil, err } pgxConfig, err := pgxpool.ParseConfig(connectionString) - if err != nil { return nil, err } - + return pgxpool.NewWithConfig(context.Background(), pgxConfig) } @@ -165,7 +163,7 @@ func (r *pgxResourceStore) Get(ctx context.Context, resource core_model.Resource var version int var creationTime, modificationTime time.Time err := row.Scan(&spec, &version, &creationTime, &modificationTime) - if err == sql.ErrNoRows { + if err == pgx.ErrNoRows { return store.ErrorResourceNotFound(resource.Descriptor().Name, opts.Name, opts.Mesh) } if err != nil { @@ -258,7 +256,7 @@ func rowToItem(resources core_model.ResourceList, rows pgx.Rows) (core_model.Res } func (r *pgxResourceStore) Close() error { - r.pool.Close() + r.pool.Close() // does not work, at least on my machine return nil } diff --git a/pkg/plugins/resources/postgres/resource_matadata.go b/pkg/plugins/resources/postgres/resource_matadata.go index d5b3399a3815..72ef048e539d 100644 --- a/pkg/plugins/resources/postgres/resource_matadata.go +++ b/pkg/plugins/resources/postgres/resource_matadata.go @@ -1,41 +1,41 @@ package postgres import ( - core_model "github.com/kumahq/kuma/pkg/core/resources/model" - "time" + "time" + + core_model "github.com/kumahq/kuma/pkg/core/resources/model" ) type resourceMetaObject struct { - Name string - Version string - Mesh string - CreationTime time.Time - ModificationTime time.Time + Name string + Version string + Mesh string + CreationTime time.Time + ModificationTime time.Time } var _ core_model.ResourceMeta = &resourceMetaObject{} func (r *resourceMetaObject) GetName() string { - return r.Name + return r.Name } func (r *resourceMetaObject) GetNameExtensions() core_model.ResourceNameExtensions { - return core_model.ResourceNameExtensionsUnsupported + return core_model.ResourceNameExtensionsUnsupported } func (r *resourceMetaObject) GetVersion() string { - return r.Version + return r.Version } func (r *resourceMetaObject) GetMesh() string { - return r.Mesh + return r.Mesh } func (r *resourceMetaObject) GetCreationTime() time.Time { - return r.CreationTime + return r.CreationTime } func (r *resourceMetaObject) GetModificationTime() time.Time { - return r.ModificationTime + return r.ModificationTime } - diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 3370db3d4a0f..3a719446b757 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -35,9 +35,8 @@ var _ = Describe("PostgresStore template", func() { return pStore } - //pgx tests failing - //test_store.ExecuteStoreTests(createStore, "pgx") - //test_store.ExecuteOwnerTests(createStore, "pgx") + test_store.ExecuteStoreTests(createStore, "pgx") + test_store.ExecuteOwnerTests(createStore, "pgx") test_store.ExecuteStoreTests(createStore, "pq") test_store.ExecuteOwnerTests(createStore, "pq") }) diff --git a/pkg/test/store/owner_test_template.go b/pkg/test/store/owner_test_template.go index e2fe23a70ddd..ced0e8e94d5b 100644 --- a/pkg/test/store/owner_test_template.go +++ b/pkg/test/store/owner_test_template.go @@ -30,7 +30,7 @@ func ExecuteOwnerTests( Expect(err).ToNot(HaveOccurred()) }) - Context("Store: " + storeName, func() { + Context("Store: "+storeName, func() { It("should delete resource when its owner is deleted", func() { // setup meshRes := core_mesh.NewMeshResource() diff --git a/tools/postgres/Dockerfile b/tools/postgres/Dockerfile index 9c05919e3ee9..3d830fd154dd 100644 --- a/tools/postgres/Dockerfile +++ b/tools/postgres/Dockerfile @@ -8,8 +8,9 @@ COPY certs/rootCA.crt /var/lib/postgresql/rootCA.crt COPY certs/postgres.server.crt /var/lib/postgresql/postgres.server.crt COPY certs/postgres.server.key /var/lib/postgresql/postgres.server.key RUN chown -R postgres /var/lib/postgresql && \ - chmod 600 /var/lib/postgresql/postgres.server.key -CMD ["-c", "ssl=on", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] + chmod 600 /var/lib/postgresql/postgres.server.key \ +# if I bump max_connections to 1000 the tests pass on my machine +CMD ["-c", "ssl=on", "-c", "max_connections=100", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] FROM postgres:latest AS pg-standard FROM pg-${MODE} From 53bee832e59107375f5d548c150a20e72bd4c653 Mon Sep 17 00:00:00 2001 From: slonka Date: Tue, 28 Mar 2023 08:45:49 +0200 Subject: [PATCH 06/28] feat(persistance): fix other tests Signed-off-by: slonka --- pkg/core/bootstrap/bootstrap.go | 2 -- pkg/plugins/resources/k8s/store_template_test.go | 4 ++-- pkg/plugins/resources/memory/store_template_test.go | 5 +++-- pkg/plugins/resources/postgres/pgx_store.go | 1 - 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/core/bootstrap/bootstrap.go b/pkg/core/bootstrap/bootstrap.go index a270ad699b27..e2bd4a0939c5 100644 --- a/pkg/core/bootstrap/bootstrap.go +++ b/pkg/core/bootstrap/bootstrap.go @@ -3,8 +3,6 @@ package bootstrap import ( "context" "net" - - "github.com/pkg/errors" mesh_proto "github.com/kumahq/kuma/api/mesh/v1alpha1" diff --git a/pkg/plugins/resources/k8s/store_template_test.go b/pkg/plugins/resources/k8s/store_template_test.go index d5d55b5d80a8..94ee69ca022b 100644 --- a/pkg/plugins/resources/k8s/store_template_test.go +++ b/pkg/plugins/resources/k8s/store_template_test.go @@ -13,7 +13,7 @@ import ( ) var _ = Describe("KubernetesStore template", func() { - test_store.ExecuteStoreTests(func() store.ResourceStore { + test_store.ExecuteStoreTests(func(string) store.ResourceStore { kubeTypes := k8s_registry.NewTypeRegistry() Expect(kubeTypes.RegisterObjectType(&mesh_proto.TrafficRoute{}, &mesh_k8s.TrafficRoute{})).To(Succeed()) @@ -30,5 +30,5 @@ var _ = Describe("KubernetesStore template", func() { }, Scheme: k8sClientScheme, } - }) + }, "kubernetes") }) diff --git a/pkg/plugins/resources/memory/store_template_test.go b/pkg/plugins/resources/memory/store_template_test.go index f1e9b951146a..525c2852c638 100644 --- a/pkg/plugins/resources/memory/store_template_test.go +++ b/pkg/plugins/resources/memory/store_template_test.go @@ -1,6 +1,7 @@ package memory_test import ( + "github.com/kumahq/kuma/pkg/core/resources/store" . "github.com/onsi/ginkgo/v2" "github.com/kumahq/kuma/pkg/plugins/resources/memory" @@ -8,6 +9,6 @@ import ( ) var _ = Describe("MemoryStore template", func() { - test_store.ExecuteStoreTests(memory.NewStore) - test_store.ExecuteOwnerTests(memory.NewStore) + test_store.ExecuteStoreTests(func(string) store.ResourceStore { return memory.NewStore() }, "memory") + test_store.ExecuteOwnerTests(func(string) store.ResourceStore { return memory.NewStore() }, "memory") }) diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index 08ca6b3fe740..5b896fbd9533 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -9,7 +9,6 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" - _ "github.com/jackc/pgx/v5/stdlib" _ "github.com/lib/pq" "github.com/pkg/errors" From 687c3c7694f3d2afcd2d9d67ec63a3adf8501fc2 Mon Sep 17 00:00:00 2001 From: slonka Date: Tue, 28 Mar 2023 08:49:13 +0200 Subject: [PATCH 07/28] feat(persistance): make check pass Signed-off-by: slonka --- pkg/core/bootstrap/bootstrap.go | 1 + pkg/plugins/resources/memory/store_template_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/core/bootstrap/bootstrap.go b/pkg/core/bootstrap/bootstrap.go index e2bd4a0939c5..f2fdf8c1de8e 100644 --- a/pkg/core/bootstrap/bootstrap.go +++ b/pkg/core/bootstrap/bootstrap.go @@ -3,6 +3,7 @@ package bootstrap import ( "context" "net" + "github.com/pkg/errors" mesh_proto "github.com/kumahq/kuma/api/mesh/v1alpha1" diff --git a/pkg/plugins/resources/memory/store_template_test.go b/pkg/plugins/resources/memory/store_template_test.go index 525c2852c638..ff131861dfad 100644 --- a/pkg/plugins/resources/memory/store_template_test.go +++ b/pkg/plugins/resources/memory/store_template_test.go @@ -1,9 +1,9 @@ package memory_test import ( - "github.com/kumahq/kuma/pkg/core/resources/store" . "github.com/onsi/ginkgo/v2" + "github.com/kumahq/kuma/pkg/core/resources/store" "github.com/kumahq/kuma/pkg/plugins/resources/memory" test_store "github.com/kumahq/kuma/pkg/test/store" ) From 1dd3c1326d3d6408a031ae643bef69b65a04dd2f Mon Sep 17 00:00:00 2001 From: slonka Date: Tue, 28 Mar 2023 09:20:49 +0200 Subject: [PATCH 08/28] feat(persistance): remove dockerfile modification Signed-off-by: slonka --- tools/postgres/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/postgres/Dockerfile b/tools/postgres/Dockerfile index 3d830fd154dd..0c2ca528133e 100644 --- a/tools/postgres/Dockerfile +++ b/tools/postgres/Dockerfile @@ -10,7 +10,7 @@ COPY certs/postgres.server.key /var/lib/postgresql/postgres.server.key RUN chown -R postgres /var/lib/postgresql && \ chmod 600 /var/lib/postgresql/postgres.server.key \ # if I bump max_connections to 1000 the tests pass on my machine -CMD ["-c", "ssl=on", "-c", "max_connections=100", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] +CMD ["-c", "ssl=on", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] FROM postgres:latest AS pg-standard FROM pg-${MODE} From c13c0ac637ab16767bc03c692a4a801e56976960 Mon Sep 17 00:00:00 2001 From: slonka Date: Tue, 28 Mar 2023 10:25:47 +0200 Subject: [PATCH 09/28] ci(circleci): kick ci Signed-off-by: slonka From d060e3464e75dd1619be437ec51d6a521e03bfd3 Mon Sep 17 00:00:00 2001 From: slonka Date: Tue, 28 Mar 2023 11:30:11 +0200 Subject: [PATCH 10/28] feat(persistance): fix dockerfile Signed-off-by: slonka --- tools/postgres/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/postgres/Dockerfile b/tools/postgres/Dockerfile index 0c2ca528133e..d1fbefcef4ca 100644 --- a/tools/postgres/Dockerfile +++ b/tools/postgres/Dockerfile @@ -8,7 +8,7 @@ COPY certs/rootCA.crt /var/lib/postgresql/rootCA.crt COPY certs/postgres.server.crt /var/lib/postgresql/postgres.server.crt COPY certs/postgres.server.key /var/lib/postgresql/postgres.server.key RUN chown -R postgres /var/lib/postgresql && \ - chmod 600 /var/lib/postgresql/postgres.server.key \ + chmod 600 /var/lib/postgresql/postgres.server.key # if I bump max_connections to 1000 the tests pass on my machine CMD ["-c", "ssl=on", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] FROM postgres:latest AS pg-standard From f3b37aede2682902ac65ea4f80e19e6c78909ec7 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 08:30:00 +0200 Subject: [PATCH 11/28] feat(persistance): tests pass locally Signed-off-by: slonka --- pkg/plugins/resources/postgres/listener_test.go | 2 ++ pkg/plugins/resources/postgres/pgx_store.go | 1 - pkg/plugins/resources/postgres/pq_store.go | 1 - pkg/plugins/resources/postgres/store_template_test.go | 7 +++++-- tools/postgres/Dockerfile | 3 +-- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/plugins/resources/postgres/listener_test.go b/pkg/plugins/resources/postgres/listener_test.go index 3c4cceb87d7d..39a0bc707259 100644 --- a/pkg/plugins/resources/postgres/listener_test.go +++ b/pkg/plugins/resources/postgres/listener_test.go @@ -115,8 +115,10 @@ func setupStore(cfg postgres_config.PostgresStoreConfig, driverName string) stor Expect(err).ToNot(HaveOccurred()) var pStore store.ResourceStore if driverName == "pgx" { + cfg.DriverName = postgres_config.DriverNamePgx pStore, err = NewPgxStore(metrics, cfg) } else { + cfg.DriverName = postgres_config.DriverNamePq pStore, err = NewPqStore(metrics, cfg) } Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index 5b896fbd9533..358567c62f1b 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -10,7 +10,6 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" _ "github.com/jackc/pgx/v5/stdlib" - _ "github.com/lib/pq" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" diff --git a/pkg/plugins/resources/postgres/pq_store.go b/pkg/plugins/resources/postgres/pq_store.go index 410c46322e85..ee25cc1c022a 100644 --- a/pkg/plugins/resources/postgres/pq_store.go +++ b/pkg/plugins/resources/postgres/pq_store.go @@ -8,7 +8,6 @@ import ( "strings" "time" - _ "github.com/jackc/pgx/v5/stdlib" _ "github.com/lib/pq" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 3a719446b757..599315476b44 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -1,6 +1,7 @@ package postgres import ( + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -26,9 +27,11 @@ var _ = Describe("PostgresStore template", func() { var pStore store.ResourceStore if storeName == "pgx" { - pStore, err = NewPgxStore(pqMetrics, *cfg) + cfg.DriverName = postgres.DriverNamePgx + pStore, err = NewPgxStore(pgxMetrics, *cfg) } else { - pStore, err = NewPqStore(pgxMetrics, *cfg) + cfg.DriverName = postgres.DriverNamePq + pStore, err = NewPqStore(pqMetrics, *cfg) } Expect(err).ToNot(HaveOccurred()) diff --git a/tools/postgres/Dockerfile b/tools/postgres/Dockerfile index d1fbefcef4ca..d27d0e783053 100644 --- a/tools/postgres/Dockerfile +++ b/tools/postgres/Dockerfile @@ -9,8 +9,7 @@ COPY certs/postgres.server.crt /var/lib/postgresql/postgres.server.crt COPY certs/postgres.server.key /var/lib/postgresql/postgres.server.key RUN chown -R postgres /var/lib/postgresql && \ chmod 600 /var/lib/postgresql/postgres.server.key -# if I bump max_connections to 1000 the tests pass on my machine -CMD ["-c", "ssl=on", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] +CMD ["-c", "ssl=on", "-c", "max_connections=10000", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] FROM postgres:latest AS pg-standard FROM pg-${MODE} From 63d63b280a95c6e513dc76b535982dc20ec96784 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 08:33:01 +0200 Subject: [PATCH 12/28] feat(persistance): make check pass Signed-off-by: slonka --- pkg/plugins/resources/postgres/store_template_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 599315476b44..937f0831f59a 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -1,10 +1,10 @@ package postgres import ( - "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" "github.com/kumahq/kuma/pkg/core/resources/store" core_metrics "github.com/kumahq/kuma/pkg/metrics" test_store "github.com/kumahq/kuma/pkg/test/store" From 3413368a677faf0368c83275691dc9c3b008cd34 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 10:31:02 +0200 Subject: [PATCH 13/28] feat(persistance): make it possible to configure pgx specific settings Signed-off-by: slonka --- .../plugins/resources/postgres/config.go | 87 +++++++++++++++---- .../plugins/resources/postgres/config_test.go | 30 +++++++ pkg/plugins/resources/postgres/pgx_store.go | 19 +++- .../resources/postgres/store_template_test.go | 1 + 4 files changed, 119 insertions(+), 18 deletions(-) diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index 6eb52a0a9852..8455cddb7b2c 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -14,10 +14,20 @@ import ( const ( DriverNamePgx = "pgx" DriverNamePq = "postgres" + DefaultMaxIdleConnections = 50 + DefaultMinOpenConnections = 0 ) var _ config.Config = &PostgresStoreConfig{} +var DefaultMinReconnectInterval = config_types.Duration{Duration: 10 * time.Second} +var DefaultMaxReconnectInterval = config_types.Duration{Duration: 60 * time.Second} +var DefaultMaxConnectionLifetime = config_types.Duration{Duration: time.Hour} +var DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 0} +var DefaultMaxConnIdleTime = config_types.Duration{Duration: time.Minute * 30} +var DefaultHealthCheckPeriod = config_types.Duration{Duration: time.Minute} +// above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 + // Postgres store configuration type PostgresStoreConfig struct { // Host of the Postgres DB @@ -34,21 +44,30 @@ type PostgresStoreConfig struct { DriverName string `json:"driverName" envconfig:"kuma_store_postgres_driver_name"` // Connection Timeout to the DB in seconds ConnectionTimeout int `json:"connectionTimeout" envconfig:"kuma_store_postgres_connection_timeout"` - // Maximum number of open connections to the database + // MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed + MaxConnectionLifetime config_types.Duration `json:"maxConnectionLifetime" envconfig:"kuma_store_postgres_max_connection_lifetime"` + // MaxConnectionLifetimeJitter (pgx only) is the duration after MaxConnectionLifetime to randomly decide to close a connection. + // This helps prevent all connections from being closed at the exact same time, starving the pool. + MaxConnectionLifetimeJitter config_types.Duration `json:"maxConnectionLifetimeJitter" envconfig:"kuma_store_postgres_max_connection_lifetime_jitter"` + // HealthCheckPeriod (pgx only) is the duration between checks of the health of idle connections. + HealthCheckPeriod config_types.Duration `json:"healthCheckPeriod" envconfig:"kuma_store_postgres_health_check_period"` + // MinOpenConnections (pgx only) is the minimum number of open connections to the database + MinOpenConnections int `json:"minOpenConnections" envconfig:"kuma_store_postgres_min_open_connections"` + // MaxOpenConnections is the maximum number of open connections to the database // `0` value means number of open connections is unlimited MaxOpenConnections int `json:"maxOpenConnections" envconfig:"kuma_store_postgres_max_open_connections"` - // Maximum number of connections in the idle connection pool + // MaxIdleConnections (pq only) is the maximum number of connections in the idle connection pool // <0 value means no idle connections and 0 means default max idle connections MaxIdleConnections int `json:"maxIdleConnections" envconfig:"kuma_store_postgres_max_idle_connections"` // TLS settings TLS TLSPostgresStoreConfig `json:"tls"` - // MinReconnectInterval controls the duration to wait before trying to + // MinReconnectInterval (pq only) controls the duration to wait before trying to // re-establish the database connection after connection loss. After each // consecutive failure this interval is doubled, until MaxReconnectInterval // is reached. Successfully completing the connection establishment procedure // resets the interval back to MinReconnectInterval. MinReconnectInterval config_types.Duration `json:"minReconnectInterval" envconfig:"kuma_store_postgres_min_reconnect_interval"` - // MaxReconnectInterval controls the maximum possible duration to wait before trying + // MaxReconnectInterval (pq only) controls the maximum possible duration to wait before trying // to re-establish the database connection after connection loss. MaxReconnectInterval config_types.Duration `json:"maxReconnectInterval" envconfig:"kuma_store_postgres_max_reconnect_interval"` } @@ -178,23 +197,59 @@ func (p *PostgresStoreConfig) Validate() error { if p.MinReconnectInterval.Duration >= p.MaxReconnectInterval.Duration { return errors.New("MinReconnectInterval should be less than MaxReconnectInterval") } + switch p.DriverName { + case DriverNamePgx: + pqAlternativeMessage := "does not have an equivalent for pgx driver. " + + "If you need this setting consider using lib/pq as a postgres driver by setting driverName='postgres' or " + + "setting KUMA_STORE_POSTGRES_DRIVER_NAME='postgres' env variable." + if p.MaxIdleConnections != DefaultMaxIdleConnections { + return errors.New("MaxIdleConnections " + pqAlternativeMessage) + } + if p.MinReconnectInterval != DefaultMinReconnectInterval { + return errors.New("MinReconnectInterval " + pqAlternativeMessage) + } + if p.MaxReconnectInterval != DefaultMaxReconnectInterval { + return errors.New("MaxReconnectInterval " + pqAlternativeMessage) + } + case DriverNamePq: + pgxAlternativeMessage := "does not have an equivalent for pq driver. " + + "If you need this setting consider using pgx as a postgres driver by setting driverName='pgx' or " + + "setting KUMA_STORE_POSTGRES_DRIVER_NAME='pgx' env variable." + if p.MinOpenConnections != DefaultMinOpenConnections { + return errors.New("MinOpenConnections " + pgxAlternativeMessage) + } + if p.MaxConnectionLifetime != DefaultMinReconnectInterval { + return errors.New("MinReconnectInterval " + pgxAlternativeMessage) + } + if p.MaxConnectionLifetimeJitter != DefaultMaxReconnectInterval { + return errors.New("MaxReconnectInterval " + pgxAlternativeMessage) + } + if p.HealthCheckPeriod != DefaultMaxReconnectInterval { + return errors.New("MaxReconnectInterval " + pgxAlternativeMessage) + } + } + return nil } func DefaultPostgresStoreConfig() *PostgresStoreConfig { return &PostgresStoreConfig{ - Host: "127.0.0.1", - Port: 15432, - User: "kuma", - Password: "kuma", - DbName: "kuma", - DriverName: DriverNamePgx, - ConnectionTimeout: 5, - MaxOpenConnections: 50, // 0 for unlimited - MaxIdleConnections: 50, // 0 for unlimited - TLS: DefaultTLSPostgresStoreConfig(), - MinReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, - MaxReconnectInterval: config_types.Duration{Duration: 60 * time.Second}, + Host: "127.0.0.1", + Port: 15432, + User: "kuma", + Password: "kuma", + DbName: "kuma", + DriverName: DriverNamePgx, + ConnectionTimeout: 5, + MaxOpenConnections: 50, // 0 for unlimited + MaxIdleConnections: DefaultMaxIdleConnections, // 0 for unlimited + TLS: DefaultTLSPostgresStoreConfig(), + MinReconnectInterval: DefaultMinReconnectInterval, + MaxReconnectInterval: DefaultMaxReconnectInterval, + MinOpenConnections: DefaultMinOpenConnections, + MaxConnectionLifetime: DefaultMaxConnectionLifetime, + MaxConnectionLifetimeJitter: DefaultMaxConnectionLifetimeJitter, + HealthCheckPeriod: DefaultHealthCheckPeriod, } } diff --git a/pkg/config/plugins/resources/postgres/config_test.go b/pkg/config/plugins/resources/postgres/config_test.go index 27f75465257c..5a48082290e4 100644 --- a/pkg/config/plugins/resources/postgres/config_test.go +++ b/pkg/config/plugins/resources/postgres/config_test.go @@ -187,5 +187,35 @@ var _ = Describe("PostgresStoreConfig", func() { }, error: "MinReconnectInterval should be less than MaxReconnectInterval", }), + Entry("Unsupported setting used with pgx driver", validateTestCase{ + config: postgres.PostgresStoreConfig{ + Host: "localhost", + User: "postgres", + Password: "postgres", + DbName: "kuma", + TLS: postgres.DefaultTLSPostgresStoreConfig(), + MinReconnectInterval: config_types.Duration{Duration: 1 * time.Second}, + MaxReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, + + DriverName: postgres.DriverNamePgx, + MaxIdleConnections: 3, + }, + error: "MaxIdleConnections does not have an equivalent for pgx driver. If you need this setting consider using lib/pq as a postgres driver by setting driverName='postgres' or setting KUMA_STORE_POSTGRES_DRIVER_NAME='postgres' env variable.", + }), + Entry("Unsupported setting used with pq driver", validateTestCase{ + config: postgres.PostgresStoreConfig{ + Host: "localhost", + User: "postgres", + Password: "postgres", + DbName: "kuma", + TLS: postgres.DefaultTLSPostgresStoreConfig(), + MinReconnectInterval: config_types.Duration{Duration: 1 * time.Second}, + MaxReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, + + DriverName: postgres.DriverNamePq, + MinOpenConnections: 3, + }, + error: "MinOpenConnections does not have an equivalent for pq driver. If you need this setting consider using pgx as a postgres driver by setting driverName='pgx' or setting KUMA_STORE_POSTGRES_DRIVER_NAME='pgx' env variable.", + }), ) }) diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index 358567c62f1b..52b495588cf0 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -3,6 +3,7 @@ package postgres import ( "context" "fmt" + "math" "strconv" "strings" "time" @@ -40,12 +41,26 @@ func NewPgxStore(metrics core_metrics.Metrics, config config.PostgresStoreConfig }, nil } -func connect(config config.PostgresStoreConfig) (*pgxpool.Pool, error) { - connectionString, err := config.ConnectionString() +func connect(postgresStoreConfig config.PostgresStoreConfig) (*pgxpool.Pool, error) { + connectionString, err := postgresStoreConfig.ConnectionString() if err != nil { return nil, err } pgxConfig, err := pgxpool.ParseConfig(connectionString) + + if postgresStoreConfig.MaxOpenConnections == 0 { + // pgx MaxCons must be > 0, see https://github.com/jackc/puddle/blob/c5402ce53663d3c6481ea83c2912c339aeb94adc/pool.go#L160 + // so unlimited is just max int + pgxConfig.MaxConns = math.MaxInt32 + } else { + pgxConfig.MaxConns = int32(postgresStoreConfig.MaxOpenConnections) + } + pgxConfig.MinConns = int32(postgresStoreConfig.MinOpenConnections) + pgxConfig.MaxConnIdleTime = time.Duration(postgresStoreConfig.ConnectionTimeout) * time.Second + pgxConfig.MaxConnLifetime = postgresStoreConfig.MaxConnectionLifetime.Duration + pgxConfig.MaxConnLifetimeJitter = postgresStoreConfig.MaxConnectionLifetime.Duration + pgxConfig.HealthCheckPeriod = postgresStoreConfig.HealthCheckPeriod.Duration + if err != nil { return nil, err } diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 937f0831f59a..8fa4abb8435f 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -15,6 +15,7 @@ var _ = Describe("PostgresStore template", func() { createStore := func(storeName string) store.ResourceStore { cfg, err := c.Config(test_postgres.WithRandomDb) Expect(err).ToNot(HaveOccurred()) + cfg.MaxOpenConnections = 2 pqMetrics, err := core_metrics.NewMetrics("Standalone") Expect(err).ToNot(HaveOccurred()) From a3e4acadbb9ced8001cd174b40ad2645b9f403fd Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 10:49:54 +0200 Subject: [PATCH 14/28] feat(persistance): make check pass Signed-off-by: slonka --- .../plugins/resources/postgres/config.go | 20 +++++++++------- .../plugins/resources/postgres/config_test.go | 24 +++++++++---------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index 8455cddb7b2c..42aabe6ff684 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -12,21 +12,23 @@ import ( ) const ( - DriverNamePgx = "pgx" - DriverNamePq = "postgres" + DriverNamePgx = "pgx" + DriverNamePq = "postgres" DefaultMaxIdleConnections = 50 DefaultMinOpenConnections = 0 ) var _ config.Config = &PostgresStoreConfig{} -var DefaultMinReconnectInterval = config_types.Duration{Duration: 10 * time.Second} -var DefaultMaxReconnectInterval = config_types.Duration{Duration: 60 * time.Second} -var DefaultMaxConnectionLifetime = config_types.Duration{Duration: time.Hour} -var DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 0} -var DefaultMaxConnIdleTime = config_types.Duration{Duration: time.Minute * 30} -var DefaultHealthCheckPeriod = config_types.Duration{Duration: time.Minute} -// above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 +var ( + DefaultMinReconnectInterval = config_types.Duration{Duration: 10 * time.Second} + DefaultMaxReconnectInterval = config_types.Duration{Duration: 60 * time.Second} + DefaultMaxConnectionLifetime = config_types.Duration{Duration: time.Hour} + DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 0} + DefaultMaxConnIdleTime = config_types.Duration{Duration: time.Minute * 30} + DefaultHealthCheckPeriod = config_types.Duration{Duration: time.Minute} + // above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 +) // Postgres store configuration type PostgresStoreConfig struct { diff --git a/pkg/config/plugins/resources/postgres/config_test.go b/pkg/config/plugins/resources/postgres/config_test.go index 5a48082290e4..f0062273664b 100644 --- a/pkg/config/plugins/resources/postgres/config_test.go +++ b/pkg/config/plugins/resources/postgres/config_test.go @@ -189,30 +189,30 @@ var _ = Describe("PostgresStoreConfig", func() { }), Entry("Unsupported setting used with pgx driver", validateTestCase{ config: postgres.PostgresStoreConfig{ - Host: "localhost", - User: "postgres", - Password: "postgres", - DbName: "kuma", - TLS: postgres.DefaultTLSPostgresStoreConfig(), + Host: "localhost", + User: "postgres", + Password: "postgres", + DbName: "kuma", + TLS: postgres.DefaultTLSPostgresStoreConfig(), MinReconnectInterval: config_types.Duration{Duration: 1 * time.Second}, MaxReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, - DriverName: postgres.DriverNamePgx, + DriverName: postgres.DriverNamePgx, MaxIdleConnections: 3, }, error: "MaxIdleConnections does not have an equivalent for pgx driver. If you need this setting consider using lib/pq as a postgres driver by setting driverName='postgres' or setting KUMA_STORE_POSTGRES_DRIVER_NAME='postgres' env variable.", }), Entry("Unsupported setting used with pq driver", validateTestCase{ config: postgres.PostgresStoreConfig{ - Host: "localhost", - User: "postgres", - Password: "postgres", - DbName: "kuma", - TLS: postgres.DefaultTLSPostgresStoreConfig(), + Host: "localhost", + User: "postgres", + Password: "postgres", + DbName: "kuma", + TLS: postgres.DefaultTLSPostgresStoreConfig(), MinReconnectInterval: config_types.Duration{Duration: 1 * time.Second}, MaxReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, - DriverName: postgres.DriverNamePq, + DriverName: postgres.DriverNamePq, MinOpenConnections: 3, }, error: "MinOpenConnections does not have an equivalent for pq driver. If you need this setting consider using pgx as a postgres driver by setting driverName='pgx' or setting KUMA_STORE_POSTGRES_DRIVER_NAME='pgx' env variable.", From c65eeed79e730658a22467e245fd3e1e517b41e6 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 10:56:41 +0200 Subject: [PATCH 15/28] feat(persistance): wire in pgx as an option to use with migrations Signed-off-by: slonka --- app/kuma-cp/cmd/migrate.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/app/kuma-cp/cmd/migrate.go b/app/kuma-cp/cmd/migrate.go index aeb5a099bb98..73f9f065f3f1 100644 --- a/app/kuma-cp/cmd/migrate.go +++ b/app/kuma-cp/cmd/migrate.go @@ -7,6 +7,7 @@ import ( "github.com/kumahq/kuma/pkg/config" kuma_cp "github.com/kumahq/kuma/pkg/config/app/kuma-cp" "github.com/kumahq/kuma/pkg/config/core/resources/store" + "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" core_plugins "github.com/kumahq/kuma/pkg/core/plugins" "github.com/kumahq/kuma/pkg/version" ) @@ -67,7 +68,14 @@ func migrate(cfg kuma_cp.Config) error { pluginName = core_plugins.Memory pluginConfig = nil case store.PostgresStore: - pluginName = core_plugins.Postgres + switch cfg.Store.Postgres.DriverName { + case postgres.DriverNamePgx: + pluginName = core_plugins.Pgx + case postgres.DriverNamePq: + pluginName = core_plugins.Postgres + default: + return errors.Errorf("unknown driver name %s", cfg.Store.Postgres.DriverName) + } pluginConfig = cfg.Store.Postgres default: return errors.Errorf("unknown store type %s", cfg.Store.Type) From 58819d95e2c6143e92ec1fed914a2b636e96fb0c Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 11:08:03 +0200 Subject: [PATCH 16/28] feat(persistance): add upgrade docs Signed-off-by: slonka --- UPGRADE.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index a0976e70f53a..e12fcf7d09da 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -8,6 +8,17 @@ does not have any particular instructions. ## Upcoming release +### Universal + +### Changed default postgres driver to pgx +- If you encounter any problems with the persistence layer please [submit an issue](https://github.com/kumahq/kuma/issues/new) and temporarily switch to the previous driver (`lib/pq`) by setting +`DriverName=postgres` configuration option or `KUMA_STORE_POSTGRES_DRIVER_NAME='postgres'` env variable. +- Several configuration settings are not supported by the new driver right now, if you needed to tune them please [submit an issue](https://github.com/kumahq/kuma/issues/new) or try removing them. +List of unsupported configuration options: + - MaxIdleConnections (used in store) + - MinReconnectInterval (used in events listener) + - MaxReconnectInterval (used in events listener) + ### K8s ### Removed deprecated annotations From 8f37227a8785e9782200623ca50081fdb01f45d4 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 11:11:42 +0200 Subject: [PATCH 17/28] feat(persistance): reword upgrade docs Signed-off-by: slonka --- UPGRADE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UPGRADE.md b/UPGRADE.md index e12fcf7d09da..eb966fa81c28 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -13,7 +13,7 @@ does not have any particular instructions. ### Changed default postgres driver to pgx - If you encounter any problems with the persistence layer please [submit an issue](https://github.com/kumahq/kuma/issues/new) and temporarily switch to the previous driver (`lib/pq`) by setting `DriverName=postgres` configuration option or `KUMA_STORE_POSTGRES_DRIVER_NAME='postgres'` env variable. -- Several configuration settings are not supported by the new driver right now, if you needed to tune them please [submit an issue](https://github.com/kumahq/kuma/issues/new) or try removing them. +- Several configuration settings are not supported by the new driver right now, if used to configure them please try running with new defaults or [submit an issue](https://github.com/kumahq/kuma/issues/new). List of unsupported configuration options: - MaxIdleConnections (used in store) - MinReconnectInterval (used in events listener) From ce499fe7baac8abf44076ed23d143fe627a551c2 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 12:13:22 +0200 Subject: [PATCH 18/28] feat(persistance): fix kuma defaults Signed-off-by: slonka --- pkg/config/app/kuma-cp/kuma-cp.defaults.yaml | 16 ++++++++++++---- pkg/config/plugins/resources/postgres/config.go | 12 ++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml index 0923ca95d9d1..74c8d0901a3b 100644 --- a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml +++ b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml @@ -29,10 +29,18 @@ store: driverName: pgx # ENV: KUMA_STORE_POSTGRES_DRIVER_NAME # Connection Timeout to the DB in seconds connectionTimeout: 5 # ENV: KUMA_STORE_POSTGRES_CONNECTION_TIMEOUT - # Maximum number of open connections to the database + # MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed + maxConnectionLifetime: "1h" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME + # MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed + maxConnectionLifetimeJitter: "0s" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER + # HealthCheckPeriod (pgx only) is the duration between checks of the health of idle connections. + healthCheckPeriod: "1m" # ENV: KUMA_STORE_POSTGRES_HEALTH_CHECK_PERIOD + # MinOpenConnections (pgx only) is the minimum number of open connections to the database + minOpenConnections: 0 # ENV: KUMA_STORE_POSTGRES_MIN_OPEN_CONNECTIONS + # MaxOpenConnections is the maximum number of open connections to the database # `0` value means number of open connections is unlimited maxOpenConnections: 50 # ENV: KUMA_STORE_POSTGRES_MAX_OPEN_CONNECTIONS - # Maximum number of connections in the idle connection pool + # MaxIdleConnections (pq only) is the maximum number of connections in the idle connection pool # <0 value means no idle connections and 0 means default max idle connections maxIdleConnections: 50 # ENV: KUMA_STORE_POSTGRES_MAX_IDLE_CONNECTIONS # TLS settings @@ -45,13 +53,13 @@ store: keyPath: # ENV: KUMA_STORE_POSTGRES_TLS_KEY_PATH # Path to the root certificate. Used in verifyCa and verifyFull modes. caPath: # ENV: KUMA_STORE_POSTGRES_TLS_ROOT_CERT_PATH - # MinReconnectInterval controls the duration to wait before trying to + # MinReconnectInterval (pq only) controls the duration to wait before trying to # re-establish the database connection after connection loss. After each # consecutive failure this interval is doubled, until MaxReconnectInterval # is reached. Successfully completing the connection establishment procedure # resets the interval back to MinReconnectInterval. minReconnectInterval: "10s" # ENV: KUMA_STORE_POSTGRES_MIN_RECONNECT_INTERVAL - # MaxReconnectInterval controls the maximum possible duration to wait before trying + # MaxReconnectInterval (pq only) controls the maximum possible duration to wait before trying # to re-establish the database connection after connection loss. maxReconnectInterval: "60s" # ENV: KUMA_STORE_POSTGRES_MAX_RECONNECT_INTERVAL diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index 42aabe6ff684..f8231bea2aa0 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -220,14 +220,14 @@ func (p *PostgresStoreConfig) Validate() error { if p.MinOpenConnections != DefaultMinOpenConnections { return errors.New("MinOpenConnections " + pgxAlternativeMessage) } - if p.MaxConnectionLifetime != DefaultMinReconnectInterval { - return errors.New("MinReconnectInterval " + pgxAlternativeMessage) + if p.MaxConnectionLifetime != DefaultMaxConnectionLifetime { + return errors.New("MaxConnectionLifetime " + pgxAlternativeMessage) } - if p.MaxConnectionLifetimeJitter != DefaultMaxReconnectInterval { - return errors.New("MaxReconnectInterval " + pgxAlternativeMessage) + if p.MaxConnectionLifetimeJitter != DefaultMaxConnectionLifetimeJitter { + return errors.New("MaxConnectionLifetimeJitter " + pgxAlternativeMessage) } - if p.HealthCheckPeriod != DefaultMaxReconnectInterval { - return errors.New("MaxReconnectInterval " + pgxAlternativeMessage) + if p.HealthCheckPeriod != DefaultHealthCheckPeriod { + return errors.New("HealthCheckPeriod " + pgxAlternativeMessage) } } From 5a769ebc32b7a334c12a89b3b7de37dfc81ea9fd Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 13:00:31 +0200 Subject: [PATCH 19/28] feat(persistance): warn instead of erroring out on a unsupported pgx or pq configuration Signed-off-by: slonka --- pkg/config/loader_test.go | 12 ++++++++ .../plugins/resources/postgres/config.go | 22 ++++++++------ .../plugins/resources/postgres/config_test.go | 30 ------------------- 3 files changed, 25 insertions(+), 39 deletions(-) diff --git a/pkg/config/loader_test.go b/pkg/config/loader_test.go index 11b21cfd1053..413fca8117e4 100644 --- a/pkg/config/loader_test.go +++ b/pkg/config/loader_test.go @@ -98,6 +98,10 @@ var _ = Describe("Config loader", func() { Expect(cfg.Store.Postgres.DbName).To(Equal("kuma")) Expect(cfg.Store.Postgres.DriverName).To(Equal("postgres")) Expect(cfg.Store.Postgres.ConnectionTimeout).To(Equal(10)) + Expect(cfg.Store.Postgres.MaxConnectionLifetime.Duration).To(Equal(123 * time.Minute)) + Expect(cfg.Store.Postgres.MaxConnectionLifetimeJitter.Duration).To(Equal(456 * time.Second)) + Expect(cfg.Store.Postgres.HealthCheckPeriod.Duration).To(Equal(78 * time.Second)) + Expect(cfg.Store.Postgres.MinOpenConnections).To(Equal(3)) Expect(cfg.Store.Postgres.MaxOpenConnections).To(Equal(300)) Expect(cfg.Store.Postgres.MaxIdleConnections).To(Equal(300)) Expect(cfg.Store.Postgres.MinReconnectInterval.Duration).To(Equal(44 * time.Second)) @@ -339,7 +343,11 @@ store: dbName: kuma driverName: postgres connectionTimeout: 10 + minOpenConnections: 3 maxOpenConnections: 300 + maxConnectionLifetime: 123m + maxConnectionLifetimeJitter: 456s + healthCheckPeriod: 78s maxIdleConnections: 300 minReconnectInterval: 44s maxReconnectInterval: 55s @@ -648,8 +656,12 @@ proxy: "KUMA_STORE_POSTGRES_DB_NAME": "kuma", "KUMA_STORE_POSTGRES_DRIVER_NAME": "postgres", "KUMA_STORE_POSTGRES_CONNECTION_TIMEOUT": "10", + "KUMA_STORE_POSTGRES_MIN_OPEN_CONNECTIONS": "3", "KUMA_STORE_POSTGRES_MAX_OPEN_CONNECTIONS": "300", "KUMA_STORE_POSTGRES_MAX_IDLE_CONNECTIONS": "300", + "KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME": "123m", + "KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER": "456s", + "KUMA_STORE_POSTGRES_HEALTH_CHECK_PERIOD": "78s", "KUMA_STORE_POSTGRES_TLS_MODE": "verifyFull", "KUMA_STORE_POSTGRES_TLS_CERT_PATH": "/path/to/cert", "KUMA_STORE_POSTGRES_TLS_KEY_PATH": "/path/to/key", diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index f8231bea2aa0..040decb3318e 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -9,6 +9,7 @@ import ( "github.com/kumahq/kuma/pkg/config" config_types "github.com/kumahq/kuma/pkg/config/types" + "github.com/kumahq/kuma/pkg/core" ) const ( @@ -18,14 +19,16 @@ const ( DefaultMinOpenConnections = 0 ) -var _ config.Config = &PostgresStoreConfig{} +var ( + _ config.Config = &PostgresStoreConfig{} + logger = core.Log.WithName("postgres-config") +) var ( DefaultMinReconnectInterval = config_types.Duration{Duration: 10 * time.Second} DefaultMaxReconnectInterval = config_types.Duration{Duration: 60 * time.Second} DefaultMaxConnectionLifetime = config_types.Duration{Duration: time.Hour} DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 0} - DefaultMaxConnIdleTime = config_types.Duration{Duration: time.Minute * 30} DefaultHealthCheckPeriod = config_types.Duration{Duration: time.Minute} // above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 ) @@ -199,35 +202,36 @@ func (p *PostgresStoreConfig) Validate() error { if p.MinReconnectInterval.Duration >= p.MaxReconnectInterval.Duration { return errors.New("MinReconnectInterval should be less than MaxReconnectInterval") } + warning := "[WARNING] " switch p.DriverName { case DriverNamePgx: pqAlternativeMessage := "does not have an equivalent for pgx driver. " + "If you need this setting consider using lib/pq as a postgres driver by setting driverName='postgres' or " + "setting KUMA_STORE_POSTGRES_DRIVER_NAME='postgres' env variable." if p.MaxIdleConnections != DefaultMaxIdleConnections { - return errors.New("MaxIdleConnections " + pqAlternativeMessage) + logger.Info(warning + "MaxIdleConnections " + pqAlternativeMessage) } if p.MinReconnectInterval != DefaultMinReconnectInterval { - return errors.New("MinReconnectInterval " + pqAlternativeMessage) + logger.Info(warning + "MinReconnectInterval " + pqAlternativeMessage) } if p.MaxReconnectInterval != DefaultMaxReconnectInterval { - return errors.New("MaxReconnectInterval " + pqAlternativeMessage) + logger.Info(warning + "MaxReconnectInterval " + pqAlternativeMessage) } case DriverNamePq: pgxAlternativeMessage := "does not have an equivalent for pq driver. " + "If you need this setting consider using pgx as a postgres driver by setting driverName='pgx' or " + "setting KUMA_STORE_POSTGRES_DRIVER_NAME='pgx' env variable." if p.MinOpenConnections != DefaultMinOpenConnections { - return errors.New("MinOpenConnections " + pgxAlternativeMessage) + logger.Info(warning + "MinOpenConnections " + pgxAlternativeMessage) } if p.MaxConnectionLifetime != DefaultMaxConnectionLifetime { - return errors.New("MaxConnectionLifetime " + pgxAlternativeMessage) + logger.Info(warning + "MaxConnectionLifetime " + pgxAlternativeMessage) } if p.MaxConnectionLifetimeJitter != DefaultMaxConnectionLifetimeJitter { - return errors.New("MaxConnectionLifetimeJitter " + pgxAlternativeMessage) + logger.Info(warning + "MaxConnectionLifetimeJitter " + pgxAlternativeMessage) } if p.HealthCheckPeriod != DefaultHealthCheckPeriod { - return errors.New("HealthCheckPeriod " + pgxAlternativeMessage) + logger.Info(warning + "HealthCheckPeriod " + pgxAlternativeMessage) } } diff --git a/pkg/config/plugins/resources/postgres/config_test.go b/pkg/config/plugins/resources/postgres/config_test.go index f0062273664b..27f75465257c 100644 --- a/pkg/config/plugins/resources/postgres/config_test.go +++ b/pkg/config/plugins/resources/postgres/config_test.go @@ -187,35 +187,5 @@ var _ = Describe("PostgresStoreConfig", func() { }, error: "MinReconnectInterval should be less than MaxReconnectInterval", }), - Entry("Unsupported setting used with pgx driver", validateTestCase{ - config: postgres.PostgresStoreConfig{ - Host: "localhost", - User: "postgres", - Password: "postgres", - DbName: "kuma", - TLS: postgres.DefaultTLSPostgresStoreConfig(), - MinReconnectInterval: config_types.Duration{Duration: 1 * time.Second}, - MaxReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, - - DriverName: postgres.DriverNamePgx, - MaxIdleConnections: 3, - }, - error: "MaxIdleConnections does not have an equivalent for pgx driver. If you need this setting consider using lib/pq as a postgres driver by setting driverName='postgres' or setting KUMA_STORE_POSTGRES_DRIVER_NAME='postgres' env variable.", - }), - Entry("Unsupported setting used with pq driver", validateTestCase{ - config: postgres.PostgresStoreConfig{ - Host: "localhost", - User: "postgres", - Password: "postgres", - DbName: "kuma", - TLS: postgres.DefaultTLSPostgresStoreConfig(), - MinReconnectInterval: config_types.Duration{Duration: 1 * time.Second}, - MaxReconnectInterval: config_types.Duration{Duration: 10 * time.Second}, - - DriverName: postgres.DriverNamePq, - MinOpenConnections: 3, - }, - error: "MinOpenConnections does not have an equivalent for pq driver. If you need this setting consider using pgx as a postgres driver by setting driverName='pgx' or setting KUMA_STORE_POSTGRES_DRIVER_NAME='pgx' env variable.", - }), ) }) From 4c9f8a256c98fb23521d71e2ff9916bcf33334e2 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 13:43:45 +0200 Subject: [PATCH 20/28] feat(persistance): fix comments in kuma-cp defaults Signed-off-by: slonka --- pkg/config/app/kuma-cp/kuma-cp.defaults.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml index 74c8d0901a3b..57f6194b3bb1 100644 --- a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml +++ b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml @@ -31,7 +31,8 @@ store: connectionTimeout: 5 # ENV: KUMA_STORE_POSTGRES_CONNECTION_TIMEOUT # MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed maxConnectionLifetime: "1h" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME - # MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed + # MaxConnectionLifetimeJitter (pgx only) is the duration after maxConnectionLifetime to randomly decide to close a connection. + # This helps prevent all connections from being closed at the exact same time, starving the pool. maxConnectionLifetimeJitter: "0s" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER # HealthCheckPeriod (pgx only) is the duration between checks of the health of idle connections. healthCheckPeriod: "1m" # ENV: KUMA_STORE_POSTGRES_HEALTH_CHECK_PERIOD From 6744cd0a73d0f68e8b9822fe19932d6f670bf3c7 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 14:39:40 +0200 Subject: [PATCH 21/28] feat(persistance): fix nitpick from pr review Signed-off-by: slonka --- pkg/config/app/kuma-cp/kuma-cp.defaults.yaml | 16 ++++++------- pkg/config/loader_test.go | 6 ++--- .../plugins/resources/postgres/config.go | 24 +++++++++---------- pkg/plugins/resources/postgres/pgx_store.go | 6 ++--- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml index 57f6194b3bb1..0ea3a300cea7 100644 --- a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml +++ b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml @@ -29,19 +29,19 @@ store: driverName: pgx # ENV: KUMA_STORE_POSTGRES_DRIVER_NAME # Connection Timeout to the DB in seconds connectionTimeout: 5 # ENV: KUMA_STORE_POSTGRES_CONNECTION_TIMEOUT - # MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed + # MaxConnectionLifetime (applied only when driverName=pgx) is the duration since creation after which a connection will be automatically closed maxConnectionLifetime: "1h" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME - # MaxConnectionLifetimeJitter (pgx only) is the duration after maxConnectionLifetime to randomly decide to close a connection. + # MaxConnectionLifetimeJitter (applied only when driverName=pgx) is the duration after maxConnectionLifetime to randomly decide to close a connection. # This helps prevent all connections from being closed at the exact same time, starving the pool. maxConnectionLifetimeJitter: "0s" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER - # HealthCheckPeriod (pgx only) is the duration between checks of the health of idle connections. - healthCheckPeriod: "1m" # ENV: KUMA_STORE_POSTGRES_HEALTH_CHECK_PERIOD - # MinOpenConnections (pgx only) is the minimum number of open connections to the database + # HealthCheckInterval (applied only when driverName=pgx) is the duration between checks of the health of idle connections. + healthCheckInterval: "1m" # ENV: KUMA_STORE_POSTGRES_HEALTH_CHECK_INTERVAL + # MinOpenConnections (applied only when driverName=pgx) is the minimum number of open connections to the database minOpenConnections: 0 # ENV: KUMA_STORE_POSTGRES_MIN_OPEN_CONNECTIONS # MaxOpenConnections is the maximum number of open connections to the database # `0` value means number of open connections is unlimited maxOpenConnections: 50 # ENV: KUMA_STORE_POSTGRES_MAX_OPEN_CONNECTIONS - # MaxIdleConnections (pq only) is the maximum number of connections in the idle connection pool + # MaxIdleConnections (applied only when driverName=postgres) is the maximum number of connections in the idle connection pool # <0 value means no idle connections and 0 means default max idle connections maxIdleConnections: 50 # ENV: KUMA_STORE_POSTGRES_MAX_IDLE_CONNECTIONS # TLS settings @@ -54,13 +54,13 @@ store: keyPath: # ENV: KUMA_STORE_POSTGRES_TLS_KEY_PATH # Path to the root certificate. Used in verifyCa and verifyFull modes. caPath: # ENV: KUMA_STORE_POSTGRES_TLS_ROOT_CERT_PATH - # MinReconnectInterval (pq only) controls the duration to wait before trying to + # MinReconnectInterval (applied only when driverName=postgres) controls the duration to wait before trying to # re-establish the database connection after connection loss. After each # consecutive failure this interval is doubled, until MaxReconnectInterval # is reached. Successfully completing the connection establishment procedure # resets the interval back to MinReconnectInterval. minReconnectInterval: "10s" # ENV: KUMA_STORE_POSTGRES_MIN_RECONNECT_INTERVAL - # MaxReconnectInterval (pq only) controls the maximum possible duration to wait before trying + # MaxReconnectInterval (applied only when driverName=postgres) controls the maximum possible duration to wait before trying # to re-establish the database connection after connection loss. maxReconnectInterval: "60s" # ENV: KUMA_STORE_POSTGRES_MAX_RECONNECT_INTERVAL diff --git a/pkg/config/loader_test.go b/pkg/config/loader_test.go index 413fca8117e4..396e857b794e 100644 --- a/pkg/config/loader_test.go +++ b/pkg/config/loader_test.go @@ -100,7 +100,7 @@ var _ = Describe("Config loader", func() { Expect(cfg.Store.Postgres.ConnectionTimeout).To(Equal(10)) Expect(cfg.Store.Postgres.MaxConnectionLifetime.Duration).To(Equal(123 * time.Minute)) Expect(cfg.Store.Postgres.MaxConnectionLifetimeJitter.Duration).To(Equal(456 * time.Second)) - Expect(cfg.Store.Postgres.HealthCheckPeriod.Duration).To(Equal(78 * time.Second)) + Expect(cfg.Store.Postgres.HealthCheckInterval.Duration).To(Equal(78 * time.Second)) Expect(cfg.Store.Postgres.MinOpenConnections).To(Equal(3)) Expect(cfg.Store.Postgres.MaxOpenConnections).To(Equal(300)) Expect(cfg.Store.Postgres.MaxIdleConnections).To(Equal(300)) @@ -347,7 +347,7 @@ store: maxOpenConnections: 300 maxConnectionLifetime: 123m maxConnectionLifetimeJitter: 456s - healthCheckPeriod: 78s + healthCheckInterval: 78s maxIdleConnections: 300 minReconnectInterval: 44s maxReconnectInterval: 55s @@ -661,7 +661,7 @@ proxy: "KUMA_STORE_POSTGRES_MAX_IDLE_CONNECTIONS": "300", "KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME": "123m", "KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER": "456s", - "KUMA_STORE_POSTGRES_HEALTH_CHECK_PERIOD": "78s", + "KUMA_STORE_POSTGRES_HEALTH_CHECK_INTERVAL": "78s", "KUMA_STORE_POSTGRES_TLS_MODE": "verifyFull", "KUMA_STORE_POSTGRES_TLS_CERT_PATH": "/path/to/cert", "KUMA_STORE_POSTGRES_TLS_KEY_PATH": "/path/to/key", diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index 040decb3318e..7d1bd6a07331 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -29,7 +29,7 @@ var ( DefaultMaxReconnectInterval = config_types.Duration{Duration: 60 * time.Second} DefaultMaxConnectionLifetime = config_types.Duration{Duration: time.Hour} DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 0} - DefaultHealthCheckPeriod = config_types.Duration{Duration: time.Minute} + DefaultHealthCheckInterval = config_types.Duration{Duration: time.Minute} // above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 ) @@ -49,30 +49,30 @@ type PostgresStoreConfig struct { DriverName string `json:"driverName" envconfig:"kuma_store_postgres_driver_name"` // Connection Timeout to the DB in seconds ConnectionTimeout int `json:"connectionTimeout" envconfig:"kuma_store_postgres_connection_timeout"` - // MaxConnectionLifetime (pgx only) is the duration since creation after which a connection will be automatically closed + // MaxConnectionLifetime (applied only when driverName=pgx) is the duration since creation after which a connection will be automatically closed MaxConnectionLifetime config_types.Duration `json:"maxConnectionLifetime" envconfig:"kuma_store_postgres_max_connection_lifetime"` - // MaxConnectionLifetimeJitter (pgx only) is the duration after MaxConnectionLifetime to randomly decide to close a connection. + // MaxConnectionLifetimeJitter (applied only when driverName=pgx) is the duration after MaxConnectionLifetime to randomly decide to close a connection. // This helps prevent all connections from being closed at the exact same time, starving the pool. MaxConnectionLifetimeJitter config_types.Duration `json:"maxConnectionLifetimeJitter" envconfig:"kuma_store_postgres_max_connection_lifetime_jitter"` - // HealthCheckPeriod (pgx only) is the duration between checks of the health of idle connections. - HealthCheckPeriod config_types.Duration `json:"healthCheckPeriod" envconfig:"kuma_store_postgres_health_check_period"` - // MinOpenConnections (pgx only) is the minimum number of open connections to the database + // HealthCheckInterval (applied only when driverName=pgx) is the duration between checks of the health of idle connections. + HealthCheckInterval config_types.Duration `json:"healthCheckInterval" envconfig:"kuma_store_postgres_health_check_interval"` + // MinOpenConnections (applied only when driverName=pgx) is the minimum number of open connections to the database MinOpenConnections int `json:"minOpenConnections" envconfig:"kuma_store_postgres_min_open_connections"` // MaxOpenConnections is the maximum number of open connections to the database // `0` value means number of open connections is unlimited MaxOpenConnections int `json:"maxOpenConnections" envconfig:"kuma_store_postgres_max_open_connections"` - // MaxIdleConnections (pq only) is the maximum number of connections in the idle connection pool + // MaxIdleConnections (applied only when driverName=postgres) is the maximum number of connections in the idle connection pool // <0 value means no idle connections and 0 means default max idle connections MaxIdleConnections int `json:"maxIdleConnections" envconfig:"kuma_store_postgres_max_idle_connections"` // TLS settings TLS TLSPostgresStoreConfig `json:"tls"` - // MinReconnectInterval (pq only) controls the duration to wait before trying to + // MinReconnectInterval (applied only when driverName=postgres) controls the duration to wait before trying to // re-establish the database connection after connection loss. After each // consecutive failure this interval is doubled, until MaxReconnectInterval // is reached. Successfully completing the connection establishment procedure // resets the interval back to MinReconnectInterval. MinReconnectInterval config_types.Duration `json:"minReconnectInterval" envconfig:"kuma_store_postgres_min_reconnect_interval"` - // MaxReconnectInterval (pq only) controls the maximum possible duration to wait before trying + // MaxReconnectInterval (applied only when driverName=postgres) controls the maximum possible duration to wait before trying // to re-establish the database connection after connection loss. MaxReconnectInterval config_types.Duration `json:"maxReconnectInterval" envconfig:"kuma_store_postgres_max_reconnect_interval"` } @@ -230,8 +230,8 @@ func (p *PostgresStoreConfig) Validate() error { if p.MaxConnectionLifetimeJitter != DefaultMaxConnectionLifetimeJitter { logger.Info(warning + "MaxConnectionLifetimeJitter " + pgxAlternativeMessage) } - if p.HealthCheckPeriod != DefaultHealthCheckPeriod { - logger.Info(warning + "HealthCheckPeriod " + pgxAlternativeMessage) + if p.HealthCheckInterval != DefaultHealthCheckInterval { + logger.Info(warning + "HealthCheckInterval " + pgxAlternativeMessage) } } @@ -255,7 +255,7 @@ func DefaultPostgresStoreConfig() *PostgresStoreConfig { MinOpenConnections: DefaultMinOpenConnections, MaxConnectionLifetime: DefaultMaxConnectionLifetime, MaxConnectionLifetimeJitter: DefaultMaxConnectionLifetimeJitter, - HealthCheckPeriod: DefaultHealthCheckPeriod, + HealthCheckInterval: DefaultHealthCheckInterval, } } diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index 52b495588cf0..d420009caf9b 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -59,7 +59,7 @@ func connect(postgresStoreConfig config.PostgresStoreConfig) (*pgxpool.Pool, err pgxConfig.MaxConnIdleTime = time.Duration(postgresStoreConfig.ConnectionTimeout) * time.Second pgxConfig.MaxConnLifetime = postgresStoreConfig.MaxConnectionLifetime.Duration pgxConfig.MaxConnLifetimeJitter = postgresStoreConfig.MaxConnectionLifetime.Duration - pgxConfig.HealthCheckPeriod = postgresStoreConfig.HealthCheckPeriod.Duration + pgxConfig.HealthCheckPeriod = postgresStoreConfig.HealthCheckInterval.Duration if err != nil { return nil, err @@ -136,7 +136,7 @@ func (r *pgxResourceStore) Update(ctx context.Context, resource core_model.Resou if err != nil { return errors.Wrapf(err, "failed to execute query %s", statement) } - if rows := result.RowsAffected(); rows != 1 { // error ignored, postgres supports RowsAffected() + if rows := result.RowsAffected(); rows != 1 { return store.ErrorResourceConflict(resource.Descriptor().Name, resource.GetMeta().GetName(), resource.GetMeta().GetMesh()) } @@ -159,7 +159,7 @@ func (r *pgxResourceStore) Delete(ctx context.Context, resource core_model.Resou if err != nil { return errors.Wrapf(err, "failed to execute query: %s", statement) } - if rows := result.RowsAffected(); rows == 0 { // error ignored, postgres supports RowsAffected() + if rows := result.RowsAffected(); rows == 0 { return store.ErrorResourceNotFound(resource.Descriptor().Name, opts.Name, opts.Mesh) } From e29a7fcc79956895d7ed0134211a352be55b680b Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 14:56:43 +0200 Subject: [PATCH 22/28] feat(persistance): use one plugin Signed-off-by: slonka --- pkg/core/bootstrap/bootstrap.go | 10 +--- .../postgres/{pgx_plugin.go => plugin.go} | 21 +++++--- pkg/plugins/resources/postgres/pq_plugin.go | 49 ------------------- 3 files changed, 15 insertions(+), 65 deletions(-) rename pkg/plugins/resources/postgres/{pgx_plugin.go => plugin.go} (63%) delete mode 100644 pkg/plugins/resources/postgres/pq_plugin.go diff --git a/pkg/core/bootstrap/bootstrap.go b/pkg/core/bootstrap/bootstrap.go index f2fdf8c1de8e..24bc982765e1 100644 --- a/pkg/core/bootstrap/bootstrap.go +++ b/pkg/core/bootstrap/bootstrap.go @@ -11,7 +11,6 @@ import ( kuma_cp "github.com/kumahq/kuma/pkg/config/app/kuma-cp" config_core "github.com/kumahq/kuma/pkg/config/core" "github.com/kumahq/kuma/pkg/config/core/resources/store" - "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" "github.com/kumahq/kuma/pkg/core" config_manager "github.com/kumahq/kuma/pkg/core/config/manager" "github.com/kumahq/kuma/pkg/core/datasource" @@ -258,14 +257,7 @@ func initializeResourceStore(cfg kuma_cp.Config, builder *core_runtime.Builder) pluginName = core_plugins.Memory pluginConfig = nil case store.PostgresStore: - switch cfg.Store.Postgres.DriverName { - case postgres.DriverNamePgx: - pluginName = core_plugins.Pgx - case postgres.DriverNamePq: - pluginName = core_plugins.Postgres - default: - return errors.Errorf("unknown driver name %s", cfg.Store.Postgres.DriverName) - } + pluginName = core_plugins.Postgres pluginConfig = cfg.Store.Postgres default: return errors.Errorf("unknown store type %s", cfg.Store.Type) diff --git a/pkg/plugins/resources/postgres/pgx_plugin.go b/pkg/plugins/resources/postgres/plugin.go similarity index 63% rename from pkg/plugins/resources/postgres/pgx_plugin.go rename to pkg/plugins/resources/postgres/plugin.go index 075485afb7e6..87686829f515 100644 --- a/pkg/plugins/resources/postgres/pgx_plugin.go +++ b/pkg/plugins/resources/postgres/plugin.go @@ -12,15 +12,15 @@ import ( postgres_events "github.com/kumahq/kuma/pkg/plugins/resources/postgres/events" ) -var _ core_plugins.ResourceStorePlugin = &pgxPlugin{} +var _ core_plugins.ResourceStorePlugin = &plugin{} -type pgxPlugin struct{} +type plugin struct{} func init() { - core_plugins.Register(core_plugins.Pgx, &pgxPlugin{}) + core_plugins.Register(core_plugins.Postgres, &plugin{}) } -func (p *pgxPlugin) NewResourceStore(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_store.ResourceStore, error) { +func (p *plugin) NewResourceStore(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_store.ResourceStore, error) { cfg, ok := config.(*postgres.PostgresStoreConfig) if !ok { return nil, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") @@ -32,10 +32,17 @@ func (p *pgxPlugin) NewResourceStore(pc core_plugins.PluginContext, config core_ if !migrated { return nil, errors.New(`database is not migrated. Run "kuma-cp migrate up" to update database to the newest schema`) } - return NewPgxStore(pc.Metrics(), *cfg) + switch cfg.DriverName { + case postgres.DriverNamePgx: + return NewPgxStore(pc.Metrics(), *cfg) + case postgres.DriverNamePq: + return NewPqStore(pc.Metrics(), *cfg) + default: + return nil, errors.New("unknown driver name " + cfg.DriverName) + } } -func (p *pgxPlugin) Migrate(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_plugins.DbVersion, error) { +func (p *plugin) Migrate(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_plugins.DbVersion, error) { cfg, ok := config.(*postgres.PostgresStoreConfig) if !ok { return 0, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") @@ -43,7 +50,7 @@ func (p *pgxPlugin) Migrate(pc core_plugins.PluginContext, config core_plugins.P return MigrateDb(*cfg) } -func (p *pgxPlugin) EventListener(pc core_plugins.PluginContext, out events.Emitter) error { +func (p *plugin) EventListener(pc core_plugins.PluginContext, out events.Emitter) error { postgresListener := postgres_events.NewListener(*pc.Config().Store.Postgres, out) return pc.ComponentManager().Add(component.NewResilientComponent(core.Log.WithName("postgres-event-listener-component"), postgresListener)) } diff --git a/pkg/plugins/resources/postgres/pq_plugin.go b/pkg/plugins/resources/postgres/pq_plugin.go deleted file mode 100644 index 233b8886013c..000000000000 --- a/pkg/plugins/resources/postgres/pq_plugin.go +++ /dev/null @@ -1,49 +0,0 @@ -package postgres - -import ( - "errors" - - "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" - "github.com/kumahq/kuma/pkg/core" - core_plugins "github.com/kumahq/kuma/pkg/core/plugins" - core_store "github.com/kumahq/kuma/pkg/core/resources/store" - "github.com/kumahq/kuma/pkg/core/runtime/component" - "github.com/kumahq/kuma/pkg/events" - postgres_events "github.com/kumahq/kuma/pkg/plugins/resources/postgres/events" -) - -var _ core_plugins.ResourceStorePlugin = &pqPlugin{} - -type pqPlugin struct{} - -func init() { - core_plugins.Register(core_plugins.Postgres, &pqPlugin{}) -} - -func (p *pqPlugin) NewResourceStore(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_store.ResourceStore, error) { - cfg, ok := config.(*postgres.PostgresStoreConfig) - if !ok { - return nil, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") - } - migrated, err := isDbMigrated(*cfg) - if err != nil { - return nil, err - } - if !migrated { - return nil, errors.New(`database is not migrated. Run "kuma-cp migrate up" to update database to the newest schema`) - } - return NewPqStore(pc.Metrics(), *cfg) -} - -func (p *pqPlugin) Migrate(pc core_plugins.PluginContext, config core_plugins.PluginConfig) (core_plugins.DbVersion, error) { - cfg, ok := config.(*postgres.PostgresStoreConfig) - if !ok { - return 0, errors.New("invalid type of the config. Passed config should be a PostgresStoreConfig") - } - return MigrateDb(*cfg) -} - -func (p *pqPlugin) EventListener(pc core_plugins.PluginContext, out events.Emitter) error { - postgresListener := postgres_events.NewListener(*pc.Config().Store.Postgres, out) - return pc.ComponentManager().Add(component.NewResilientComponent(core.Log.WithName("postgres-event-listener-component"), postgresListener)) -} From a3d7dbbe6f71d17b206a0f808666726655e9577a Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 15:00:41 +0200 Subject: [PATCH 23/28] feat(persistance): format change Signed-off-by: slonka --- pkg/config/loader_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/config/loader_test.go b/pkg/config/loader_test.go index 396e857b794e..79dc3c337d11 100644 --- a/pkg/config/loader_test.go +++ b/pkg/config/loader_test.go @@ -661,7 +661,7 @@ proxy: "KUMA_STORE_POSTGRES_MAX_IDLE_CONNECTIONS": "300", "KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME": "123m", "KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER": "456s", - "KUMA_STORE_POSTGRES_HEALTH_CHECK_INTERVAL": "78s", + "KUMA_STORE_POSTGRES_HEALTH_CHECK_INTERVAL": "78s", "KUMA_STORE_POSTGRES_TLS_MODE": "verifyFull", "KUMA_STORE_POSTGRES_TLS_CERT_PATH": "/path/to/cert", "KUMA_STORE_POSTGRES_TLS_KEY_PATH": "/path/to/key", From d670e7e13ad674d93665bb55292040505728f0a9 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 15:36:44 +0200 Subject: [PATCH 24/28] feat(persistance): remove leftovers from pgx plugin Signed-off-by: slonka --- app/kuma-cp/cmd/migrate.go | 10 +--------- pkg/core/plugins/registry.go | 1 - 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/app/kuma-cp/cmd/migrate.go b/app/kuma-cp/cmd/migrate.go index 73f9f065f3f1..aeb5a099bb98 100644 --- a/app/kuma-cp/cmd/migrate.go +++ b/app/kuma-cp/cmd/migrate.go @@ -7,7 +7,6 @@ import ( "github.com/kumahq/kuma/pkg/config" kuma_cp "github.com/kumahq/kuma/pkg/config/app/kuma-cp" "github.com/kumahq/kuma/pkg/config/core/resources/store" - "github.com/kumahq/kuma/pkg/config/plugins/resources/postgres" core_plugins "github.com/kumahq/kuma/pkg/core/plugins" "github.com/kumahq/kuma/pkg/version" ) @@ -68,14 +67,7 @@ func migrate(cfg kuma_cp.Config) error { pluginName = core_plugins.Memory pluginConfig = nil case store.PostgresStore: - switch cfg.Store.Postgres.DriverName { - case postgres.DriverNamePgx: - pluginName = core_plugins.Pgx - case postgres.DriverNamePq: - pluginName = core_plugins.Postgres - default: - return errors.Errorf("unknown driver name %s", cfg.Store.Postgres.DriverName) - } + pluginName = core_plugins.Postgres pluginConfig = cfg.Store.Postgres default: return errors.Errorf("unknown store type %s", cfg.Store.Type) diff --git a/pkg/core/plugins/registry.go b/pkg/core/plugins/registry.go index f513b8b47fc8..f8e2c01f3ece 100644 --- a/pkg/core/plugins/registry.go +++ b/pkg/core/plugins/registry.go @@ -26,7 +26,6 @@ const ( Universal PluginName = "universal" Memory PluginName = "memory" Postgres PluginName = "postgres" - Pgx PluginName = "pgx" CaBuiltin PluginName = "builtin" CaProvided PluginName = "provided" From acda410ffb44ba0f88afeb699c98d4c84fa22355 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 15:45:03 +0200 Subject: [PATCH 25/28] feat(persistance): rewrite createStore as higher order function Signed-off-by: slonka --- .../resources/k8s/store_template_test.go | 4 +- .../resources/memory/store_template_test.go | 5 +- .../resources/postgres/store_template_test.go | 14 +- pkg/test/store/owner_test_template.go | 291 ++++---- pkg/test/store/store_test_template.go | 689 +++++++++--------- 5 files changed, 499 insertions(+), 504 deletions(-) diff --git a/pkg/plugins/resources/k8s/store_template_test.go b/pkg/plugins/resources/k8s/store_template_test.go index 94ee69ca022b..d5d55b5d80a8 100644 --- a/pkg/plugins/resources/k8s/store_template_test.go +++ b/pkg/plugins/resources/k8s/store_template_test.go @@ -13,7 +13,7 @@ import ( ) var _ = Describe("KubernetesStore template", func() { - test_store.ExecuteStoreTests(func(string) store.ResourceStore { + test_store.ExecuteStoreTests(func() store.ResourceStore { kubeTypes := k8s_registry.NewTypeRegistry() Expect(kubeTypes.RegisterObjectType(&mesh_proto.TrafficRoute{}, &mesh_k8s.TrafficRoute{})).To(Succeed()) @@ -30,5 +30,5 @@ var _ = Describe("KubernetesStore template", func() { }, Scheme: k8sClientScheme, } - }, "kubernetes") + }) }) diff --git a/pkg/plugins/resources/memory/store_template_test.go b/pkg/plugins/resources/memory/store_template_test.go index ff131861dfad..f1e9b951146a 100644 --- a/pkg/plugins/resources/memory/store_template_test.go +++ b/pkg/plugins/resources/memory/store_template_test.go @@ -3,12 +3,11 @@ package memory_test import ( . "github.com/onsi/ginkgo/v2" - "github.com/kumahq/kuma/pkg/core/resources/store" "github.com/kumahq/kuma/pkg/plugins/resources/memory" test_store "github.com/kumahq/kuma/pkg/test/store" ) var _ = Describe("MemoryStore template", func() { - test_store.ExecuteStoreTests(func(string) store.ResourceStore { return memory.NewStore() }, "memory") - test_store.ExecuteOwnerTests(func(string) store.ResourceStore { return memory.NewStore() }, "memory") + test_store.ExecuteStoreTests(memory.NewStore) + test_store.ExecuteOwnerTests(memory.NewStore) }) diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 8fa4abb8435f..76b79dbb29b1 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -12,7 +12,7 @@ import ( ) var _ = Describe("PostgresStore template", func() { - createStore := func(storeName string) store.ResourceStore { + createStore := func(storeName string) func() store.ResourceStore { cfg, err := c.Config(test_postgres.WithRandomDb) Expect(err).ToNot(HaveOccurred()) cfg.MaxOpenConnections = 2 @@ -36,11 +36,13 @@ var _ = Describe("PostgresStore template", func() { } Expect(err).ToNot(HaveOccurred()) - return pStore + return func() store.ResourceStore { + return pStore + } } - test_store.ExecuteStoreTests(createStore, "pgx") - test_store.ExecuteOwnerTests(createStore, "pgx") - test_store.ExecuteStoreTests(createStore, "pq") - test_store.ExecuteOwnerTests(createStore, "pq") + test_store.ExecuteStoreTests(createStore("pgx")) + test_store.ExecuteOwnerTests(createStore("pgx")) + test_store.ExecuteStoreTests(createStore("pq")) + test_store.ExecuteOwnerTests(createStore("pq")) }) diff --git a/pkg/test/store/owner_test_template.go b/pkg/test/store/owner_test_template.go index ced0e8e94d5b..5937723e23f8 100644 --- a/pkg/test/store/owner_test_template.go +++ b/pkg/test/store/owner_test_template.go @@ -15,14 +15,13 @@ import ( ) func ExecuteOwnerTests( - createStore func(name string) store.ResourceStore, - storeName string, + createStore func() store.ResourceStore, ) { const mesh = "default-mesh" var s store.ClosableResourceStore BeforeEach(func() { - s = store.NewStrictResourceStore(createStore(storeName)) + s = store.NewStrictResourceStore(createStore()) }) AfterEach(func() { @@ -30,47 +29,81 @@ func ExecuteOwnerTests( Expect(err).ToNot(HaveOccurred()) }) - Context("Store: "+storeName, func() { - It("should delete resource when its owner is deleted", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete resource when its owner is deleted", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - name := "resource-1" - trRes := core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, + name := "resource-1" + trRes := core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", }, }, - } - err = s.Create(context.Background(), &trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) + }, + } + err = s.Create(context.Background(), &trRes, + store.CreateByKey(name, mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) - }) + // then + actual := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) + }) - It("should delete resource when its owner is deleted after owner update", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete resource when its owner is deleted after owner update", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + name := "resource-1" + trRes := core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, + }, + }, + } + err = s.Create(context.Background(), &trRes, + store.CreateByKey(name, mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) + + // when owner is updated + Expect(s.Update(context.Background(), meshRes)).To(Succeed()) + + // and only then deleted + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - name := "resource-1" - trRes := core_mesh.TrafficRouteResource{ + // then + actual := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) + }) + + It("should delete several resources when their owner is deleted", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + for i := 0; i < 10; i++ { + tr := core_mesh.TrafficRouteResource{ Spec: &v1alpha1.TrafficRoute{ Conf: &v1alpha1.TrafficRoute_Conf{ Destination: map[string]string{ @@ -79,137 +112,101 @@ func ExecuteOwnerTests( }, }, } - err = s.Create(context.Background(), &trRes, - store.CreateByKey(name, mesh), + err = s.Create(context.Background(), &tr, + store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), store.CreatedAt(time.Now()), store.CreateWithOwner(meshRes)) Expect(err).ToNot(HaveOccurred()) + } + actual := core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(HaveLen(10)) - // when owner is updated - Expect(s.Update(context.Background(), meshRes)).To(Succeed()) - - // and only then deleted - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) - }) + // then + actual = core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(BeEmpty()) + }) - It("should delete several resources when their owner is deleted", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete owners chain", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - for i := 0; i < 10; i++ { - tr := core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, + var prev model.Resource = meshRes + for i := 0; i < 10; i++ { + curr := &core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", }, }, - } - err = s.Create(context.Background(), &tr, - store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) + }, } - actual := core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(HaveLen(10)) - - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - // then - actual = core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(BeEmpty()) - }) - - It("should delete owners chain", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + err := s.Create(context.Background(), curr, + store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(prev)) Expect(err).ToNot(HaveOccurred()) + prev = curr + } - var prev model.Resource = meshRes - for i := 0; i < 10; i++ { - curr := &core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, - }, - }, - } - err := s.Create(context.Background(), curr, - store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(prev)) - Expect(err).ToNot(HaveOccurred()) - prev = curr - } - - actual := core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(HaveLen(10)) + actual := core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(HaveLen(10)) - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual = core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(BeEmpty()) - }) + // then + actual = core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(BeEmpty()) + }) - It("should delete a parent after children is deleted", func() { - // given - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete a parent after children is deleted", func() { + // given + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - name := "resource-1" - trRes := &core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, + name := "resource-1" + trRes := &core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", }, }, - } - err = s.Create(context.Background(), trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) + }, + } + err = s.Create(context.Background(), trRes, + store.CreateByKey(name, mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) - // when children is deleted - err = s.Delete(context.Background(), core_mesh.NewTrafficRouteResource(), store.DeleteByKey(name, mesh)) + // when children is deleted + err = s.Delete(context.Background(), core_mesh.NewTrafficRouteResource(), store.DeleteByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // when parent is deleted - err = s.Delete(context.Background(), core_mesh.NewMeshResource(), store.DeleteByKey(mesh, model.NoMesh)) + // when parent is deleted + err = s.Delete(context.Background(), core_mesh.NewMeshResource(), store.DeleteByKey(mesh, model.NoMesh)) - // then - Expect(err).ToNot(HaveOccurred()) - }) + // then + Expect(err).ToNot(HaveOccurred()) }) } diff --git a/pkg/test/store/store_test_template.go b/pkg/test/store/store_test_template.go index 80270601a030..12e25887950a 100644 --- a/pkg/test/store/store_test_template.go +++ b/pkg/test/store/store_test_template.go @@ -17,14 +17,13 @@ import ( ) func ExecuteStoreTests( - createStore func(name string) store.ResourceStore, - storeName string, + createStore func() store.ResourceStore, ) { const mesh = "default-mesh" var s store.ClosableResourceStore BeforeEach(func() { - s = store.NewStrictResourceStore(store.NewPaginationStore(createStore(storeName))) + s = store.NewStrictResourceStore(store.NewPaginationStore(createStore())) }) AfterEach(func() { @@ -57,436 +56,434 @@ func ExecuteStoreTests( return &res } - Context("Store: "+storeName, func() { - Describe("Create()", func() { - It("should create a new resource", func() { - // given - name := "resource1.demo" + Describe("Create()", func() { + It("should create a new resource", func() { + // given + name := "resource1.demo" - // when - created := createResource(name) + // when + created := createResource(name) - // when retrieve created object - resource := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when retrieve created object + resource := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // and it has same data - Expect(resource.Meta.GetName()).To(Equal(name)) - Expect(resource.Meta.GetMesh()).To(Equal(mesh)) - Expect(resource.Meta.GetVersion()).ToNot(BeEmpty()) - Expect(resource.Meta.GetCreationTime().Unix()).ToNot(Equal(0)) - Expect(resource.Meta.GetCreationTime()).To(Equal(resource.Meta.GetModificationTime())) - Expect(resource.Spec).To(MatchProto(created.Spec)) - }) + // and it has same data + Expect(resource.Meta.GetName()).To(Equal(name)) + Expect(resource.Meta.GetMesh()).To(Equal(mesh)) + Expect(resource.Meta.GetVersion()).ToNot(BeEmpty()) + Expect(resource.Meta.GetCreationTime().Unix()).ToNot(Equal(0)) + Expect(resource.Meta.GetCreationTime()).To(Equal(resource.Meta.GetModificationTime())) + Expect(resource.Spec).To(MatchProto(created.Spec)) + }) - It("should not create a duplicate record", func() { - // given - name := "duplicated-record.demo" - resource := createResource(name) + It("should not create a duplicate record", func() { + // given + name := "duplicated-record.demo" + resource := createResource(name) - // when try to create another one with same name - resource.SetMeta(nil) - err := s.Create(context.Background(), resource, store.CreateByKey(name, mesh)) + // when try to create another one with same name + resource.SetMeta(nil) + err := s.Create(context.Background(), resource, store.CreateByKey(name, mesh)) - // then - Expect(err).To(MatchError(store.ErrorResourceAlreadyExists(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).To(MatchError(store.ErrorResourceAlreadyExists(resource.Descriptor().Name, name, mesh))) }) + }) - Describe("Update()", func() { - It("should return an error if resource is not found", func() { - // given - name := "to-be-updated.demo" - resource := createResource(name) - - // when delete resource - err := s.Delete( - context.Background(), - resource, - store.DeleteByKey(resource.Meta.GetName(), mesh), - ) + Describe("Update()", func() { + It("should return an error if resource is not found", func() { + // given + name := "to-be-updated.demo" + resource := createResource(name) - // then - Expect(err).ToNot(HaveOccurred()) + // when delete resource + err := s.Delete( + context.Background(), + resource, + store.DeleteByKey(resource.Meta.GetName(), mesh), + ) - // when trying to update nonexistent resource - err = s.Update(context.Background(), resource) - - // then - Expect(err).To(MatchError(store.ErrorResourceConflict(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).ToNot(HaveOccurred()) - It("should update an existing resource", func() { - // given a resources in storage - name := "to-be-updated.demo" - resource := createResource(name) - modificationTime := time.Now().Add(time.Second) - versionBeforeUpdate := resource.Meta.GetVersion() + // when trying to update nonexistent resource + err = s.Update(context.Background(), resource) - // when - resource.Spec.Conf.Destination["path"] = "new-path" - err := s.Update(context.Background(), resource, store.ModifiedAt(modificationTime)) + // then + Expect(err).To(MatchError(store.ErrorResourceConflict(resource.Descriptor().Name, name, mesh))) + }) - // then - Expect(err).ToNot(HaveOccurred()) + It("should update an existing resource", func() { + // given a resources in storage + name := "to-be-updated.demo" + resource := createResource(name) + modificationTime := time.Now().Add(time.Second) + versionBeforeUpdate := resource.Meta.GetVersion() - // and meta is updated (version and modification time) - Expect(resource.Meta.GetVersion()).ToNot(Equal(versionBeforeUpdate)) - if reflect.TypeOf(createStore(storeName)) != reflect.TypeOf(&resources_k8s.KubernetesStore{}) { - Expect(resource.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) - } + // when + resource.Spec.Conf.Destination["path"] = "new-path" + err := s.Update(context.Background(), resource, store.ModifiedAt(modificationTime)) - // when retrieve the resource - res := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), res, store.GetByKey(name, mesh)) + // then + Expect(err).ToNot(HaveOccurred()) - // then - Expect(err).ToNot(HaveOccurred()) + // and meta is updated (version and modification time) + Expect(resource.Meta.GetVersion()).ToNot(Equal(versionBeforeUpdate)) + if reflect.TypeOf(createStore()) != reflect.TypeOf(&resources_k8s.KubernetesStore{}) { + Expect(resource.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) + } - // and - Expect(res.Spec.Conf.Destination["path"]).To(Equal("new-path")) + // when retrieve the resource + res := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), res, store.GetByKey(name, mesh)) - // and modification time is updated - // on K8S modification time is always the creation time, because there is no data for modification time - if reflect.TypeOf(createStore(storeName)) == reflect.TypeOf(&resources_k8s.KubernetesStore{}) { - Expect(res.Meta.GetModificationTime()).To(Equal(res.Meta.GetCreationTime())) - } else { - Expect(res.Meta.GetModificationTime()).ToNot(Equal(res.Meta.GetCreationTime())) - Expect(res.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) - } - }) + // then + Expect(err).ToNot(HaveOccurred()) - // todo(jakubdyszkiewicz) write tests for optimistic locking + // and + Expect(res.Spec.Conf.Destination["path"]).To(Equal("new-path")) + + // and modification time is updated + // on K8S modification time is always the creation time, because there is no data for modification time + if reflect.TypeOf(createStore()) == reflect.TypeOf(&resources_k8s.KubernetesStore{}) { + Expect(res.Meta.GetModificationTime()).To(Equal(res.Meta.GetCreationTime())) + } else { + Expect(res.Meta.GetModificationTime()).ToNot(Equal(res.Meta.GetCreationTime())) + Expect(res.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) + } }) - Describe("Delete()", func() { - It("should throw an error if resource is not found", func() { - // given - name := "non-existent-name.demo" - resource := core_mesh.NewTrafficRouteResource() + // todo(jakubdyszkiewicz) write tests for optimistic locking + }) - // when - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) + Describe("Delete()", func() { + It("should throw an error if resource is not found", func() { + // given + name := "non-existent-name.demo" + resource := core_mesh.NewTrafficRouteResource() - // then - Expect(err).To(HaveOccurred()) - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) + // when + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) - It("should not delete resource from another mesh", func() { - // given - name := "tr-1.demo" - resource := createResource(name) + // then + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - // when - resource.SetMeta(nil) // otherwise the validation from strict client fires that mesh is different - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, "different-mesh")) + It("should not delete resource from another mesh", func() { + // given + name := "tr-1.demo" + resource := createResource(name) - // then - Expect(err).To(HaveOccurred()) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) + // when + resource.SetMeta(nil) // otherwise the validation from strict client fires that mesh is different + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, "different-mesh")) - // and when getting the given resource - getResource := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), getResource, store.GetByKey(name, mesh)) + // then + Expect(err).To(HaveOccurred()) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) - // then resource still exists - Expect(err).ToNot(HaveOccurred()) - }) + // and when getting the given resource + getResource := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), getResource, store.GetByKey(name, mesh)) - It("should delete an existing resource", func() { - // given a resources in storage - name := "to-be-deleted.demo" - createResource(name) + // then resource still exists + Expect(err).ToNot(HaveOccurred()) + }) - // when - resource := core_mesh.NewTrafficRouteResource() - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) + It("should delete an existing resource", func() { + // given a resources in storage + name := "to-be-deleted.demo" + createResource(name) - // then - Expect(err).ToNot(HaveOccurred()) + // when + resource := core_mesh.NewTrafficRouteResource() + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) - // when query for deleted resource - resource = core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // then + Expect(err).ToNot(HaveOccurred()) - // then resource cannot be found - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) + // when query for deleted resource + resource = core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + + // then resource cannot be found + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) }) + }) - Describe("Get()", func() { - It("should return an error if resource is not found", func() { - // given - name := "non-existing-resource.demo" - resource := core_mesh.NewTrafficRouteResource() + Describe("Get()", func() { + It("should return an error if resource is not found", func() { + // given + name := "non-existing-resource.demo" + resource := core_mesh.NewTrafficRouteResource() - // when - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).To(MatchError(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).To(MatchError(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - It("should return an error if resource is not found in given mesh", func() { - // given a resources in mesh "mesh" - name := "existing-resource.demo" - mesh := "different-mesh" - createResource(name) + It("should return an error if resource is not found in given mesh", func() { + // given a resources in mesh "mesh" + name := "existing-resource.demo" + mesh := "different-mesh" + createResource(name) - // when - resource := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when + resource := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - It("should return an existing resource", func() { - // given a resources in storage - name := "get-existing-resource.demo" - createdResource := createResource(name) + It("should return an existing resource", func() { + // given a resources in storage + name := "get-existing-resource.demo" + createdResource := createResource(name) - // when - res := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), res, store.GetByKey(name, mesh)) + // when + res := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), res, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // and - Expect(res.Meta.GetName()).To(Equal(name)) - Expect(res.Meta.GetVersion()).ToNot(BeEmpty()) - Expect(res.Spec).To(MatchProto(createdResource.Spec)) - }) + // and + Expect(res.Meta.GetName()).To(Equal(name)) + Expect(res.Meta.GetVersion()).ToNot(BeEmpty()) + Expect(res.Spec).To(MatchProto(createdResource.Spec)) + }) - It("should get resource by version", func() { - // given - name := "existing-resource.demo" - res := createResource(name) + It("should get resource by version", func() { + // given + name := "existing-resource.demo" + res := createResource(name) - // when trying to retrieve resource with proper version - err := s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion(res.GetMeta().GetVersion())) + // when trying to retrieve resource with proper version + err := s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion(res.GetMeta().GetVersion())) - // then resource is found - Expect(err).ToNot(HaveOccurred()) + // then resource is found + Expect(err).ToNot(HaveOccurred()) - // when trying to retrieve resource with different version - err = s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion("9999999")) + // when trying to retrieve resource with different version + err = s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion("9999999")) - // then resource precondition failed error occurred - Expect(store.IsResourcePreconditionFailed(err)).To(BeTrue()) - }) + // then resource precondition failed error occurred + Expect(store.IsResourcePreconditionFailed(err)).To(BeTrue()) }) + }) - Describe("List()", func() { - It("should return an empty list if there are no matching resources", func() { - // given - list := core_mesh.TrafficRouteResourceList{} + Describe("List()", func() { + It("should return an empty list if there are no matching resources", func() { + // given + list := core_mesh.TrafficRouteResourceList{} - // when - err := s.List(context.Background(), &list, store.ListByMesh(mesh)) + // when + err := s.List(context.Background(), &list, store.ListByMesh(mesh)) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(0))) - // and - Expect(list.Items).To(HaveLen(0)) - }) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(0))) + // and + Expect(list.Items).To(HaveLen(0)) + }) - It("should return a list of resources", func() { - // given two resources - createResource("res-1.demo") - createResource("res-2.demo") + It("should return a list of resources", func() { + // given two resources + createResource("res-1.demo") + createResource("res-2.demo") - list := core_mesh.TrafficRouteResourceList{} + list := core_mesh.TrafficRouteResourceList{} - // when - err := s.List(context.Background(), &list) + // when + err := s.List(context.Background(), &list) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(HaveLen(2)) - // and - names := []string{list.Items[0].Meta.GetName(), list.Items[1].Meta.GetName()} - Expect(names).To(ConsistOf("res-1.demo", "res-2.demo")) - Expect(list.Items[0].Meta.GetMesh()).To(Equal(mesh)) - Expect(list.Items[0].Spec.Conf.Destination["path"]).To(Equal("demo")) - Expect(list.Items[1].Meta.GetMesh()).To(Equal(mesh)) - Expect(list.Items[1].Spec.Conf.Destination["path"]).To(Equal("demo")) - }) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(HaveLen(2)) + // and + names := []string{list.Items[0].Meta.GetName(), list.Items[1].Meta.GetName()} + Expect(names).To(ConsistOf("res-1.demo", "res-2.demo")) + Expect(list.Items[0].Meta.GetMesh()).To(Equal(mesh)) + Expect(list.Items[0].Spec.Conf.Destination["path"]).To(Equal("demo")) + Expect(list.Items[1].Meta.GetMesh()).To(Equal(mesh)) + Expect(list.Items[1].Spec.Conf.Destination["path"]).To(Equal("demo")) + }) - It("should not return a list of resources in different mesh", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") + It("should not return a list of resources in different mesh", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") - list := core_mesh.TrafficRouteResourceList{} + list := core_mesh.TrafficRouteResourceList{} - // when - err := s.List(context.Background(), &list, store.ListByMesh("different-mesh")) + // when + err := s.List(context.Background(), &list, store.ListByMesh("different-mesh")) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(0))) - // and - Expect(list.Items).To(HaveLen(0)) - }) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(0))) + // and + Expect(list.Items).To(HaveLen(0)) + }) - It("should return a list of resources with prefix from all meshes", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") - createResource("list-mes-1.demo") + It("should return a list of resources with prefix from all meshes", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") + createResource("list-mes-1.demo") - list := core_mesh.TrafficRouteResourceList{} + list := core_mesh.TrafficRouteResourceList{} - // when - err := s.List(context.Background(), &list, store.ListByNameContains("list-res")) + // when + err := s.List(context.Background(), &list, store.ListByNameContains("list-res")) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { - var res []string - for _, v := range itms { - res = append(res, v.GetMeta().GetName()) - } - return res - }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) - }) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { + var res []string + for _, v := range itms { + res = append(res, v.GetMeta().GetName()) + } + return res + }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) + }) - It("should return a list of resources with prefix from the specific mesh", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") - createResource("list-mes-1.demo") + It("should return a list of resources with prefix from the specific mesh", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") + createResource("list-mes-1.demo") - list := core_mesh.TrafficRouteResourceList{} + list := core_mesh.TrafficRouteResourceList{} - // when - err := s.List(context.Background(), &list, store.ListByNameContains("list-res"), store.ListByMesh(mesh)) + // when + err := s.List(context.Background(), &list, store.ListByNameContains("list-res"), store.ListByMesh(mesh)) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { - var res []string - for _, v := range itms { - res = append(res, v.GetMeta().GetName()) - } - return res - }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) - }) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { + var res []string + for _, v := range itms { + res = append(res, v.GetMeta().GetName()) + } + return res + }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) + }) + + Describe("Pagination", func() { + It("should list all resources using pagination", func() { + // given + offset := "" + pageSize := 2 + numOfResources := 5 + resourceNames := map[string]bool{} + + // setup create resources + for i := 0; i < numOfResources; i++ { + createResource(fmt.Sprintf("res-%d.demo", i)) + } - Describe("Pagination", func() { - It("should list all resources using pagination", func() { - // given - offset := "" - pageSize := 2 - numOfResources := 5 - resourceNames := map[string]bool{} - - // setup create resources - for i := 0; i < numOfResources; i++ { - createResource(fmt.Sprintf("res-%d.demo", i)) - } - - // when list first two pages with 2 elements - for i := 1; i <= 2; i++ { - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) - - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).ToNot(BeEmpty()) - Expect(list.Items).To(HaveLen(2)) - - resourceNames[list.Items[0].GetMeta().GetName()] = true - resourceNames[list.Items[1].GetMeta().GetName()] = true - offset = list.Pagination.NextOffset - } - - // when list third page with 1 element (less than page size) + // when list first two pages with 2 elements + for i := 1; i <= 2; i++ { list := core_mesh.TrafficRouteResourceList{} err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) - // then Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.Total).To(Equal(uint32(numOfResources))) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - Expect(list.Items).To(HaveLen(1)) + Expect(list.Pagination.NextOffset).ToNot(BeEmpty()) + Expect(list.Items).To(HaveLen(2)) + resourceNames[list.Items[0].GetMeta().GetName()] = true + resourceNames[list.Items[1].GetMeta().GetName()] = true + offset = list.Pagination.NextOffset + } - // and all elements were retrieved - Expect(resourceNames).To(HaveLen(numOfResources)) - for i := 0; i < numOfResources; i++ { - Expect(resourceNames).To(HaveKey(fmt.Sprintf("res-%d.demo", i))) - } - }) + // when list third page with 1 element (less than page size) + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) - It("next offset should be null when queried collection with less elements than page has", func() { - // setup - createResource("res-1.demo") + // then + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.Total).To(Equal(uint32(numOfResources))) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + Expect(list.Items).To(HaveLen(1)) + resourceNames[list.Items[0].GetMeta().GetName()] = true + + // and all elements were retrieved + Expect(resourceNames).To(HaveLen(numOfResources)) + for i := 0; i < numOfResources; i++ { + Expect(resourceNames).To(HaveKey(fmt.Sprintf("res-%d.demo", i))) + } + }) - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(5, "")) + It("next offset should be null when queried collection with less elements than page has", func() { + // setup + createResource("res-1.demo") - // then - Expect(list.Pagination.Total).To(Equal(uint32(1))) - Expect(list.Items).To(HaveLen(1)) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(5, "")) - It("next offset should be null when queried about size equals to elements available", func() { - // setup - createResource("res-1.demo") + // then + Expect(list.Pagination.Total).To(Equal(uint32(1))) + Expect(list.Items).To(HaveLen(1)) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(1, "")) + It("next offset should be null when queried about size equals to elements available", func() { + // setup + createResource("res-1.demo") - // then - Expect(list.Pagination.Total).To(Equal(uint32(1))) - Expect(list.Items).To(HaveLen(1)) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(1, "")) - It("next offset should be null when queried empty collection", func() { - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "")) + // then + Expect(list.Pagination.Total).To(Equal(uint32(1))) + Expect(list.Items).To(HaveLen(1)) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - // then - Expect(list.Pagination.Total).To(Equal(uint32(0))) - Expect(list.Items).To(BeEmpty()) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + It("next offset should be null when queried empty collection", func() { + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "")) - It("next offset should return error when query with invalid offset", func() { - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "123invalidOffset")) + // then + Expect(list.Pagination.Total).To(Equal(uint32(0))) + Expect(list.Items).To(BeEmpty()) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - // then - Expect(list.Pagination.Total).To(Equal(uint32(0))) - Expect(err).To(Equal(store.ErrorInvalidOffset)) - }) + It("next offset should return error when query with invalid offset", func() { + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "123invalidOffset")) + + // then + Expect(list.Pagination.Total).To(Equal(uint32(0))) + Expect(err).To(Equal(store.ErrorInvalidOffset)) }) }) }) From e143233d9b201201720bd6f03b35660c56c89a65 Mon Sep 17 00:00:00 2001 From: slonka Date: Wed, 29 Mar 2023 21:53:58 +0200 Subject: [PATCH 26/28] feat(persistance): fix nil pointer dereference in create store Signed-off-by: slonka --- .../resources/k8s/store_template_test.go | 2 +- .../resources/memory/store_template_test.go | 4 +- .../resources/postgres/store_template_test.go | 53 +- pkg/test/store/owner_test_template.go | 287 ++++---- pkg/test/store/store_test_template.go | 685 +++++++++--------- 5 files changed, 518 insertions(+), 513 deletions(-) diff --git a/pkg/plugins/resources/k8s/store_template_test.go b/pkg/plugins/resources/k8s/store_template_test.go index d5d55b5d80a8..adfe27a851ec 100644 --- a/pkg/plugins/resources/k8s/store_template_test.go +++ b/pkg/plugins/resources/k8s/store_template_test.go @@ -30,5 +30,5 @@ var _ = Describe("KubernetesStore template", func() { }, Scheme: k8sClientScheme, } - }) + }, "kubernetes") }) diff --git a/pkg/plugins/resources/memory/store_template_test.go b/pkg/plugins/resources/memory/store_template_test.go index f1e9b951146a..16b932f735a8 100644 --- a/pkg/plugins/resources/memory/store_template_test.go +++ b/pkg/plugins/resources/memory/store_template_test.go @@ -8,6 +8,6 @@ import ( ) var _ = Describe("MemoryStore template", func() { - test_store.ExecuteStoreTests(memory.NewStore) - test_store.ExecuteOwnerTests(memory.NewStore) + test_store.ExecuteStoreTests(memory.NewStore, "memory") + test_store.ExecuteOwnerTests(memory.NewStore, "memory") }) diff --git a/pkg/plugins/resources/postgres/store_template_test.go b/pkg/plugins/resources/postgres/store_template_test.go index 76b79dbb29b1..3738d4a24cb6 100644 --- a/pkg/plugins/resources/postgres/store_template_test.go +++ b/pkg/plugins/resources/postgres/store_template_test.go @@ -13,36 +13,35 @@ import ( var _ = Describe("PostgresStore template", func() { createStore := func(storeName string) func() store.ResourceStore { - cfg, err := c.Config(test_postgres.WithRandomDb) - Expect(err).ToNot(HaveOccurred()) - cfg.MaxOpenConnections = 2 - - pqMetrics, err := core_metrics.NewMetrics("Standalone") - Expect(err).ToNot(HaveOccurred()) - - pgxMetrics, err := core_metrics.NewMetrics("Standalone") - Expect(err).ToNot(HaveOccurred()) - - _, err = MigrateDb(*cfg) - Expect(err).ToNot(HaveOccurred()) - - var pStore store.ResourceStore - if storeName == "pgx" { - cfg.DriverName = postgres.DriverNamePgx - pStore, err = NewPgxStore(pgxMetrics, *cfg) - } else { - cfg.DriverName = postgres.DriverNamePq - pStore, err = NewPqStore(pqMetrics, *cfg) - } - Expect(err).ToNot(HaveOccurred()) - return func() store.ResourceStore { + cfg, err := c.Config(test_postgres.WithRandomDb) + Expect(err).ToNot(HaveOccurred()) + cfg.MaxOpenConnections = 2 + + pqMetrics, err := core_metrics.NewMetrics("Standalone") + Expect(err).ToNot(HaveOccurred()) + + pgxMetrics, err := core_metrics.NewMetrics("Standalone") + Expect(err).ToNot(HaveOccurred()) + + _, err = MigrateDb(*cfg) + Expect(err).ToNot(HaveOccurred()) + + var pStore store.ResourceStore + if storeName == "pgx" { + cfg.DriverName = postgres.DriverNamePgx + pStore, err = NewPgxStore(pgxMetrics, *cfg) + } else { + cfg.DriverName = postgres.DriverNamePq + pStore, err = NewPqStore(pqMetrics, *cfg) + } + Expect(err).ToNot(HaveOccurred()) return pStore } } - test_store.ExecuteStoreTests(createStore("pgx")) - test_store.ExecuteOwnerTests(createStore("pgx")) - test_store.ExecuteStoreTests(createStore("pq")) - test_store.ExecuteOwnerTests(createStore("pq")) + test_store.ExecuteStoreTests(createStore("pgx"), "pgx") + test_store.ExecuteOwnerTests(createStore("pgx"), "pgx") + test_store.ExecuteStoreTests(createStore("pq"), "pq") + test_store.ExecuteOwnerTests(createStore("pq"), "pq") }) diff --git a/pkg/test/store/owner_test_template.go b/pkg/test/store/owner_test_template.go index 5937723e23f8..979b37a20ff6 100644 --- a/pkg/test/store/owner_test_template.go +++ b/pkg/test/store/owner_test_template.go @@ -16,6 +16,7 @@ import ( func ExecuteOwnerTests( createStore func() store.ResourceStore, + storeName string, ) { const mesh = "default-mesh" var s store.ClosableResourceStore @@ -29,81 +30,15 @@ func ExecuteOwnerTests( Expect(err).ToNot(HaveOccurred()) }) - It("should delete resource when its owner is deleted", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - name := "resource-1" - trRes := core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, - }, - }, - } - err = s.Create(context.Background(), &trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) - - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - // then - actual := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) - }) - - It("should delete resource when its owner is deleted after owner update", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - name := "resource-1" - trRes := core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", - }, - }, - }, - } - err = s.Create(context.Background(), &trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) - - // when owner is updated - Expect(s.Update(context.Background(), meshRes)).To(Succeed()) - - // and only then deleted - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) - - // then - actual := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) - }) - - It("should delete several resources when their owner is deleted", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + Context("Store: "+storeName, func() { + It("should delete resource when its owner is deleted", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - for i := 0; i < 10; i++ { - tr := core_mesh.TrafficRouteResource{ + name := "resource-1" + trRes := core_mesh.TrafficRouteResource{ Spec: &v1alpha1.TrafficRoute{ Conf: &v1alpha1.TrafficRoute_Conf{ Destination: map[string]string{ @@ -112,37 +47,30 @@ func ExecuteOwnerTests( }, }, } - err = s.Create(context.Background(), &tr, - store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + err = s.Create(context.Background(), &trRes, + store.CreateByKey(name, mesh), store.CreatedAt(time.Now()), store.CreateWithOwner(meshRes)) Expect(err).ToNot(HaveOccurred()) - } - actual := core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(HaveLen(10)) - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual = core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(BeEmpty()) - }) + // then + actual := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) + }) - It("should delete owners chain", func() { - // setup - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete resource when its owner is deleted after owner update", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - var prev model.Resource = meshRes - for i := 0; i < 10; i++ { - curr := &core_mesh.TrafficRouteResource{ + name := "resource-1" + trRes := core_mesh.TrafficRouteResource{ Spec: &v1alpha1.TrafficRoute{ Conf: &v1alpha1.TrafficRoute_Conf{ Destination: map[string]string{ @@ -151,62 +79,137 @@ func ExecuteOwnerTests( }, }, } - err := s.Create(context.Background(), curr, - store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + err = s.Create(context.Background(), &trRes, + store.CreateByKey(name, mesh), store.CreatedAt(time.Now()), - store.CreateWithOwner(prev)) + store.CreateWithOwner(meshRes)) Expect(err).ToNot(HaveOccurred()) - prev = curr - } - actual := core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(HaveLen(10)) + // when owner is updated + Expect(s.Update(context.Background(), meshRes)).To(Succeed()) - // when - err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + // and only then deleted + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - // then - actual = core_mesh.TrafficRouteResourceList{} - err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) - Expect(err).ToNot(HaveOccurred()) - Expect(actual.Items).To(BeEmpty()) - }) + // then + actual := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), actual, store.GetByKey(name, mesh)) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) + }) - It("should delete a parent after children is deleted", func() { - // given - meshRes := core_mesh.NewMeshResource() - err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) - Expect(err).ToNot(HaveOccurred()) + It("should delete several resources when their owner is deleted", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) - name := "resource-1" - trRes := &core_mesh.TrafficRouteResource{ - Spec: &v1alpha1.TrafficRoute{ - Conf: &v1alpha1.TrafficRoute_Conf{ - Destination: map[string]string{ - "path": "demo", + for i := 0; i < 10; i++ { + tr := core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, + }, + }, + } + err = s.Create(context.Background(), &tr, + store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) + } + actual := core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(HaveLen(10)) + + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + // then + actual = core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(BeEmpty()) + }) + + It("should delete owners chain", func() { + // setup + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + var prev model.Resource = meshRes + for i := 0; i < 10; i++ { + curr := &core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, + }, + }, + } + err := s.Create(context.Background(), curr, + store.CreateByKey(fmt.Sprintf("resource-%d", i), mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(prev)) + Expect(err).ToNot(HaveOccurred()) + prev = curr + } + + actual := core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(HaveLen(10)) + + // when + err = s.Delete(context.Background(), meshRes, store.DeleteByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + // then + actual = core_mesh.TrafficRouteResourceList{} + err = s.List(context.Background(), &actual, store.ListByMesh(mesh)) + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Items).To(BeEmpty()) + }) + + It("should delete a parent after children is deleted", func() { + // given + meshRes := core_mesh.NewMeshResource() + err := s.Create(context.Background(), meshRes, store.CreateByKey(mesh, model.NoMesh)) + Expect(err).ToNot(HaveOccurred()) + + name := "resource-1" + trRes := &core_mesh.TrafficRouteResource{ + Spec: &v1alpha1.TrafficRoute{ + Conf: &v1alpha1.TrafficRoute_Conf{ + Destination: map[string]string{ + "path": "demo", + }, }, }, - }, - } - err = s.Create(context.Background(), trRes, - store.CreateByKey(name, mesh), - store.CreatedAt(time.Now()), - store.CreateWithOwner(meshRes)) - Expect(err).ToNot(HaveOccurred()) + } + err = s.Create(context.Background(), trRes, + store.CreateByKey(name, mesh), + store.CreatedAt(time.Now()), + store.CreateWithOwner(meshRes)) + Expect(err).ToNot(HaveOccurred()) - // when children is deleted - err = s.Delete(context.Background(), core_mesh.NewTrafficRouteResource(), store.DeleteByKey(name, mesh)) + // when children is deleted + err = s.Delete(context.Background(), core_mesh.NewTrafficRouteResource(), store.DeleteByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // when parent is deleted - err = s.Delete(context.Background(), core_mesh.NewMeshResource(), store.DeleteByKey(mesh, model.NoMesh)) + // when parent is deleted + err = s.Delete(context.Background(), core_mesh.NewMeshResource(), store.DeleteByKey(mesh, model.NoMesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) + }) }) } diff --git a/pkg/test/store/store_test_template.go b/pkg/test/store/store_test_template.go index 12e25887950a..7fccead523ac 100644 --- a/pkg/test/store/store_test_template.go +++ b/pkg/test/store/store_test_template.go @@ -18,6 +18,7 @@ import ( func ExecuteStoreTests( createStore func() store.ResourceStore, + storeName string, ) { const mesh = "default-mesh" var s store.ClosableResourceStore @@ -56,434 +57,436 @@ func ExecuteStoreTests( return &res } - Describe("Create()", func() { - It("should create a new resource", func() { - // given - name := "resource1.demo" + Context("Store: "+storeName, func() { + Describe("Create()", func() { + It("should create a new resource", func() { + // given + name := "resource1.demo" - // when - created := createResource(name) + // when + created := createResource(name) - // when retrieve created object - resource := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when retrieve created object + resource := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).ToNot(HaveOccurred()) - // and it has same data - Expect(resource.Meta.GetName()).To(Equal(name)) - Expect(resource.Meta.GetMesh()).To(Equal(mesh)) - Expect(resource.Meta.GetVersion()).ToNot(BeEmpty()) - Expect(resource.Meta.GetCreationTime().Unix()).ToNot(Equal(0)) - Expect(resource.Meta.GetCreationTime()).To(Equal(resource.Meta.GetModificationTime())) - Expect(resource.Spec).To(MatchProto(created.Spec)) - }) + // and it has same data + Expect(resource.Meta.GetName()).To(Equal(name)) + Expect(resource.Meta.GetMesh()).To(Equal(mesh)) + Expect(resource.Meta.GetVersion()).ToNot(BeEmpty()) + Expect(resource.Meta.GetCreationTime().Unix()).ToNot(Equal(0)) + Expect(resource.Meta.GetCreationTime()).To(Equal(resource.Meta.GetModificationTime())) + Expect(resource.Spec).To(MatchProto(created.Spec)) + }) - It("should not create a duplicate record", func() { - // given - name := "duplicated-record.demo" - resource := createResource(name) + It("should not create a duplicate record", func() { + // given + name := "duplicated-record.demo" + resource := createResource(name) - // when try to create another one with same name - resource.SetMeta(nil) - err := s.Create(context.Background(), resource, store.CreateByKey(name, mesh)) + // when try to create another one with same name + resource.SetMeta(nil) + err := s.Create(context.Background(), resource, store.CreateByKey(name, mesh)) - // then - Expect(err).To(MatchError(store.ErrorResourceAlreadyExists(resource.Descriptor().Name, name, mesh))) + // then + Expect(err).To(MatchError(store.ErrorResourceAlreadyExists(resource.Descriptor().Name, name, mesh))) + }) }) - }) - - Describe("Update()", func() { - It("should return an error if resource is not found", func() { - // given - name := "to-be-updated.demo" - resource := createResource(name) - - // when delete resource - err := s.Delete( - context.Background(), - resource, - store.DeleteByKey(resource.Meta.GetName(), mesh), - ) - // then - Expect(err).ToNot(HaveOccurred()) + Describe("Update()", func() { + It("should return an error if resource is not found", func() { + // given + name := "to-be-updated.demo" + resource := createResource(name) - // when trying to update nonexistent resource - err = s.Update(context.Background(), resource) + // when delete resource + err := s.Delete( + context.Background(), + resource, + store.DeleteByKey(resource.Meta.GetName(), mesh), + ) - // then - Expect(err).To(MatchError(store.ErrorResourceConflict(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).ToNot(HaveOccurred()) - It("should update an existing resource", func() { - // given a resources in storage - name := "to-be-updated.demo" - resource := createResource(name) - modificationTime := time.Now().Add(time.Second) - versionBeforeUpdate := resource.Meta.GetVersion() + // when trying to update nonexistent resource + err = s.Update(context.Background(), resource) - // when - resource.Spec.Conf.Destination["path"] = "new-path" - err := s.Update(context.Background(), resource, store.ModifiedAt(modificationTime)) + // then + Expect(err).To(MatchError(store.ErrorResourceConflict(resource.Descriptor().Name, name, mesh))) + }) - // then - Expect(err).ToNot(HaveOccurred()) + It("should update an existing resource", func() { + // given a resources in storage + name := "to-be-updated.demo" + resource := createResource(name) + modificationTime := time.Now().Add(time.Second) + versionBeforeUpdate := resource.Meta.GetVersion() - // and meta is updated (version and modification time) - Expect(resource.Meta.GetVersion()).ToNot(Equal(versionBeforeUpdate)) - if reflect.TypeOf(createStore()) != reflect.TypeOf(&resources_k8s.KubernetesStore{}) { - Expect(resource.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) - } + // when + resource.Spec.Conf.Destination["path"] = "new-path" + err := s.Update(context.Background(), resource, store.ModifiedAt(modificationTime)) - // when retrieve the resource - res := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), res, store.GetByKey(name, mesh)) + // then + Expect(err).ToNot(HaveOccurred()) - // then - Expect(err).ToNot(HaveOccurred()) + // and meta is updated (version and modification time) + Expect(resource.Meta.GetVersion()).ToNot(Equal(versionBeforeUpdate)) + if reflect.TypeOf(createStore()) != reflect.TypeOf(&resources_k8s.KubernetesStore{}) { + Expect(resource.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) + } - // and - Expect(res.Spec.Conf.Destination["path"]).To(Equal("new-path")) - - // and modification time is updated - // on K8S modification time is always the creation time, because there is no data for modification time - if reflect.TypeOf(createStore()) == reflect.TypeOf(&resources_k8s.KubernetesStore{}) { - Expect(res.Meta.GetModificationTime()).To(Equal(res.Meta.GetCreationTime())) - } else { - Expect(res.Meta.GetModificationTime()).ToNot(Equal(res.Meta.GetCreationTime())) - Expect(res.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) - } - }) + // when retrieve the resource + res := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), res, store.GetByKey(name, mesh)) - // todo(jakubdyszkiewicz) write tests for optimistic locking - }) + // then + Expect(err).ToNot(HaveOccurred()) - Describe("Delete()", func() { - It("should throw an error if resource is not found", func() { - // given - name := "non-existent-name.demo" - resource := core_mesh.NewTrafficRouteResource() + // and + Expect(res.Spec.Conf.Destination["path"]).To(Equal("new-path")) - // when - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) + // and modification time is updated + // on K8S modification time is always the creation time, because there is no data for modification time + if reflect.TypeOf(createStore()) == reflect.TypeOf(&resources_k8s.KubernetesStore{}) { + Expect(res.Meta.GetModificationTime()).To(Equal(res.Meta.GetCreationTime())) + } else { + Expect(res.Meta.GetModificationTime()).ToNot(Equal(res.Meta.GetCreationTime())) + Expect(res.Meta.GetModificationTime().Round(time.Millisecond).Nanosecond() / 1e6).To(Equal(modificationTime.Round(time.Millisecond).Nanosecond() / 1e6)) + } + }) - // then - Expect(err).To(HaveOccurred()) - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + // todo(jakubdyszkiewicz) write tests for optimistic locking }) - It("should not delete resource from another mesh", func() { - // given - name := "tr-1.demo" - resource := createResource(name) + Describe("Delete()", func() { + It("should throw an error if resource is not found", func() { + // given + name := "non-existent-name.demo" + resource := core_mesh.NewTrafficRouteResource() - // when - resource.SetMeta(nil) // otherwise the validation from strict client fires that mesh is different - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, "different-mesh")) + // when + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) - // then - Expect(err).To(HaveOccurred()) - Expect(store.IsResourceNotFound(err)).To(BeTrue()) + // then + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - // and when getting the given resource - getResource := core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), getResource, store.GetByKey(name, mesh)) + It("should not delete resource from another mesh", func() { + // given + name := "tr-1.demo" + resource := createResource(name) - // then resource still exists - Expect(err).ToNot(HaveOccurred()) - }) + // when + resource.SetMeta(nil) // otherwise the validation from strict client fires that mesh is different + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, "different-mesh")) - It("should delete an existing resource", func() { - // given a resources in storage - name := "to-be-deleted.demo" - createResource(name) + // then + Expect(err).To(HaveOccurred()) + Expect(store.IsResourceNotFound(err)).To(BeTrue()) - // when - resource := core_mesh.NewTrafficRouteResource() - err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) + // and when getting the given resource + getResource := core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), getResource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then resource still exists + Expect(err).ToNot(HaveOccurred()) + }) - // when query for deleted resource - resource = core_mesh.NewTrafficRouteResource() - err = s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + It("should delete an existing resource", func() { + // given a resources in storage + name := "to-be-deleted.demo" + createResource(name) - // then resource cannot be found - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) - }) + // when + resource := core_mesh.NewTrafficRouteResource() + err := s.Delete(context.TODO(), resource, store.DeleteByKey(name, mesh)) - Describe("Get()", func() { - It("should return an error if resource is not found", func() { - // given - name := "non-existing-resource.demo" - resource := core_mesh.NewTrafficRouteResource() + // then + Expect(err).ToNot(HaveOccurred()) - // when - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when query for deleted resource + resource = core_mesh.NewTrafficRouteResource() + err = s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).To(MatchError(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + // then resource cannot be found + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) }) - It("should return an error if resource is not found in given mesh", func() { - // given a resources in mesh "mesh" - name := "existing-resource.demo" - mesh := "different-mesh" - createResource(name) + Describe("Get()", func() { + It("should return an error if resource is not found", func() { + // given + name := "non-existing-resource.demo" + resource := core_mesh.NewTrafficRouteResource() - // when - resource := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) + // when + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) - }) + // then + Expect(err).To(MatchError(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - It("should return an existing resource", func() { - // given a resources in storage - name := "get-existing-resource.demo" - createdResource := createResource(name) + It("should return an error if resource is not found in given mesh", func() { + // given a resources in mesh "mesh" + name := "existing-resource.demo" + mesh := "different-mesh" + createResource(name) - // when - res := core_mesh.NewTrafficRouteResource() - err := s.Get(context.Background(), res, store.GetByKey(name, mesh)) + // when + resource := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), resource, store.GetByKey(name, mesh)) - // then - Expect(err).ToNot(HaveOccurred()) + // then + Expect(err).To(Equal(store.ErrorResourceNotFound(resource.Descriptor().Name, name, mesh))) + }) - // and - Expect(res.Meta.GetName()).To(Equal(name)) - Expect(res.Meta.GetVersion()).ToNot(BeEmpty()) - Expect(res.Spec).To(MatchProto(createdResource.Spec)) - }) + It("should return an existing resource", func() { + // given a resources in storage + name := "get-existing-resource.demo" + createdResource := createResource(name) - It("should get resource by version", func() { - // given - name := "existing-resource.demo" - res := createResource(name) + // when + res := core_mesh.NewTrafficRouteResource() + err := s.Get(context.Background(), res, store.GetByKey(name, mesh)) - // when trying to retrieve resource with proper version - err := s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion(res.GetMeta().GetVersion())) + // then + Expect(err).ToNot(HaveOccurred()) - // then resource is found - Expect(err).ToNot(HaveOccurred()) + // and + Expect(res.Meta.GetName()).To(Equal(name)) + Expect(res.Meta.GetVersion()).ToNot(BeEmpty()) + Expect(res.Spec).To(MatchProto(createdResource.Spec)) + }) - // when trying to retrieve resource with different version - err = s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion("9999999")) + It("should get resource by version", func() { + // given + name := "existing-resource.demo" + res := createResource(name) - // then resource precondition failed error occurred - Expect(store.IsResourcePreconditionFailed(err)).To(BeTrue()) - }) - }) + // when trying to retrieve resource with proper version + err := s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion(res.GetMeta().GetVersion())) - Describe("List()", func() { - It("should return an empty list if there are no matching resources", func() { - // given - list := core_mesh.TrafficRouteResourceList{} + // then resource is found + Expect(err).ToNot(HaveOccurred()) - // when - err := s.List(context.Background(), &list, store.ListByMesh(mesh)) + // when trying to retrieve resource with different version + err = s.Get(context.Background(), core_mesh.NewTrafficRouteResource(), store.GetByKey(name, mesh), store.GetByVersion("9999999")) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(0))) - // and - Expect(list.Items).To(HaveLen(0)) + // then resource precondition failed error occurred + Expect(store.IsResourcePreconditionFailed(err)).To(BeTrue()) + }) }) - It("should return a list of resources", func() { - // given two resources - createResource("res-1.demo") - createResource("res-2.demo") + Describe("List()", func() { + It("should return an empty list if there are no matching resources", func() { + // given + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list, store.ListByMesh(mesh)) - // when - err := s.List(context.Background(), &list) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(0))) + // and + Expect(list.Items).To(HaveLen(0)) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(HaveLen(2)) - // and - names := []string{list.Items[0].Meta.GetName(), list.Items[1].Meta.GetName()} - Expect(names).To(ConsistOf("res-1.demo", "res-2.demo")) - Expect(list.Items[0].Meta.GetMesh()).To(Equal(mesh)) - Expect(list.Items[0].Spec.Conf.Destination["path"]).To(Equal("demo")) - Expect(list.Items[1].Meta.GetMesh()).To(Equal(mesh)) - Expect(list.Items[1].Spec.Conf.Destination["path"]).To(Equal("demo")) - }) + It("should return a list of resources", func() { + // given two resources + createResource("res-1.demo") + createResource("res-2.demo") - It("should not return a list of resources in different mesh", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list) - // when - err := s.List(context.Background(), &list, store.ListByMesh("different-mesh")) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(HaveLen(2)) + // and + names := []string{list.Items[0].Meta.GetName(), list.Items[1].Meta.GetName()} + Expect(names).To(ConsistOf("res-1.demo", "res-2.demo")) + Expect(list.Items[0].Meta.GetMesh()).To(Equal(mesh)) + Expect(list.Items[0].Spec.Conf.Destination["path"]).To(Equal("demo")) + Expect(list.Items[1].Meta.GetMesh()).To(Equal(mesh)) + Expect(list.Items[1].Spec.Conf.Destination["path"]).To(Equal("demo")) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(0))) - // and - Expect(list.Items).To(HaveLen(0)) - }) + It("should not return a list of resources in different mesh", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") - It("should return a list of resources with prefix from all meshes", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") - createResource("list-mes-1.demo") + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list, store.ListByMesh("different-mesh")) - // when - err := s.List(context.Background(), &list, store.ListByNameContains("list-res")) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(0))) + // and + Expect(list.Items).To(HaveLen(0)) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { - var res []string - for _, v := range itms { - res = append(res, v.GetMeta().GetName()) - } - return res - }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) - }) + It("should return a list of resources with prefix from all meshes", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") + createResource("list-mes-1.demo") - It("should return a list of resources with prefix from the specific mesh", func() { - // given two resources - createResource("list-res-1.demo") - createResource("list-res-2.demo") - createResource("list-mes-1.demo") + list := core_mesh.TrafficRouteResourceList{} - list := core_mesh.TrafficRouteResourceList{} + // when + err := s.List(context.Background(), &list, store.ListByNameContains("list-res")) - // when - err := s.List(context.Background(), &list, store.ListByNameContains("list-res"), store.ListByMesh(mesh)) + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { + var res []string + for _, v := range itms { + res = append(res, v.GetMeta().GetName()) + } + return res + }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) + }) - // then - Expect(err).ToNot(HaveOccurred()) - // and - Expect(list.Pagination.Total).To(Equal(uint32(2))) - // and - Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { - var res []string - for _, v := range itms { - res = append(res, v.GetMeta().GetName()) - } - return res - }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) - }) + It("should return a list of resources with prefix from the specific mesh", func() { + // given two resources + createResource("list-res-1.demo") + createResource("list-res-2.demo") + createResource("list-mes-1.demo") - Describe("Pagination", func() { - It("should list all resources using pagination", func() { - // given - offset := "" - pageSize := 2 - numOfResources := 5 - resourceNames := map[string]bool{} - - // setup create resources - for i := 0; i < numOfResources; i++ { - createResource(fmt.Sprintf("res-%d.demo", i)) - } + list := core_mesh.TrafficRouteResourceList{} + + // when + err := s.List(context.Background(), &list, store.ListByNameContains("list-res"), store.ListByMesh(mesh)) + + // then + Expect(err).ToNot(HaveOccurred()) + // and + Expect(list.Pagination.Total).To(Equal(uint32(2))) + // and + Expect(list.Items).To(WithTransform(func(itms []*core_mesh.TrafficRouteResource) []string { + var res []string + for _, v := range itms { + res = append(res, v.GetMeta().GetName()) + } + return res + }, Equal([]string{"list-res-1.demo", "list-res-2.demo"}))) + }) - // when list first two pages with 2 elements - for i := 1; i <= 2; i++ { + Describe("Pagination", func() { + It("should list all resources using pagination", func() { + // given + offset := "" + pageSize := 2 + numOfResources := 5 + resourceNames := map[string]bool{} + + // setup create resources + for i := 0; i < numOfResources; i++ { + createResource(fmt.Sprintf("res-%d.demo", i)) + } + + // when list first two pages with 2 elements + for i := 1; i <= 2; i++ { + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) + + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).ToNot(BeEmpty()) + Expect(list.Items).To(HaveLen(2)) + + resourceNames[list.Items[0].GetMeta().GetName()] = true + resourceNames[list.Items[1].GetMeta().GetName()] = true + offset = list.Pagination.NextOffset + } + + // when list third page with 1 element (less than page size) list := core_mesh.TrafficRouteResourceList{} err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) + // then Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).ToNot(BeEmpty()) - Expect(list.Items).To(HaveLen(2)) - + Expect(list.Pagination.Total).To(Equal(uint32(numOfResources))) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + Expect(list.Items).To(HaveLen(1)) resourceNames[list.Items[0].GetMeta().GetName()] = true - resourceNames[list.Items[1].GetMeta().GetName()] = true - offset = list.Pagination.NextOffset - } - // when list third page with 1 element (less than page size) - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(pageSize, offset)) - - // then - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.Total).To(Equal(uint32(numOfResources))) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - Expect(list.Items).To(HaveLen(1)) - resourceNames[list.Items[0].GetMeta().GetName()] = true - - // and all elements were retrieved - Expect(resourceNames).To(HaveLen(numOfResources)) - for i := 0; i < numOfResources; i++ { - Expect(resourceNames).To(HaveKey(fmt.Sprintf("res-%d.demo", i))) - } - }) + // and all elements were retrieved + Expect(resourceNames).To(HaveLen(numOfResources)) + for i := 0; i < numOfResources; i++ { + Expect(resourceNames).To(HaveKey(fmt.Sprintf("res-%d.demo", i))) + } + }) - It("next offset should be null when queried collection with less elements than page has", func() { - // setup - createResource("res-1.demo") + It("next offset should be null when queried collection with less elements than page has", func() { + // setup + createResource("res-1.demo") - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(5, "")) + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(5, "")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(1))) - Expect(list.Items).To(HaveLen(1)) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // then + Expect(list.Pagination.Total).To(Equal(uint32(1))) + Expect(list.Items).To(HaveLen(1)) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - It("next offset should be null when queried about size equals to elements available", func() { - // setup - createResource("res-1.demo") + It("next offset should be null when queried about size equals to elements available", func() { + // setup + createResource("res-1.demo") - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(1, "")) + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh(mesh), store.ListByPage(1, "")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(1))) - Expect(list.Items).To(HaveLen(1)) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // then + Expect(list.Pagination.Total).To(Equal(uint32(1))) + Expect(list.Items).To(HaveLen(1)) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - It("next offset should be null when queried empty collection", func() { - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "")) + It("next offset should be null when queried empty collection", func() { + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(0))) - Expect(list.Items).To(BeEmpty()) - Expect(err).ToNot(HaveOccurred()) - Expect(list.Pagination.NextOffset).To(BeEmpty()) - }) + // then + Expect(list.Pagination.Total).To(Equal(uint32(0))) + Expect(list.Items).To(BeEmpty()) + Expect(err).ToNot(HaveOccurred()) + Expect(list.Pagination.NextOffset).To(BeEmpty()) + }) - It("next offset should return error when query with invalid offset", func() { - // when - list := core_mesh.TrafficRouteResourceList{} - err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "123invalidOffset")) + It("next offset should return error when query with invalid offset", func() { + // when + list := core_mesh.TrafficRouteResourceList{} + err := s.List(context.Background(), &list, store.ListByMesh("unknown-mesh"), store.ListByPage(2, "123invalidOffset")) - // then - Expect(list.Pagination.Total).To(Equal(uint32(0))) - Expect(err).To(Equal(store.ErrorInvalidOffset)) + // then + Expect(list.Pagination.Total).To(Equal(uint32(0))) + Expect(err).To(Equal(store.ErrorInvalidOffset)) + }) }) }) }) From f0515e01c746caffc2ebeb5050524496eaab2b71 Mon Sep 17 00:00:00 2001 From: slonka Date: Thu, 30 Mar 2023 11:31:13 +0200 Subject: [PATCH 27/28] feat(persistance): charly review nits Signed-off-by: slonka --- pkg/config/app/kuma-cp/kuma-cp.defaults.yaml | 4 ++-- pkg/config/plugins/resources/postgres/config.go | 6 +++--- tools/postgres/Dockerfile | 1 + 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml index 0ea3a300cea7..459cf2b846ea 100644 --- a/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml +++ b/pkg/config/app/kuma-cp/kuma-cp.defaults.yaml @@ -33,9 +33,9 @@ store: maxConnectionLifetime: "1h" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME # MaxConnectionLifetimeJitter (applied only when driverName=pgx) is the duration after maxConnectionLifetime to randomly decide to close a connection. # This helps prevent all connections from being closed at the exact same time, starving the pool. - maxConnectionLifetimeJitter: "0s" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER + maxConnectionLifetimeJitter: "1m" # ENV: KUMA_STORE_POSTGRES_MAX_CONNECTION_LIFETIME_JITTER # HealthCheckInterval (applied only when driverName=pgx) is the duration between checks of the health of idle connections. - healthCheckInterval: "1m" # ENV: KUMA_STORE_POSTGRES_HEALTH_CHECK_INTERVAL + healthCheckInterval: "30s" # ENV: KUMA_STORE_POSTGRES_HEALTH_CHECK_INTERVAL # MinOpenConnections (applied only when driverName=pgx) is the minimum number of open connections to the database minOpenConnections: 0 # ENV: KUMA_STORE_POSTGRES_MIN_OPEN_CONNECTIONS # MaxOpenConnections is the maximum number of open connections to the database diff --git a/pkg/config/plugins/resources/postgres/config.go b/pkg/config/plugins/resources/postgres/config.go index 7d1bd6a07331..b89c09c4cae6 100644 --- a/pkg/config/plugins/resources/postgres/config.go +++ b/pkg/config/plugins/resources/postgres/config.go @@ -28,9 +28,9 @@ var ( DefaultMinReconnectInterval = config_types.Duration{Duration: 10 * time.Second} DefaultMaxReconnectInterval = config_types.Duration{Duration: 60 * time.Second} DefaultMaxConnectionLifetime = config_types.Duration{Duration: time.Hour} - DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 0} - DefaultHealthCheckInterval = config_types.Duration{Duration: time.Minute} - // above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 + DefaultMaxConnectionLifetimeJitter = config_types.Duration{Duration: 1 * time.Minute} + DefaultHealthCheckInterval = config_types.Duration{Duration: 30 * time.Second} + // some of the above settings taken from pgx https://github.com/jackc/pgx/blob/ca022267dbbfe7a8ba7070557352a5cd08f6cb37/pgxpool/pool.go#L18-L22 ) // Postgres store configuration diff --git a/tools/postgres/Dockerfile b/tools/postgres/Dockerfile index d27d0e783053..2e07ee89f056 100644 --- a/tools/postgres/Dockerfile +++ b/tools/postgres/Dockerfile @@ -11,6 +11,7 @@ RUN chown -R postgres /var/lib/postgresql && \ chmod 600 /var/lib/postgresql/postgres.server.key CMD ["-c", "ssl=on", "-c", "max_connections=10000", "-c", "ssl_cert_file=/var/lib/postgresql/postgres.server.crt", "-c", "ssl_key_file=/var/lib/postgresql/postgres.server.key", "-c", "ssl_ca_file=/var/lib/postgresql/rootCA.crt", "-c", "hba_file=/var/lib/postgresql/pg_hba.conf"] FROM postgres:latest AS pg-standard +CMD ["-c", "max_connections=10000"] FROM pg-${MODE} RUN echo ${UNIQUEID} /tmp/uniqueID From a105ebea2fd795b63c61054e6c3892eaa5fc564b Mon Sep 17 00:00:00 2001 From: slonka Date: Thu, 30 Mar 2023 11:34:08 +0200 Subject: [PATCH 28/28] feat(persistance): mike review Signed-off-by: slonka --- pkg/plugins/resources/postgres/pgx_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/plugins/resources/postgres/pgx_store.go b/pkg/plugins/resources/postgres/pgx_store.go index d420009caf9b..b41570fe59ee 100644 --- a/pkg/plugins/resources/postgres/pgx_store.go +++ b/pkg/plugins/resources/postgres/pgx_store.go @@ -269,7 +269,7 @@ func rowToItem(resources core_model.ResourceList, rows pgx.Rows) (core_model.Res } func (r *pgxResourceStore) Close() error { - r.pool.Close() // does not work, at least on my machine + r.pool.Close() return nil }