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

GCP backend cleanup and safety improvements #3766

Merged
merged 4 commits into from
May 27, 2020
Merged

Conversation

awly
Copy link
Contributor

@awly awly commented May 26, 2020

  • change key->documentID conversion in lib/backend/firestore to avoid data corruption
    • different keys could map to the same documentIDs
    • change escaping to URL-safe base64 to prevent that
    • this is a backwards-incompatible change, need to discuss with firestore backend users (cc @joshdurbin)
  • prevent session overwrite on upload
  • set default ACLs when auto-creating the bucket
  • handle errors from GetAll calls
  • improve error logging

Fixes #3015

Andrew Lytvynov added 2 commits May 26, 2020 15:26
- close all io.Closers where missing
- add error handling where missing
- improve error messages
- before uploading a session, check that it doesn't already exist; we
  don't want to lose an existing session
- when auto-creating a missing bucket at startup, set predefined ACLs to
  `projectPrivate`, which means "Project team members get access
  according to their roles." See
  https://cloud.google.com/storage/docs/json_api/v1/buckets/insert#parameters
@awly awly force-pushed the andrew/gcp-backend-cleanup branch from 1db8c54 to db634da Compare May 26, 2020 22:34
Andrew Lytvynov added 2 commits May 26, 2020 15:40
Tests run against an in-memory mock GCS, no need to guard them with a
build tag.
Reimplement cleanup code in the test itself, since the helper method is
gone.
@awly awly marked this pull request as ready for review May 26, 2020 23:08
@awly
Copy link
Contributor Author

awly commented May 27, 2020

retest this please

@joshdurbin
Copy link
Contributor

joshdurbin commented May 27, 2020

LGTM. 👍

As discussed with @awly over e-mail my current use case is for two clusters that aren't super critical (and are managed by Puppet and can easily be rebuilt) and have event data we are able to forfeit.

@awly awly merged commit 983b735 into master May 27, 2020
@awly awly deleted the andrew/gcp-backend-cleanup branch May 27, 2020 18:37
awly pushed a commit that referenced this pull request Jul 14, 2020
#3766 removed a nil
pointer check when creating firestore indexes. This was a legitimate
check, for when indexes already exist.

Tested this manually. Unit testing is trickier because the firestore
emulator in gcloud doesn't support indexes.
awly pushed a commit that referenced this pull request Jul 15, 2020
#3766 removed a nil
pointer check when creating firestore indexes. This was a legitimate
check, for when indexes already exist.

Tested this manually. Unit testing is trickier because the firestore
emulator in gcloud doesn't support indexes.
awly pushed a commit that referenced this pull request Jul 15, 2020
#3766 removed a nil
pointer check when creating firestore indexes. This was a legitimate
check, for when indexes already exist.

Tested this manually. Unit testing is trickier because the firestore
emulator in gcloud doesn't support indexes.
awly pushed a commit that referenced this pull request Jul 15, 2020
#3766 removed a nil
pointer check when creating firestore indexes. This was a legitimate
check, for when indexes already exist.

Tested this manually. Unit testing is trickier because the firestore
emulator in gcloud doesn't support indexes.
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.

Security Review of GCP Backend
4 participants