Repository: google/exposure-notifications-server
Pull request #1139 (2020-11-04 23:06:14)

Add basic latency alerts as Terraform config
#1036 

google-oss-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/1139#" title="Author self-approved">yegle</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **sethvargo** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @sethvargo` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

<details open>
Needs approval from an approver in each of these files:

- **[terraform/OWNERS](https://github.com/google/exposure-notifications-server/blob/main/terraform/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["sethvargo"]} -->
google-oss-robot: @yegle: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-en-server-release-unit | 560d9b0ffa204dc7bcd414a2985f41fdd20afd05 | [link](https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/google_exposure-notifications-server/1139/pull-en-server-release-unit/1326610117625384960) | `/test pull-en-server-release-unit`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/go.mod b/go.mod
index f019a52..24ec8c1 100644
--- a/go.mod
+++ b/go.mod
@@ -8,6 +8,7 @@ require (
 	contrib.go.opencensus.io/exporter/ocagent v0.7.1-0.20200615190824-f8c219d2d895
 	contrib.go.opencensus.io/exporter/prometheus v0.2.1-0.20200609204449-6bcf6f8577f0
 	contrib.go.opencensus.io/exporter/stackdriver v0.13.4
+	contrib.go.opencensus.io/integrations/ocsql v0.1.6
 	github.com/Azure/azure-pipeline-go v0.2.3 // indirect
 	github.com/Azure/azure-sdk-for-go v46.3.0+incompatible
 	github.com/Azure/azure-storage-blob-go v0.10.0
@@ -68,7 +69,7 @@ require (
 	github.com/lib/pq v1.8.0 // indirect
 	github.com/lstoll/awskms v0.0.0-20200603175638-a388516467f1
 	github.com/mattn/go-colorable v0.1.7 // indirect
-	github.com/mikehelmick/go-chaff v0.3.0
+	github.com/mikehelmick/go-chaff v0.4.1
 	github.com/mitchellh/cli v1.1.1 // indirect
 	github.com/mitchellh/go-testing-interface v1.14.1 // indirect
 	github.com/mitchellh/mapstructure v1.3.3
diff --git a/go.sum b/go.sum
index bdf5aae..1d38b89 100644
--- a/go.sum
+++ b/go.sum
@@ -50,6 +50,8 @@ contrib.go.opencensus.io/exporter/prometheus v0.2.1-0.20200609204449-6bcf6f8577f
 contrib.go.opencensus.io/exporter/prometheus v0.2.1-0.20200609204449-6bcf6f8577f0/go.mod h1:MjHoxkI7Ny27toPeFkRbXbzVjzIGkwOAptrAy8Mxtm8=
 contrib.go.opencensus.io/exporter/stackdriver v0.13.4 h1:ksUxwH3OD5sxkjzEqGxNTl+Xjsmu3BnC/300MhSVTSc=
 contrib.go.opencensus.io/exporter/stackdriver v0.13.4/go.mod h1:aXENhDJ1Y4lIg4EUaVTwzvYETVNZk10Pu26tevFKLUc=
+contrib.go.opencensus.io/integrations/ocsql v0.1.6 h1:9qmZJBlnMtffShflmfhW4EZK7M+CujIDG4bEwUrg+ms=
+contrib.go.opencensus.io/integrations/ocsql v0.1.6/go.mod h1:8DsSdjz3F+APR+0z0WkU1aRorQCFfRxvqjUUPMbF3fE=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
 git.apache.org/thrift.git v0.12.0/go.mod h1:fPE2ZNJGynbRyZ4dJvy6G277gSllfV2HJqblrnkyeyg=
 github.com/Azure/azure-pipeline-go v0.2.2/go.mod h1:4rQ/NZncSvGqNkkOsNpOU1tgoNuIlp9AfUH5G1tvCHc=
@@ -896,8 +898,8 @@ github.com/michaelklishin/rabbit-hole v0.0.0-20191008194146-93d9988f0cd5/go.mod
 github.com/miekg/dns v1.0.14/go.mod h1:W1PPwlIAgtquWBMBEV9nkV9Cazfe8ScdGz/Lj7v3Nrg=
 github.com/miekg/dns v1.1.15 h1:CSSIDtllwGLMoA6zjdKnaE6Tx6eVUxQ29LUgGetiDCI=
 github.com/miekg/dns v1.1.15/go.mod h1:W1PPwlIAgtquWBMBEV9nkV9Cazfe8ScdGz/Lj7v3Nrg=
-github.com/mikehelmick/go-chaff v0.3.0 h1:bnU5oF8onKCoe9B/iQ22dvhtzSlEcLJ1R82b97T4BAU=
-github.com/mikehelmick/go-chaff v0.3.0/go.mod h1:mFry3zNW17oxNGmZpQV3PEOmzTNyly3nLDYawCT/iCE=
+github.com/mikehelmick/go-chaff v0.4.1 h1:0UsypkU2XcsROrwvL+mN7mgRiiB6oEwt+iNVOHlXO0M=
+github.com/mikehelmick/go-chaff v0.4.1/go.mod h1:mFry3zNW17oxNGmZpQV3PEOmzTNyly3nLDYawCT/iCE=
 github.com/mitchellh/cli v1.0.0/go.mod h1:hNIlj7HEI86fIcpObd7a0FcrxTWetlwJDGcceTlRvqc=
 github.com/mitchellh/cli v1.1.1 h1:J64v/xD7Clql+JVKSvkYojLOXu1ibnY9ZjGLwSt/89w=
 github.com/mitchellh/cli v1.1.1/go.mod h1:xcISNoH86gajksDmfB23e/pu+B+GeFRMYmoHXxx3xhI=
diff --git a/internal/publish/model/exposure_model.go b/internal/publish/model/exposure_model.go
index 2f5d682..ff92b94 100644
--- a/internal/publish/model/exposure_model.go
+++ b/internal/publish/model/exposure_model.go
@@ -173,6 +173,10 @@ func (e *Exposure) Revise(in *Exposure) (bool, error) {
 	if e.ReportType == in.ReportType {
 		return false, nil
 	}
+	// Key is being published again, but has already been revised to target report type.
+	if e.RevisedAt != nil && e.RevisedReportType != nil && *e.RevisedReportType == in.ReportType {
+		return false, nil
+	}
 	if !e.LocalProvenance {
 		nonLocalOK := false
 		if e.ExportImportID != nil {
diff --git a/internal/publish/publish.go b/internal/publish/publish.go
index 7923005..13897b8 100644
--- a/internal/publish/publish.go
+++ b/internal/publish/publish.go
@@ -98,12 +98,17 @@ func NewHandler(ctx context.Context, config *Config, env *serverenv.ServerEnv) (
 		return nil, fmt.Errorf("config validation: %w", err)
 	}
 
+	chaffer, err := chaff.NewTracker(chaff.NewJSONResponder(chaffPublishResponse), chaff.DefaultCapacity)
+	if err != nil {
+		return nil, fmt.Errorf("error making chaffer: %v", err)
+	}
+
 	return &PublishHandler{
 		serverenv:             env,
 		transformer:           transformer,
 		config:                config,
 		database:              database.New(env.Database()),
-		tracker:               chaff.New(),
+		tracker:               chaffer,
 		tokenManager:          tm,
 		tokenAAD:              aadBytes,
 		authorizedAppProvider: env.AuthorizedAppProvider(),
@@ -444,3 +449,8 @@ func (h *PublishHandler) process(ctx context.Context, data *verifyapi.Publish, b
 		},
 	}
 }
+
+// chaffPushResponse takes a chaffing string, and builds a chaff response.
+func chaffPublishResponse(s string) interface{} {
+	return verifyapi.PublishResponse{Padding: s}
+}
diff --git a/pkg/observability/observability.go b/pkg/observability/observability.go
index d3be329..1184487 100644
--- a/pkg/observability/observability.go
+++ b/pkg/observability/observability.go
@@ -17,21 +17,41 @@ package observability
 
 import (
 	"context"
+	"database/sql"
 	"fmt"
 	"io"
 	"sync"
 
+	"contrib.go.opencensus.io/integrations/ocsql"
 	"go.opencensus.io/plugin/ocgrpc"
 	"go.opencensus.io/plugin/ochttp"
 	"go.opencensus.io/stats/view"
 )
 
+// OCSQLDriverName is the name of the SQL driver wrapped by OpenCensus
+// instrumentation code.
+const OCSQLDriverName = "ocsql"
+
+func defaultViews() []*view.View {
+	var ret []*view.View
+	ret = append(ret, ochttp.DefaultClientViews...)
+	ret = append(ret, ochttp.DefaultServerViews...)
+	ret = append(ret, ocgrpc.DefaultClientViews...)
+	ret = append(ret, ocgrpc.DefaultServerViews...)
+
+	for _, d := range sql.Drivers() {
+		if d == OCSQLDriverName {
+			ret = append(ret, ocsql.DefaultViews...)
+			break
+		}
+	}
+	return ret
+}
+
 var collectedViews = struct {
 	views []*view.View
 	sync.Mutex
-}{
-	views: append(append(append(append([]*view.View{}, ochttp.DefaultClientViews...), ochttp.DefaultServerViews...), ocgrpc.DefaultClientViews...), ocgrpc.DefaultServerViews...),
-}
+}{}
 
 // CollectViews collects all the OpenCensus views and register at a later time
 // when we setup the metric exporter.
@@ -53,7 +73,7 @@ func CollectViews(views ...*view.View) {
 func AllViews() []*view.View {
 	collectedViews.Lock()
 	defer collectedViews.Unlock()
-	return collectedViews.views
+	return append(collectedViews.views, defaultViews()...)
 }
 
 // Exporter defines the minimum shared functionality for an observability exporter
diff --git a/terraform/alerting/alerts.tf b/terraform/alerting/alerts.tf
new file mode 100644
index 0000000..f1910d7
--- /dev/null
+++ b/terraform/alerting/alerts.tf
@@ -0,0 +1,67 @@
+# Copyright 2020 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+locals {
+  slower_services = format("(%s)", join("|", sort([
+    "cleanup-export",
+    "cleanup-exposure",
+  ])))
+  slowest_services = format("(%s)", join("|", sort([
+    "export",
+  ])))
+}
+
+resource "google_monitoring_alert_policy" "LatencyTooHigh" {
+  combiner     = "OR"
+  display_name = "LatencyTooHigh"
+  conditions {
+    display_name = "p50 request latency"
+    condition_monitoring_query_language {
+      duration = "180s"
+      # NOTE: this is a bit complex because we want to have a single latency
+      # alert (not two alerts one for slow and one for normal), and we can only
+      # have one mql query in a conditions block.
+      query = <<-EOT
+      fetch
+      cloud_run_revision :: run.googleapis.com/request_latencies
+      | add
+      [type:
+      if(resource.service_name =~ '${local.slowest_services}', 'SLOWEST'
+      if(resource.service_name =~ '${local.slower_services}', 'SLOWER',
+      'NORMAL'
+      ))]
+      | align delta(1m)
+      | every 1m
+      | group_by [resource.service_name, type],
+      [val: percentile(value.request_latencies, 50)]
+      | condition
+          (type == 'SLOWEST' && val > 60000 'ms')
+        ||(type == 'SLOWER' && val > 20000 'ms')
+        ||(type == 'NORMAL' && val > 10000 'ms')
+      EOT
+      trigger {
+        count = 1
+      }
+    }
+  }
+  documentation {
+    content   = <<-EOT
+    Our latency is too high for one or more Cloud Run services!
+    EOT
+    mime_type = "text/markdown"
+  }
+  notification_channels = [
+    google_monitoring_notification_channel.email.id
+  ]
+}
diff --git a/terraform/alerting/main.tf b/terraform/alerting/main.tf
new file mode 100644
index 0000000..394520d
--- /dev/null
+++ b/terraform/alerting/main.tf
@@ -0,0 +1,29 @@
+# Copyright 2020 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+provider "google" {
+  project = var.project
+}
+
+provider "google-beta" {
+  project = var.project
+}
+
+resource "google_project_service" "services" {
+  for_each = toset([
+    "monitoring.googleapis.com",
+  ])
+  service            = each.value
+  disable_on_destroy = false
+}
diff --git a/terraform/alerting/notifications.tf b/terraform/alerting/notifications.tf
new file mode 100644
index 0000000..07c5f83
--- /dev/null
+++ b/terraform/alerting/notifications.tf
@@ -0,0 +1,22 @@
+# Copyright 2020 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+resource "google_monitoring_notification_channel" "email" {
+  provider     = google-beta
+  display_name = "Email Notification Channel"
+  type         = "email"
+  labels = {
+    email_address = var.notification-email
+  }
+}
diff --git a/terraform/alerting/variables.tf b/terraform/alerting/variables.tf
new file mode 100644
index 0000000..70a4737
--- /dev/null
+++ b/terraform/alerting/variables.tf
@@ -0,0 +1,39 @@
+# Copyright 2020 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+variable "project" {
+  type        = string
+  description = "GCP project for key server. Required."
+}
+
+variable "notification-email" {
+  type        = string
+  default     = "nobody@example.com"
+  description = "Email address for alerts to go to."
+}
+
+terraform {
+  required_version = ">= 0.13"
+
+  required_providers {
+    google = {
+      source  = "hashicorp/google"
+      version = "~> 3.46"
+    }
+    google-beta = {
+      source  = "hashicorp/google-beta"
+      version = "~> 3.46"
+    }
+  }
+}
diff --git a/terraform/vars.tf b/terraform/vars.tf
index 294c595..523080f 100644
--- a/terraform/vars.tf
+++ b/terraform/vars.tf
@@ -244,11 +244,11 @@ terraform {
     }
     local = {
       source  = "hashicorp/local"
-      version = "~> 1.4"
+      version = "~> 2.0"
     }
     null = {
       source  = "hashicorp/null"
-      version = "~> 2.1"
+      version = "~> 3.0"
     }
     random = {
       source  = "hashicorp/random"


Test patch:
diff --git a/internal/integration/integration_test.go b/internal/integration/integration_test.go
index 820a73b..ba6310f 100644
--- a/internal/integration/integration_test.go
+++ b/internal/integration/integration_test.go
@@ -34,6 +34,7 @@ import (
 	testutil "github.com/google/exposure-notifications-server/internal/utils"
 	verifyapi "github.com/google/exposure-notifications-server/pkg/api/v1"
 	"github.com/google/exposure-notifications-server/pkg/base64util"
+	"github.com/google/exposure-notifications-server/pkg/timeutils"
 	"github.com/google/exposure-notifications-server/pkg/util"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
@@ -80,6 +81,7 @@ func TestIntegration(t *testing.T) {
 			}
 
 			keys := util.GenerateExposureKeys(3, -1, false)
+			onsetTime := timeutils.UTCMidnight(timeutils.SubtractDays(time.Now().UTC(), 2))
 
 			// Publish 3 keys
 			payload := &verifyapi.Publish{
@@ -88,6 +90,7 @@ func TestIntegration(t *testing.T) {
 			}
 			jwtCfg.ExposureKeys = keys
 			jwtCfg.JWTWarp = tc.JWTWrap
+			jwtCfg.SymptomOnsetInterval = uint32(publishmodel.IntervalNumber(onsetTime))
 			verification, salt := testutil.IssueJWT(t, jwtCfg)
 			payload.VerificationPayload = verification
 			payload.HMACKey = salt
@@ -418,8 +421,9 @@ func getExposures(db *database.DB, criteria publishdb.IterateExposuresCriteria)
 func exportedKeysFrom(tb testing.TB, keys []verifyapi.ExposureKey) []*export.TemporaryExposureKey {
 	s := make([]*export.TemporaryExposureKey, len(keys))
 	sort.Slice(keys, func(i, j int) bool {
-		return keys[i].IntervalNumber >= keys[j].IntervalNumber
+		return keys[i].IntervalNumber < keys[j].IntervalNumber
 	})
+	daysSince := int32(-1)
 	for i, key := range keys {
 		decoded, err := base64util.DecodeString(key.Key)
 		if err != nil {
@@ -432,8 +436,9 @@ func exportedKeysFrom(tb testing.TB, keys []verifyapi.ExposureKey) []*export.Tem
 			RollingStartIntervalNumber: proto.Int32(key.IntervalNumber),
 			ReportType:                 export.TemporaryExposureKey_CONFIRMED_TEST.Enum(),
 			// Keys are generated 1 day ago and then -1 day for each additional.
-			DaysSinceOnsetOfSymptoms: proto.Int32(int32(3 - i)),
+			DaysSinceOnsetOfSymptoms: proto.Int32(daysSince),
 		}
+		daysSince++
 	}
 
 	sortTEKs(s)
diff --git a/internal/publish/database/exposure_test.go b/internal/publish/database/exposure_test.go
index 5cdac02..c01b31d 100644
--- a/internal/publish/database/exposure_test.go
+++ b/internal/publish/database/exposure_test.go
@@ -577,6 +577,9 @@ func TestReviseExposures(t *testing.T) {
 			t.Errorf("expected %d to be %d: %#v", got, want, reviseResp)
 		}
 
+		// Change report, type, attempt to revise again.
+		exposure.ReportType = verifyapi.ReportTypeNegative
+
 		// Attempt to revise the same key again - this should fail.
 		if _, err := pubDB.InsertAndReviseExposures(ctx, &InsertAndReviseExposuresRequest{
 			Incoming:     []*model.Exposure{exposure},
diff --git a/internal/publish/model/exposure_model_test.go b/internal/publish/model/exposure_model_test.go
index 3e6e920..46a6252 100644
--- a/internal/publish/model/exposure_model_test.go
+++ b/internal/publish/model/exposure_model_test.go
@@ -1536,6 +1536,42 @@ func TestExposureReview(t *testing.T) {
 			needsRevision: true,
 			err:           "",
 		},
+		{
+			name: "double_revise",
+			previous: &Exposure{
+				ReportType:            verifyapi.ReportTypeClinical,
+				LocalProvenance:       true,
+				HealthAuthorityID:     int64Ptr(2),
+				Regions:               []string{"US", "CA"},
+				TransmissionRisk:      4,
+				CreatedAt:             createdAt,
+				DaysSinceSymptomOnset: int32Ptr(-1),
+				RevisedAt:             &createdAt,
+				RevisedReportType:     stringPtr(verifyapi.ReportTypeConfirmed),
+			},
+			incoming: &Exposure{
+				ReportType:            verifyapi.ReportTypeConfirmed,
+				HealthAuthorityID:     int64Ptr(3),
+				Regions:               []string{"MX"},
+				TransmissionRisk:      5,
+				CreatedAt:             revisedAt,
+				DaysSinceSymptomOnset: int32Ptr(0),
+			},
+			want: &Exposure{
+				ReportType:                   verifyapi.ReportTypeClinical,
+				LocalProvenance:              true,
+				HealthAuthorityID:            int64Ptr(3),
+				Regions:                      []string{"US", "CA", "MX"},
+				TransmissionRisk:             4,
+				CreatedAt:                    createdAt,
+				DaysSinceSymptomOnset:        int32Ptr(-1),
+				RevisedReportType:            stringPtr(verifyapi.ReportTypeConfirmed),
+				RevisedAt:                    &revisedAt,
+				RevisedDaysSinceSymptomOnset: int32Ptr(0),
+				RevisedTransmissionRisk:      intPtr(5),
+			},
+			needsRevision: false,
+		},
 	}
 
 	for _, tc := range cases {
diff --git a/internal/publish/publish_test.go b/internal/publish/publish_test.go
index 8b6a6f5..0f80773 100644
--- a/internal/publish/publish_test.go
+++ b/internal/publish/publish_test.go
@@ -862,9 +862,9 @@ func TestKeyRevision(t *testing.T) {
 					t.Fatal(err)
 				}
 
-				// Revise them
+				// Revise them to negative - test will try to revise to positive.
 				for _, key := range incoming {
-					key.ReportType = verifyapi.ReportTypeConfirmed
+					key.ReportType = verifyapi.ReportTypeNegative
 				}
 				if _, err := pubDB.InsertAndReviseExposures(ctx, &pubdb.InsertAndReviseExposuresRequest{
 					Incoming: incoming,
diff --git a/scripts/ci-e2e-test.sh b/scripts/ci-e2e-test.sh
index 48d0e97..835af73 100755
--- a/scripts/ci-e2e-test.sh
+++ b/scripts/ci-e2e-test.sh
@@ -43,6 +43,10 @@ function incremental() {
   fi
 
   ${ROOT}/scripts/ci-terraform.sh init
+
+  # Running "${ROOT}/scripts/deploy" would fail when a service doesn't not exist,
+  # e.g. a new cloud run service introduced, due to `--no-traffic` not allowed for
+  # newly created service. So let terraform create the service first.
   pushd "${ROOT}/terraform-e2e-ci" >/dev/null 2>&1
   # Force terraform to redo build and migrate. This step might fail when the previous e2e run failed,
   # as build or migrate was destroyed but not restalled in terraform state.
@@ -61,6 +65,10 @@ function incremental() {
   export E2E_DB_PASSWORD="secret://${E2E_DB_PASSWORD}"
   export E2E_DB_SSLMODE=disable
 
+  ${ROOT}/scripts/build
+  ${ROOT}/scripts/deploy
+  ${ROOT}/scripts/promote
+
   run_e2e_test
 }