Require authentication for dry run function and run gcloud auth when … #5171

scholtzan · 2024-03-05T21:42:15Z

…not logged in

I opened https://mozilla-hub.atlassian.net/browse/DSRE-1558 to actually require authentication for the dryrun function and give dryrun permissions against restricted datasets. The PR should be merged before we apply these changes.

Fixes #5168

Checklist for reviewer:

Commits should reference a bug or github issue, if relevant (if a bug is referenced, the pull request should include the bug number in the title).
If the PR comes from a fork, trigger integration CI tests by running the Push to upstream workflow and provide the <username>:<branch> of the fork as parameter. The parameter will also show up
in the logs of the manual-trigger-required-for-fork CI task together with more detailed instructions.
If adding a new field to a query, ensure that the schema and dependent downstream schemas have been updated.
When adding a new derived dataset, ensure that data is not available already (fully or partially) and recommend extending an existing dataset in favor of creating new ones. Data can be available in the bigquery-etl repository, looker-hub or in looker-spoke-default.

For modifications to schemas in restricted namespaces (see CODEOWNERS):

Follow the change control procedure

┆Issue is synchronized with this Jira Task

…not logged in

scholtzan · 2024-03-05T23:16:24Z

bigquery_etl/cli/utils.py

+        try:
+            subprocess.run(["gcloud", "auth", "application-default", "login"])
+        except Exception as e:
+            print(f"Could not log in to GCP: {e}")
+            return False


this will prompt the browser authentication flow

If that gcloud command somehow ends up getting run in a non-interactive context (e.g. Airflow tasks, CI jobs) it seems like it could hang the process waiting for the browser-based auth to be completed, unless the command is smart enough to detect that it's running in a non-interactive context or the attempt to open a browser fails. Is there a reasonable way to guard against that (e.g. checking for particular environment variables)?

Even in an interactive context this could cause a headache if it gets called in a parallelized way (e.g. pytest, SQL generators), where it could spam open a bunch of browser windows. To guard against that I think we'd want to make sure authentication is triggered either prior to the parallelization starting, or in some controlled way (e.g. for pytest maybe using some scoped fixture?).

While putting this logic into the is_authenticated() function is expedient, IMO it conflicts with the natural expectations people would have based on the function name that it's just a status check with no side effects. I'd suggest putting this logic in a new imperatively-named function like authenticate() or ensure_authenticated(), which would raise an appropriate error if authentication fails.

It might be a good idea to cache this function (and/or the alternate imperative function if you go that route) to avoid the authentication getting checked repeatedly unnecessarily.

Also, I believe check=True needs to be specified for subprocess.run() to raise an exception if the command exited with a non-zero exit code.

Yeah, that's a good point. I don't see a reliable way to check for non-interactive environment. Perhaps for now it's best to not call gcloud auth and print out an error and a suggestion to run authentication.

I opened #5174 for now, to keep track of this. I think for now we just keep the flow as is, and ask users to run gcloud auth themselves.

.circleci/workflows.yml

bigquery_etl/cli/dryrun.py

sean-rose · 2024-03-06T04:11:09Z

bigquery_etl/cli/utils.py

@@ -44,7 +45,11 @@ def is_authenticated():
    try:
        bigquery.Client(project="")


In testing this locally I didn't get prompted to authenticate because I did have application default credentials, but they were expired. I think an alternate approach is needed, like calling google.auth.default() to get the credentials (which will also raise DefaultCredentialsError if no default credentials were found) and then checking its valid property.

sean-rose · 2024-03-06T04:43:35Z

bigquery_etl/cli/utils.py

+        try:
+            subprocess.run(["gcloud", "auth", "application-default", "login"])
+        except Exception as e:
+            print(f"Could not log in to GCP: {e}")
+            return False


If that gcloud command somehow ends up getting run in a non-interactive context (e.g. Airflow tasks, CI jobs) it seems like it could hang the process waiting for the browser-based auth to be completed, unless the command is smart enough to detect that it's running in a non-interactive context or the attempt to open a browser fails. Is there a reasonable way to guard against that (e.g. checking for particular environment variables)?

