damccorm commented on code in PR #25994:
URL: https://github.com/apache/beam/pull/25994#discussion_r1164248286


##########
playground/backend/new_scio_project.sh:
##########
@@ -15,6 +15,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-{ printf scio\\nscio\\n; yes; } | sbt new spotify/scio-template.g8
+if [ -d /opt/sbt-template/scio ]; then

Review Comment:
   Why do we need this set of changes?



##########
playground/terraform/infrastructure/private_dns/googleapis_com.tf:
##########
@@ -0,0 +1,52 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you 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_dns_managed_zone" "private-zone-private-googleapis" {
+  project     = var.project_id 
+
+  name        = "${var.network_name}googleapis-com"
+  dns_name    = "googleapis.com."
+  description = "Private GoogleApi Zone"
+  
+  visibility = "private"
+
+  private_visibility_config {
+    networks {
+      network_url = var.network_id
+    }
+  }
+}
+
+resource "google_dns_record_set" "a-private-googleapis" {
+  name = "private.googleapis.com."
+  type = "A"
+  ttl  = 300
+
+  managed_zone = google_dns_managed_zone.private-zone-private-googleapis.name
+
+  rrdatas = ["199.36.153.8", "199.36.153.9", "199.36.153.10", "199.36.153.11"]

Review Comment:
   Same question, where do these come from?



##########
playground/infrastructure/helm-playground/templates/autoscaling-go.yaml:
##########
@@ -12,15 +12,27 @@
 # 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.
-apiVersion: autoscaling/v1
+apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
 metadata:
   name: playground-go
 spec:
-  maxReplicas: 10
-  minReplicas: 1
+  maxReplicas: {{ .Values.autoscaling.runners.maxReplicas }}
+  minReplicas: {{ .Values.autoscaling.runners.minReplicas }}
   scaleTargetRef:
     apiVersion: apps/v1
     kind: Deployment
     name: playground-go
-  targetCPUUtilizationPercentage: 90
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 95

Review Comment:
   Same comment applies to other files



##########
playground/infrastructure/helm-playground/templates/autoscaling-go.yaml:
##########
@@ -12,15 +12,27 @@
 # 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.
-apiVersion: autoscaling/v1
+apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
 metadata:
   name: playground-go
 spec:
-  maxReplicas: 10
-  minReplicas: 1
+  maxReplicas: {{ .Values.autoscaling.runners.maxReplicas }}
+  minReplicas: {{ .Values.autoscaling.runners.minReplicas }}
   scaleTargetRef:
     apiVersion: apps/v1
     kind: Deployment
     name: playground-go
-  targetCPUUtilizationPercentage: 90
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 95

Review Comment:
   Should these averageUtilization targets be congfigurable values (like 
max/min replicas?)



##########
playground/terraform/infrastructure/firewall/firewall.tf:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You 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_compute_firewall" "playground-firewall-deny-egress" {
+  name    = "${var.network_name}-deny-egress"
+  network = var.network_name
+  direction     = "EGRESS"
+  priority      = 1001
+  deny {
+    protocol      = "all"
+  }
+  destination_ranges = ["0.0.0.0/0"]
+  target_tags = ["beam-playground"]
+}
+
+resource "google_compute_firewall" "playground-firewall-allow-controlplane" {
+  name    = "${var.network_name}-allow-controlplane"
+  network = var.network_name
+  direction     = "EGRESS"
+  priority      = 1000
+  allow {
+    protocol      = "all"
+  }
+  destination_ranges = [var.gke_controlplane_cidr]
+  target_tags = ["beam-playground"]
+}
+
+resource "google_compute_firewall" "playground-firewall-allow-dns" {
+  name    = "${var.network_name}-allow-dns"
+  network = var.network_name
+  direction     = "EGRESS"
+  priority      = 1000
+  allow {
+    protocol = "tcp"
+    ports    = ["53"]
+
+  }
+  allow {
+    protocol = "udp"
+    ports = ["53"]
+  }
+  destination_ranges = ["0.0.0.0/0"]
+  target_tags = ["beam-playground"]
+}
+
+resource "google_compute_firewall" "playground-firewall-allow-privateapi" {
+  name    = "${var.network_name}-allow-privateapi"
+  network = var.network_name
+  direction     = "EGRESS"
+  priority      = 1000
+  allow {
+    protocol = "all"
+  }
+
+  destination_ranges = ["199.36.153.8/30"]

Review Comment:
   Where is this destination range coming from?



##########
playground/terraform/infrastructure/memorystore/variables.tf:
##########
@@ -48,7 +48,7 @@ variable "tier" {
 
 variable "replicas_mode" {
   description = "If enabled read endpoint will be provided and the instance 
can scale up and down the number of replicas"
-  default     = "READ_REPLICAS_ENABLED"
+  default     = "READ_REPLICAS_DISABLED"

Review Comment:
   Why did we change our default here?



##########
playground/terraform/infrastructure/appengine/variables.tf:
##########
@@ -36,3 +36,13 @@ variable "location_id_us" {
   description = "Location of App"
   default = "us-central"
 }
+
+variable "location_id_eu" {
+  description = "Location of App"
+  default = "europe-west"
+}
+
+variable "feature_flag" {

Review Comment:
   Could you add a description (and maybe a more descriptive name)? What is 
this a feature flag for?



##########
playground/terraform/README.md:
##########
@@ -53,29 +54,37 @@ Ensure that the account has at least following privileges:
 * [Docker](https://docs.docker.com/engine/install/)
 * [Terraform](https://www.terraform.io/downloads)
 * [gcloud CLI](https://cloud.google.com/sdk/docs/install-sdk)
+* [Kubectl 
authentication](https://cloud.google.com/blog/products/containers-kubernetes/kubectl-auth-changes-in-gke)
 
 6. Apache Beam Git repository cloned locally
 
 # Prepare deployment configuration:
-Playground uses `terraform.tfvars` located in 
`playground/terraform/environment/environment_name` to define variables 
specific to an environment (e.g., prod, test, staging).<br>
-1. Create a folder (further referred as `environment_name`) to define a new 
environment and place configuration files into it:
+Playground uses `terraform.tfvars` located in 
`playground/terraform/environment/<environment_name>` to define variables 
specific to an environment (e.g., prod, test, staging).<br>
+1. Create a folder (further referred as <environment_name>) to define a new 
environment and place configuration files into it:
 
 * `terraform.tfvars` environment variables:
 ```
-project_id           = "project_id"          #GCP Project ID
-network_name         = "network_name"        #GCP VPC Network Name for 
Playground deployment
-gke_name             = "playground-backend"  #Playground GKE Cluster name
-region               = "us-east1"            #Set the deployment region
-location             = "us-east1-b"          #Select the deployment location 
from available in the specified region
-state_bucket         = "bucket_name"         #GCS bucket name for Beam 
Playground temp files
-redis_name           = "playground_redis"    #Choose the name for redis 
instance
-min_count            = 2                     #Min node count for GKE cluster
-max_count            = 6                     #Max node count for GKE cluster
+project_id           = "project_id"          # GCP Project ID
+network_name         = "playground-network"        # GCP VPC Network Name for 
Playground deployment

Review Comment:
   Nit: Could we keep spacing consistent here so that `#` characters are 
aligned?



##########
playground/terraform/infrastructure/gke/variables.tf:
##########
@@ -54,4 +54,9 @@ variable "min_count" {
 variable "max_count" {
   description = "Max cluster node count"
   default     = 6
-}
\ No newline at end of file
+}
+variable "control_plane_cidr" {
+  description = "CIDR block for GKE controlplane"
+  default     = "10.129.0.0/28"

Review Comment:
   Same question, where is this range coming from?



##########
playground/terraform/applications/backend/backend-go/main.tf:
##########
@@ -1,73 +0,0 @@
-#
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements.  See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership.  The ASF licenses this file
-# to you 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_app_engine_flexible_app_version" "backend_app_go" {

Review Comment:
   I'm a little bit confused - why do we no longer need these `.tf` files? 
Could you give me some more context on this please?



##########
playground/terraform/infrastructure/private_dns/zones.tf:
##########
@@ -0,0 +1,63 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you 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_dns_managed_zone" "private-zone-apis" {
+
+  for_each = var.private_zones
+
+  project     = var.project_id 
+
+  name        = "${var.network_name}-${replace(each.key,".","-")}"
+  dns_name    = format("%s%s",each.key,".")
+  description = "Private ${each.key} Zone"
+  
+  visibility = "private"
+
+  private_visibility_config {
+    networks {
+      network_url = var.network_id
+    }
+  }
+}
+
+resource "google_dns_record_set" "a-private-zone" {
+
+  for_each = google_dns_managed_zone.private-zone-apis
+
+  name = each.value.dns_name
+  type = "A"
+  ttl  = 300
+
+  managed_zone = each.value.name
+
+  rrdatas = ["199.36.153.8", "199.36.153.9", "199.36.153.10", "199.36.153.11"]

Review Comment:
   Could we abstract these out into a variable since we're using it multiple 
places?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to