Canary test (with GCP bucket)

[Ford544]

Adds a canary test for verifying that any changes to Dataform Core does not break the GCP managed service. The test runs as part of the CloudBuild test script.

The test does the following:

1. Builds core
2. Uploads the bundle to a GCP bucket
3. Creates an ephemeral Dataform workspace and upload a sample project there, with a package.json file specifying the bucket file as source for @dataform/core installation
4. Attempts installing the core bundle and compiling the project via the GCP API
5. Passes if the compilation succeeds, fails otherwise

Verified that the test passes with unchanged core, and fails after deliberately introducing an error.

[apilaskowski]

This writes the Cloud Build service account's OAuth token into a file persisted in a Dataform workspace. It is readable by anyone with access to canary-repo (and by the npm install runtime), and if cleanup doesn't run (timeout/SIGKILL) it stays there. Could we instead use a short-lived V4 signed URL for the tarball in package.json and drop .npmrc entirely? Alternatively, a dedicated bucket readable only by the Dataform service agent.

[Ford544]

Good point, will fix once the permission issues on CloudBuild are sorted out

[apilaskowski]

Since run_canary_test (and tools/canary-project/*) execute with this token, can we make sure the trigger requires approval for fork/external PRs, or skip the canary when _HEAD_REPO_URL isn't upstream? Otherwise a PR could modify the script to exfiltrate the token.

[Ford544]

This runs on the same trigger as the other tests, so it requires approval via PR comment unless submitted by owner/collaborator

[apilaskowski]

This writes the Cloud Build SA's own access token into a file in the workspace. That SA has broad permissions in dataform-open-source (BigQuery admin, Artifact Registry admin, Secret Manager accessor, KMS, ...), and the token stays valid for up to 1h even after the workspace is deleted.

We're adding a dedicated read-only SA for this: dataform-canary-reader@dataform-open-source.iam.gserviceaccount.com. It gets roles/storage.objectViewer, restricted by an IAM condition to gs://dataform-open-source-canary-builds only, and the Cloud Build SA can mint tokens for it (internal config change in progress). Could you switch .npmrc to that token? e.g. in cloudbuild-test.yaml:

gcloud auth print-access-token \
  --impersonate-service-account=dataform-canary-reader@dataform-open-source.iam.gserviceaccount.com \
  > ~/gcp_reader_token.txt

and here:

Suggested change

	NPMRC_CONTENTS=$(printf "//storage.googleapis.com/:_authToken=%s\n" "${GCP_TOKEN}" | base64 -w 0)
	NPMRC_CONTENTS=$(printf "//storage.googleapis.com/:_authToken=%s\n" "${READER_TOKEN}" | base64 -w 0)

(loading READER_TOKEN from ~/gcp_reader_token.txt next to GCP_TOKEN). Uploads and Dataform API calls keep using GCP_TOKEN.

[Ford544]

The script now creates a signed URL which is then passed to package.json - the URL grants permissions equivalent to dataform-canary-reader@dataform-open-source.iam.gserviceaccount.com to the bucket object.

[apilaskowski]

Writing the token to ~/gcp_token.txt is fine since /builder/home is shared between steps, but note the token is minted at the very start and the canary runs after lint + unit + integration tests. With a 3600s build timeout it can expire (default lifetime 1h) before run_canary_test uses it. Could we mint the token(s) in a separate step right before the canary, or fetch them inside the script (e.g. from the metadata server: curl -H 'Metadata-Flavor: Google' http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/token)?

[Ford544]

We could recreate the token before the canary test, but I don't think that's necessary - the entire script typically takes less than 10m to execute, 1h is plenty.

[apilaskowski]

run_tests_on_cloudbuild already switches the registry and runs yarn install --frozen-lockfile (lines 4-7), so this is redundant there. Also, cleanup runs git restore yarn.lock, which undoes the parent script's registry switch as a side effect. Suggest moving this into an if that only runs when the script is invoked standalone, or dropping it and documenting the precondition.

[Ford544]

This will be gone after rebase, since main switched to pnpm and manipulating the lockfile is no longer necessary

[apilaskowski]

The CI identity doesn't have Dataform permissions in dataform-open-source yet, so these calls would currently fail on the real trigger. We'll grant workspace/compilation access on our side, but intentionally not repository-create rights.

Instead, canary-repo will be created once, manually, by a project maintainer before this lands. Could you drop the repositories.create call here (and treat a missing repo as a clear setup error pointing to that one-time step, e.g. a short note in the script header)?

[apilaskowski]

Or was creating new repo each time planned? Lets align about this, I am a bit hesitant to grant too wide permissions.

[Ford544]

No, the idea was for the repo to be persistend and only recreated if needed. I will drop the creation step and throw error if the repo's not found.

[apilaskowski]

This adds a hard dependency on the managed service (and on npm/GCS from the Dataform backend) to every PR's presubmit, so a transient 5xx or a regional incident would block all PRs. Could we add a small retry for 429/5xx on the API calls (e.g. curl --retry 3 --retry-all-errors --retry-delay 5) and make sure a failure message clearly says it's the canary (infra) rather than a core regression?

[Ford544]

Done

[apilaskowski]

Security check: this step runs PR-controlled code (the canary script, the build, and the project files) with the project's Cloud Build credentials. Can you confirm the PR trigger requires maintainer approval (/gcbrun / comment control) for PRs from forks and non-collaborators? Otherwise any external PR can use those credentials. Also, since a failing canary is mostly an infra/managed-service signal, consider running it as its own Cloud Build step so it shows up separately in the logs and can be retried on its own.

[Ford544]

The trigger requires approval when submitted by anyone who's not a collaborator in this repo (I think whether it's from fork or not is irrelevant). You can see it here for example: #2323

[Ford544]

I split the canary test into two steps (tarball build and the test proper) which simplifies the code somewhat as we can use Bazel commands in the first step and gcloud commands in the latter.

I also switched from using the timestamp to using CloudBuild's RUN_ID as the second part of the identifier - this gives better guarantees of uniqueness (no chance of collision if two jobs are submitted on the same second). In local runs it still falls back to the timestamp.

[rafal-hawrylak]

Please think how to add a negative canary test that would smoke prove that the mechanism is correct. Where the negative test would go: either at the end of scripts/run_canary_test (after the positive run, reusing the same workspace and token). Or as a 5th step. It would repack the tarball with a broken bundle.js, install and compile again, and pass only if the compile fails with the expected message.

[Ford544]

Re: the negative canary test - are we sure we would want to do that on every run? As long as we make sure the test is catching errors correctly, submitting a broken bundle each time a new PR version is submitted wouldn't prove anything but the fact that the broken bundle is still broken.

The only way this kind of test could be useful is if the test itself could unexpectedly break in a way that would result in false negatives (broken bundles reported as correct), and I don't see how could this occur (Dataform does not fallback to any other Core version if the one specified in package.json is unavailable, so there's no risk that the test could accidentally use the official version instead of the provided canary).

[Ford544]

If we are concerned with some kind of GCP-side failure (though I'm not sure what kind of a failure could result in false negatives instead of simply throwing errors), it might also be better to do it as some kind of scheduled workflow, so as to not add unneccessary overhead to the tests

[rafal-hawrylak]

To speed up some test runs we could exlude running canary test if the changes are limited to https://github.com/dataform-co/dataform/tree/main/cli only.

[rafal-hawrylak]

If we are concerned with some kind of GCP-side failure (though I'm not sure what kind of a failure could result in false negatives instead of simply throwing errors), it might also be better to do it as some kind of scheduled workflow, so as to not add unneccessary overhead to the tests

I was thinking about cases were GCP reports 2xx even if some kind of failure happened. Asserting on the exit code seems not enough. Maybe we could add assertion on the compilation result to strengthen the canary test.

[rafal-hawrylak]

Re: the negative canary test - are we sure we would want to do that on every run? As long as we make sure the test is catching errors correctly, submitting a broken bundle each time a new PR version is submitted wouldn't prove anything but the fact that the broken bundle is still broken.

The only way this kind of test could be useful is if the test itself could unexpectedly break in a way that would result in false negatives (broken bundles reported as correct), and I don't see how could this occur (Dataform does not fallback to any other Core version if the one specified in package.json is unavailable, so there's no risk that the test could accidentally use the official version instead of the provided canary).

Here my concern is that some changes break the canary test itself. So maybe such a test would be triggered only if scripts/build_canary_core_bundle, scripts/run_canary_test or cloudbuild-test.yaml has changed.

[Ford544]

To speed up some test runs we could exlude running canary test if the changes are limited to https://github.com/dataform-co/dataform/tree/main/cli only.

Yes, or we could run it only if changes include specific directiories relevant to Core so that documention or test changes are also excluded. The best way to do that would probably be to create a new CloudBuild trigger and extract the canary test to a seperate cloudbuild-*.yaml file

[Ford544]

If we are concerned with some kind of GCP-side failure (though I'm not sure what kind of a failure could result in false negatives instead of simply throwing errors), it might also be better to do it as some kind of scheduled workflow, so as to not add unneccessary overhead to the tests

I was thinking about cases were GCP reports 2xx even if some kind of failure happened. Asserting on the exit code seems not enough. Maybe we could add assertion on the compilation result to strengthen the canary test.

Just to be clear, the script already checks if there are any compilation errors and makes sure the core version matches the canary package, it's not just exit code. I suppose we could also compare the entire output against a prepared json, though it would then need to be kept up to date if the output changes in new versions.

[rafal-hawrylak]

To speed up some test runs we could exlude running canary test if the changes are limited to https://github.com/dataform-co/dataform/tree/main/cli only.

Yes, or we could run it only if changes include specific directiories relevant to Core so that documention or test changes are also excluded. The best way to do that would probably be to create a new CloudBuild trigger and extract the canary test to a seperate cloudbuild-*.yaml file

I'd rather block list here as we might not include all the relavant dirs now or future ones that affect building core.

[rafal-hawrylak]

If we are concerned with some kind of GCP-side failure (though I'm not sure what kind of a failure could result in false negatives instead of simply throwing errors), it might also be better to do it as some kind of scheduled workflow, so as to not add unneccessary overhead to the tests

I was thinking about cases were GCP reports 2xx even if some kind of failure happened. Asserting on the exit code seems not enough. Maybe we could add assertion on the compilation result to strengthen the canary test.

Just to be clear, the script already checks if there are any compilation errors and makes sure the core version matches the canary package, it's not just exit code. I suppose we could also compare the entire output against a prepared json, though it would then need to be kept up to date if the output changes in new versions.

We assert compilation errors only (if non empty). To get compilation results we'll need to make another call as those are not included in this response.

[Ford544]

To speed up some test runs we could exlude running canary test if the changes are limited to https://github.com/dataform-co/dataform/tree/main/cli only.

Yes, or we could run it only if changes include specific directiories relevant to Core so that documention or test changes are also excluded. The best way to do that would probably be to create a new CloudBuild trigger and extract the canary test to a seperate cloudbuild-*.yaml file

I'd rather block list here as we might not include all the relavant dirs now or future ones that affect building core.

Makes sense; we would still probably want to create a new trigger for that

[Ford544]

Added a compilation result check against a prepared expected .json file.

[Ford544]

Extracted the canary test into a seperate workflow with a trigger only activating if files potentially relevant to Core were modified

[rafal-hawrylak]

The comparison is an exact match on the full API objects, including internalMetadata, incrementalTableConfig: {} and leading whitespace in selectQuery. Two kinds of noise:

1. if the backend adds or changes a field in the :query response, every core PR fails although core didn't change
2. every intended change to compiled output needs a hand-edited baseline.

Compare only the fields core controls (targets, relation type, dependencies, queries with whitespace normalised), or check that the expected fields are a subset of the actual ones. Print the command to regenerate the baseline in the failure message.