Even in an interactive context this could cause a headache if it gets called in a parallelized way (e.g. pytest, SQL generators), where it could spam open a bunch of browser windows. To guard against that I think we'd want to make sure authentication is triggered either prior to the parallelization starting, or in some controlled way (e.g. for pytest maybe using some scoped fixture?).

While putting this logic into the is_authenticated() function is expedient, IMO it conflicts with the natural expectations people would have based on the function name that it's just a status check with no side effects. I'd suggest putting this logic in a new imperatively-named function like authenticate() or ensure_authenticated(), which would raise an appropriate error if authentication fails.

It might be a good idea to cache this function (and/or the alternate imperative function if you go that route) to avoid the authentication getting checked repeatedly unnecessarily.

bigquery_etl/dryrun.py

scholtzan · 2024-03-06T19:35:47Z

bqetl_project.yaml

+  - sql/moz-fx-data-shared-prod/fenix_derived/ltv_state_values_v1/query.sql
+  - sql/moz-fx-data-shared-prod/fenix_derived/ltv_state_values_v2/query.sql


As discussed in the Jira ticket, the dryrun service account won't have access to analysis. These are the only two datasets atm that access an analysis dataset. They'll get productionized soon, so until then skip dryruns.

sean-rose

r+wc

sean-rose · 2024-03-06T21:03:55Z

.circleci/workflows.yml

+            - &skip_forked_pr
+              run:
+                name: Early return if this build is from a forked PR
+                command: |
+                  if [ -n "$CIRCLE_PR_NUMBER" ]; then
+                    echo "Cannot pass creds to forked PRs," \
+                      "so marking this step successful"
+                    circleci-agent step halt
+                  fi


The test-sql and test-routines jobs use similar forked-PR-detection logic. Should they use this instead?

sean-rose · 2024-03-06T21:12:56Z

.circleci/workflows.yml

+            - &authenticate
+              run:
+                name: Authenticate to GCP
+                command: |
+                  export GOOGLE_APPLICATION_CREDENTIALS="/tmp/gcp.json"
+                  echo 'export GOOGLE_APPLICATION_CREDENTIALS="/tmp/gcp.json"' >> "$BASH_ENV"
+                  echo "$GCLOUD_SERVICE_KEY" > "$GOOGLE_APPLICATION_CREDENTIALS"


The test-sql, integration, and private-generate-sql jobs don't set up authentication. Should they?

private-generate-sql does indeed need this. At least the current tests in integration and test-sql don't depend on dryruns. So they don't need to change

At least the current tests in integration and test-sql don't depend on dryruns. So they don't need to change

But the test-sql job has forked-PR-detection logic that says "Cannot pass creds to forked PRs". Is that referring to different credentials?

Yeah, those are different credentials afaik. for running some of these SQL tests we write test data to a BigQuery test project that needs these credentials

sean-rose · 2024-03-06T21:42:16Z

bigquery_etl/dryrun.py

+
+        if not is_authenticated():
+            print(
+                "Authentication to GCP required. Run `gcloud auth login` "


I tested and verified that the specified gcloud auth login command isn't sufficient without the additional --update-adc argument.

Suggested change

"Authentication to GCP required. Run `gcloud auth login` "

"Authentication to GCP required. Run `gcloud auth login --update-adc` "

(this exact same message is used in a bunch of other places, but updating all those is probably beyond the scope of this PR).

sean-rose · 2024-03-06T21:44:42Z

bigquery_etl/dryrun.py

+                "Authentication to GCP required. Run `gcloud auth login` "
+                "and check that the project is set correctly."
+            )
+            sys.exit(1)


Now that authentication is being required in many more cases, it might be good to update the "GCP CLI tools" section of the README to instruct non-DEs to authenticate.

dataops-ci-bot · 2024-03-06T22:37:07Z

Integration report for "Refactor skip_fork in CI, clarify login requirements"

`sql.diff`

No content detected.

Link to full diff

#5171) * Require authentication for dry run function and run gcloud auth when not logged in * authenticate step in CI, remove interactive gcloud auth * Skip dryrun for ltv_state_values_v2 * Refactor skip_fork in CI, clarify login requirements

scholtzan force-pushed the authenticated-dryrun-function branch 6 times, most recently from a0627cc to fe03660 Compare March 5, 2024 22:55