resolve comments; update mock test

apis/refs/v1beta1/networkref.go

@@ -15,7 +15,7 @@
 package v1beta1
 
 type ComputeNetworkRef struct {
-	/* The , when not managed by KCC. */
+	/* The compute network selflink of form "projects/<project>/global/networks/<network>", when not managed by KCC. */
 	External string `json:"external,omitempty"`
 	/* The `name` field of a `ComputeNetwork` resource. */
 	Name string `json:"name,omitempty"`

mockgcp/mockcloudbuild/service.go

@@ -60,6 +60,10 @@ func (s *MockService) NewHTTPMux(ctx context.Context, conn *grpc.ClientConn) (ht
 	if err != nil {
 		return nil, err
 	}
-
+	mux.RewriteError = func(ctx context.Context, error *httpmux.ErrorResponse) {
+		if error.Code == 404 {
+			error.Errors = nil
+		}
+	}
 	return mux, nil
 }

mockgcp/mockcloudbuild/workerpool.go

@@ -16,6 +16,7 @@ package mockcloudbuild
 
 import (
 	"context"
+	"reflect"
 	"strings"
 
 	"cloud.google.com/go/longrunning/autogen/longrunningpb"
@@ -43,6 +44,9 @@ func (s *CloudBuildV1) GetWorkerPool(ctx context.Context, req *pb.GetWorkerPoolR
 
 	obj := &pb.WorkerPool{}
 	if err := s.storage.Get(ctx, fqn, obj); err != nil {
+		if status.Code(err) == codes.NotFound {
+			return nil, status.Errorf(codes.NotFound, "Requested entity was not found.")
+		}
 		return nil, err
 	}
 	return obj, nil
@@ -59,8 +63,6 @@ func (s *CloudBuildV1) CreateWorkerPool(ctx context.Context, req *pb.CreateWorke
 
 	obj := proto.Clone(req.GetWorkerPool()).(*pb.WorkerPool)
 	obj.Name = fqn
-	// 	s.populateDefaultsForSpannerInstance(obj, obj)
-
 	obj.CreateTime = now
 	obj.UpdateTime = now
 	if err := s.storage.Create(ctx, fqn, obj); err != nil {
@@ -87,25 +89,22 @@ func (s *CloudBuildV1) UpdateWorkerPool(ctx context.Context, req *pb.UpdateWorke
 	}
 	now := timestamppb.Now()
 	obj.UpdateTime = now
-	/*
-		source := reflect.ValueOf(req.WorkerPool)
-		target := reflect.ValueOf(obj).Elem()
-		for _, path := range req.FieldMask.Paths {
-			f := target.FieldByName(path)
-			if f.IsValid() && f.CanSet() {
-				switch f.Kind() {
-				case reflect.Int:
-					intVal := source.FieldByName(path).Int()
-					f.SetInt(intVal)
-				case reflect.String:
-					stringVal := source.FieldByName(path).String()
-					f.SetString(stringVal)
-				}
-
+	source := reflect.ValueOf(req.WorkerPool)
+	target := reflect.ValueOf(obj).Elem()
+	for _, path := range req.UpdateMask.Paths {
+		f := target.FieldByName(path)
+		if f.IsValid() && f.CanSet() {
+			switch f.Kind() {
+			case reflect.Int:
+				intVal := source.FieldByName(path).Int()
+				f.SetInt(intVal)
+			case reflect.String:
+				stringVal := source.FieldByName(path).String()
+				f.SetString(stringVal)
 			}
-		}*/
 
-	// s.populateDefaultsForSpannerInstance(req.Instance, obj)
+		}
+	}
 	if err := s.storage.Update(ctx, fqn, obj); err != nil {
 		return nil, err
 	}
@@ -124,13 +123,23 @@ func (s *CloudBuildV1) DeleteWorkerPool(ctx context.Context, req *pb.DeleteWorke
 	}
 
 	fqn := name.String()
+	now := timestamppb.Now()
 
 	existing := &pb.WorkerPool{}
-	if err := s.storage.Delete(ctx, fqn, existing); err != nil {
-		return nil, err
+	err = s.storage.Delete(ctx, fqn, existing)
+	if err != nil {
+		if status.Code(err) == codes.NotFound && req.AllowMissing {
+			return s.operations.NewLRO(ctx)
+		}
+		return &longrunningpb.Operation{}, err
+	}
+	metadata := &pb.DeleteWorkerPoolOperationMetadata{
+		WorkerPool:   fqn,
+		CreateTime:   now,
+		CompleteTime: now,
 	}
+	return s.operations.DoneLRO(ctx, name.String(), metadata, &pb.WorkerPool{})
 
-	return s.operations.NewLRO(ctx)
 }
 
 type workerPoolName struct {
@@ -147,7 +156,7 @@ func (n *workerPoolName) String() string {
 func (s *MockService) parseWorkerPoolName(name string) (*workerPoolName, error) {
 	tokens := strings.Split(name, "/")
 
-	if len(tokens) == 4 && tokens[0] == "projects" && tokens[2] == "locations" && tokens[4] == "workerPools" {
+	if len(tokens) == 6 && tokens[0] == "projects" && tokens[2] == "locations" && tokens[4] == "workerPools" {
 		project, err := s.Projects.GetProjectByID(tokens[1])
 		if err != nil {
 			return nil, err

mockgcp/mockserviceusage/knownservices.go

@@ -25,6 +25,7 @@ var allServices = []string{
 	"multiclusteringress.googleapis.com",
 	"multiclusterservicediscovery.googleapis.com",
 	"mesh.googleapis.com",
+	"servicenetworking.googleapis.com",
 }
 
 func isKnownService(serviceName string) bool {

pkg/controller/direct/cloudbuild/workerpool_controller.go

@@ -105,13 +105,13 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 	location := obj.Spec.Parent.Location
 
 	// Get computeNetwork
-	networkID := ""
 	if obj.Spec.PrivatePoolConfig.NetworkConfig != nil {
 		networkRef, err := references.ResolveComputeNetwork(ctx, reader, obj, &obj.Spec.PrivatePoolConfig.NetworkConfig.PeeredNetworkRef)
 		if err != nil {
 			return nil, err
+
 		}
-		networkID = networkRef.String()
+		obj.Spec.PrivatePoolConfig.NetworkConfig.PeeredNetworkRef.External = networkRef.String()
 	}
 
 	// Get CloudBuild GCP client
@@ -122,7 +122,6 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 	return &Adapter{
 		resourceID: resourceID,
 		projectID:  projectID,
-		networkID:  networkID,
 		location:   location,
 		gcpClient:  gcpClient,
 		desired:    obj,
@@ -132,7 +131,6 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 type Adapter struct {
 	resourceID string
 	projectID  string
-	networkID  string
 	location   string
 	gcpClient  *gcp.Client
 	desired    *krm.CloudBuildWorkerPool
@@ -171,18 +169,13 @@ func (a *Adapter) Create(ctx context.Context, u *unstructured.Unstructured) erro
 		return fmt.Errorf("resourceID is empty")
 	}
 
-	// Use verified compute network ID.
-	// TODO: This is hard for users to understand. We need to make the code more readabile and self-explain.
 	desired := a.desired.DeepCopy()
-	if desired.Spec.PrivatePoolConfig.NetworkConfig != nil {
-		desired.Spec.PrivatePoolConfig.NetworkConfig.PeeredNetworkRef.External = a.networkID
-	}
 	wp := &cloudbuildpb.WorkerPool{
 		Name: a.fullyQualifiedName(),
 	}
 	err := krm.Convert_WorkerPool_KRM_To_API_v1(desired, wp)
 	if err != nil {
-		return fmt.Errorf("convert workerpool spec to api: %w", err)
+		return fmt.Errorf("converting workerpool spec to api: %w", err)
 	}
 	req := &cloudbuildpb.CreateWorkerPoolRequest{
 		Parent:       a.getParent(),
@@ -247,7 +240,7 @@ func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) erro
 		// 2. the gcp workerpool stores the network with "project_number", different from the spec which uses the "project_id".
 		//    * projects/<project_number>/global/networks/<network_id>
 		//    * projects/<project_id>/global/networks/<network_id>
-		desiredNetwork := strings.Split(a.networkID, "/")
+		desiredNetwork := strings.Split(desiredConfig.NetworkConfig.PeeredNetworkRef.External, "/")
 		actualNetwork := strings.Split(actualConfig.NetworkConfig.PeeredNetwork, "/")
 		if len(desiredNetwork) == 5 && len(actualNetwork) == 5 && !reflect.DeepEqual(desiredNetwork[4], actualNetwork[4]) {
 			return fmt.Errorf("peered_network is immutable field")
@@ -268,12 +261,9 @@ func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) erro
 		Name: a.fullyQualifiedName(),
 	}
 	desired := a.desired.DeepCopy()
-	if desired.Spec.PrivatePoolConfig.NetworkConfig != nil {
-		desired.Spec.PrivatePoolConfig.NetworkConfig.PeeredNetworkRef.External = a.networkID
-	}
 	err := krm.Convert_WorkerPool_KRM_To_API_v1(desired, wp)
 	if err != nil {
-		return fmt.Errorf("convert workerpool spec to api: %w", err)
+		return fmt.Errorf("converting workerpool spec to api: %w", err)
 	}
 	req := &cloudbuildpb.UpdateWorkerPoolRequest{
 		WorkerPool: wp,

pkg/controller/direct/references/computenetworkref.go

@@ -75,14 +75,14 @@ func ResolveComputeNetwork(ctx context.Context, reader client.Reader, src client
 	})
 	if err := reader.Get(ctx, key, computenetwork); err != nil {
 		if apierrors.IsNotFound(err) {
-			return nil, fmt.Errorf("referenced computenetwork %v not found", key)
+			return nil, fmt.Errorf("referenced ComputeNetwork %v not found", key)
 		}
-		return nil, fmt.Errorf("error reading referenced computenetwork %v: %w", key, err)
+		return nil, fmt.Errorf("error reading referenced ComputeNetwork %v: %w", key, err)
 	}
 
 	computenetworkID, _, err := unstructured.NestedString(computenetwork.Object, "spec", "resourceID")
 	if err != nil {
-		return nil, fmt.Errorf("reading spec.resourceID from computenetwork %v: %w", key, err)
+		return nil, fmt.Errorf("reading spec.resourceID from ComputeNetwork %v: %w", key, err)
 	}
 	if computenetworkID == "" {
 		computenetworkID = computenetwork.GetName()

pkg/controller/direct/references/projectref.go

@@ -61,6 +61,7 @@ func ResolveProject(ctx context.Context, reader client.Reader, src client.Object
 		return nil, fmt.Errorf("must specify either name or external on project reference")
 	}
 
+	// TODO: test case for ProjectRef with non-default namespace. What shall we expect?
 	key := types.NamespacedName{
 		Namespace: ref.Namespace,
 		Name:      ref.Name,

pkg/k8s/constants.go

@@ -129,7 +129,6 @@ var (
 	KCCVersionLabel      = FormatAnnotation("version")
 	ScopedNamespaceLabel = FormatAnnotation("scoped-namespace")
 	DCL2CRDLabel         = FormatAnnotation("dcl2crd")
-	DirectCRDLabel       = FormatAnnotation("directcrd")
 	KCCStabilityLabel    = FormatAnnotation("stability-level")
 
 	MutableButUnreadableFieldsAnnotation = FormatAnnotation("mutable-but-unreadable-fields")

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

@@ -31,4 +31,14 @@ status:
     reason: UpToDate
     status: "True"
     type: Ready
+  createTime: "1970-01-01T00:00:00Z"
   observedGeneration: 2
+  observedState:
+    networkConfig:
+      egressOption: NO_PUBLIC_EGRESS
+      peeredNetwork: projects/${projectId}/global/networks/computenetwork-${uniqueId}
+      peeredNetworkIpRange: /29
+    workerConfig:
+      diskSizeGb: 100
+      machineType: e2-medium
+  updateTime: "1970-01-01T00:00:00Z"