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 3 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}
| nativeDSN, isNativeDB2, err := nativeDB2DSN(opts) | ||
| if err != nil { | ||
| return nil, Unknown, err | ||
| } | ||
| if isNativeDB2 { | ||
| db, err := db2.Connect(ctx, nativeDSN) | ||
| if err != nil { | ||
| return nil, Unknown, err | ||
| } | ||
| return db, DB2, nil | ||
| } |
There was a problem hiding this comment.
🟠 Bug: the native path hands the DSN to the driver verbatim and so ignores opts.Database. ConnectMany relies on perOpts.Database = name to open one handle per database (database.go:423-425), so a databases.static / discovery_query config plus a native DB2 DSN opens N handles that all point at the DSN's DATABASE=, keyed by the different names — every "database" then syncs identical rows under a distinct _database label, producing duplicated/mislabeled resources instead of an error. The single-database case has the mirror problem: ResolveDatabaseName returns opts.Database first (line 359), so connect.database: FOO with a native DSN keys the handle FOO while the connection is to the DSN's database. Either rewrite the DATABASE= keyword in the DSN when opts.Database is set, or reject the native-DSN + database/databases combination with an explicit error.
| if !strings.Contains(dsn, "HOSTNAME=") && !strings.Contains(dsn, "DATABASE=") { | ||
| return "", false, nil | ||
| } | ||
| return dsn, true, nil |
There was a problem hiding this comment.
🟡 Suggestion: once this returns true, the remaining structured connect fields (Host, Port, User, Password, Params) are silently dropped, since the native path never reaches buildConnectionURL. A config mixing a native DSN with e.g. connect.params will connect without those settings and give no signal. Consider returning an error when any of those fields are non-empty alongside a native DSN, and noting the exclusivity in docs/db2.md.
Connector PR Review: CXH-2380: reach the DB2 native DSN form through connector configBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: native DB2 DSN routing in Security IssuesNone found. The new rejection error does not echo the DSN, and no credential material is added to logs, errors, or spans. Correctness IssuesNone found. Native-DSN routing only captures scheme-less DSNs that carry a Suggestions
Prompt for AI agentsReviewed head |
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.
| if strings.Contains(dsn, "://") { | ||
| return "", false, nil | ||
| } | ||
| if !strings.Contains(dsn, "HOSTNAME=") && !strings.Contains(dsn, "DATABASE=") { | ||
| return "", false, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: native-DSN detection and DATABASE= extraction are both byte-exact/uppercase-only, but ODBC keywords are case-insensitive and ; spacing is common, so two valid native DSNs still misbehave. hostname=h;database=X matches neither marker, falls through to the URL path, and hits the exact database scheme must be specified error this PR set out to fix. Worse, HOSTNAME=h; DATABASE=X (or HOSTNAME=h;database=X) is detected and connects, but db2DSNDatabase (line 374) fails its CutPrefix("DATABASE=") and returns "" — openDatabases then keys the handle with "" (connector.go:191), query.go:819 skips injecting the Database row column, and resource IDs no longer match the equivalent db2://.../X URL, defeating the stated ID-stability goal. Consider matching keywords case-insensitively and trimming whitespace around each ;-separated part in both nativeDB2DSN and db2DSNDatabase.
| run: go build ./cmd/baton-sql | ||
| - name: Run sync tests | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v2 | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v4 |
There was a problem hiding this comment.
🟡 Suggestion: account-provisioning@v3 (line 51) still resolves get-baton@v2, which installs the CLI from conductorone/baton (pre-pebble) — the same incompatibility this bump fixes. It passes today only because get-baton.sh early-exits when baton is already on PATH, so this sync-test@v4 step has already installed the baton-sdk CLI. Reordering, skipping, or removing this step would silently reintroduce c1z: invalid file in the provisioning step; bumping account-provisioning to @v4 (which pins get-baton@v4) removes the ordering dependency.
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.
| func splitDB2DSN(dsn string) []string { | ||
| var parts []string | ||
| start := 0 | ||
| inBraces := false | ||
| for i, r := range dsn { | ||
| switch r { | ||
| case '{': | ||
| inBraces = true | ||
| case '}': | ||
| inBraces = false | ||
| case ';': | ||
| if !inBraces { | ||
| parts = append(parts, dsn[start:i]) | ||
| start = i + 1 | ||
| } | ||
| } | ||
| } | ||
| return append(parts, dsn[start:]) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: brace tracking here toggles on any {/}, but ODBC only treats braces as quoting when a value starts with { (ending at the matching }). So HOSTNAME=h;PWD=p{q;DATABASE=TESTDB collapses into two parts (HOSTNAME=h, PWD=p{q;DATABASE=TESTDB): the driver still connects to TESTDB (it reads p{q literally), but db2DSNDatabase returns "", so the handle key is empty, rowColDatabase is dropped (pkg/bsql/query.go:819) and resource IDs diverge from the equivalent db2:// URL. Consider entering brace mode only when the char after = is {, and leaving it at the first }. (confidence: high on behavior, narrow trigger)
| func isNativeDB2Format(dsn string) bool { | ||
| if urlSchemeRegex.MatchString(dsn) { | ||
| return false | ||
| } | ||
| for _, part := range strings.Split(dsn, ";") { | ||
| keyword, _, found := strings.Cut(strings.TrimSpace(part), "=") | ||
| if found && (strings.EqualFold(keyword, "HOSTNAME") || strings.EqualFold(keyword, "DATABASE")) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this duplicates database.hasDB2Marker + urlSchemeDSNRegex, and the two copies already disagree — this one splits with plain strings.Split(dsn, ";") while hasDB2Marker uses the brace-aware splitDB2DSN. So UID=u;PWD={x;DATABASE=y} is "native" here but not to nativeDB2DSN, which makes the nativeDB2DSN doc claim "Markers match db2.convertToDB2DSN's passthrough" inaccurate. Consider exporting one detector from this package (e.g. db2.IsNativeDSN) and having pkg/database call it, so the routing decision and the passthrough decision cannot drift.
| DB2's native form is also accepted as-is (ODBC keywords are case-insensitive and may carry | ||
| spaces after each `;`): |
There was a problem hiding this comment.
🟡 Suggestion: the case-insensitivity claim is backed by the new EqualFold detection, but the whitespace claim only covers baton-sql's own parsing — the native DSN is handed to go_ibm_db/CLI verbatim, spaces included, so HOSTNAME=h; DATABASE=X connects only if the driver tolerates the leading space in the keyword. The commit notes a lowercase DSN was verified live but not a spaced one; worth confirming against the driver (or trimming each part before passthrough) before documenting it as supported. (confidence: low — depends on driver behavior)
2 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.