| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main). |
Sorry, something went wrong.
There was a problem hiding this comment.
This docs-only PR corrects Azure Entra OAuth support documentation for the Kernel backend.
Changes:
CONNECTION_PARAMETERS.md:65
| `azureTenantId` / `useDatabricksOAuthInAzure` | `string` / `boolean` | ✅ | ⚠️ | — | **Honored on both.** Kernel: on an Azure host, `databricks-oauth` **M2M** (secret present) with `useDatabricksOAuthInAzure` absent/`false` routes to Entra-direct service-principal M2M (native `AzureSpM2m` mode, creds ride `oauthClientId`/`oauthClientSecret`); `true` routes to workspace-OIDC M2M. `azureTenantId` is optional (kernel auto-discovers from the workspace `/aad/auth` redirect when omitted). **U2M** ignores the flag — the kernel's cloud-blind in-house flow works against Azure. One divergence: the kernel treats `.databricks.azure.us` as Azure in every arm, whereas Thrift's `useDatabricksOAuthInAzure`-true arm does not. (`lib/kernel/KernelAuth.ts` `buildKernelConnectionOptions`.) |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
| | `oauthClientId` (U2M) | `string` | ✅ | ✅ | napi default `client_id` when absent | The kernel adapter (`buildKernelConnectionOptions`) forwards a custom `oauthClientId` verbatim on the U2M arm; when it is absent the napi binding applies its own default `client_id`. Whether the native binding then honors or rejects a custom id is not observable from this repo — the TypeScript layer neither hardcodes an id nor rejects one. | | ||
| | `oauthClientId` + no secret | `string` | ✅ (U2M) | ✅ (U2M) | — | **Parity.** The kernel keys flow selection off `oauthClientSecret` presence exactly like Thrift, so `oauthClientId` + no secret routes to **U2M** (with the id forwarded) — it does **not** throw an M2M "secret required" error. | | ||
| | `azureTenantId` / `useDatabricksOAuthInAzure` | `string` / `boolean` | ✅ | ❌ | — | **Thrift-only.** Kernel rejects Azure-direct (Entra) OAuth; workspace-OIDC discovery covers Azure workspaces without it. | | ||
| | `azureTenantId` / `useDatabricksOAuthInAzure` | `string` / `boolean` | ✅ | ⚠️ | — | **Honored on both.** Kernel: on an Azure host, `databricks-oauth` **M2M** (secret present) with `useDatabricksOAuthInAzure` absent/`false` routes to Entra-direct service-principal M2M (native `AzureSpM2m` mode, creds ride `oauthClientId`/`oauthClientSecret`); `true` routes to workspace-OIDC M2M. `azureTenantId` is optional (kernel auto-discovers from the workspace `/aad/auth` redirect when omitted). **U2M** ignores the flag — the kernel's cloud-blind in-house flow works against Azure. One divergence: the kernel treats `.databricks.azure.us` as Azure in every arm, whereas Thrift's `useDatabricksOAuthInAzure`-true arm does not. (`lib/kernel/KernelAuth.ts` `buildKernelConnectionOptions`.) | |
There was a problem hiding this comment.
I've completed the task. Summary: the reviewer's concern was valid — the docs claimed Entra-direct M2M works on the shipped Kernel backend, but the native packages are pinned to 0.2.0, which predates the AzureSpM2m mode. Since bumping the package.json pins is out of scope for this job, I took the reviewer's alternative and qualified both affected doc rows with the newer-kernel-binary requirement.
Pushed edec82e (bundled with 1 other thread(s)).
Sorry, something went wrong.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main). |
Sorry, something went wrong.
There was a problem hiding this comment.
Verdict: 1 Medium
Docs-only PR; the technical content is accurate against lib/kernel/KernelAuth.ts (AzureSpM2m mode, U2M flag inertness, tenant auto-discovery all confirmed). One medium concern: the Azure row's Kernel cell is set to ✅, but the note describes partial/divergent behavior that maps to ⚠️ per the legend — and the PR description itself says the change should be ❌ → ⚠️.
Sorry, something went wrong.
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main). |
Sorry, something went wrong.
There was a problem hiding this comment.
Verdict: 1 Medium
Docs-only change; the note text is accurate against lib/kernel/KernelAuth.ts (the AzureSpM2m mode, optional/auto-discovered azureTenantId, U2M inertness, and US-gov divergence all check out). One medium concern: the Azure Kernel cell is set to ✅, but the PR description says it should be ⚠️ and the doc's own legend + sibling divergent rows support ⚠️. Note also that the diff adds three previously-undisclosed rows (token, staticToken, oauthClientSecret) beyond the "only the Azure row" scope stated in the description — their content is accurate, but the description undersells the change.
Sorry, something went wrong.
Addresses: - #3847919330 at CONNECTION_PARAMETERS.md:65 - #3848004541 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main). |
Sorry, something went wrong.
Addresses: - #3848036730 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main). |
Sorry, something went wrong.
…ference The connection-parameter reference (added in #457) predated #505 (feat(kernel): thread Azure Entra auth (U2M + SP M2M) through the kernel path) and still claimed the kernel "rejects Azure-direct (Entra) OAuth". That is now inverted: buildKernelConnectionOptions (lib/kernel/KernelAuth.ts) has a dedicated AzureSpM2m mode for Entra- direct service-principal M2M and consumes both azureTenantId and useDatabricksOAuthInAzure. - Authentication table: azureTenantId / useDatabricksOAuthInAzure kernel cell ❌ → ⚠️, with an accurate flow-selection note. - authType "supported on both" note now mentions Azure Entra SP M2M. - Summary of gaps: drop Azure from "missing/ignored on Kernel" (renumber) and record it under "Behavioral divergences to watch" instead. Other cross-backend rows spot-checked against source and left as-is. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Per design intent: all U2M routes to the cloud-blind in-house OAuth U2M flow (no Azure-specific U2M mode), so `.databricks.azure.us` (US-gov) is always supported via that flow — that is intended completeness, not a support gap. Both `azureTenantId` and `useDatabricksOAuthInAzure` are read and forwarded on both backends, so the kernel cell is ✅ (not ⚠️). Notes reframed: US-gov behavior now reads as the kernel being more complete than Thrift (whose useDatabricksOAuthInAzure-true arm rejects US-gov), and useDatabricksOAuthInAzure documented as selecting only the M2M mechanism. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
The Authentication table documented `authType` and the OAuth modifier options but never gave the credential-bearing fields their own rows — `oauthClientSecret` in particular was only named inline as the flow selector, despite its non-trivial kernel mapping (remapped to `azureClientSecret` on the Entra-direct arm) and a blank-handling divergence (rejected only on the Azure SP arm). Adds three rows, each with the Thrift↔kernel mapping verified against source: - `token` (PAT): Thrift PlainHttpAuthentication vs kernel native `Pat`; kernel rejects blank/reserved + OAuth-field pairing, Thrift does not. - `staticToken`: Thrift StaticTokenProvider (federation opt-in) vs kernel `Pat` bearer with federation always on. - `oauthClientSecret` (M2M): flow selector on both; kernel native `oauthClientSecret` or remapped `azureClientSecret`. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Addresses: - #3847919330 at CONNECTION_PARAMETERS.md:65 - #3848004541 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Addresses: - #3848036730 at CONNECTION_PARAMETERS.md:68 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Addresses: - #3848073178 at CONNECTION_PARAMETERS.md:60 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
The table had two oauthClientId entries both describing the U2M/no-secret case (one about defaulting, one about flow-selection), which read as two different options, used inconsistent cell notation (✅ vs ✅ (U2M)), and omitted M2M entirely. The flow-selection parity is also now covered by the oauthClientSecret row. Merged into a single oauthClientId row covering both flows: forwarded verbatim when set; defaults to `databricks-sql-connector` when absent (Thrift getClientId() for both flows; kernel `?? DEFAULT_OAUTH_CLIENT_ID` on M2M, napi's own default on U2M); retains the "id + no secret ⇒ U2M, no secret-required error" parity note. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
The review-bot hedged the Azure row to ⚠️ because the pinned 0.2.0 native binary might predate the AzureSpM2m surface. With a 1.0.0 kernel binary (which carries kernel#282), the Entra-direct M2M arm is implemented at runtime, so the row is ✅ again. Replaces the "0.2.0 may predate / not observable / confirm your build" hedges (Azure row + authType note) with a concrete, checkable version floor: the Entra-direct M2M arm needs the native binary >= 1.0.0; U2M works on any build. Per the ✅ legend (option is read and forwarded), the connector honors these options on both backends; the binary floor is a deployment note, not a support downgrade. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
1.0.0 is not published yet, so "carried by the native binary from 1.0.0 onward" overstated it as a shipping fact. Reword the Azure row and the authType note: the connector forwards AzureSpM2m today, but its runtime path needs the kernel Azure SP surface (kernel#282), which the currently pinned 0.2.0 binary predates — it lands in a forthcoming release (planned 1.0.0, not yet published). U2M still needs no minimum. Row stays ✅ (the options are read and forwarded per the legend). Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Addresses: - #3848192767 at CONNECTION_PARAMETERS.md:67 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Per direction, treat the kernel Azure SP surface as included. Remove the "0.2.0 predates it / planned 1.0.0, not yet published" caveats from both the Azure row and the authType note; Azure is ✅ on both backends with a clean flow-selection description. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Addresses: - #3848237239 at CONNECTION_PARAMETERS.md:66 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Reformat the Authentication table to match prettier's column padding so the lint check passes. Whitespace-only, no content change. Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
| Back | FazBrowse Home | New Git URL |
What
Corrects and expands the Authentication section of CONNECTION_PARAMETERS.md (the Thrift-vs-kernel connection-parameter reference), verified against source.
Intended final state
Notes
This pull request and its description were written by Isaac.