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 12 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: 2 | Threads Resolved: 0 Review SummaryThe new commits narrow native-DB2 detection so a Security IssuesNone found. The new 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.
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.
| // 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.)
There was a problem hiding this comment.
Still open — openDatabases still calls database.Connect for the discovery-query admin handle and runs the query before ConnectMany's native-DSN + databases mutual-exclusion check fires. Low severity like you said, just confirming it's still there.
There was a problem hiding this comment.
yeah, still there. low priority like you said. the combo is still rejected, just after a wasted admin connect and discovery run.
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.
| 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.
There was a problem hiding this comment.
routed resolveConnectScheme through the shared IsNativeDSN instead of a TODO, so the three detectors agree and a native dsn resolves to db2 there now.
| 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.
There was a problem hiding this comment.
done, folded IsNativeDSN and DSNDatabase into one ParseNativeDSN pass.
Address FeliLucero1's review on PR #149: - splitDB2DSN now keeps the value-start position across whitespace, so a space between '=' and a brace ("DATABASE= {my;db}") is still brace-quoted instead of letting the ';' inside the braces split the part and truncate the database name. - IsNativeDSN and DSNDatabase trim the keyword and value, so "DATABASE = X" and "DATABASE= X" match the keyword and resolve the value without the leading space that previously leaked into the handle key and synthetic Database column. - An unterminated '{' is now treated as a literal char (lookahead for a closing '}'), so it no longer swallows the rest of the DSN and hide the HOSTNAME/DATABASE markers; the malformed value reaches the driver's own error instead of the generic "scheme must be specified" misroute. - Note the third, separate scheme check in pkg/bsql/offline_validate.go resolveConnectScheme on IsNativeDSN so it stays in sync if v1 ever accepts DB2. Verified live against a local DB2 container: a native DSN with "DATABASE= TESTDB" (space after '=') connects and syncs. New unit cases cover the whitespace and unterminated-brace paths.
resolveConnectScheme did its own ad-hoc "://" scheme detection, separate from db2.IsNativeDSN, so a native DB2 DSN (no scheme prefix) resolved to the confusing "scheme missing from dsn" error, and a native DSN with "://" inside a value (e.g. PWD=my://secret) was misread as a URL scheme. Route native detection through the shared db2.IsNativeDSN so this path matches the engine's connection routing: a native DB2 DSN now resolves to scheme "db2", giving the clear "scheme \"db2\" is not supported in v1" message today and correct routing if v1 ever accepts DB2. No behavior change for postgres or URL-form DSNs.
…pass Consolidate db2.IsNativeDSN and db2.DSNDatabase onto a single db2.ParseNativeDSN that returns both facts from one splitDB2DSN pass; the two remain as thin wrappers for existing callers. nativeDB2DSN now returns the DATABASE value alongside the DSN, so ResolveDatabaseName no longer re-splits the string (it dropped from two passes to one) while Connect's single-pass cost is unchanged.
| } | ||
|
|
||
| var sqlState interface{ SQLState() string } | ||
| if errors.As(err, &sqlState) && strings.HasPrefix(sqlState.SQLState(), "28") { |
There was a problem hiding this comment.
AuthError currently recognizes only drivers implementing SQLState() string plus MySQL 1045, so it misses the DB2 engine this PR targets: go_ibm_db exposes SQLSTATE through *go_ibm_db.Error.Diag[].State, not a method. It also returns status.Error, which discards the original driver error and prevents downstream errors.As. Add per-driver extraction (including DB2 class 28) and wrap the original with uhttp.WrapErrors(codes.Unauthenticated, ..., err); add a DB2 diagnostic test. Driver shape at this HEAD:
baton-sql/vendor/github.com/ibmdb/go_ibm_db/error.go
Lines 29 to 45 in 296d680
…x-the-db2-native-dsn-form-being-unreachable # Conflicts: # docs/db2.md # pkg/database/autherror.go
|
|
||
| // IsAuthError reports whether err is a DB2 auth failure. go_ibm_db exposes SQLSTATE | ||
| // via Error.Diag[].State, not a SQLState() method, so class 28 must be matched here. | ||
| func IsAuthError(err error) bool { |
There was a problem hiding this comment.
🟡 Suggestion: this real IsAuthError (and the new pkg/database/db2/autherror_test.go, which is //go:build db2) is never compiled or run by any workflow in .github/workflows/ — verify.yaml/ci.yaml build and test without -tags db2, so only the always-false stub in db2_stub.go is exercised. The vet-db2 and test-db2 Makefile targets already exist but aren't wired into CI; adding at least a go vet -tags db2 ./... step would keep this path from silently breaking.
| switch { | ||
| case strings.EqualFold(keyword, "HOSTNAME"): | ||
| native = true | ||
| case strings.EqualFold(keyword, "DATABASE"): | ||
| native = true | ||
| if !haveDB { // first DATABASE= wins | ||
| value = strings.TrimSpace(value) | ||
| if strings.HasPrefix(value, "{") && strings.HasSuffix(value, "}") { | ||
| value = value[1 : len(value)-1] | ||
| } | ||
| database, haveDB = value, true | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: nativeness is keyed only on HOSTNAME/DATABASE, so a DB2 CLI connection string that leads with another keyword — e.g. the cataloged/ODBC data-source form DSN=MYALIAS;UID=u;PWD=p — still falls through to the URL path and dies with "database scheme must be specified", the same confusing error this PR is removing for the HOSTNAME=... form. Consider also treating DSN/DBALIAS as native markers (or documenting in docs/db2.md that only the HOSTNAME=/DATABASE= form is supported and the alias form needs scheme: db2 plus a db2:// URL).
…olders; InvalidArgument on config errors; add db name to auth error; drop vertica from autherror doc
| err = errors.Join(err, fmt.Errorf("environment variable %s is not set", varName)) | ||
| return match | ||
| } | ||
| if strings.ContainsAny(value, ";{}=") { |
There was a problem hiding this comment.
🟡 Suggestion: rejecting = in an expanded value looks over-strict. ODBC (and this repo's own splitDB2DSN + strings.Cut(part, "=")) delimits keywords on ; and takes the first =, so a value containing = cannot introduce a new keyword — only ; { } can. As written, PWD=${DB2_PASSWORD} with a base64-ish password like Sup3r=Secret== now fails config with "must not contain ODBC keyword separators" and has no workaround. Similarly, a deliberately braced template (PWD={${PW}}) still rejects a ; in the value even though the braces quote it. Consider dropping = from the reject set (and, optionally, allowing ; when the placeholder is already brace-quoted).
| DB2's native form is also accepted as-is (ODBC keywords are case-insensitive and may carry | ||
| spaces after each `;`): | ||
|
|
||
| ``` | ||
| HOSTNAME=localhost;PORT=50000;DATABASE=TESTDB;UID=db2inst1;PWD=pass123;PROTOCOL=TCPIP | ||
| ``` |
There was a problem hiding this comment.
🟡 Suggestion: this commit adds two user-visible constraints to the native form that the doc doesn't state. (1) HOSTNAME= is now the required marker for native detection — a DSN with only DATABASE=/PROTOCOL= (local/cataloged connections) silently falls through to the URL path and fails with "database scheme must be specified". (2) ${VAR} placeholders inside a native DSN now reject any expanded value containing ; { } =, unless the entire DSN is a single ${VAR}. Both surface as config-time errors, so worth documenting here alongside the self-contained note.
4 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.