[Cloud Run] Identity Platform + Cloud SQL sample #1984

averikitsch · 2020-09-25T19:23:37Z

@grayside - I am specifically hesitant about the secret loading in cloud-sql.js.
@kurtisvg - tagging for a Cloud SQL review.

grayside · 2020-09-28T18:29:11Z

I've started an extensive review, planning to send by end of day.

grayside

Lots to review here. The basic structure of the app seems good, and all the logic makes sense. Most of my feedback is around clarity, CI/CD practices, Secret handling practices, and library selection.

run/idp-sql/Dockerfile

run/idp-sql/README.md

run/idp-sql/cloud-sql.js

run/idp-sql/app.js

run/idp-sql/cloud-sql.js

run/idp-sql/Dockerfile

run/idp-sql/README.md

run/idp-sql/cloud-sql.js

…nodejs-docs-samples into cats-v-dogs

run/idp-sql/app.json

averikitsch · 2020-10-07T19:49:46Z

run/idp-sql/cloud-run-button-script.sh

@@ -0,0 +1,64 @@
+# Copyright 2020 Google LLC


note: script provisions infrastructure when running cloud run button

Is this script only usable in the context of cloud run button? If not, maybe it should have a name like setup.sh

fhinkel

Rubberstamp lgtm

run/idp-sql/static/firebase.js

run/idp-sql/middleware.js

run/idp-sql/secrets.js

run/idp-sql/static/firebase.js

run/idp-sql/cloud-run-button-script.sh

run/idp-sql/app.json

run/idp-sql/cloud-run-button-script.sh

averikitsch · 2020-10-14T16:51:13Z

@grayside from my investigation, most of the flaky tests come from trying to delete a resource already deleted or deleting it too soon. I haven't seen a case where the build or deploy fails when it should be successful. Using the cloud build for setup and clean up should prevent resources from being cleaned up before tests finish running. Though I've added a retry.sh utility to check for the resource before deleting and retrying if the deleting fails.

run/idp-sql/app.js

run/idp-sql/app.json

run/idp-sql/index.js

run/idp-sql/logging.js

run/idp-sql/secrets.js

grayside · 2020-10-14T17:54:53Z

run/idp-sql/test/e2e_test_cleanup.yaml

+    - '-c'
+    - |
+      . /workspace/test/retry.sh
+      retry "gcloud secrets describe ${_SERVICE}-secrets" \


This looks nice and clean. We should spread the word. Note if retry.sh is directly the script we would just need to confirm it's executable in one step and not need to source it in each step for use.

grayside · 2020-10-14T17:55:37Z

run/idp-sql/test/e2e_test_setup.yaml

+  args:
+  - '-c'
+  - |
+    gcloud run deploy ${_SERVICE} \


While the "ensure-exists" model of the current retry script wouldn't work, we do want to retry this as well.

I don't think I've experience an error deploying unless it's my fault the service doesn't start. Since deployment failures seem rare, I would lean on not implementing retries.

I don't know that I've seen a trend in which operations fail most on nightly tests, I've seen build, deploy, and delete all have issues. We can leave it at this for now, but I'm strongly recommending every API interaction have a retry.

grayside · 2020-10-14T17:58:26Z

run/idp-sql/test/system.test.js

+    if (!BASE_URL) throw Error('Cloud Run service URL not found');
+    console.log('Cloud Run service URL found.');
+
+    const idToken = execSync('gcloud auth print-identity-token');


nit: I think using the client library is better than print-identity-token. This gcloud command is used in CI for lack of a better option, but it seems to me that it's meant to facilitate local dev with a tradeoff of reducing project security.

grayside · 2020-10-14T18:00:20Z

run/idp-sql/test/system.test.js

+
+    const url = execSync(
+      `gcloud run services describe ${SERVICE_NAME} --project=${GOOGLE_CLOUD_PROJECT} ` +
+      `--platform=managed --region=us-central1 --format='value(status.url)'`);


nit: Since this file "owns" how the testing will work and the Cloud Build file is configurable, ideally the build substitutions will be set from this file instead of rely on synchronized defaults.

grayside · 2020-10-14T18:05:35Z

run/idp-sql/test/system.test.js

