Skip to content

NO-SNOW: add Snowflake DSN connection-string parser to sf_core - #2

Closed
zeroshade wants to merge 2 commits into
mainfrom
dsn-connection-string
Closed

NO-SNOW: add Snowflake DSN connection-string parser to sf_core#2
zeroshade wants to merge 2 commits into
mainfrom
dsn-connection-string

Conversation

@zeroshade

Copy link
Copy Markdown
Owner

This adds a Snowflake DSN connection-string parser to sf_core. A new config::dsn::parse_dsn decomposes user[:password]@account/db/schema-style connection strings — with an optional, case-insensitive snowflake:// prefix so the same string can be written as a full URL — into sf_core's canonical connection parameters, returning ParsedDsn { params, warnings }, and is re-exported from the config module. It supports the three documented DSN forms (.../db/schema, .../db, and host:port/db/schema?account=...). Only parameters sf_core actually recognizes are emitted; every other key (format-only options, region, OCSP toggles, unknown keys) is dropped and surfaced in warnings.

The motivation is to give the language wrappers (Go, ODBC, Python, JDBC) a single-string connect affordance without each reimplementing the DSN grammar. Centralizing the parse in the core keeps the connection-string vocabulary in one tested place and lets every wrapper share identical semantics.

The parser is a pure function that feeds the existing connection_set_options -> resolver::resolve pipeline, so alias resolution, host derivation, and validation are unchanged — it deliberately does not build hostnames or bypass the downstream is_allowed_account_char allow-list. It is hand-rolled with a single code path: the optional snowflake:// prefix is stripped rather than routed through url::Url, guaranteeing byte-for-byte identical results with or without the prefix. User, password, database, schema, and query values are URL-unescaped (%XX and +->space) while account and host are taken verbatim, matching the reference DSN behavior; host derivation from account remains the resolver's responsibility.

Risk is low: the change is an additive module with no edits to existing connection, auth, or validation logic. Because the input is an untrusted connection string, credential handling was given particular attention — error variants and warnings never echo a decoded value (only keys or static labels), the InvalidPort path is redacted so a mistyped user:secret cannot leak, ports are range-validated as u16, and an empty account= no longer satisfies the account requirement.

Testing: 25 unit tests cover the three forms, prefix parity and case-insensitivity, unescaping and +->space, every supported mapping (including the inverted CRL-cache booleans and certRevocationCheckMode), credential redaction, empty-account handling, and port validation; cargo test -p sf_core --lib config passes 380/380. The change was reviewed with roborev (security review passed; the default review's single finding — that requestTimeout should map to request_timeout — was verified against retry.rs as a name-based false positive, since query_timeout is sf_core's query-execution budget while request_timeout is the non-query HTTP retry budget that query execution deliberately ignores; documented and closed).

Not included here: an optional Connection::apply_dsn convenience / protobuf RPC for wrapper wiring, and defense-in-depth non-empty account validation in connection_config.

