feat(csharp/src/Drivers/Databricks): Add CI for E2E Databricks tests #3115

alexguo-db · 2025-07-08T00:36:04Z

Currently the Databricks driver E2E tests do not run on PRs, leading to breaking changes going undetected
This PR adds E2E tests to the CI, which trigger when there are changes to any Databricks ADBC driver-related code
This PR adds a dummy test, since not all E2E tests are passing currently, once they are we can test all of them in CI
The workflow uses workload identity federation, so no secrets need to be stored

Before merging:

Since the tests are E2E, we would like maintainers to approve workflows instead of automatically triggering them
I would like to request the creation of an environment protection rule for the databricks-e2e environment, so that approval is required
I would like to request Databricks users to be added to the list of approvers, if possible

lidavidm · 2025-07-08T00:44:16Z

We don't have access to things like that. We could ask INFRA, but I'm not sure if they would add non-committers to the approval list or not.

zeroshade · 2025-08-08T19:52:59Z

.asf.yaml

+  environments:
+    databricks-e2e:
+      wait_timer: 0
+      required_reviewers:
+        - alexguo-db
+        - jadewang-db
+      deployment_branch_policy:
+        protected_branches: true


should probably add these to a collaborators key https://github.com/apache/infrastructure-asfyaml/blob/main/README.md#assigning-the-github-triage-role-to-external-collaborators

zeroshade · 2025-08-08T20:01:33Z

.github/workflows/csharp_databricks_e2e.yml

+            -d "subject_token_type=urn:ietf:params:oauth:token-type:jwt" \
+            -d "scope=sql")
+
+          DATABRICKS_TOKEN=$(echo "$OAUTH_RESPONSE" | jq -r '.access_token')


You might want to use ::add-mask:: in some areas here to ensure that the access tokens don't show up in the logs

zeroshade · 2025-08-08T20:03:07Z

This seems fine to me in general, but appears to be non-functional as the ACTIONS_ID_TOKEN_URI isn't populated and is an invalid URL, causing the failed deployments

alexguo-db · 2025-08-08T20:30:59Z

This seems fine to me in general, but appears to be non-functional as the ACTIONS_ID_TOKEN_URI isn't populated and is an invalid URL, causing the failed deployments

Yeah, apparently this is not populated on forks which is why I changed the target from pull_request to pull_request_target. So, this doesn't execute on this PR but any future PRs should execute the main branch's version of this workflow
actions/attest-build-provenance#99

zeroshade

Seems good to me.

@kou or @lidavidm Any issues you can see?

kou · 2025-08-20T02:20:48Z

.github/workflows/csharp_databricks_e2e.yml

+        with:
+          dotnet-version: ${{ matrix.dotnet }}
+      - name: Checkout ADBC
+        uses: actions/checkout@v4


Suggested change

uses: actions/checkout@v4

uses: actions/checkout@v5

kou · 2025-08-20T02:23:45Z

.github/workflows/csharp_databricks_e2e.yml

+          ref: ${{ github.event.pull_request.head.sha || github.sha }}
+          fetch-depth: 0
+          submodules: recursive
+      - name: Build
+        shell: bash
+        run: ci/scripts/csharp_build.sh $(pwd)


Does this mean that we may run ci/scripts/csharp_build.sh in forked repository with the pull_request_target context (that has write access to apache/arrow-adbc)?

I think that it's not acceptable based on the ASF GitHub Actions policy: https://infra.apache.org/github-actions-policy.html

Triggers

You MUST NOT use pull_request_target as a trigger on ANY action that exports ANY confidential credentials or tokens such as GITHUB_TOKEN or NPM_TOKEN.

Can we run this on fork not apache/arrow-adbc by removing branches: [main] from on.push?

Hmm, I suppose the point is that the manual approval for the environment protects against this. But Infra may not have intended for environments to be used this way.

I believe @zeroshade checked with ASF Infra about this use case. Can you confirm with ASF Infra that we can bypass the triggers policy if we have an environment with required reviewers? Otherwise, I don't see how other Apache repos can implement this click-approval pattern

Can we run this on fork not apache/arrow-adbc by removing branches: [main] from on.push?

@kou Sorry, I'm not following why excluding it from being run on the upstream repo would improve the security

If we run this job on apache/arrow-adbc, evil pull requests can get GITHUB_TOKEN that has id-token: write permission for apache/arrow-adbc. They may abuse it.

If we run this job on fork repository, evil developers can get only GITHUB_TOKEN for their fork repositories. It's not a problem because they already have permissions that these GITHUB_TOKEN have. They can't get additional permissions for apache/arrow-adbc.

Within our own repo we found out that our GITHUB_TOKEN would only be valid for 5 minutes thus the exchanged Databricks token is also 5 mins long and any test case passing that limit will fail because of invalidation.
We will probably need to supply a Databricks PAT token instead, which will bypass the GITHUB_TOKEN entirely, would that address the concern here?

@kou That won't work for us, the Databricks service principal is set up to only allow OIDC token exchange when the Github OIDC token originates from the main repo.

If we run this job on fork repository, evil developers can get only GITHUB_TOKEN for their fork repositories.

If we allowed the service principal to accept any Github OIDC token then anybody can create a fork and run malicious queries against the Databricks workspace.

The alternative is to use a Databricks personal access token secret but that runs into the same problem where we can only store it only on the main repo (so only people with main branch permissions can copy the branch to main and run the E2E tests)

How is this click approval pattern implemented elsewhere?

We will probably need to supply a Databricks PAT token instead, which will bypass the GITHUB_TOKEN entirely, would that address the concern here?

Can we avoid using pull_request_target by this? If so, it addresses the concern.

If we allowed the service principal to accept any Github OIDC token then anybody can create a fork and run malicious queries against the Databricks workspace.

Can we accept only trusted fork repositories not any fork repositories in Databricks side?

How is this click approval pattern implemented elsewhere?

I haven't seen the implementation in apache/* repositories...

apache/airflow-publish may have an implementation of it...?

Add test-pypi environment to providers release workflow airflow-publish#14

Add workflow to publish airflow packages to Test PyPI airflow-publish#7

Add providers workflow to publish packages to Test PyPI airflow-publish#9

BTW, can Databricks provide local test tool like MinIO for AWS S3, Azurite fhttps://github.com/Azure/Azurite or Azure Storage and Storage Testbench https://github.com/googleapis/storage-testbench for Google Cloud Storage?

kou · 2025-08-20T02:32:09Z

.asf.yaml

+      required_reviewers:
+        - alexguo-db
+        - jadewang-db
+        - eric-wang-1990


It seems that this syntax is wrong:

See also: https://github.com/apache/infrastructure-asfyaml?tab=readme-ov-file#repository-deployment-environments

Suggested change

required_reviewers:

- alexguo-db

- jadewang-db

- eric-wang-1990

required_reviewers:

- id: alexguo-db

type: User

- id: jadewang-db

type: User

- id: eric-wang-1990

type: User

kou · 2025-08-20T02:38:11Z

.asf.yaml

    - krlmlr
    - nbenn
+    - alexguo-db
+    - jadewang-db
+    - eric-wang-1990


Could you keep this list in alphabetical order?

alexguo-db had a problem deploying to databricks-e2e July 8, 2025 00:36 — with GitHub Actions Failure

feat(csharp/src/Drivers/Databricks): Add CI for E2E Databricks tests

c64ea70

alexguo-db force-pushed the alex-guo_data/alexguo-add-databricks-e2e-ci branch from 502ec81 to c64ea70 Compare July 8, 2025 16:09

alexguo-db had a problem deploying to databricks-e2e July 8, 2025 16:09 — with GitHub Actions Failure

alexguo-db had a problem deploying to databricks-e2e July 8, 2025 16:14 — with GitHub Actions Failure

Debug

6785dc1

alexguo-db force-pushed the alex-guo_data/alexguo-add-databricks-e2e-ci branch from e43ea36 to 6785dc1 Compare July 8, 2025 16:25

alexguo-db had a problem deploying to databricks-e2e July 8, 2025 16:25 — with GitHub Actions Failure

Update

5901ee0

alexguo-db force-pushed the alex-guo_data/alexguo-add-databricks-e2e-ci branch from 1ee8037 to 5901ee0 Compare July 8, 2025 16:43

Remove debug

a66afa3

alexguo-db force-pushed the alex-guo_data/alexguo-add-databricks-e2e-ci branch from fc78074 to a66afa3 Compare July 8, 2025 17:06

Lint

01997af

alexguo-db force-pushed the alex-guo_data/alexguo-add-databricks-e2e-ci branch from 9a0b830 to 01997af Compare July 8, 2025 17:57

Add databricks-e2e environment

48821e7

alexguo-db marked this pull request as ready for review July 28, 2025 18:44

alexguo-db requested a review from lidavidm as a code owner July 28, 2025 18:44

alexguo-db requested review from kou and zeroshade as code owners July 28, 2025 18:44

github-actions bot added this to the ADBC Libraries 20 milestone Jul 28, 2025

zeroshade reviewed Aug 8, 2025

View reviewed changes

Add masks and add collaborators

02e15c3

alexguo-db requested a review from zeroshade August 14, 2025 17:38

zeroshade approved these changes Aug 19, 2025

View reviewed changes

kou reviewed Aug 20, 2025

View reviewed changes

Address comments

d8b945a

lidavidm modified the milestones: ADBC Libraries 20, ADBC Libraries 21 Sep 9, 2025

lidavidm modified the milestones: ADBC Libraries 21, ADBC Libraries 22 Nov 3, 2025

lidavidm modified the milestones: ADBC Libraries 22, ADBC Libraries 23 Jan 6, 2026

-      required_reviewers:
-        - alexguo-db
-        - jadewang-db
-        - eric-wang-1990
+      required_reviewers:
+        - id: alexguo-db
+          type: User
+        - id: jadewang-db
+          type: User
+        - id: eric-wang-1990
+          type: User

feat(csharp/src/Drivers/Databricks): Add CI for E2E Databricks tests #3115

Are you sure you want to change the base?

feat(csharp/src/Drivers/Databricks): Add CI for E2E Databricks tests #3115

Uh oh!

Conversation

alexguo-db commented Jul 8, 2025

Uh oh!

lidavidm commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Aug 8, 2025

Uh oh!

alexguo-db commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexguo-db Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexguo-db commented Aug 8, 2025 •

edited

Loading

alexguo-db Aug 22, 2025 •

edited

Loading