From 5b3c9ed8f5b5b8433d0955062e6b1960979051f9 Mon Sep 17 00:00:00 2001 From: Hank Donnay Date: Thu, 6 Jul 2023 13:49:29 -0500 Subject: [PATCH] config: add multiple database configuration This will give us the ability to add additional database backends if the need arises. Signed-off-by: Hank Donnay --- config/database.go | 142 ++++++++++++++++++++++++++++++++++++++-- config/database_test.go | 40 +++++++++++ config/go.mod | 2 +- config/indexer.go | 21 +++--- config/lint_test.go | 6 +- config/matcher.go | 25 +++---- config/notifier.go | 24 ++++--- 7 files changed, 222 insertions(+), 38 deletions(-) create mode 100644 config/database_test.go diff --git a/config/database.go b/config/database.go index a6e6ac3916..ce6cf865e3 100644 --- a/config/database.go +++ b/config/database.go @@ -1,12 +1,93 @@ package config import ( + "fmt" "net/url" "os" "strings" ) -func checkDSN(s string) (w []Warning, err error) { +// CheckConnString reports a warning for using the "connstring" member instead +// of the "database" member. +// +// This will panic if the type parameter is not [Matcher], [Indexer], or [Notifier]. +func checkConnString[T any](ws *[]Warning, v *T) { + var cs *string + var d *Database + switch v := any(v).(type) { + case *Matcher: + cs = &v.ConnString + d = v.Database + case *Indexer: + cs = &v.ConnString + d = v.Database + case *Notifier: + cs = &v.ConnString + d = v.Database + default: + panic(fmt.Sprintf("programmer error: passed unexpected type: %T", v)) + } + if *cs != "" { + *ws = append(*ws, errConnString) + } + if d == nil { + *ws = append(*ws, Warning{ + path: ".database", + msg: `missing database configuration`, + }) + } +} + +// ErrConnString is reported by [checkConnString] if the "connstring" member is in use. +var errConnString = Warning{ + path: ".connstring", + inner: fmt.Errorf(`using bare-string for database configuration deprecated: %w`, ErrDeprecated), +} + +// SetConnString adjusts the passed variable by porting from the "connstring" +// member if necessary. +// +// This will panic if the type parameter is not [Matcher], [Indexer], or [Notifier]. +func setConnString[T any](ws *[]Warning, v *T) { + var cs *string + var d *Database + var m *bool + switch v := any(v).(type) { + case *Matcher: + cs = &v.ConnString + d = v.Database + m = v.Migrations + case *Indexer: + cs = &v.ConnString + d = v.Database + m = v.Migrations + case *Notifier: + cs = &v.ConnString + d = v.Database + m = v.Migrations + default: + panic(fmt.Sprintf("programmer error: passed unexpected type: %T", v)) + } + switch { + case *cs != "" && d != nil: + *cs = "" + case *cs != "" && d == nil: + d = &Database{ + Name: `postgresql`, + PostgreSQL: &DatabasePostgreSQL{DSN: *cs}, + Migrations: m, + } + *cs = "" + case *cs == "" && d != nil: // OK, use as-is. + case *cs == "" && d == nil: // Will probably explode later. + } +} + +// CheckPostgresqlDSN is a (very) light check that the value provided isn't completely bogus. +// +// Implementing more rigorous checks would be much more complicated. +// That's not to say it'd be an unwelcome addition, just that it's very large and probably not needed. +func checkPostgresqlDSN(s string) (w []Warning) { switch { case s == "": // Nothing specified, make sure something's in the environment. @@ -27,16 +108,69 @@ func checkDSN(s string) (w []Warning, err error) { if _, err := url.Parse(s); err != nil { w = append(w, Warning{inner: err}) } - case strings.ContainsRune(s, '='): - // Looks like a DSN case strings.Contains(s, `://`): w = append(w, Warning{ msg: "connection string looks like a URL but scheme is unrecognized", }) + case strings.ContainsRune(s, '='): + // Looks like a DSN default: w = append(w, Warning{ msg: "unable to make sense of connection string", }) } - return w, nil + return w +} + +// Database indicates the database configuration. +type Database struct { + // Name indicates which database backend to use. + // + // This value must match the json/yaml tag. + Name string `json:"name" yaml:"name"` + // Migrations indicates if database migrations should run automatically. + Migrations *bool `json:"migrations,omitempty" yaml:"migrations,omitempty"` + // PostgreSQL is the PostgreSQL configuration. + PostgreSQL *DatabasePostgreSQL `json:"postgresql,omitempty" yaml:"postgresql,omitempty"` +} + +func (d *Database) lint() (ws []Warning, err error) { + switch n := d.Name; n { + case "postgresql": // OK + case "postgres": + ws = append(ws, Warning{ + msg: fmt.Sprintf("unknown database: %q (did you mean %q?)", n, "postgresql"), + path: ".name", + }) + default: + ws = append(ws, Warning{ + msg: fmt.Sprintf("unknown database: %q", n), + path: ".name", + }) + } + return ws, nil +} +func (d *Database) validate(_ Mode) ([]Warning, error) { + return d.lint() +} + +// DatabasePostgreSQL is the PostgreSQL-specific database configuration. +// +// Validation assumes that if the "DSN" member is empty but any environment variables with a "PG" prefix are present, +// the configuration is specified in via environment variables. +// This package implements no checking for the specifics of the DSN/URL/environment variables; +// providing malformed values will fail at the point of use instead of configuration validation. +type DatabasePostgreSQL struct { + // DSN is a data source name (aka "connection string") as documented for [libpq], with the extensions supported by [pgxpool]. + // + // [libpq]: https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING + // [pgxpool]: https://pkg.go.dev/github.com/jackc/pgx/v4/pgxpool#ParseConfig + DSN string `json:"dsn" yaml:"dsn"` +} + +func (d *DatabasePostgreSQL) lint() ([]Warning, error) { + return checkPostgresqlDSN(d.DSN), nil +} +func (d *DatabasePostgreSQL) validate(_ Mode) ([]Warning, error) { + return d.lint() } diff --git a/config/database_test.go b/config/database_test.go new file mode 100644 index 0000000000..49fc75b72a --- /dev/null +++ b/config/database_test.go @@ -0,0 +1,40 @@ +package config + +import ( + "encoding/json" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestDatabaseUnmarshal(t *testing.T) { + want := Database{ + Name: "postgresql", + PostgreSQL: &DatabasePostgreSQL{ + DSN: "host=test", + }, + } + input := []string{ + `{"name":"postgresql","postgresql":{"dsn":"host=test"}}`, + } + + for _, tc := range input { + t.Logf("testing: %#q", tc) + var got Database + if err := json.Unmarshal([]byte(tc), &got); err != nil { + t.Error(err) + continue + } + ws, err := got.lint() + if err != nil { + t.Error(err) + continue + } + for _, w := range ws { + t.Logf("got lint: %v", &w) + } + if !cmp.Equal(&got, &want) { + t.Error(cmp.Diff(&got, &want)) + } + } +} diff --git a/config/go.mod b/config/go.mod index 170b2506aa..08daf0aa94 100644 --- a/config/go.mod +++ b/config/go.mod @@ -1,5 +1,5 @@ module github.com/quay/clair/config -go 1.17 +go 1.18 require github.com/google/go-cmp v0.6.0 diff --git a/config/indexer.go b/config/indexer.go index 762127af01..7c2d78f821 100644 --- a/config/indexer.go +++ b/config/indexer.go @@ -12,7 +12,11 @@ type Indexer struct { // url: "postgres://pqgotest:password@localhost/pqgotest?sslmode=verify-full" // or // string: "user=pqgotest dbname=pqgotest sslmode=verify-full" - ConnString string `yaml:"connstring" json:"connstring"` + // + // Deprecated: Use the ".database" member instead. + ConnString string `yaml:"connstring,omitempty" json:"connstring,omitempty"` + // Database is the database configuration. + Database *Database `yaml:"database,omitempty" json:"database,omitempty"` // A positive value representing seconds. // // Concurrent Indexers lock on manifest scans to avoid clobbering. @@ -34,7 +38,9 @@ type Indexer struct { // A "true" or "false" value // // Whether Indexer nodes handle migrations to their database. - Migrations bool `yaml:"migrations,omitempty" json:"migrations,omitempty"` + // + // Deprecated: Use the ".database.migrations" member instead. + Migrations *bool `yaml:"migrations,omitempty" json:"migrations,omitempty"` // Airgap disables HTTP access to the Internet. This affects both indexers and // the layer fetcher. Database connections are unaffected. // @@ -66,18 +72,15 @@ func (i *Indexer) validate(mode Mode) (ws []Warning, err error) { msg: `automatically sizing number of concurrent requests`, }) } + lws, err := i.lint() + setConnString(&ws, i) return append(ws, lws...), err } func (i *Indexer) lint() (ws []Warning, err error) { - ws, err = checkDSN(i.ConnString) - if err != nil { - return ws, err - } - for i := range ws { - ws[i].path = ".connstring" - } + checkConnString(&ws, i) + if i.ScanLockRetry > 10 { // Guess at what a "large" value is here. ws = append(ws, Warning{ path: ".scanlock_retry", diff --git a/config/lint_test.go b/config/lint_test.go index 9f96549eeb..d1970e3b16 100644 --- a/config/lint_test.go +++ b/config/lint_test.go @@ -14,11 +14,11 @@ func ExampleLint() { // error: // warning: http listen address not provided, default will be used (at $.http_listen_addr) // warning: introspection address not provided, default will be used (at $.introspection_addr) - // warning: connection string is empty and no relevant environment variables found (at $.indexer.connstring) - // warning: connection string is empty and no relevant environment variables found (at $.matcher.connstring) + // warning: missing database configuration (at $.indexer.database) + // warning: missing database configuration (at $.matcher.database) // warning: updater period is very aggressive: most sources are updated daily (at $.matcher.period) // warning: update garbage collection is off (at $.matcher.update_retention) - // warning: connection string is empty and no relevant environment variables found (at $.notifier.connstring) + // warning: missing database configuration (at $.notifier.database) // warning: interval is very fast: may result in increased workload (at $.notifier.poll_interval) // warning: interval is very fast: may result in increased workload (at $.notifier.delivery_interval) } diff --git a/config/matcher.go b/config/matcher.go index b69627f431..e7ad61da04 100644 --- a/config/matcher.go +++ b/config/matcher.go @@ -13,7 +13,11 @@ type Matcher struct { // url: "postgres://pqgotest:password@localhost/pqgotest?sslmode=verify-full" // or // string: "user=pqgotest dbname=pqgotest sslmode=verify-full" + // + // Deprecated: Use the ".database" member instead. ConnString string `yaml:"connstring" json:"connstring"` + // Database is the database configuration. + Database *Database `yaml:"database,omitempty" json:"database,omitempty"` // A string in : format where can be an empty string. // // A Matcher contacts an Indexer to create a VulnerabilityReport. @@ -36,7 +40,7 @@ type Matcher struct { // Clair allows for a custom connection pool size. This number will // directly set how many active sql connections are allowed concurrently. // - // Deprecated: Pool size should be set through the ConnString member. + // Deprecated: Pool size should be set through the database configuration. // Currently, Clair only uses the "pgxpool" package to connect to the // database, so see // https://pkg.go.dev/github.com/jackc/pgx/v4/pgxpool#ParseConfig for more @@ -51,7 +55,9 @@ type Matcher struct { // A "true" or "false" value // // Whether Matcher nodes handle migrations to their databases. - Migrations bool `yaml:"migrations,omitempty" json:"migrations,omitempty"` + // + // Deprecated: Use the ".database.migrations" member instead. + Migrations *bool `yaml:"migrations,omitempty" json:"migrations,omitempty"` // DisableUpdaters disables the updater's running of matchers. // // This should be toggled on if vulnerabilities are being provided by @@ -59,7 +65,7 @@ type Matcher struct { DisableUpdaters bool `yaml:"disable_updaters,omitempty" json:"disable_updaters,omitempty"` } -func (m *Matcher) validate(mode Mode) ([]Warning, error) { +func (m *Matcher) validate(mode Mode) (ws []Warning, err error) { if mode != ComboMode && mode != MatcherMode { return nil, nil } @@ -90,17 +96,14 @@ func (m *Matcher) validate(mode Mode) ([]Warning, error) { default: panic("programmer error") } - return m.lint() + + lws, err := m.lint() + setConnString(&ws, m) + return append(ws, lws...), err } func (m *Matcher) lint() (ws []Warning, err error) { - ws, err = checkDSN(m.ConnString) - if err != nil { - return ws, err - } - for i := range ws { - ws[i].path = ".connstring" - } + checkConnString(&ws, m) if m.Period < Duration(DefaultMatcherPeriod) { ws = append(ws, Warning{ diff --git a/config/notifier.go b/config/notifier.go index 5d2ba419a4..d09af00fc0 100644 --- a/config/notifier.go +++ b/config/notifier.go @@ -26,7 +26,11 @@ type Notifier struct { // url: "postgres://pqgotest:password@localhost/pqgotest?sslmode=verify-full" // or // string: "user=pqgotest dbname=pqgotest sslmode=verify-full" + // + // Deprecated: Use the ".database" member instead. ConnString string `yaml:"connstring" json:"connstring"` + // Database is the database configuration. + Database *Database `yaml:"database,omitempty" json:"database,omitempty"` // A string in : format where can be an empty string. // // A Notifier contacts an Indexer to create obtain manifests affected by vulnerabilities. @@ -63,10 +67,12 @@ type Notifier struct { // A "true" or "false" value // // Whether Notifier nodes handle migrations to their database. - Migrations bool `yaml:"migrations,omitempty" json:"migrations,omitempty"` + // + // Deprecated: Use the ".database.migrations" member instead. + Migrations *bool `yaml:"migrations,omitempty" json:"migrations,omitempty"` } -func (n *Notifier) validate(mode Mode) ([]Warning, error) { +func (n *Notifier) validate(mode Mode) (ws []Warning, err error) { if mode != ComboMode && mode != NotifierMode { return nil, nil } @@ -88,17 +94,15 @@ func (n *Notifier) validate(mode Mode) ([]Warning, error) { default: panic("programmer error") } - return n.lint() + + lws, err := n.lint() + setConnString(&ws, n) + return append(ws, lws...), err } func (n *Notifier) lint() (ws []Warning, err error) { - ws, err = checkDSN(n.ConnString) - if err != nil { - return ws, err - } - for i := range ws { - ws[i].path = ".connstring" - } + checkConnString(&ws, n) + got := 0 if n.AMQP != nil { got++