CXH-2380: reach the DB2 native DSN form through connector config - #149
CXH-2380: reach the DB2 native DSN form through connector config#149al-conductorone wants to merge 6 commits into
Conversation
A native DB2 DSN (opaque ODBC keyword string like HOSTNAME=...;DATABASE=...) was documented in docs/db2.md but unreachable through config: the engine forced every DSN through url.Parse/String, so a scheme-less native form hit "database scheme must be specified" and a native form with scheme:db2 got mangled into db2://HOSTNAME=... and hit "database name is required in DSN path". Connect now detects a native DB2 DSN and hands it to the driver verbatim, bypassing the URL builder. ResolveDatabaseName extracts DATABASE= so the resolved database name matches the equivalent db2://.../DB URL, keeping resource IDs stable across the two DSN forms.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXH-2380: reach the DB2 native DSN form through connector configBlocking Issues: 0 | Suggestions: 8 | Threads Resolved: 0 Review SummaryThe full PR diff was re-scanned for security and correctness; the new commit adds Security IssuesNone found. (Carried over: the placeholder-expansion concern at Correctness IssuesNone found. Suggestions
Prompt for AI agents |
sync-test@v2 installs the CLI from conductorone/baton (latest v0.4.5, pre-pebble), so baton grants rejects the pebble-format c1z with "c1z: invalid file". @v4 pulls the CLI from conductorone/baton-sdk, which reads pebble.
There was a problem hiding this comment.
No new blocking issues in this pass, but the two blocking findings from the previous review are still unaddressed at 209040e (pkg/database/database.go:446 ignoring opts.Database on the native path, and pkg/database/database.go:551 silently discarding structured connect fields). Two new suggestions posted inline.
Address the CI review findings on the native DB2 DSN path: - Reject a native DSN combined with structured connect fields (host, port, user, password, params) or a per-database override (connect.database, databases). The verbatim path never reaches buildConnectionURL, so those were silently dropped and multi-database sync opened every handle against the DSN's single DATABASE=. Now it errors clearly instead. - Detect native DSNs and extract DATABASE= case-insensitively and with whitespace tolerance (ODBC keywords are case-insensitive; "; " spacing is common). A lowercase/spaced native DSN previously fell through to the URL path and hit "scheme must be specified", or resolved an empty database name. - Apply the same case-insensitive passthrough in db2.convertToDB2DSN, which the native path now reaches for lowercase DSNs. - Anchor the URL-shape check to a leading scheme so a native DSN whose value contains "://" (e.g. PWD=my://secret) is not misread as a URL. - Bump account-provisioning to @v4 so it no longer depends on step ordering to pick up the pebble-compatible CLI. - Document native-DSN exclusivity and case-insensitivity in docs/db2.md. Verified live against a local DB2 container: lowercase native DSN syncs, native DSN + connect.database is rejected, uppercase and db2:// URL forms unaffected.
Address review: splitDB2DSN now enters brace mode only when a value starts with '{'
(so PWD=p{q no longer swallows the next ';'), and detection/DATABASE-extraction move
to db2.IsNativeDSN/db2.DSNDatabase, used by both the router and convertToDB2DSN so they
cannot drift. Spaced native DSN verified live against Db2 v12.1.
| // DSNDatabase returns the DATABASE keyword value from a native DB2 DSN, or "" if absent. | ||
| func DSNDatabase(dsn string) string { | ||
| for _, part := range splitDB2DSN(dsn) { | ||
| keyword, value, found := strings.Cut(strings.TrimSpace(part), "=") |
There was a problem hiding this comment.
🟡 Suggestion: whitespace handling is asymmetric — TrimSpace is applied to the whole part (so a space after ; is tolerated, as documented), but not around the =. DATABASE= TESTDB yields the value " TESTDB", which becomes the dbs map key and the synthetic database row column while the driver connects to TESTDB; DATABASE = TESTDB doesn't match the keyword at all, so IsNativeDSN returns false and the DSN falls back to the URL path and the exact "scheme must be specified" error this PR fixes. Also DATABASE= {my;db} isn't brace-detected by splitDB2DSN since atValueStart is cleared by the space. Trimming the value (and allowing space before =/{) would make parsing match the ODBC leniency the docs advertise.
There was a problem hiding this comment.
Yeah can confirm this one, ran it through an actual go test: atValueStart gets reset by ANY intervening char in the default case, space included, so DATABASE= {SAM;PLE} (space right after =) never flips braced on — the ; inside the braces splits for real and DSNDatabase comes back with {SAM instead of SAM;PLE. No error, just a silently wrong database name flowing into ResolveDatabaseName. That said couldn't find anywhere in this repo (docs, examples, the DSN generator) that would ever actually emit a space between = and {, so real-world likelihood seems low — still, worth trimming the value (or skipping whitespace before checking for {) since the failure mode is silent corruption, not a loud error.
| return "", false, nil | ||
| } | ||
|
|
||
| dsn, err := expandValue(opts.DSN, lookup) |
There was a problem hiding this comment.
🟡 Suggestion: expandValue is a raw ${VAR} substitution with no ODBC quoting, and the result is handed to the driver verbatim — so a placeholder value containing ;, =, or } changes the DSN's structure rather than being treated as data. HOSTNAME=${H};DATABASE=PROD with H="x;DATABASE=DEV" silently connects to DEV, and a password containing ; breaks the connection with a confusing driver error. The db2:// path is safe here because quoteDB2Value brace-quotes such values; the native path has no equivalent. Worth documenting in docs/db2.md that placeholder values with ;/=/spaces must be brace-quoted in the DSN (PWD={${DB_PASS}}).
addressed in dc98939
Replace os.Exit(1) in main.go with exit.LogExit(err) so an auth failure exits with the mapped gRPC status code instead of a bare 1, letting the CI sync-test auth-error check actually assert.
| // A native DB2 DSN is an opaque ODBC keyword=value string, not a URL. Routing it | ||
| // through buildConnectionURL corrupts it (url.Parse/.String mangles the opaque form), | ||
| // so hand it to the driver verbatim. See docs/db2.md. | ||
| nativeDSN, isNativeDB2, err := nativeDB2DSN(opts) |
There was a problem hiding this comment.
🟡 Suggestion: this native-DB2 probe runs before the parsedDsn.Scheme == "" check, and IsNativeDSN returns true for any scheme-less DSN with a DATABASE= part — including non-DB2 ODBC/ADO connection strings such as Server=x;Database=y;User Id=u (see the DATABASE=TESTDB;HOST=x case in native_db2_dsn_test.go:25). Such a config now fails with a DB2-specific message ("DB2 support not compiled...") instead of the clear "database scheme must be specified in DSN or configuration". Consider requiring an explicit db2 scheme when the DSN is not URL-shaped, or requiring the DB2-specific HOSTNAME keyword rather than DATABASE alone. (Confidence: medium.)
| // ConnectMany for databases.static / discovery_query) would be silently dropped | ||
| // on the verbatim path, so reject the combination instead of connecting to the | ||
| // wrong database. See docs/db2.md. | ||
| if hasStructuredConnectFields(opts) { |
There was a problem hiding this comment.
🟡 Suggestion: the mutual-exclusion check fires per-Connect, so the databases.discovery_query path (pkg/connector/connector.go:201) opens a real admin connection and executes the discovery query before ConnectMany rejects the native-DSN + databases combination. databases.static fails fast, but discovery does a full round trip first. Consider validating "native DSN + connect.databases/connect.database" once in openDatabases (or config validation) so the error surfaces before any connection is opened. (Confidence: high on the behavior, low severity.)
| // on the verbatim path, so reject the combination instead of connecting to the | ||
| // wrong database. See docs/db2.md. | ||
| if hasStructuredConnectFields(opts) { | ||
| return nil, Unknown, errors.New( |
There was a problem hiding this comment.
🟡 Suggestion: this is a pure user-config error, but a bare errors.New carries no gRPC status. With the exit.LogExit change in this same PR (cmd/baton-sql/main.go:33), exitCode falls through to codes.Unknown and the process exits 2 rather than 3 (InvalidArgument). Wrapping with status.Error(codes.InvalidArgument, ...) would make the new exit-code mapping actually distinguish misconfiguration from an internal failure. (Confidence: high.)
Pairs with the exit.LogExit change: Validate wrapped the ping error plainly, so exit mapped auth failures to Unknown(2). database.AuthError maps SQLSTATE class 28 (Postgres/Redshift/Vertica/etc.) and MySQL 1045 to codes.Unauthenticated.
| var sqlState interface{ SQLState() string } | ||
| if errors.As(err, &sqlState) && strings.HasPrefix(sqlState.SQLState(), "28") { |
There was a problem hiding this comment.
🟡 Suggestion: among this repo's vendored drivers, only pgx implements SQLState() string (vendor/github.com/jackc/pgx/v5/pgconn/errors.go:58) — so this branch effectively covers Postgres only. Vertica exposes it as a struct field (VError.SQLState, vendor/github.com/vertica/vertica-sql-go/errors.go:46), go_ibm_db puts SQLSTATE in Error.Diag[].State, and go-mssqldb only has SQLErrorNumber() (18456 = login failed); Oracle and HANA likewise. errors.As won't match any of them, so their bad-credential failures still surface as Unknown — including DB2, the engine this PR targets. Worth either adding per-driver branches or dropping "Vertica" from the doc comment so it doesn't read as covered. (Also, MySQL 1698/1044 are access-denied variants that 1045 alone misses.)
| if authErr := database.AuthError(err); authErr != nil { | ||
| return nil, authErr | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: returning authErr directly drops both the database name and the driver's original error, so with a multi-database config an operator sees only "database authentication failed" with no indication of which database rejected the credentials, and errors.Is/As on the driver error no longer works upstream. status.FromError resolves wrapped statuses via errors.As and preserves the code, so wrapping keeps the Unauthenticated exit behavior:
| if authErr := database.AuthError(err); authErr != nil { | |
| return nil, authErr | |
| } | |
| if authErr := database.AuthError(err); authErr != nil { | |
| return nil, fmt.Errorf("database %q ping failed: %w", name, authErr) | |
| } |
| case '}': | ||
| braced = false | ||
| atValueStart = false | ||
| case '{': |
There was a problem hiding this comment.
[Review] unterminated { eats the rest of the DSN, including a later HOSTNAME/DATABASE marker
If a value opens with { right after = but never gets closed (typo'd DSN, e.g. PWD={oops;DATABASE=X), braced just stays true for the rest of the scan — nothing ever un-braces it, so the whole remainder collapses into one part. That means IsNativeDSN never even sees the DATABASE= marker and returns false, so the string falls through to buildConnectionURL/url.Parse (which "succeeds" since there's no ://) and the user gets the generic "database scheme must be specified in DSN or configuration" instead of anything DB2-shaped.
Only a malformed-DSN edge case, not a regression for anything that currently works, but might be worth at least detecting the unterminated brace and erroring loudly instead of silently misrouting to a confusing message.
| // than a URL. Shared by pkg/database's routing and convertToDB2DSN's passthrough so | ||
| // the two decisions cannot drift. ODBC keywords are case-insensitive and parts may | ||
| // carry whitespace after the ';', so both are normalized. | ||
| func IsNativeDSN(dsn string) bool { |
There was a problem hiding this comment.
[Review] heads up, there's a third independent DSN-scheme detector elsewhere that doesn't know about this
pkg/bsql/offline_validate.go's resolveConnectScheme does its own ad-hoc strings.Index(dsn, "://") + url.Parse check, totally separate from IsNativeDSN here. Right now it's harmless because RejectNonV1ProductFeatures in that same file hard-rejects anything that isn't "postgres" regardless — but the moment that v1 restriction gets relaxed to allow DB2, it'll misclassify a native DB2 DSN (no :// at all) instead of routing it correctly.
Out of scope for this PR since it's a different file/package, but figured worth flagging now so it doesn't bite later — might be worth a one-line TODO pointing at this function.
| return expanded | ||
| } | ||
| } | ||
| if nativeDSN, isNativeDB2, err := nativeDB2DSN(opts); err == nil && isNativeDB2 { |
There was a problem hiding this comment.
[Review] minor: parses the DSN twice
nativeDB2DSN internally runs IsNativeDSN -> splitDB2DSN, then right after you call db2.DSNDatabase(nativeDSN) which runs splitDB2DSN again on the exact same string. Not a big deal at all given these are short strings parsed once per connection setup, not a hot loop — just noting it in case it's an easy freebie to fix while this code is fresh.
3 participants
The Db2 native connection-string form documented for this connector now works through configuration, so a customer following the docs can connect instead of hitting a confusing setup error.