Merge pull request GoogleCloudPlatform#3079 from justinsb/networkref_…

…refactor

Refactor CloudNetworkRef to use Ref / ID split pattern

apis/refs/v1beta1/computerefs.go

@@ -17,8 +17,11 @@ package v1beta1
 import (
 	"context"
 	"fmt"
+	"strconv"
 	"strings"
 
+	resourcemanager "cloud.google.com/go/resourcemanager/apiv3"
+	resourcemanagerpb "cloud.google.com/go/resourcemanager/apiv3/resourcemanagerpb"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -44,61 +47,87 @@ type ComputeNetworkRef struct {
 	Name string `json:"name,omitempty"`
 	/* The `namespace` field of a `ComputeNetwork` resource. */
 	Namespace string `json:"namespace,omitempty"`
-
-	ProjectNumber string `json:"-"`
-}
-
-func (networkRef *ComputeNetworkRef) WithProjectNumber() string {
-	_, id, _ := ParseComputeNetworkExternal(networkRef.External)
-	return buildNetworkExternal(networkRef.ProjectNumber, id)
-}
-
-type ComputeNetwork struct {
-	Project          string
-	ComputeNetworkID string
 }
 
-func (c *ComputeNetwork) String() string {
-	return buildNetworkExternal(c.Project, c.ComputeNetworkID)
+type ComputeNetworkID struct {
+	Project string
+	Network string
 }
 
-func buildNetworkExternal(project, network string) string {
-	return fmt.Sprintf("projects/%s/global/networks/%s", project, network)
+func (c *ComputeNetworkID) String() string {
+	return fmt.Sprintf("projects/%s/global/networks/%s", c.Project, c.Network)
 }
 
-func ParseComputeNetworkExternal(external string) (string, string, error) {
+func ParseComputeNetworkID(external string) (*ComputeNetworkID, error) {
 	if external == "" {
-		return "", "", fmt.Errorf("parse empty ComputeNetwork external value")
+		return nil, fmt.Errorf("empty ComputeNetwork external value")
 	}
 	external = fixStaleExternalFormat(external)
 	tokens := strings.Split(external, "/")
 	if len(tokens) == 5 && tokens[0] == "projects" && tokens[2] == "global" && tokens[3] == "networks" {
-		return tokens[1], tokens[4], nil
+		return &ComputeNetworkID{
+			Project: tokens[1],
+			Network: tokens[4],
+		}, nil
 	}
-	return "", "", fmt.Errorf("format of computenetwork external=%q was not known (use projects/<project>/global/networks/<networkid>)", external)
+	return nil, fmt.Errorf("format of computenetwork external=%q was not known (use projects/<project>/global/networks/<networkid>)", external)
 }
 
-func ResolveComputeNetwork(ctx context.Context, reader client.Reader, src client.Object, ref *ComputeNetworkRef) (*ComputeNetwork, error) {
+// ConvertToProjectNumber converts the external reference to use a project number.
+func (ref *ComputeNetworkRef) ConvertToProjectNumber(ctx context.Context, projectsClient *resourcemanager.ProjectsClient) error {
 	if ref == nil {
-		return nil, nil
+		return nil
+	}
+
+	id, err := ParseComputeNetworkID(ref.External)
+	if err != nil {
+		return err
+	}
+
+	// Check if the project number is already a valid integer
+	// If not, we need to look it up
+	projectNumber, err := strconv.ParseInt(id.Project, 10, 64)
+	if err != nil {
+		req := &resourcemanagerpb.GetProjectRequest{
+			Name: "projects/" + id.Project,
+		}
+		project, err := projectsClient.GetProject(ctx, req)
+		if err != nil {
+			return fmt.Errorf("error getting project %q: %w", req.Name, err)
+		}
+		n, err := strconv.ParseInt(strings.TrimPrefix(project.Name, "projects/"), 10, 64)
+		if err != nil {
+			return fmt.Errorf("error parsing project number for %q: %w", project.Name, err)
+		}
+		projectNumber = n
+	}
+	id.Project = strconv.FormatInt(projectNumber, 10)
+	ref.External = id.String()
+	return nil
+}
+
+func (ref *ComputeNetworkRef) Normalize(ctx context.Context, reader client.Reader, src client.Object) error {
+	if ref == nil {
+		return nil
 	}
 
 	if ref.External != "" && ref.Name != "" {
-		return nil, fmt.Errorf("cannot specify both name and external on computenetwork reference")
+		return fmt.Errorf("cannot specify both name and external on computenetwork reference")
 	}
 
 	if ref.External != "" {
-		project, networkID, err := ParseComputeNetworkExternal(ref.External)
+		id, err := ParseComputeNetworkID(ref.External)
 		if err != nil {
-			return nil, err
+			return err
+		}
+		*ref = ComputeNetworkRef{
+			External: id.String(),
 		}
-		return &ComputeNetwork{
-			Project:          project,
-			ComputeNetworkID: networkID}, nil
+		return nil
 	}
 
 	if ref.Name == "" {
-		return nil, fmt.Errorf("must specify either name or external on computenetwork reference")
+		return fmt.Errorf("must specify either name or external on computenetwork reference")
 	}
 
 	key := types.NamespacedName{
@@ -109,32 +138,37 @@ func ResolveComputeNetwork(ctx context.Context, reader client.Reader, src client
 		key.Namespace = src.GetNamespace()
 	}
 
-	computenetwork := &unstructured.Unstructured{}
-	computenetwork.SetGroupVersionKind(schema.GroupVersionKind{
+	computeNetwork := &unstructured.Unstructured{}
+	computeNetwork.SetGroupVersionKind(schema.GroupVersionKind{
 		Group:   "compute.cnrm.cloud.google.com",
 		Version: "v1beta1",
 		Kind:    "ComputeNetwork",
 	})
-	if err := reader.Get(ctx, key, computenetwork); err != nil {
+	if err := reader.Get(ctx, key, computeNetwork); err != nil {
 		if apierrors.IsNotFound(err) {
-			return nil, k8s.NewReferenceNotFoundError(computenetwork.GroupVersionKind(), key)
+			return k8s.NewReferenceNotFoundError(computeNetwork.GroupVersionKind(), key)
 		}
-		return nil, fmt.Errorf("error reading referenced ComputeNetwork %v: %w", key, err)
+		return fmt.Errorf("error reading referenced ComputeNetwork %v: %w", key, err)
 	}
 
-	computenetworkID, err := GetResourceID(computenetwork)
+	resourceID, err := GetResourceID(computeNetwork)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	computeNetworkProjectID, err := ResolveProjectID(ctx, reader, computenetwork)
+	projectID, err := ResolveProjectID(ctx, reader, computeNetwork)
 	if err != nil {
-		return nil, err
+		return err
 	}
-	return &ComputeNetwork{
-		Project:          computeNetworkProjectID,
-		ComputeNetworkID: computenetworkID,
-	}, nil
+
+	id := ComputeNetworkID{
+		Project: projectID,
+		Network: resourceID,
+	}
+	*ref = ComputeNetworkRef{
+		External: id.String(),
+	}
+	return nil
 }
 
 type ComputeSubnetworkRef struct {

mockgcp/mockcloudbuild/workerpool.go

@@ -16,6 +16,7 @@ package mockcloudbuild
 
 import (
 	"context"
+	"fmt"
 	"strconv"
 	"strings"
 
@@ -67,6 +68,11 @@ func (s *CloudBuildV1) CreateWorkerPool(ctx context.Context, req *pb.CreateWorke
 	obj.CreateTime = now
 
 	populateDefaultsForWorkerPool(obj)
+
+	if err := s.validateAndNormalizeWorkerPool(obj); err != nil {
+		return nil, err
+	}
+
 	if err := s.storage.Create(ctx, fqn, obj); err != nil {
 		return nil, err
 	}
@@ -81,19 +87,40 @@ func (s *CloudBuildV1) CreateWorkerPool(ctx context.Context, req *pb.CreateWorke
 
 func populateDefaultsForWorkerPool(wp *pb.WorkerPool) {
 	now := timestamppb.Now()
-	network := wp.GetPrivatePoolV1Config().GetNetworkConfig()
-	if network != nil {
-		tokens := strings.Split(network.PeeredNetwork, "/")
-		if len(tokens) == 5 {
-			network.PeeredNetwork = tokens[0] + "/" + "${projectNumber}" + "/" + tokens[2] + "/" + tokens[3] + "/" + tokens[4]
-		}
-	}
 	wp.UpdateTime = now
 	wp.State = pb.WorkerPool_RUNNING
 	wp.Etag = fields.ComputeWeakEtag(wp)
 	wp.Uid = "11111111111111111111"
 }
 
+func (s *CloudBuildV1) validateAndNormalizeWorkerPool(wp *pb.WorkerPool) error {
+	privatePoolV1Config := wp.GetPrivatePoolV1Config()
+
+	// Normalize the peered network link to always use the project number
+	if privatePoolV1Config != nil && privatePoolV1Config.NetworkConfig != nil {
+		peeredNetwork := privatePoolV1Config.NetworkConfig.GetPeeredNetwork()
+		if peeredNetwork == "" {
+			return status.Errorf(codes.InvalidArgument, "peeredNetwork is required")
+		} else {
+			tokens := strings.Split(peeredNetwork, "/")
+			projectToken := ""
+			if len(tokens) == 5 && tokens[0] == "projects" && tokens[2] == "global" && tokens[3] == "networks" {
+				projectToken = tokens[1]
+			} else {
+				return fmt.Errorf("format of peered network %q was not known (use projects/<project>/global/networks/<networkid>)", peeredNetwork)
+			}
+
+			project, err := s.Projects.GetProjectByIDOrNumber(projectToken)
+			if err != nil {
+				return fmt.Errorf("error getting project %q: %w", projectToken, err)
+			}
+
+			privatePoolV1Config.NetworkConfig.PeeredNetwork = fmt.Sprintf("projects/%d/global/networks/%s", project.Number, tokens[4])
+		}
+	}
+	return nil
+}
+
 func (s *CloudBuildV1) UpdateWorkerPool(ctx context.Context, req *pb.UpdateWorkerPoolRequest) (*longrunningpb.Operation, error) {
 	name, err := s.parseWorkerPoolName(req.WorkerPool.Name)
 	if err != nil {
@@ -111,7 +138,13 @@ func (s *CloudBuildV1) UpdateWorkerPool(ctx context.Context, req *pb.UpdateWorke
 	if err := fields.UpdateByFieldMask(obj, req.WorkerPool, req.UpdateMask.Paths); err != nil {
 		return nil, err
 	}
+
 	populateDefaultsForWorkerPool(obj)
+
+	if err := s.validateAndNormalizeWorkerPool(obj); err != nil {
+		return nil, err
+	}
+
 	if err := s.storage.Update(ctx, fqn, obj); err != nil {
 		return nil, err
 	}

pkg/controller/direct/cloudbuild/workerpool_controller.go

@@ -23,6 +23,7 @@ import (
 
 	gcp "cloud.google.com/go/cloudbuild/apiv1/v2"
 	cloudbuildpb "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb"
+	cloudresourcemanager "cloud.google.com/go/resourcemanager/apiv3"
 	"google.golang.org/protobuf/types/known/fieldmaskpb"
 
 	krm "github.com/GoogleCloudPlatform/k8s-config-connector/apis/cloudbuild/v1beta1"
@@ -68,6 +69,19 @@ func (m *model) client(ctx context.Context) (*gcp.Client, error) {
 	return gcpClient, err
 }
 
+func (m *model) projectsClient(ctx context.Context) (*cloudresourcemanager.ProjectsClient, error) {
+	opts, err := m.config.RESTClientOptions()
+	if err != nil {
+		return nil, err
+	}
+
+	crmClient, err := cloudresourcemanager.NewProjectsRESTClient(ctx, opts...)
+	if err != nil {
+		return nil, fmt.Errorf("building cloudresourcemanager client: %w", err)
+	}
+	return crmClient, err
+}
+
 func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *unstructured.Unstructured) (directbase.Adapter, error) {
 	obj := &krm.CloudBuildWorkerPool{}
 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &obj); err != nil {
@@ -123,21 +137,17 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 	// Get computeNetwork
 	networkSpec := obj.Spec.PrivatePoolConfig.NetworkConfig
 	if networkSpec != nil {
-		peeredNetworkRef, err := refs.ResolveComputeNetwork(ctx, reader, obj, &networkSpec.PeeredNetworkRef)
-		if err != nil {
+		if err := networkSpec.PeeredNetworkRef.Normalize(ctx, reader, obj); err != nil {
 			return nil, err
+		}
 
+		projectsClient, err := m.projectsClient(ctx)
+		if err != nil {
+			return nil, err
 		}
-		obj.Spec.PrivatePoolConfig.NetworkConfig.PeeredNetworkRef.External = peeredNetworkRef.String()
-		if obj.Status.ObservedState != nil {
-			fromStatus := obj.Status.ObservedState.NetworkConfig.PeeredNetwork
-			if fromStatus != nil {
-				projectNumber, _, err := refs.ParseComputeNetworkExternal(*fromStatus)
-				if err != nil {
-					return nil, err
-				}
-				networkSpec.PeeredNetworkRef.ProjectNumber = projectNumber
-			}
+
+		if err := networkSpec.PeeredNetworkRef.ConvertToProjectNumber(ctx, projectsClient); err != nil {
+			return nil, err
 		}
 	}
 
@@ -252,15 +262,8 @@ func (a *Adapter) Update(ctx context.Context, updateOp *directbase.UpdateOperati
 	}
 	wp.Name = a.id.FullyQualifiedName()
 	wp.Etag = a.actual.Etag
-	// the peered_network has a different HTTP request and response format.
-	// The HTTP request uses ProjectID, in the form of /projects/<projectID>/global/networks/<network>
-	// The HTTP response uses ProjectNumber, in the form of /projects/<ProjectNum>/global/networks/<network>
-	// When comparing the desired and actual fields, we need to align the project format, to avoid updating the
-	// "peered_network" field.
-	// Why we can't just update the "peered_network" field? Because it is immutable. ¯\_(ツ)_/¯
-	wp.GetPrivatePoolV1Config().NetworkConfig.PeeredNetwork = desired.Spec.PrivatePoolConfig.NetworkConfig.PeeredNetworkRef.WithProjectNumber()
-	paths, err := common.CompareProtoMessage(wp, a.actual, common.BasicDiff)
 
+	paths, err := common.CompareProtoMessage(wp, a.actual, common.BasicDiff)
 	if err != nil {
 		return err
 	}

pkg/controller/direct/dataflow/refs.go

@@ -53,13 +53,9 @@ func (r *refNormalizer) VisitField(path string, v any) error {
 	}
 
 	if networkRef, ok := v.(*refs.ComputeNetworkRef); ok {
-		resolved, err := refs.ResolveComputeNetwork(r.ctx, r.kube, r.src, networkRef)
-		if err != nil {
+		if err := networkRef.Normalize(r.ctx, r.kube, r.src); err != nil {
 			return err
 		}
-		*networkRef = refs.ComputeNetworkRef{
-			External: resolved.String(),
-		}
 	}
 
 	if subnetworkRef, ok := v.(*refs.ComputeSubnetworkRef); ok {

pkg/controller/direct/networkconnectivity/refs.go

@@ -53,13 +53,9 @@ func (r *refNormalizer) VisitField(path string, v any) error {
 	}
 
 	if networkRef, ok := v.(*refs.ComputeNetworkRef); ok {
-		resolved, err := refs.ResolveComputeNetwork(r.ctx, r.kube, r.src, networkRef)
-		if err != nil {
+		if err := networkRef.Normalize(r.ctx, r.kube, r.src); err != nil {
 			return err
 		}
-		*networkRef = refs.ComputeNetworkRef{
-			External: resolved.String(),
-		}
 	}
 
 	if subnetworkRef, ok := v.(*refs.ComputeSubnetworkRef); ok {

pkg/controller/direct/redis/cluster/refs.go

@@ -53,13 +53,9 @@ func (r *refNormalizer) VisitField(path string, v any) error {
 	}
 
 	if networkRef, ok := v.(*refs.ComputeNetworkRef); ok {
-		resolved, err := refs.ResolveComputeNetwork(r.ctx, r.kube, r.src, networkRef)
-		if err != nil {
+		if err := networkRef.Normalize(r.ctx, r.kube, r.src); err != nil {
 			return err
 		}
-		*networkRef = refs.ComputeNetworkRef{
-			External: resolved.String(),
-		}
 	}
 
 	return nil

pkg/controller/direct/sql/sqlinstance_resolverefs.go

@@ -228,12 +228,11 @@ func resolvePrivateNetworkRef(ctx context.Context, kube client.Reader, obj *krm.
 		Name:      resRef.Name,
 		Namespace: resRef.Namespace,
 	}
-	net, err := refs.ResolveComputeNetwork(ctx, kube, obj, netRef)
-	if err != nil {
+	if err := netRef.Normalize(ctx, kube, obj); err != nil {
 		return err
 	}
 
-	obj.Spec.Settings.IpConfiguration.PrivateNetworkRef.External = net.String()
+	obj.Spec.Settings.IpConfiguration.PrivateNetworkRef.External = netRef.External
 
 	return nil
 }