NO-SNOW: add Snowflake DSN connection-string parser to sf_core - #2
Closed
zeroshade wants to merge 2 commits into
Closed
NO-SNOW: add Snowflake DSN connection-string parser to sf_core#2zeroshade wants to merge 2 commits into
zeroshade wants to merge 2 commits into
Conversation
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.
Owner
Author
|
Moved to upstream: snowflakedb#1336 |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a Snowflake DSN connection-string parser to
sf_core. A newconfig::dsn::parse_dsndecomposesuser[:password]@account/db/schema-style connection strings — with an optional, case-insensitivesnowflake://prefix so the same string can be written as a full URL — into sf_core's canonical connection parameters, returningParsedDsn { params, warnings }, and is re-exported from theconfigmodule. It supports the three documented DSN forms (.../db/schema,.../db, andhost: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 inwarnings.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::resolvepipeline, so alias resolution, host derivation, and validation are unchanged — it deliberately does not build hostnames or bypass the downstreamis_allowed_account_charallow-list. It is hand-rolled with a single code path: the optionalsnowflake://prefix is stripped rather than routed throughurl::Url, guaranteeing byte-for-byte identical results with or without the prefix. User, password, database, schema, and query values are URL-unescaped (%XXand+->space) whileaccountandhostare taken verbatim, matching the reference DSN behavior; host derivation fromaccountremains 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
InvalidPortpath is redacted so a mistypeduser:secretcannot leak, ports are range-validated asu16, and an emptyaccount=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 andcertRevocationCheckMode), credential redaction, empty-account handling, and port validation;cargo test -p sf_core --lib configpasses 380/380. The change was reviewed with roborev (security review passed; the default review's single finding — thatrequestTimeoutshould map torequest_timeout— was verified againstretry.rsas a name-based false positive, sincequery_timeoutis sf_core's query-execution budget whilerequest_timeoutis the non-query HTTP retry budget that query execution deliberately ignores; documented and closed).Not included here: an optional
Connection::apply_dsnconvenience / protobuf RPC for wrapper wiring, and defense-in-depth non-emptyaccountvalidation inconnection_config.