feat: add ExternalReference (selfLink, resourceRef) with GCP update/d…

…eletion uniqueness guardrail

apis/cloudbuild/v1alpha1/workerpool_types.go

@@ -68,6 +68,11 @@ type CloudBuildWorkerPoolStatus struct {
 	// +optional
 	ObservedGeneration *int64 `json:"observedGeneration,omitempty"`
 
+	/* The GCP URL to the cloudbuild workerpool resource.
+	The cloudbuild workerpool does not have selfLink. */
+	// +optional
+	ExternalRef *string `json:"externalRef,omitempty"`
+
 	/* ObservedState is the state of the resource as most recently observed in GCP. */
 	// +optional
 	ObservedState *CloudBuildWorkerPoolObservedState `json:"observedState,omitempty"`

config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_cloudbuildworkerpools.cloudbuild.cnrm.cloud.google.com.yaml

@@ -164,6 +164,10 @@ spec:
                       type: string
                   type: object
                 type: array
+              externalRef:
+                description: The GCP URL to the cloudbuild workerpool resource. The
+                  cloudbuild workerpool does not have selfLink.
+                type: string
               observedGeneration:
                 description: ObservedGeneration is the generation of the resource
                   that was most recently observed by the Config Connector controller.

go.mod

@@ -9,6 +9,7 @@ replace github.com/GoogleCloudPlatform/k8s-config-connector/mockgcp => ./mockgcp
 require (
 	cloud.google.com/go/apikeys v1.1.5
 	cloud.google.com/go/cloudbuild v1.16.1
+	cloud.google.com/go/compute v1.25.1
 	cloud.google.com/go/monitoring v1.19.0
 	cloud.google.com/go/profiler v0.1.0
 	cloud.google.com/go/resourcemanager v1.9.7
@@ -75,7 +76,6 @@ require (
 	cloud.google.com/go/auth v0.3.0 // indirect
 	cloud.google.com/go/auth/oauth2adapt v0.2.2 // indirect
 	cloud.google.com/go/bigtable v1.19.0 // indirect
-	cloud.google.com/go/compute v1.25.1 // indirect
 	cloud.google.com/go/compute/metadata v0.3.0 // indirect
 	cloud.google.com/go/iam v1.1.7 // indirect
 	cloud.google.com/go/longrunning v0.5.6 // indirect

pkg/controller/direct/cloudbuild/externalresource.go

@@ -0,0 +1,51 @@
+/*
+Copyright 2024.
+
+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.
+*/
+
+package cloudbuild
+
+import (
+	"strings"
+
+	cloudbuildpb "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb"
+	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/externalresource"
+)
+
+func NewResourceRef(gcpObj *cloudbuildpb.WorkerPool) *ResourceRef {
+	return &ResourceRef{
+		externalresource.New("https://cloudbuild.googleapis.com/v1/", gcpObj),
+	}
+}
+
+type ResourceRef struct {
+	*externalresource.ExternalResourceReference
+}
+
+func (e *ResourceRef) GetResourceID() string {
+	segments := strings.Split(*e.Get(), "/workerPools/")
+	return segments[1]
+}
+
+func (e *ResourceRef) GetLocation() string {
+	segments := strings.Split(*e.Get(), "/locations/")
+	segment := strings.Split(segments[1], "/workerPools/")
+	return segment[0]
+}
+
+func (e *ResourceRef) GetProject() string {
+	segments := strings.Split(*e.Get(), "/projects/")
+	segment := strings.Split(segments[1], "/locations/")
+	return segment[0]
+}

pkg/controller/direct/cloudbuild/workerpool_controller.go

@@ -197,10 +197,14 @@ func (a *Adapter) Create(ctx context.Context, u *unstructured.Unstructured) erro
 	}
 	status.ObservedState.CreateTime = ToOpenAPIDateTime(created.GetCreateTime())
 	status.ObservedState.UpdateTime = ToOpenAPIDateTime(created.GetUpdateTime())
+	status.ExternalRef = NewResourceRef(created).Get()
 	return setStatus(u, status)
 }
 
 func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) error {
+	if err := a.ValidateExternalResource(); err != nil {
+		return err
+	}
 
 	updateMask := &fieldmaskpb.FieldMask{}
 
@@ -283,6 +287,8 @@ func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) erro
 	}
 	status.ObservedState.CreateTime = ToOpenAPIDateTime(updated.GetCreateTime())
 	status.ObservedState.UpdateTime = ToOpenAPIDateTime(updated.GetUpdateTime())