Add config::dsn::parse_dsn to decompose user[:password]@account/db/schema style connection strings (with an optional snowflake:// prefix) into canonical sf_core connection parameters. Only parameters sf_core recognizes are emitted; unsupported/unknown keys are collected as warnings. Re-exported from the config module.
The malformed-parameter warning previously echoed the raw query segment; a fat-fingered token/password passed as a segment without '=' could leak into warnings. Omit the segment content and only note that a malformed parameter was dropped. Updates the corresponding unit test.
@zeroshade

Copy link
Copy Markdown
Owner Author

Moved to upstream: snowflakedb#1336

@zeroshade zeroshade closed this Jul 20, 2026
zeroshade pushed a commit that referenced this pull request Aug 11, 2026
> **Stack** (merge bottom → top):
> - snowflakedb#727 — extract shared pre-upload skip-decision helper ← **this PR**
> - snowflakedb#719 — S3 skip upload on content match (PUT OVERWRITE=TRUE)
> - snowflakedb#728 — GCS honors skip_upload_on_content_match (opt-in)

## What & why

Pure refactor extracting the pre-upload skip-decision logic into a
shared helper. `SkipDecision` (the enum) and `classify_pre_upload_skip`
(the pure decision function) are hoisted out of `azure_transfer.rs` into
`file_manager/mod.rs` as `pub(crate)`, and Azure's
`upload_to_azure_or_skip` is refactored to call the shared helper.

The helper takes primitives (`overwrite`,
`skip_upload_on_content_match`, `remote_exists`, `remote_digest:
Option<&str>`, `local_digest`) rather than a cloud-specific
HEAD-response type, so each cloud's call site projects its own response
shape down to one signature — avoiding a parallel decision pipeline per
code-review-design-discipline principle #2. This is the base of the S3
stack (child PR snowflakedb#719), which reuses the helper instead of duplicating
it; a later GCS pass can adopt it too.

## Behavior

Unchanged. Azure's six pure-decision unit tests moved alongside the
hoisted function; all Azure wiremock scenarios continue to pass. S3 is
untouched in this PR and remains a no-op for the flag (its
`skip_upload_on_content_match_is_no_op_on_s3` pin still passes here).

## Test plan

- `cargo test -p sf_core --lib`: passes, including the 6 relocated
`classify_*` tests and the retained S3 no-op pin.
- `cargo clippy --all-targets --all-features -- -Dclippy::all`: clean.
- `cargo fmt --check`: clean.

## Logging note

The shared `skip_upload_decision` step unifies the pre-upload skip log
lines: skips now emit `tracing::info!("{cloud}: remote object already
exists, skipping upload: {key}")` and `"{cloud}: remote content matches
local digest, skipping upload: {key}"` (tagged by cloud), replacing the
previous per-cloud wordings (e.g. Azure's `"Blob already exists in
Azure: ..."`). Info-level only; no error/SQLSTATE strings change.
Flagged for anyone grepping these log lines.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitOrigin-RevId: 4b8806d
zeroshade pushed a commit that referenced this pull request Aug 11, 2026
## Summary

Enable external forks of the public mirror to run the **full per-PR CI
at parity**, on their own GitHub Actions infrastructure, against their
own Snowflake account(s).

Forks already trigger the full pipeline (the `test-*.yml` org gate fires
on any non-`snowflakedb` owner), and `PARAMETERS_SECRET` is the **only**
secret in the per-PR path with all runners GitHub-hosted — so credential
injection is the sole blocker.

- **`scripts/decode_secrets.sh`**: new plaintext per-cloud branch. If
`PARAMETERS_JSON_<CLOUD>` (e.g. `PARAMETERS_JSON_AWS`) is set, write it
to the output file and skip the GPG bundle + bulk decode. Precedence:
**plaintext > `PARAMETERS_SECRET` (GPG) > 1Password**, so the
maintainers' own CI (sets `PARAMETERS_SECRET`, never
`PARAMETERS_JSON_*`) is byte-for-byte unchanged.
- **Workflow wiring (13 files)**: map `PARAMETERS_JSON_AWS/GCP/AZURE`
into every *Decode secrets* step's `env:`. GitHub does not auto-export
secrets to the environment, so the script can only read them if each
step passes them explicitly. These are empty/unused upstream (where
`PARAMETERS_SECRET` drives the GPG path); 30 blocks wired.
- **`CONTRIBUTING.md`** (new): fork-CI guide — enable Actions, set
per-cloud secrets, trigger via push-to-fork-`main` or an in-fork PR,
what runs vs. what's nightly/label-gated, troubleshooting.

Deferred (per "full parity now, cut back later"): graceful per-cloud
matrix-skip when a cloud secret is absent (`_fork-clouds.yml` +
`--restrict-clouds`), and direct trusted-fork→internal merge (a later
phase).

Follow-up (design-discipline #2 — change amplification): the
plaintext-secret mapping is now duplicated across 30 decode steps. A
`./.github/actions/decode-secrets` composite action wrapping the script
+ env would centralize it; out of scope here.

## Test Results

- `bash -n scripts/decode_secrets.sh` — clean.
- Functional (local): plaintext path writes `parameters.json` and skips
the GPG bundle; plaintext wins over `PARAMETERS_SECRET`; branch is inert
when unset (falls through to GPG — internal path unchanged).
- All `.github/workflows/*.yml` parse as valid YAML (pyyaml);
secret-mapping counts verified 30/30/30/30, each new line correctly
placed under its `env:` block.
- `pre-commit run --files <changed>` — Tests format validator
**Passed**; other hooks Skipped (no Rust/Python/JDBC files changed).
- Copybara `verify_match` trip-wire scan of the diff — clean (no new
internal-org slug / hostname / vault strings).

## References

- Wiki: `~/notes/notes/tasks/Public Repo PR CI/Public Repo PR CI.md`
(design: `Public Repo Fork CI Design.md`)
- Jira: none

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitOrigin-RevId: 19eb139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant