Skip to content

Add Impersonate Service Account argument#2015

Open
wintermi wants to merge 9 commits intodataform-co:mainfrom
Conundrm:main
Open

Add Impersonate Service Account argument#2015
wintermi wants to merge 9 commits intodataform-co:mainfrom
Conundrm:main

Conversation

@wintermi
Copy link
Copy Markdown

This PR adds an --impersonate-service-account argument to the run and test commands, along with the required changes to allow for the impersonation of service accounts without the need to change ADC or call gcloud

This would resolve issue #2000 and would be an alternative to solution than PR #2001

Impersonation could then be achieved by executing:

dataform run --impersonate-service-account=<sSERVICE_ACCT_EMAIL>

@wintermi wintermi requested a review from a team as a code owner September 11, 2025 06:43
@wintermi wintermi requested review from Ceridan and removed request for a team September 11, 2025 06:43
@google-cla
Copy link
Copy Markdown

google-cla bot commented Sep 11, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@camilleAmaury
Copy link
Copy Markdown

+1, this would enable to use impersonation in CI rather than giving the rights directly to the CI service account.
There is no way to workaround that currently.

@kolina
Copy link
Copy Markdown
Contributor

kolina commented Nov 11, 2025

/gcbrun

@Ceridan Ceridan requested review from kolina and removed request for Ceridan November 13, 2025 09:15
@kolina
Copy link
Copy Markdown
Contributor

kolina commented Nov 13, 2025

Sorry for the late review. A couple of things:

  • Integration tests are failing, can you take a look at fixing them? Now we have a guide of running them locally
  • Let's resolve conflicts

Comment thread cli/api/dbadapters/bigquery.ts Outdated
Comment thread cli/api/dbadapters/bigquery.ts Outdated
Comment thread cli/api/dbadapters/bigquery.ts Outdated
clientConfig.authClient = new Impersonated({
sourceClient: authClient,
targetPrincipal: this.bigQueryCredentials.impersonateServiceAccount,
targetScopes: ['https://www.googleapis.com/auth/cloud-platform']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXTRA_GOOGLE_SCOPES?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you would like done here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean using EXTRA_GOOGLE_SCOPES here instead of hard-coding

@kolina
Copy link
Copy Markdown
Contributor

kolina commented Jan 2, 2026

@wintermi, in the current version tests are failing due to linter checks: output

You can check errors using this lint script.

@wintermi
Copy link
Copy Markdown
Author

wintermi commented Jan 5, 2026

Resynced the PR with the latest commit

@wintermi
Copy link
Copy Markdown
Author

wintermi commented Jan 5, 2026

@wintermi, in the current version tests are failing due to linter checks: output

You can check errors using this lint script.

Fixed the linter issues

@wintermi
Copy link
Copy Markdown
Author

wintermi commented Jan 5, 2026

@kolina ready for retesting, thanks

@wintermi wintermi requested a review from kolina January 7, 2026 05:35
@kolina
Copy link
Copy Markdown
Contributor

kolina commented Jan 7, 2026

/gcbrun

Comment thread cli/api/dbadapters/bigquery.ts Outdated
clientConfig.authClient = new Impersonated({
sourceClient: authClient,
targetPrincipal: this.bigQueryCredentials.impersonateServiceAccount,
targetScopes: ['https://www.googleapis.com/auth/cloud-platform']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean using EXTRA_GOOGLE_SCOPES here instead of hard-coding

Comment thread cli/api/dbadapters/bigquery.ts
…st' commands and the required changes to allow for the impersonation of service accounts without the need to change ADC
…st' commands and the required changes to allow for the impersonation of service accounts without the need to change ADC
@wintermi
Copy link
Copy Markdown
Author

I removed the parse-duration dependency to avoid CVE-2025-25283
Moving to the parse-duration@2.1.3 seemed a far bigger change than necessary.

@wintermi
Copy link
Copy Markdown
Author

I added a IMPERSONATION_GOOGLE_SCOPES as the EXTRA_GOOGLE_SCOPES only mention drive.

@wintermi wintermi requested a review from kolina April 10, 2026 01:09
projectId = projectId || this.bigQueryCredentials.projectId;
if (!this.clients.has(projectId)) {
this.clients.set(
const clientConfig: any = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BigQueryOptions instead of any?

Comment on lines 276 to +277
projectId,
new BigQuery({
scopes: EXTRA_GOOGLE_SCOPES,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are projectId and scopes here used by the auth library if you ovewrite authClient? If they're not used in this case, I'd only set them in else branch below

projectId,
scopes: EXTRA_GOOGLE_SCOPES,
location: this.bigQueryCredentials.location,
scopes: IMPERSONATION_GOOGLE_SCOPES,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an explaining comment why we're passing different set of scopes with impersonation and without

Comment thread cli/index.ts
getCredentialsPath(argv[projectDirOption.name], argv[credentialsOption.name])
);
if (argv[impersonateServiceAccountOption.name]) {
(readCredentials as any).impersonateServiceAccount =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extend dataform.IBigQuery with your new option to avoid dynamic casts breaking static typing?

Comment thread cli/util.ts
return `${value} ${units[i]}`;
}

const DURATION_UNITS_IN_MILLIS: { [unit: string]: number } = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate about the effort to avoid it and upgrade parse-duration dependency?

import Long from "long";
import { PromisePoolExecutor } from "promise-pool-executor";

import { BigQuery, GetTablesResponse, TableField, TableMetadata } from "@google-cloud/bigquery";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to break lint checks

Step #1: ERROR: /workspace/cli/api/dbadapters/bigquery.ts:4:1 - Import sources within a group must be alphabetized.
Step #1: ERROR: /workspace/cli/api/dbadapters/bigquery.ts:5:1 - Imports from this module are not allowed in this group.  The expected groups (in order) are: external, internal.

Comment thread cli/util.ts
throw new Error("Duration cannot be empty.");
}

if (/^[+-]?\d+(\.\d+)?$/.test(normalizedDuration)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint checks are failing

Step #1: ERROR: /workspace/cli/util.ts:90:7 - Unsafe Regular Expression
Step #1: ERROR: /workspace/cli/util.ts:97:27 - Unsafe Regular Expression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants