Repository: osbuild/osbuild-composer
Pull request #1380 (2021-04-27 19:28:22)

GCP: Fix panic while parsing a specific build job log
The `cloudbuildResourcesFromBuildLog()` function from the internal GCP
package could cause panic while parsing Build job log which failed early
and didn't create any Compute Engine resources. The function relied on
the `Regexp.FindStringSubmatch()` method to always return a match
while being used on the build log. Accessing a member of a nil slice
would cause a panic in `osbuild-worker`, such as:

```
Stack trace of thread 185316:
#0  0x0000564e5393b5e1 runtime.raise (osbuild-worker)
#1  0x0000564e5391fa1e runtime.sigfwdgo (osbuild-worker)
#2  0x0000564e5391e354 runtime.sigtrampgo (osbuild-worker)
#3  0x0000564e5393b953 runtime.sigtramp (osbuild-worker)
#4  0x00007f37e98e3b20 __restore_rt (libpthread.so.0)
#5  0x0000564e5393b5e1 runtime.raise (osbuild-worker)
#6  0x0000564e5391f5ea runtime.crash (osbuild-worker)
#7  0x0000564e53909306 runtime.fatalpanic (osbuild-worker)
#8  0x0000564e53908ca1 runtime.gopanic (osbuild-worker)
#9  0x0000564e53906b65 runtime.goPanicIndex (osbuild-worker)
#10 0x0000564e5420b36e github.com/osbuild/osbuild-composer/internal/cloud/gcp.cloudbuildResourcesFromBuildLog (osbuild-worker)
#11 0x0000564e54209ebb github.com/osbuild/osbuild-composer/internal/cloud/gcp.(*GCP).CloudbuildBuildCleanup (osbuild-worker)
#12 0x0000564e54b05a9b main.(*OSBuildJobImpl).Run (osbuild-worker)
#13 0x0000564e54b08854 main.main (osbuild-worker)
#14 0x0000564e5390b722 runtime.main (osbuild-worker)
#15 0x0000564e53939a11 runtime.goexit (osbuild-worker)
```

Add a unit test testing this scenario.

Make the `cloudbuildResourcesFromBuildLog()` function more robust and
not blindly expect to find matches in the build log. As a result the
`cloudbuildBuildResources` struct instance returned from the function
may be empty. Subsequently make sure that the `CloudbuildBuildCleanup()`
method handles an empty `cloudbuildBuildResources` instance correctly.
Specifically the `storageCacheDir.bucket` may be an empty string and
thus won't exist. Ensure that this does not result in infinite loop by
checking for `storage.ErrBucketNotExist` while iterating the bucket
objects.

Signed-off-by: Tomas Hozza <thozza@redhat.com>


This pull request includes:

- [x] adequate testing for the new functionality or fixed issue
- [ ] adequate documentation informing people about the change such as
  - [ ] create a file in [news/unreleased](https://github.com/osbuild/osbuild-composer/tree/main/docs/news/unreleased) directory if this change should be mentioned in the release news
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

For user-visible changes, "adequate documentation" is an entry describing the
change for users in docs/news. Please refer to docs/news/README.md for details.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`
-->



Patch:
diff --git a/distribution/osbuild-composer-clouddot-template.yml b/distribution/osbuild-composer-clouddot-template.yml
new file mode 100644
index 000000000..3610290b5
--- /dev/null
+++ b/distribution/osbuild-composer-clouddot-template.yml
@@ -0,0 +1,79 @@
+apiVersion: v1
+kind: Template
+labels:
+  app: osbuild-composer
+  template: osbuild-composer
+metadata:
+  annotations:
+    description: OCP template for osbuild-composer in cloud.redhat.com
+  name: osbuild-composer
+objects:
+
+- apiVersion: apps/v1
+  kind: Deployment
+  metadata:
+    labels:
+      service: osbuild-composer
+    name: osbuild-composer
+  spec:
+    replicas: 1
+    selector:
+      matchLabels:
+        name: osbuild-composer
+    strategy:
+      # Update pods 1 at a time
+      type: RollingUpdate
+      rollingUpdate:
+        # Create at most 1 extra pod over .spec.replicas
+        maxSurge: 1
+        # At all times there should be .spec.replicas available
+        maxUnavailable: 0
+    template:
+      metadata:
+        labels:
+          name: osbuild-composer
+      spec:
+        containers:
+        - image: "${IMAGE_NAME}:${IMAGE_TAG}"
+          name: osbuild-composer
+          ports:
+          - name: api
+            containerPort: 443
+            protocol: TCP
+          - name: workers
+            containerPort: 8700
+          volumeMounts:
+          - name: composer-config
+            mountPath: "/etc/osbuild-composer"
+            readOnly: true
+        volumes:
+        - name: composer-config
+          secret:
+            secretName: composer-config
+
+- apiVersion: v1
+  kind: Service
+  metadata:
+    labels:
+      service: osbuild-composer
+    name: osbuild-composer
+  spec:
+    ports:
+      - name: composer-api
+        protocol: TCP
+        port: ${{API_LISTENER_PORT}}
+        targetPort: 443
+    selector:
+      name: osbuild-composer
+
+parameters:
+  - description: osbuild-composer image name
+    name: IMAGE_NAME
+    value: quay.io/cloudservices/osbuild-composer
+    required: true
+  - description: image-builder image tag
+    name: IMAGE_TAG
+    required: true
+  - description: api listener port
+    name: API_LISTENER_PORT
+    value: "8080"
diff --git a/internal/cloud/gcp/cloudbuild.go b/internal/cloud/gcp/cloudbuild.go
index 075a15b9e..fd878aaa0 100644
--- a/internal/cloud/gcp/cloudbuild.go
+++ b/internal/cloud/gcp/cloudbuild.go
@@ -148,7 +148,7 @@ func (g *GCP) CloudbuildBuildCleanup(ctx context.Context, buildID string) ([]str
 	objects := bucket.Objects(ctx, &storage.Query{Prefix: resources.storageCacheDir.dir})
 	for {
 		objAttrs, err := objects.Next()
-		if err == iterator.Done {
+		if err == iterator.Done || err == storage.ErrBucketNotExist {
 			break
 		}
 		if err != nil {
@@ -178,7 +178,9 @@ func cloudbuildResourcesFromBuildLog(buildLog string) (*cloudbuildBuildResources
 		return &resources, err
 	}
 	zoneMatch := zoneRe.FindStringSubmatch(buildLog)
-	resources.zone = zoneMatch[1]
+	if zoneMatch != nil {
+		resources.zone = zoneMatch[1]
+	}
 
 	// extract Storage cache directory
 	// [inflate]: 2021-03-12T13:13:10Z Workflow GCSPath: gs://ascendant-braid-303513-daisy-bkt-us-central1/gce-image-import-2021-03-12T13:13:08Z-btgtd
@@ -187,8 +189,10 @@ func cloudbuildResourcesFromBuildLog(buildLog string) (*cloudbuildBuildResources
 		return &resources, err
 	}
 	cacheDirMatch := cacheDirRe.FindStringSubmatch(buildLog)
-	resources.storageCacheDir.bucket = cacheDirMatch[1]
-	resources.storageCacheDir.dir = cacheDirMatch[2]
+	if cacheDirMatch != nil {
+		resources.storageCacheDir.bucket = cacheDirMatch[1]
+		resources.storageCacheDir.dir = cacheDirMatch[2]
+	}
 
 	// extract Compute disks
 	// [inflate.setup-disks]: 2021-03-12T13:13:11Z CreateDisks: Creating disk "disk-importer-inflate-7366y".
diff --git a/internal/cloud/gcp/compute.go b/internal/cloud/gcp/compute.go
index 4158dd99f..e631ba646 100644
--- a/internal/cloud/gcp/compute.go
+++ b/internal/cloud/gcp/compute.go
@@ -2,7 +2,10 @@ package gcp
 
 import (
 	"context"
+	"crypto/rand"
 	"fmt"
+	"math/big"
+	"strings"
 	"time"
 
 	cloudbuild "cloud.google.com/go/cloudbuild/apiv1"
@@ -60,8 +63,43 @@ func (g *GCP) ComputeImageImport(ctx context.Context, bucket, object, imageName,
 		"-timeout=7000s",
 		"-client_id=api",
 	}
+
 	if region != "" {
 		buildStepArgs = append(buildStepArgs, fmt.Sprintf("-storage_location=%s", region))
+
+		// Set the region to be used by the daisy workflow when creating resources
+		// If not specified, the workflow seems to always default to us-central1.
+
+		// The Region passed as the argument is a Google Storage Region, which can be a multi or dual region.
+		// Multi and Dual regions don't work with GCE API, therefore we need to get the list of GCE regions
+		// that they map to. If the passed Region is not a multi or dual Region, then the returned slice contains
+		// only the Region passed as an argument.
+		gceRegions, err := g.storageRegionToComputeRegions(ctx, region)
+		if err != nil {
+			return nil, fmt.Errorf("failed to translate Google Storage Region to GCE Region: %v", err)
+		}
+		// Pick a random GCE Region to be used by the image import workflow
+		gceRegionIndex, err := rand.Int(rand.Reader, big.NewInt(int64(len(gceRegions))))
+		if err != nil {
+			return nil, fmt.Errorf("failed to pick random GCE Region: %v", err)
+		}
+		// The expecation is that Google won't have more regions listed for multi/dual
+		// regions than what can potentially fit into int32.
+		gceRegion := gceRegions[int(gceRegionIndex.Int64())]
+
+		availableZones, err := g.computeZonesInRegion(ctx, gceRegion)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get available GCE Zones within Region '%s': %v", region, err)
+		}
+		// Pick random zone from the list
+		gceZoneIndex, err := rand.Int(rand.Reader, big.NewInt(int64(len(availableZones))))
+		if err != nil {
+			return nil, fmt.Errorf("failed to pick random GCE Zone: %v", err)
+		}
+		// The expecation is that Google won't have more zones in a region than what can potentially fit into int32
+		zone := availableZones[int(gceZoneIndex.Int64())]
+
+		buildStepArgs = append(buildStepArgs, fmt.Sprintf("-zone=%s", zone))
 	}
 	if os != "" {
 		buildStepArgs = append(buildStepArgs, fmt.Sprintf("-os=%s", os))
@@ -288,3 +326,109 @@ func (g *GCP) ComputeDiskDelete(ctx context.Context, zone, disk string) error {
 
 	return err
 }
+
+// storageRegionToComputeRegion translates a Google Storage Region to GCE Region.
+// This is useful mainly for multi and dual Storage Regions. For each valid multi
+// or dual Region name, a slice with relevant GCE Regions is returned. If the
+// Region provided as an argument is not multi or dual Region, a slice with the
+// provided argument as the only item is returned.
+//
+// In general, Storage Regions correspond to the Compute Engine Regions. However,
+// Storage allows also Multi and Dual regions, which must be mapped to GCE Regions,
+// since these can not be used with GCE API calls.
+//
+// Uses:
+//  - Compute Engine API
+func (g *GCP) storageRegionToComputeRegions(ctx context.Context, region string) ([]string, error) {
+	regionLower := strings.ToLower(region)
+
+	// Handle Dual-Regions
+	// https://cloud.google.com/storage/docs/locations#location-dr
+	if regionLower == "asia1" {
+		return []string{"asia-northeast1", "asia-northeast2"}, nil
+	} else if regionLower == "eur4" {
+		return []string{"europe-north1", "europe-west4"}, nil
+	} else if regionLower == "nam4" {
+		return []string{"us-central1", "us-east1"}, nil
+	}
+
+	// Handle Regular Region
+	if regionLower != "asia" && regionLower != "eu" && regionLower != "us" {
+		// Just return a slice with the region, which we got as
+		return []string{regionLower}, nil
+	}
+
+	// Handle Multi-Regions
+	// https://cloud.google.com/storage/docs/locations#location-mr
+	computeService, err := compute.NewService(ctx, option.WithCredentials(g.creds))
+	if err != nil {
+		return nil, fmt.Errorf("failed to get Compute Engine client: %v", err)
+	}
+
+	regionObjList, err := computeService.Regions.List(g.creds.ProjectID).Context(ctx).Do()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get available Compute Engine Regions: %v", err)
+	}
+
+	regionsMap := make(map[string][]string)
+	for _, regionObj := range regionObjList.Items {
+		regionPrefix := strings.Split(regionObj.Name, "-")[0]
+		regionsMap[regionPrefix] = append(regionsMap[regionPrefix], regionObj.Name)
+	}
+
+	switch regionLower {
+	case "asia", "us":
+		return regionsMap[regionLower], nil
+	case "eu":
+		var euRegions []string
+		for _, euRegion := range regionsMap["europe"] {
+			// "europe-west2" (London) and "europe-west6" (Zurich) are excluded
+			// see https://cloud.google.com/storage/docs/locations#location-mr
+			if euRegion != "europe-west2" && euRegion != "europe-west6" {
+				euRegions = append(euRegions, euRegion)
+			}
+		}
+		return euRegions, nil
+	default:
+		// This case should never happen, since the "default" case is handled above by
+		// if regionLower != "asia" && regionLower != "eu" && regionLower != "us"
+		return nil, fmt.Errorf("failed to translate Google Storage Region '%s' to Compute Engine Region", regionLower)
+	}
+}
+
+// computeZonesInRegion returns list of zones within the given GCE Region, which are "UP".
+//
+// Uses:
+//  - Compute Engine API
+func (g *GCP) computeZonesInRegion(ctx context.Context, region string) ([]string, error) {
+	var zones []string
+
+	computeService, err := compute.NewService(ctx, option.WithCredentials(g.creds))
+	if err != nil {
+		return nil, fmt.Errorf("failed to get Compute Engine client: %v", err)
+	}
+
+	// Get available zones in the given region
+	regionObj, err := computeService.Regions.Get(g.creds.ProjectID, region).Context(ctx).Do()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get information about Compute Engine region '%s': %v", region, err)
+	}
+
+	for _, zoneURL := range regionObj.Zones {
+		// zone URL example - "https://www.googleapis.com/compute/v1/projects/<PROJECT_ID>/zones/us-central1-a"
+		zoneNameSs := strings.Split(zoneURL, "/")
+		zoneName := zoneNameSs[len(zoneNameSs)-1]
+
+		zoneObj, err := computeService.Zones.Get(g.creds.ProjectID, zoneName).Context(ctx).Do()
+		if err != nil {
+			return nil, fmt.Errorf("failed to get information about Compute Engine zone '%s': %v", zoneName, err)
+		}
+
+		// Make sure to return only Zones, which can be used
+		if zoneObj.Status == "UP" {
+			zones = append(zones, zoneName)
+		}
+	}
+
+	return zones, nil
+}


Test patch:
diff --git a/internal/cloud/gcp/cloudbuild_test.go b/internal/cloud/gcp/cloudbuild_test.go
index 6cb842708..d8f3a0267 100644
--- a/internal/cloud/gcp/cloudbuild_test.go
+++ b/internal/cloud/gcp/cloudbuild_test.go
@@ -407,6 +407,63 @@ ERROR: build step 0 "gcr.io/compute-image-tools/gce_vm_image_import:release" fai
 				},
 			},
 		},
+		{
+			buildLog: `starting build "21eb22bb-b92e-41f7-972d-35e75dae2a2c"
+
+FETCHSOURCE
+BUILD
+Pulling image: gcr.io/compute-image-tools/gce_vm_image_import:release
+release: Pulling from compute-image-tools/gce_vm_image_import
+a352db2f02b6: Pulling fs layer
+8e0ed4351c49: Pulling fs layer
+7ef2d30124da: Pulling fs layer
+7558c9498dac: Pulling fs layer
+ac1adc2a272f: Pulling fs layer
+1bfe08dab915: Pulling fs layer
+d7a35a584f97: Pulling fs layer
+14ef19cde991: Pulling fs layer
+e3a2159de935: Pulling fs layer
+7558c9498dac: Waiting
+ac1adc2a272f: Waiting
+1bfe08dab915: Waiting
+d7a35a584f97: Waiting
+14ef19cde991: Waiting
+e3a2159de935: Waiting
+7ef2d30124da: Verifying Checksum
+7ef2d30124da: Download complete
+7558c9498dac: Verifying Checksum
+7558c9498dac: Download complete
+ac1adc2a272f: Verifying Checksum
+ac1adc2a272f: Download complete
+a352db2f02b6: Verifying Checksum
+a352db2f02b6: Download complete
+8e0ed4351c49: Verifying Checksum
+8e0ed4351c49: Download complete
+14ef19cde991: Verifying Checksum
+14ef19cde991: Download complete
+1bfe08dab915: Verifying Checksum
+1bfe08dab915: Download complete
+e3a2159de935: Verifying Checksum
+e3a2159de935: Download complete
+d7a35a584f97: Verifying Checksum
+d7a35a584f97: Download complete
+a352db2f02b6: Pull complete
+8e0ed4351c49: Pull complete
+7ef2d30124da: Pull complete
+7558c9498dac: Pull complete
+ac1adc2a272f: Pull complete
+1bfe08dab915: Pull complete
+d7a35a584f97: Pull complete
+14ef19cde991: Pull complete
+e3a2159de935: Pull complete
+Digest: sha256:63ab233c04139087154f27797efbebc9e55302f465ffb56e2dce34c2b5bf5d8a
+Status: Downloaded newer image for gcr.io/compute-image-tools/gce_vm_image_import:release
+gcr.io/compute-image-tools/gce_vm_image_import:release
+[import-image]: 2021-04-27T08:08:40Z The resource 'image-8c27cb332db33890146b290a3989198d829ae456dabf96e5a4461147' already exists. Please pick an image name that isn't already used.
+ERROR
+ERROR: build step 0 "gcr.io/compute-image-tools/gce_vm_image_import:release" failed: step exited with non-zero status: 1`,
+			resources: cloudbuildBuildResources{},
+		},
 	}
 
 	for i, tc := range testCases {