+const uuid = require('short-uuid');
+const {execSync} = require('child_process');
+
+describe('System Tests', () => {


Now/Future Enhancement: Given all the setup overhead for end-to-end tests, deeper coverage is worthwhile. It will cost seconds and we're already invested a couple minutes.

The existing "light" test coverage is not because it's preferred but because that's a level of test coverage I could add to many of the samples in an afternoon.

I'm not sure how to extend test coverage here. Since we deploy with authentication, that conflicts with the authentication of POST request route.

We can special case this to deploy without authentication.

The principle is that apps are not publicly accessible, but what you're saying is that this (mostly) isn't, because of a different auth mechanism.

grayside

Aside from the package.json engine update, this is LGTM. Approving in anticipation of the easy fix before or after merge.

grayside · 2020-10-19T17:51:39Z

.kokoro/build-with-run.sh

+export DB_NAME="kokoro_ci"
+export DB_USER="kokoro_ci"
+export DB_PASSWORD=$(cat $KOKORO_GFILE_DIR/secrets-sql-password.txt)
+export CLOUD_SQL_CONNECTION_NAME=$(cat $KOKORO_GFILE_DIR/secrets-pg-connection-name.txt)
+
+export FIREBASE_KEY=$(cat $KOKORO_GFILE_DIR/secrets-firebase-key.txt)


We could configure this to only work if the idp-sql sample is being tested. That seems like a higher level of secret paranoia than this repo applies.

grayside · 2020-10-19T17:57:13Z

run/idp-sql/app.js

+  // Save the data to the database.
+  try {
+    await insertVote(vote);
+    logger.info({message: 'vote_inserted', vote, traceId: req.traceId})


nit/future: traceId here is ok, but makes the trace elements higher visibility around the code.

The pattern I'd expect is for the middleware to duplicate the global logger, set request-specific defaults, and provide a request-scoped logger for this controller. Given this code is not region tagged, it's a low priority.

run/idp-sql/cloud-sql.js

run/idp-sql/package.json

grayside · 2020-10-19T18:14:43Z

run/idp-sql/index.js

+  if (!process.env.GOOGLE_CLOUD_PROJECT) {
+    try {
+      const project = await auth.getProjectId();
+      process.env.GOOGLE_CLOUD_PROJECT = project; // Set GOOGLE_CLOUD_PROJECT for log correlation 


nit/future: code smell to pass this around as an environment variable. We could pass this in to the logger object immediately.

run/idp-sql/index.js

run/idp-sql/postcreate.sh

run/idp-sql/secrets.js

run/idp-sql/test/retry.sh

run/idp-sql/cloud-sql.js

grayside · 2020-10-20T00:36:33Z

I wonder if this should do a knex.destroy() on sigterm.

…nodejs-docs-samples into cats-v-dogs

averikitsch · 2020-10-21T16:46:06Z

Tests failing are unrelated. @fhinkel @grayside can you merge this?

averikitsch added 3 commits September 16, 2020 15:04

App Drafted

a5f92fb

Rename

c41ff9a

updates

012918f

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 25, 2020

averikitsch requested review from grayside and kurtisvg September 25, 2020 19:23

averikitsch added 2 commits September 25, 2020 12:24

minor style fixes

f262529

Merge branch 'master' into cats-v-dogs

18c0063

product-auto-label bot added the samples Issues that are directly related to samples. label Sep 26, 2020

grayside requested changes Sep 28, 2020

View reviewed changes

Merge branch 'master' into cats-v-dogs

5e2363d

kurtisvg reviewed Oct 1, 2020

View reviewed changes

run/idp-sql/Dockerfile Outdated Show resolved Hide resolved

run/idp-sql/README.md Outdated Show resolved Hide resolved

run/idp-sql/cloud-sql.js Outdated Show resolved Hide resolved

run/idp-sql/cloud-sql.js Show resolved Hide resolved

averikitsch added 9 commits October 1, 2020 09:48

Updates

e9c54de

Update 2

816a95a

Add more tests and updates

4cc2a23

Merge branch 'cats-v-dogs' of https://github.com/GoogleCloudPlatform/…

7e44fe7

…nodejs-docs-samples into cats-v-dogs

Comment updates

08eb839

Tests updated

c04234e

Add createTable

bd25555

Add scripts

fffabc2

secret manager clean up

3718c26

averikitsch commented Oct 7, 2020

View reviewed changes

run/idp-sql/app.json Show resolved Hide resolved

averikitsch commented Oct 7, 2020

View reviewed changes

averikitsch and others added 3 commits October 7, 2020 12:55

Add Kokoro set up

b17cf63

Remove trace agent

8af2289

Merge branch 'master' into cats-v-dogs

1699e48

fhinkel approved these changes Oct 8, 2020

View reviewed changes

averikitsch added 2 commits October 8, 2020 08:24

Merge branch 'master' into cats-v-dogs

e039444

Update tests

b7ccc59

Add and drop table for unit tests

86a74d4

averikitsch force-pushed the cats-v-dogs branch 2 times, most recently from 5fafcd7 to 98affc0 Compare October 8, 2020 21:27

Add debugging

60d5254

averikitsch force-pushed the cats-v-dogs branch from 98affc0 to 60d5254 Compare October 8, 2020 21:38

Update README

1bdc6e0

grayside requested changes Oct 9, 2020

View reviewed changes

PR comments

eff522c

averikitsch force-pushed the cats-v-dogs branch from 7d1672c to eff522c Compare October 13, 2020 23:31

grayside reviewed Oct 14, 2020

View reviewed changes

averikitsch added 4 commits October 14, 2020 17:12

Restructure startup

eee77a1

Update retry

9777c17

Update testing

11bd6ec

add more tests

71a7bda

grayside approved these changes Oct 19, 2020

View reviewed changes

kurtisvg approved these changes Oct 19, 2020

View reviewed changes

run/idp-sql/cloud-sql.js Show resolved Hide resolved

run/idp-sql/cloud-sql.js Outdated Show resolved Hide resolved

averikitsch added 2 commits October 19, 2020 16:29

Small updates

4e4295d

Update CR button and cloud sql create table

1091913

averikitsch added 7 commits October 20, 2020 08:26

Merge branch 'master' into cats-v-dogs

5577713

fix tests

072391e

Merge branch 'cats-v-dogs' of https://github.com/GoogleCloudPlatform/…

3e8b565

…nodejs-docs-samples into cats-v-dogs

Update logging

e90bb27

Update comment

be99235

Add sigterm handling

f540bbe

Update log

3a4b161

JustinBeckwith merged commit edae294 into master Oct 21, 2020

fhinkel deleted the cats-v-dogs branch November 5, 2020 17:38

[Cloud Run] Identity Platform + Cloud SQL sample #1984

[Cloud Run] Identity Platform + Cloud SQL sample #1984

Uh oh!

Conversation

averikitsch commented Sep 25, 2020

Uh oh!

grayside commented Sep 28, 2020

Uh oh!

grayside left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhinkel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

averikitsch commented Oct 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grayside left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!