Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create Google app creds file from env var as needed #4366

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

groovecoder
Copy link
Member

@groovecoder groovecoder commented Jan 29, 2024

This PR fixes MPP-3467.

It changes the profiling code to automatically create the necessary GCP key JSON file from the value of the GOOGLE_CLOUD_PROFILER_CREDENTIALS_B64 environment variable. This should work identically across local, dev, stage, and prod environments.

How to test:

  1. Follow the doc!
  2. When you runserver you should see output lines: HTTPAdapter.get_connection url: http://metadata/computeMetadata/v1/instance/zone which indicates that the profiler code is working
  • l10n changes have been submitted to the l10n repository, if any.
  • I've added a unit test to test for potential regressions of this bug.
  • I've added or updated relevant docs in the docs/ directory.
  • All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /frontend/src/styles/tokens.scss).
  • Commits in this PR are minimal and have descriptive commit messages.

@groovecoder groovecoder force-pushed the create-gcp_key.json-as-needed branch 3 times, most recently from d3f47e8 to 4f58897 Compare February 6, 2024 21:07
@groovecoder groovecoder marked this pull request as ready for review February 6, 2024 21:13
@groovecoder groovecoder requested a review from jwhitlock February 6, 2024 21:13
@groovecoder groovecoder force-pushed the create-gcp_key.json-as-needed branch from 4f58897 to c965fe5 Compare February 6, 2024 21:27
Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

Thanks @groovecoder, this looks a little safer. Please consider what should happen if GOOGLE_APPLICATION_CREDENTIALS is a directory - raise an exception, or skip loading the profiler?

gcp_key_json_path = Path(settings.GOOGLE_APPLICATION_CREDENTIALS)
if gcp_key_json_path.exists() and gcp_key_json_path.is_file():
if not gcp_key_json_path.exists() or not gcp_key_json_path.is_file():
Copy link
Member

Choose a reason for hiding this comment

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

If you set GOOGLE_APPLICATION_CREDENTIALS to "privaterelay", this fails with IsADirectoryError: [Errno 21] Is a directory: 'privaterelay'. I think the line should be:

Suggested change
if not gcp_key_json_path.exists() or not gcp_key_json_path.is_file():
if not gcp_key_json_path.exists() and gcp_key_json_path.is_file():

At the same time, you'll get a IsADirectoryError in a couple of lines when you try to read it. So, maybe drop the is_file() check and let Relay crash on startup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the is_file check and expanded the try/except block around the code for the file so that the Relay app can still start up and run even if google cloud profiler can't.

@groovecoder groovecoder force-pushed the create-gcp_key.json-as-needed branch from c965fe5 to 92c341f Compare February 7, 2024 16:44
Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

Thanks, this worked with my tests (folder, empty json). One suggestion you could take or leave.

Comment on lines 69 to 74
except:
print(
f"exception while starting google cloud profiler with key file: {gcp_key_json_path}"
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: print more of the error:

Suggested change
except:
print(
f"exception while starting google cloud profiler with key file: {gcp_key_json_path}"
)
except Exception as exc:
print(
f"exception {repr(exc)}"
" while starting google cloud profiler"
f" with key file: {gcp_key_json_path}"
)

@groovecoder groovecoder force-pushed the create-gcp_key.json-as-needed branch from 92c341f to fbc2553 Compare February 7, 2024 19:13
@groovecoder groovecoder added this pull request to the merge queue Feb 7, 2024
Merged via the queue into main with commit 5675303 Feb 7, 2024
27 checks passed
@groovecoder groovecoder deleted the create-gcp_key.json-as-needed branch February 7, 2024 19:35
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.

2 participants