+	// This value should not be updated. Just in case.
+	status.ExternalRef = NewResourceRef(updated).Get()
 	return setStatus(u, status)
 }
 
@@ -292,9 +298,10 @@ func (a *Adapter) Export(ctx context.Context) (*unstructured.Unstructured, error
 
 // Delete implements the Adapter interface.
 func (a *Adapter) Delete(ctx context.Context) (bool, error) {
-	if a.resourceID == "" {
-		return false, nil
+	if err := a.ValidateExternalResource(); err != nil {
+		return false, err
 	}
+
 	req := &cloudbuildpb.DeleteWorkerPoolRequest{Name: a.fullyQualifiedName(), AllowMissing: true}
 	op, err := a.gcpClient.DeleteWorkerPool(ctx, req)
 	if err != nil {
@@ -313,6 +320,38 @@ func (a *Adapter) Delete(ctx context.Context) (bool, error) {
 	return true, nil
 }
 
+// ValidateExternalResource compares the `status.externalRef` with the `spec` Project, Location and
+// (external) resourceID to make sure those fields are immutable and matches the previous deployed value.
+func (a *Adapter) ValidateExternalResource() error {
+	actualExternalRef := NewResourceRef(a.actual)
+
+	desiredExternalRef := "https://cloudbuild.googleapis.com/v1/" + a.fullyQualifiedName()
+	if *actualExternalRef.Get() == desiredExternalRef {
+		return nil
+	}
+
+	// Give user guidance on how to fix the CloudBuildWorkerPool spec.
+	if a.desired.Spec.ResourceID != nil && *a.desired.Spec.ResourceID != actualExternalRef.GetResourceID() {
+		return fmt.Errorf("`spec.resourceID` is immutable field, expect %s, got %s",
+			actualExternalRef.GetResourceID(), *a.desired.Spec.ResourceID)
+	}
+	if a.desired.Spec.Location != actualExternalRef.GetLocation() {
+		return fmt.Errorf("`spec.location` is immutable field, expect %s, got %s",
+			actualExternalRef.GetLocation(), a.desired.Spec.Location)
+	}
+	// TODO: Some Selflink may change the project from projectID to projectNum.
+	/*
+		if a.desired.Spec.ProjectRef.Name != "" {
+			return fmt.Errorf("`spec.projectRef.name` is immutable field, expect project %s",
+				actualExternalRef.GetProject())
+		}
+		if a.desired.Spec.ProjectRef.External != "" {
+			return fmt.Errorf("`spec.projectRef.external` is immutable field, expect project %s",
+				actualExternalRef.GetProject())
+		}*/
+	return nil
+}
+
 func (a *Adapter) fullyQualifiedName() string {
 	return fmt.Sprintf("projects/%s/locations/%s/workerPools/%s", a.projectID, a.location, a.resourceID)
 }

pkg/controller/direct/directbase/directbase_controller.go

@@ -225,6 +225,18 @@ func (r *reconcileContext) doReconcile(ctx context.Context, u *unstructured.Unst
 		return false, r.handleUpdateFailed(ctx, u, err)
 	}
 
+	// To create, update or delete the GCP object, we need to get the GCP object first.
+	// Because the object contains the cloud service information like `selfLink` `ID` required to validate
+	// the resource uniqueness before updating/deleting.
+	existsAlready, err := adapter.Find(ctx)
+	if err != nil {
+		if unwrappedErr, ok := lifecyclehandler.CausedByUnresolvableDeps(err); ok {
+			logger.Info(unwrappedErr.Error(), "resource", k8s.GetNamespacedName(u))
+			return r.handleUnresolvableDeps(ctx, u, unwrappedErr)
+		}
+		return false, r.handleUpdateFailed(ctx, u, err)
+	}
+
 	defer execution.RecoverWithInternalError(&err)
 	if !u.GetDeletionTimestamp().IsZero() {
 		if !k8s.HasFinalizer(u, k8s.ControllerFinalizerName) {
@@ -255,14 +267,6 @@ func (r *reconcileContext) doReconcile(ctx context.Context, u *unstructured.Unst
 		return false, r.handleDeleted(ctx, u)
 	}
 
-	existsAlready, err := adapter.Find(ctx)
-	if err != nil {
-		if unwrappedErr, ok := lifecyclehandler.CausedByUnresolvableDeps(err); ok {
-			logger.Info(unwrappedErr.Error(), "resource", k8s.GetNamespacedName(u))
-			return r.handleUnresolvableDeps(ctx, u, unwrappedErr)
-		}
-		return false, r.handleUpdateFailed(ctx, u, err)
-	}
 	k8s.EnsureFinalizers(u, k8s.ControllerFinalizerName, k8s.DeletionDefenderFinalizerName)
 
 	// set the etag to an empty string, since IAMPolicy is the authoritative intent, KCC wants to overwrite the underlying policy regardless

pkg/controller/direct/externalresource/resource.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2024.
+
+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.
+*/
+
+package externalresource
+
+import (
+	"reflect"
+)
+
+type ExternalResourceReference struct {
+	hasSelfLink bool
+	externalRef string
+}
+
+type GCPResource interface {
+	GetName() string
+}
+
+// New builds the External Reference for CloudBuildWorkerPool when the object already exists on the GCP server.
+// Ideally, it expects to use the GCP object's selfLink value as the external reference. However, if
+// the resource does not have `selfLink` (like CloudBuildWorkerPool), it builds the URL from the GCP object.
+func New(host string, gcpObj GCPResource) *ExternalResourceReference {
+	baseExternalRef := &ExternalResourceReference{
+		hasSelfLink: false,
+		externalRef: host + gcpObj.GetName(),
+	}
+	// Ignore the actual GCP proto, get the `selfLink` in general.
+	s := reflect.ValueOf(gcpObj).Elem()
+	if s.Kind() != reflect.Struct {
+		return baseExternalRef
+	}
+
+	selfLink := s.FieldByName("SelfLink")
+
+	if !selfLink.IsValid() || selfLink.IsZero() {
+		return baseExternalRef
+	}
+	switch selfLink.Kind() {
+	case reflect.String:
+		baseExternalRef.hasSelfLink = true
+		baseExternalRef.externalRef = selfLink.String()
+	case reflect.Pointer:
+		baseExternalRef.hasSelfLink = true
+		baseExternalRef.externalRef = selfLink.Elem().String()
+	}
+	return baseExternalRef
+}
+
+func (e *ExternalResourceReference) Get() *string {
+	return &e.externalRef
+}

pkg/controller/direct/externalresource/resource_test.go

@@ -0,0 +1,61 @@
+/*
+Copyright 2024.
+
+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.
+*/
+package externalresource
+
+import (
+	"testing"
+
+	"cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb"
+	"cloud.google.com/go/compute/apiv1/computepb"
+)
+
+var (
+	networkSelfLink = "https://www.googleapis.com/mockservice/v1/projects/yuwenma-gke-dev/global/networks/computenetwork-52ldcmpbgxfmizhrk74q"
+)
+
+func TestNew(t *testing.T) {
+	tests := []struct {
+		name                string
+		gcpResource         GCPResource
+		expectedHasSelfLink bool
+		expectedExternalRef string
+	}{
+		{
+			name:                "cloudbuild workerpool, no selfLink",
+			gcpResource:         &cloudbuildpb.WorkerPool{Name: "projects/mock-project/locations/us-central1/workerPools/mock-pool"},
+			expectedHasSelfLink: false,
+			expectedExternalRef: "https://www.googleapis.com/mockservice/v1/projects/mock-project/locations/us-central1/workerPools/mock-pool",
+		},
+		{
+			name:                "compute network, has seflLink as string pointer",
+			gcpResource:         &computepb.Network{SelfLink: &networkSelfLink},
+			expectedHasSelfLink: true,
+			expectedExternalRef: networkSelfLink,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			gcpObj := test.gcpResource
+			actualRef := New("https://www.googleapis.com/mockservice/v1/", gcpObj)
+			if actualRef.hasSelfLink != test.expectedHasSelfLink {
+				t.Errorf("expected %v, got %v", test.expectedHasSelfLink, actualRef.hasSelfLink)
+			}
+			if actualRef.externalRef != test.expectedExternalRef {
+				t.Errorf("expected %v, got %v", test.expectedExternalRef, actualRef.externalRef)
+			}
+		})
+	}
+}

pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_generated_object_cloudbuildworkerpool.golden.yaml

@@ -30,6 +30,7 @@ status:
     reason: UpToDate
     status: "True"
     type: Ready
+  externalRef: https://cloudbuild.googleapis.com/v1/projects/${projectId}/locations/us-central1/workerPools/cloudbuildworkerpool-${uniqueId}
   observedGeneration: 2
   observedState:
     createTime: "1970-01-01T00:00:00Z"