From e33dba7bff0b6d7bbbcda342b808a6d1cb70cbc7 Mon Sep 17 00:00:00 2001
From: Joyce Ma <maqiuyu@google.com>
Date: Thu, 19 Dec 2024 08:51:56 +0000
Subject: [PATCH] Address further comments

---
 apis/alloydb/v1alpha1/instance_identity.go    |  27 ++--
 apis/alloydb/v1alpha1/instance_reference.go   |  12 +-
 apis/alloydb/v1alpha1/instance_types.go       |   2 -
 apis/alloydb/v1beta1/instance_identity.go     |  27 ++--
 apis/alloydb/v1beta1/instance_reference.go    |  12 +-
 apis/alloydb/v1beta1/instance_types.go        |   2 -
 apis/common/utils.go                          |   8 +
 apis/refs/v1beta1/alloydbref.go               | 141 ++----------------
 apis/refs/v1beta1/helper.go                   |   8 -
 ...stances.alloydb.cnrm.cloud.google.com.yaml |   8 +-
 .../direct/alloydb/instance_controller.go     |  44 ++++--
 ...ed_object_basicalloydbinstance.golden.yaml |   2 +-
 ..._basicsecondaryalloydbinstance.golden.yaml |   2 +-
 ...ted_object_fullalloydbinstance.golden.yaml |   2 +-
 ...ted_object_readalloydbinstance.golden.yaml |   2 +-
 ...ed_object_zonalalloydbinstance.golden.yaml |   2 +-
 .../resource-docs/alloydb/alloydbinstance.md  |   2 +-
 17 files changed, 95 insertions(+), 208 deletions(-)

diff --git a/apis/alloydb/v1alpha1/instance_identity.go b/apis/alloydb/v1alpha1/instance_identity.go
index b0613fc4b3..44dfb55475 100644
--- a/apis/alloydb/v1alpha1/instance_identity.go
+++ b/apis/alloydb/v1alpha1/instance_identity.go
@@ -24,10 +24,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-const serviceDomain = "//alloydb.googleapis.com"
-
-// InstanceIdentity defines the resource reference to AlloyDBInstance, which "External" field
-// holds the GCP identifier for the KRM object.
+// InstanceIdentity defines the resource reference to AlloyDBInstance.
 type InstanceIdentity struct {
 	parent *InstanceParent
 	id     string
@@ -45,18 +42,14 @@ func (i *InstanceIdentity) Parent() *InstanceParent {
 	return i.parent
 }
 
-// AsExternalRef builds a externalRef from a PrivilegedAccessManagerEntitlement.
-func (i *InstanceIdentity) AsExternalRef() *string {
-	er := serviceDomain + "/" + i.String()
-	return &er
-}
-
 type InstanceParent struct {
-	clusterName string
+	projectID string
+	location  string
+	clusterID string
 }
 
 func (p *InstanceParent) String() string {
-	return p.clusterName
+	return "projects/" + p.projectID + "/locations/" + p.location + "/clusters/" + p.clusterID
 }
 
 // New builds a InstanceIdentity from the Config Connector Instance object.
@@ -81,12 +74,12 @@ func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *AlloyDB
 	externalRef := common.ValueOf(obj.Status.ExternalRef)
 	if externalRef != "" {
 		// Validate desired with actual
-		actualParent, actualResourceID, err := ParseInstanceExternalRef(externalRef)
+		actualParent, actualResourceID, err := ParseInstanceExternal(externalRef)
 		if err != nil {
 			return nil, err
 		}
-		if actualParent.clusterName != clusterRef.String() {
-			return nil, fmt.Errorf("spec.clusterRef changed, expect %s, got %s", actualParent.clusterName, clusterRef)
+		if actualParent.String() != clusterRef.String() {
+			return nil, fmt.Errorf("spec.clusterRef changed, expect %s, got %s", actualParent.String(), clusterRef)
 		}
 		if actualResourceID != resourceID {
 			return nil, fmt.Errorf("cannot reset `metadata.name` or `spec.resourceID` to %s, since it has already assigned to %s",
@@ -95,7 +88,9 @@ func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *AlloyDB
 	}
 	return &InstanceIdentity{
 		parent: &InstanceParent{
-			clusterName: clusterRef.String(),
+			projectID: clusterRef.ProjectID,
+			location:  clusterRef.Location,
+			clusterID: clusterRef.ClusterID,
 		},
 		id: resourceID,
 	}, nil
diff --git a/apis/alloydb/v1alpha1/instance_reference.go b/apis/alloydb/v1alpha1/instance_reference.go
index b07f519c40..291dc1dfb5 100644
--- a/apis/alloydb/v1alpha1/instance_reference.go
+++ b/apis/alloydb/v1alpha1/instance_reference.go
@@ -83,21 +83,15 @@ func (r *InstanceRef) NormalizedExternal(ctx context.Context, reader client.Read
 	return r.External, nil
 }
 
-func ParseInstanceExternalRef(externalRef string) (parent *InstanceParent, resourceID string, err error) {
-	if !strings.HasPrefix(externalRef, serviceDomain) {
-		return nil, "", fmt.Errorf("externalRef should have prefix %s, got %s", serviceDomain, externalRef)
-	}
-	path := strings.TrimPrefix(externalRef, serviceDomain+"/")
-	return ParseInstanceExternal(path)
-}
-
 func ParseInstanceExternal(external string) (parent *InstanceParent, resourceID string, err error) {
 	tokens := strings.Split(external, "/")
 	if len(tokens) != 8 || tokens[0] != "projects" || tokens[2] != "locations" || tokens[4] != "clusters" || tokens[6] != "instances" {
 		return nil, "", fmt.Errorf("format of AlloyDBInstance external=%q was not known (use projects/<projectId>/locations/<location>/clusters/<clusterID>/instances/<instanceID>)", external)
 	}
 	parent = &InstanceParent{
-		clusterName: fmt.Sprintf("%s/%s/%s/%s/%s/%s", tokens[0], tokens[1], tokens[2], tokens[3], tokens[4], tokens[5]),
+		projectID: tokens[1],
+		location:  tokens[3],
+		clusterID: tokens[5],
 	}
 	resourceID = tokens[7]
 	return parent, resourceID, nil
diff --git a/apis/alloydb/v1alpha1/instance_types.go b/apis/alloydb/v1alpha1/instance_types.go
index 8f4c5c5daf..b49b436f2e 100644
--- a/apis/alloydb/v1alpha1/instance_types.go
+++ b/apis/alloydb/v1alpha1/instance_types.go
@@ -30,7 +30,6 @@ type AlloyDBInstanceSpec struct {
 	// +required
 	ClusterRef *refs.AlloyDBClusterRef `json:"clusterRef,omitempty"`
 
-	// Immutable.
 	// Optional. The instanceId of the resource. If not given, the metadata.name will be used.
 	ResourceID *string `json:"resourceID,omitempty"`
 
@@ -160,7 +159,6 @@ type AlloyDBInstanceObservedState struct {
 
 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
-// TODO(user): make sure the pluralizaiton below is correct
 // +kubebuilder:resource:categories=gcp,shortName=gcpalloydbinstance;gcpalloydbinstances
 // +kubebuilder:subresource:status
 // +kubebuilder:metadata:labels="cnrm.cloud.google.com/tf2crd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true"
diff --git a/apis/alloydb/v1beta1/instance_identity.go b/apis/alloydb/v1beta1/instance_identity.go
index 20bae0d996..b5ee7221a7 100644
--- a/apis/alloydb/v1beta1/instance_identity.go
+++ b/apis/alloydb/v1beta1/instance_identity.go
@@ -24,10 +24,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-const serviceDomain = "//alloydb.googleapis.com"
-
-// InstanceIdentity defines the resource reference to AlloyDBInstance, which "External" field
-// holds the GCP identifier for the KRM object.
+// InstanceIdentity defines the resource reference to AlloyDBInstance.
 type InstanceIdentity struct {
 	parent *InstanceParent
 	id     string
@@ -45,18 +42,14 @@ func (i *InstanceIdentity) Parent() *InstanceParent {
 	return i.parent
 }
 
-// AsExternalRef builds a externalRef from a PrivilegedAccessManagerEntitlement.
-func (i *InstanceIdentity) AsExternalRef() *string {
-	er := serviceDomain + "/" + i.String()
-	return &er
-}
-
 type InstanceParent struct {
-	clusterName string
+	projectID string
+	location  string
+	clusterID string
 }
 
 func (p *InstanceParent) String() string {
-	return p.clusterName
+	return "projects/" + p.projectID + "/locations/" + p.location + "/clusters/" + p.clusterID
 }
 
 // New builds a InstanceIdentity from the Config Connector Instance object.
@@ -81,12 +74,12 @@ func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *AlloyDB
 	externalRef := common.ValueOf(obj.Status.ExternalRef)
 	if externalRef != "" {
 		// Validate desired with actual
-		actualParent, actualResourceID, err := ParseInstanceExternalRef(externalRef)
+		actualParent, actualResourceID, err := ParseInstanceExternal(externalRef)
 		if err != nil {
 			return nil, err
 		}
-		if actualParent.clusterName != clusterRef.String() {
-			return nil, fmt.Errorf("spec.clusterRef changed, expect %s, got %s", actualParent.clusterName, clusterRef)
+		if actualParent.String() != clusterRef.String() {
+			return nil, fmt.Errorf("spec.clusterRef changed, expect %s, got %s", actualParent.String(), clusterRef)
 		}
 		if actualResourceID != resourceID {
 			return nil, fmt.Errorf("cannot reset `metadata.name` or `spec.resourceID` to %s, since it has already assigned to %s",
@@ -95,7 +88,9 @@ func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *AlloyDB
 	}
 	return &InstanceIdentity{
 		parent: &InstanceParent{
-			clusterName: clusterRef.String(),
+			projectID: clusterRef.ProjectID,
+			location:  clusterRef.Location,
+			clusterID: clusterRef.ClusterID,
 		},
 		id: resourceID,
 	}, nil
diff --git a/apis/alloydb/v1beta1/instance_reference.go b/apis/alloydb/v1beta1/instance_reference.go
index af24e27d1f..19c032120c 100644
--- a/apis/alloydb/v1beta1/instance_reference.go
+++ b/apis/alloydb/v1beta1/instance_reference.go
@@ -83,21 +83,15 @@ func (r *InstanceRef) NormalizedExternal(ctx context.Context, reader client.Read
 	return r.External, nil
 }
 
-func ParseInstanceExternalRef(externalRef string) (parent *InstanceParent, resourceID string, err error) {
-	if !strings.HasPrefix(externalRef, serviceDomain) {
-		return nil, "", fmt.Errorf("externalRef should have prefix %s, got %s", serviceDomain, externalRef)
-	}
-	path := strings.TrimPrefix(externalRef, serviceDomain+"/")
-	return ParseInstanceExternal(path)
-}
-
 func ParseInstanceExternal(external string) (parent *InstanceParent, resourceID string, err error) {
 	tokens := strings.Split(external, "/")
 	if len(tokens) != 8 || tokens[0] != "projects" || tokens[2] != "locations" || tokens[4] != "clusters" || tokens[6] != "instances" {
 		return nil, "", fmt.Errorf("format of AlloyDBInstance external=%q was not known (use projects/<projectId>/locations/<location>/clusters/<clusterID>/instances/<instanceID>)", external)
 	}
 	parent = &InstanceParent{
-		clusterName: fmt.Sprintf("%s/%s/%s/%s/%s/%s", tokens[0], tokens[1], tokens[2], tokens[3], tokens[4], tokens[5]),
+		projectID: tokens[1],
+		location:  tokens[3],
+		clusterID: tokens[5],
 	}
 	resourceID = tokens[7]
 	return parent, resourceID, nil
diff --git a/apis/alloydb/v1beta1/instance_types.go b/apis/alloydb/v1beta1/instance_types.go
index 9d20b47bd9..3694a50ab7 100644
--- a/apis/alloydb/v1beta1/instance_types.go
+++ b/apis/alloydb/v1beta1/instance_types.go
@@ -30,7 +30,6 @@ type AlloyDBInstanceSpec struct {
 	// +required
 	ClusterRef *refs.AlloyDBClusterRef `json:"clusterRef,omitempty"`
 
-	// Immutable.
 	// Optional. The instanceId of the resource. If not given, the metadata.name will be used.
 	ResourceID *string `json:"resourceID,omitempty"`
 
@@ -160,7 +159,6 @@ type AlloyDBInstanceObservedState struct {
 
 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
-// TODO(user): make sure the pluralizaiton below is correct
 // +kubebuilder:resource:categories=gcp,shortName=gcpalloydbinstance;gcpalloydbinstances
 // +kubebuilder:subresource:status
 // +kubebuilder:metadata:labels="cnrm.cloud.google.com/tf2crd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true"
diff --git a/apis/common/utils.go b/apis/common/utils.go
index 13e60763d0..295c13eb58 100644
--- a/apis/common/utils.go
+++ b/apis/common/utils.go
@@ -21,3 +21,11 @@ func ValueOf[T any](t *T) T {
 	}
 	return *t
 }
+
+func LazyPtr[V comparable](v V) *V {
+	var defaultV V
+	if v == defaultV {
+		return nil
+	}
+	return &v
+}
diff --git a/apis/refs/v1beta1/alloydbref.go b/apis/refs/v1beta1/alloydbref.go
index 5f58ac7677..6afa4bd169 100644
--- a/apis/refs/v1beta1/alloydbref.go
+++ b/apis/refs/v1beta1/alloydbref.go
@@ -19,6 +19,8 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/GoogleCloudPlatform/k8s-config-connector/apis/common"
+
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -36,11 +38,12 @@ type AlloyDBClusterRef struct {
 }
 
 type AlloyDBCluster struct {
-	projectID string
-	location  string
-	clusterID string
+	ProjectID string
+	Location  string
+	ClusterID string
 }
 
+// TODO: Remove after AlloyDBCluster is migrated to direct controller.
 func ResolveAlloyDBCluster(ctx context.Context, reader client.Reader, src client.Object, ref *AlloyDBClusterRef) (*AlloyDBCluster, error) {
 	if ref == nil {
 		return nil, nil
@@ -59,9 +62,9 @@ func ResolveAlloyDBCluster(ctx context.Context, reader client.Reader, src client
 		tokens := strings.Split(ref.External, "/")
 		if len(tokens) == 6 && tokens[0] == "projects" && tokens[2] == "locations" && tokens[4] == "clusters" {
 			return &AlloyDBCluster{
-				projectID: tokens[1],
-				location:  tokens[3],
-				clusterID: tokens[5],
+				ProjectID: tokens[1],
+				Location:  tokens[3],
+				ClusterID: tokens[5],
 			}, nil
 		}
 		return nil, fmt.Errorf("format of AlloyDBClusterRef external=%q was not known (use projects/[projectId]/locations/[location]/clusters/[clusterId])", ref.External)
@@ -101,9 +104,9 @@ func ResolveAlloyDBCluster(ctx context.Context, reader client.Reader, src client
 		return nil, err
 	}
 	return &AlloyDBCluster{
-		projectID: projectID,
-		location:  location,
-		clusterID: clusterID,
+		ProjectID: projectID,
+		Location:  location,
+		ClusterID: clusterID,
 	}, nil
 }
 
@@ -130,7 +133,7 @@ func ResolveAlloyDBClusterType(ctx context.Context, reader client.Reader, src cl
 
 	// External is provided.
 	if ref.External != "" {
-		return lazyPtr(ref.External), nil
+		return common.LazyPtr(ref.External), nil
 	}
 
 	// Fetch AlloyDBCluster object to construct the external form.
@@ -161,123 +164,9 @@ func ResolveAlloyDBClusterType(ctx context.Context, reader client.Reader, src cl
 		// clusterType is defaulted to "PRIMARY" when not set.
 		clusterType = "PRIMARY"
 	}
-	return lazyPtr(clusterType), nil
-}
-
-func ResolveAlloyDBClusterName(ctx context.Context, reader client.Reader, obj *unstructured.Unstructured) (string, error) {
-	clusterRefExternal, _, _ := unstructured.NestedString(obj.Object, "spec", "clusterRef", "external")
-	if clusterRefExternal != "" {
-		clusterRef := AlloyDBClusterRef{
-			External: clusterRefExternal,
-		}
-
-		cluster, err := ResolveAlloyDBCluster(ctx, reader, obj, &clusterRef)
-		if err != nil {
-			return "", fmt.Errorf("cannot parse clusterRef.external %q in %v %v/%v: %w", clusterRefExternal, obj.GetKind(), obj.GetNamespace(), obj.GetName(), err)
-		}
-		return cluster.String(), nil
-	}
-
-	clusterRefName, _, _ := unstructured.NestedString(obj.Object, "spec", "clusterRef", "name")
-	if clusterRefName != "" {
-		clusterRefNamespace, _, _ := unstructured.NestedString(obj.Object, "spec", "clusterRef", "namespace")
-
-		clusterRef := AlloyDBClusterRef{
-			Name:      clusterRefName,
-			Namespace: clusterRefNamespace,
-		}
-		if clusterRef.Namespace == "" {
-			clusterRef.Namespace = obj.GetNamespace()
-		}
-
-		cluster, err := ResolveAlloyDBCluster(ctx, reader, obj, &clusterRef)
-		if err != nil {
-			return "", fmt.Errorf("cannot parse clusterRef in %v %v/%v: %w", obj.GetKind(), obj.GetNamespace(), obj.GetName(), err)
-		}
-		return cluster.String(), nil
-	}
-
-	return "", fmt.Errorf("cannot find AlloyDB cluster name for %v %v/%v", obj.GetKind(), obj.GetNamespace(), obj.GetName())
+	return common.LazyPtr(clusterType), nil
 }
 
 func (c *AlloyDBCluster) String() string {
-	return fmt.Sprintf("projects/%s/locations/%s/clusters/%s", c.projectID, c.location, c.clusterID)
-}
-
-type AlloyDBInstanceRef struct {
-	// If provided must be in the format `projects/[projectId]/locations/[location]/clusters/[clusterId]/instances/[instanceId]`.
-	External string `json:"external,omitempty"`
-	// The `metadata.name` field of a `AlloyDBInstance` resource.
-	Name string `json:"name,omitempty"`
-	// The `metadata.namespace` field of a `AlloyDBInstance` resource.
-	Namespace string `json:"namespace,omitempty"`
-}
-
-type AlloyDBInstance struct {
-	clusterName string
-	instanceID  string
-}
-
-func ResolveAlloyDBInstance(ctx context.Context, reader client.Reader, src client.Object, ref *AlloyDBInstanceRef) (*AlloyDBInstance, error) {
-	if ref == nil {
-		return nil, nil
-	}
-
-	if ref.Name == "" && ref.External == "" {
-		return nil, fmt.Errorf("must specify either name or external on AlloyDBInstanceRef")
-	}
-	if ref.Name != "" && ref.External != "" {
-		return nil, fmt.Errorf("cannot specify both name and external on AlloyDBInstanceRef")
-	}
-
-	// External is provided.
-	if ref.External != "" {
-		// External should be in the `projects/[projectId]/locations/[location]/clusters/[clusterId]/instances/[instanceId]` format.
-		tokens := strings.Split(ref.External, "/")
-		if len(tokens) == 6 && tokens[0] == "projects" && tokens[2] == "locations" && tokens[4] == "clusters" && tokens[6] == "instances" {
-			return &AlloyDBInstance{
-				clusterName: fmt.Sprintf("%s/%s/%s/%s/%s/%s", tokens[0], tokens[1], tokens[2], tokens[3], tokens[4], tokens[5]),
-				instanceID:  tokens[7],
-			}, nil
-		}
-		return nil, fmt.Errorf("format of AlloyDBInstanceRef external=%q was not known (use projects/[projectId]/locations/[location]/clusters/[clusterId]/instances/[instanceId])", ref.External)
-
-	}
-
-	// Fetch AlloyDBInstance object to construct the external form.
-	instance := &unstructured.Unstructured{}
-	instance.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "alloydb.cnrm.cloud.google.com",
-		Version: "v1beta1",
-		Kind:    "AlloyDBInstance",
-	})
-	nn := types.NamespacedName{
-		Namespace: ref.Namespace,
-		Name:      ref.Name,
-	}
-	if nn.Namespace == "" {
-		nn.Namespace = src.GetNamespace()
-	}
-	if err := reader.Get(ctx, nn, instance); err != nil {
-		if apierrors.IsNotFound(err) {
-			return nil, fmt.Errorf("referenced AlloyDBInstance %v not found", nn)
-		}
-		return nil, fmt.Errorf("error reading referenced AlloyDBInstance %v: %w", nn, err)
-	}
-	clusterName, err := ResolveAlloyDBClusterName(ctx, reader, instance)
-	if err != nil {
-		return nil, err
-	}
-	instanceID, err := GetResourceID(instance)
-	if err != nil {
-		return nil, err
-	}
-	return &AlloyDBInstance{
-		clusterName: clusterName,
-		instanceID:  instanceID,
-	}, nil
-}
-
-func (i *AlloyDBInstance) String() string {
-	return fmt.Sprintf("%s/instances/%s", i.clusterName, i.instanceID)
+	return fmt.Sprintf("projects/%s/locations/%s/clusters/%s", c.ProjectID, c.Location, c.ClusterID)
 }
diff --git a/apis/refs/v1beta1/helper.go b/apis/refs/v1beta1/helper.go
index 0d0b81b732..df6733442b 100644
--- a/apis/refs/v1beta1/helper.go
+++ b/apis/refs/v1beta1/helper.go
@@ -41,11 +41,3 @@ func GetLocation(u *unstructured.Unstructured) (string, error) {
 	}
 	return location, nil
 }
-
-func lazyPtr[V comparable](v V) *V {
-	var defaultV V
-	if v == defaultV {
-		return nil
-	}
-	return &v
-}
diff --git a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml
index bf78746d31..75c95e1f6b 100644
--- a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml
+++ b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml
@@ -219,8 +219,8 @@ spec:
                     type: integer
                 type: object
               resourceID:
-                description: Immutable. Optional. The instanceId of the resource.
-                  If not given, the metadata.name will be used.
+                description: Optional. The instanceId of the resource. If not given,
+                  the metadata.name will be used.
                 type: string
             required:
             - clusterRef
@@ -506,8 +506,8 @@ spec:
                     type: integer
                 type: object
               resourceID:
-                description: Immutable. Optional. The instanceId of the resource.
-                  If not given, the metadata.name will be used.
+                description: Optional. The instanceId of the resource. If not given,
+                  the metadata.name will be used.
                 type: string
             required:
             - clusterRef
diff --git a/pkg/controller/direct/alloydb/instance_controller.go b/pkg/controller/direct/alloydb/instance_controller.go
index e2447ae3de..fc7d60d409 100644
--- a/pkg/controller/direct/alloydb/instance_controller.go
+++ b/pkg/controller/direct/alloydb/instance_controller.go
@@ -64,11 +64,7 @@ func (m *instanceModel) client(ctx context.Context) (*gcp.AlloyDBAdminClient, er
 	return gcpClient, err
 }
 
-func validateRequiredFields(ctx context.Context, reader client.Reader, obj *krm.AlloyDBInstance) error {
-	if obj.Spec.InstanceType != nil && obj.Spec.InstanceTypeRef != nil {
-		return fmt.Errorf("one and only one of 'spec.InstanceTypeRef' " +
-			"and 'spec.InstanceType' should be configured: both are configured")
-	}
+func resolveInstanceType(ctx context.Context, reader client.Reader, obj *krm.AlloyDBInstance, isDeletion bool) error {
 	if obj.Spec.InstanceType == nil && obj.Spec.InstanceTypeRef == nil {
 		return fmt.Errorf("one and only one of 'spec.InstanceTypeRef' " +
 			"and 'spec.InstanceType' should be configured: neither is configured")
@@ -76,16 +72,26 @@ func validateRequiredFields(ctx context.Context, reader client.Reader, obj *krm.
 
 	var instanceType *string
 	if obj.Spec.InstanceType != nil {
+		instanceType = obj.Spec.InstanceType
 		if *instanceType == "" {
 			return fmt.Errorf("'spec.InstanceType' should be configured with a non-empty string")
 		}
-		instanceType = obj.Spec.InstanceType
 	}
 	if obj.Spec.InstanceTypeRef != nil {
+		plainTextInstanceType := instanceType
 		var err error
 		instanceType, err = refsv1beta1.ResolveAlloyDBClusterType(ctx, reader, obj, obj.Spec.InstanceTypeRef)
 		if err != nil {
-			return fmt.Errorf("cannot resolve `spec.InstanceTypeRef`: %w", err)
+			if !isDeletion {
+				// TODO: Read instance type from observed state because it's necessary during deletion.
+				return fmt.Errorf("cannot resolve `spec.InstanceTypeRef`: %w", err)
+			}
+		}
+		if plainTextInstanceType != nil && *plainTextInstanceType != *instanceType {
+			return fmt.Errorf("'spec.InstanceTypeRef' and 'spec.InstanceType' "+
+				"resolve into different values: spec.InstanceTypeRef resolves to "+
+				"instanceType %q, spec.InstanceType is %q: they must be the same",
+				*instanceType, *plainTextInstanceType)
 		}
 	}
 	obj.Spec.InstanceType = instanceType
@@ -160,7 +166,7 @@ func (a *instanceAdapter) Create(ctx context.Context, createOp *directbase.Creat
 	log.V(2).Info("creating instance", "name", a.id)
 	mapCtx := &direct.MapContext{}
 
-	if err := validateRequiredFields(ctx, a.reader, a.desired); err != nil {
+	if err := resolveInstanceType(ctx, a.reader, a.desired, false); err != nil {
 		return err
 	}
 
@@ -217,7 +223,7 @@ func (a *instanceAdapter) Create(ctx context.Context, createOp *directbase.Creat
 	if mapCtx.Err() != nil {
 		return mapCtx.Err()
 	}
-	status.ExternalRef = a.id.AsExternalRef()
+	status.ExternalRef = direct.LazyPtr(a.id.String())
 	return createOp.UpdateStatus(ctx, status, nil)
 }
 
@@ -227,7 +233,7 @@ func (a *instanceAdapter) Update(ctx context.Context, updateOp *directbase.Updat
 	log.V(2).Info("updating instance", "name", a.id)
 	mapCtx := &direct.MapContext{}
 
-	if err := validateRequiredFields(ctx, a.reader, a.desired); err != nil {
+	if err := resolveInstanceType(ctx, a.reader, a.desired, false); err != nil {
 		return err
 	}
 
@@ -253,6 +259,14 @@ func (a *instanceAdapter) Update(ctx context.Context, updateOp *directbase.Updat
 
 	if len(updatePaths) == 0 {
 		log.V(2).Info("no field needs update", "name", a.id)
+		if *a.desired.Status.ExternalRef == "" {
+			// If it is the first reconciliation after switching to direct controller,
+			// then update Status to fill out the ExternalRef even if there is
+			// no update.
+			status := a.desired.Status
+			status.ExternalRef = direct.LazyPtr(a.id.String())
+			return updateOp.UpdateStatus(ctx, status, nil)
+		}
 		return nil
 	}
 	updateMask := &fieldmaskpb.FieldMask{
@@ -278,6 +292,11 @@ func (a *instanceAdapter) Update(ctx context.Context, updateOp *directbase.Updat
 	log.V(2).Info("successfully updated instance", "name", a.id)
 
 	status := AlloyDBInstanceStatus_FromProto(mapCtx, updated)
+	if *a.desired.Status.ExternalRef == "" {
+		// If it is the first reconciliation after switching to direct controller,
+		// then fill out the ExternalRef.
+		status.ExternalRef = direct.LazyPtr(a.id.String())
+	}
 	if mapCtx.Err() != nil {
 		return mapCtx.Err()
 	}
@@ -377,6 +396,11 @@ func (a *instanceAdapter) Delete(ctx context.Context, deleteOp *directbase.Delet
 	log := klog.FromContext(ctx)
 	log.V(2).Info("deleting instance", "name", a.id)
 
+	// instanceType must be resolved before calling DELETE.
+	if err := resolveInstanceType(ctx, a.reader, a.desired, true); err != nil {
+		return false, err
+	}
+
 	// Returning true directly if it is to delete a secondary instance.
 	// Technically the secondary instance is only abandoned but not deleted.
 	// This is because deletion of secondary instance is not supported. Instead,
diff --git a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicalloydbinstance/_generated_object_basicalloydbinstance.golden.yaml b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicalloydbinstance/_generated_object_basicalloydbinstance.golden.yaml
index 2be4d71488..c705fe9f55 100644
--- a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicalloydbinstance/_generated_object_basicalloydbinstance.golden.yaml
+++ b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicalloydbinstance/_generated_object_basicalloydbinstance.golden.yaml
@@ -30,7 +30,7 @@ status:
     status: "True"
     type: Ready
   createTime: "1970-01-01T00:00:00Z"
-  externalRef: //alloydb.googleapis.com/projects/${projectId}/locations/europe-west1/clusters/alloydbcluster-${uniqueId}/instances/alloydbinstance-${uniqueId}
+  externalRef: projects/${projectId}/locations/europe-west1/clusters/alloydbcluster-${uniqueId}/instances/alloydbinstance-${uniqueId}
   ipAddress: 10.1.2.3
   name: projects/${projectId}/locations/europe-west1/clusters/alloydbcluster-${uniqueId}/instances/alloydbinstance-${uniqueId}
   observedGeneration: 2
diff --git a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicsecondaryalloydbinstance/_generated_object_basicsecondaryalloydbinstance.golden.yaml b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicsecondaryalloydbinstance/_generated_object_basicsecondaryalloydbinstance.golden.yaml
index 76483d572d..4e77bf6d4f 100644
--- a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicsecondaryalloydbinstance/_generated_object_basicsecondaryalloydbinstance.golden.yaml
+++ b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/basicsecondaryalloydbinstance/_generated_object_basicsecondaryalloydbinstance.golden.yaml
@@ -30,7 +30,7 @@ status:
     status: "True"
     type: Ready
   createTime: "1970-01-01T00:00:00Z"
-  externalRef: //alloydb.googleapis.com/projects/${projectId}/locations/europe-west2/clusters/alloydbcluster-2-${uniqueId}/instances/alloydbinstance-2-${uniqueId}
+  externalRef: projects/${projectId}/locations/europe-west2/clusters/alloydbcluster-2-${uniqueId}/instances/alloydbinstance-2-${uniqueId}
   ipAddress: 10.1.2.3
   name: projects/${projectId}/locations/europe-west2/clusters/alloydbcluster-2-${uniqueId}/instances/alloydbinstance-2-${uniqueId}
   observedGeneration: 2
diff --git a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/fullalloydbinstance/_generated_object_fullalloydbinstance.golden.yaml b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/fullalloydbinstance/_generated_object_fullalloydbinstance.golden.yaml
index 9f324e9ede..509ee51e29 100644
--- a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/fullalloydbinstance/_generated_object_fullalloydbinstance.golden.yaml
+++ b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/fullalloydbinstance/_generated_object_fullalloydbinstance.golden.yaml
@@ -35,7 +35,7 @@ status:
     status: "True"
     type: Ready
   createTime: "1970-01-01T00:00:00Z"
-  externalRef: //alloydb.googleapis.com/projects/${projectId}/locations/europe-north1/clusters/alloydbcluster${uniqueId}/instances/alloydbinstance${uniqueId}
+  externalRef: projects/${projectId}/locations/europe-north1/clusters/alloydbcluster${uniqueId}/instances/alloydbinstance${uniqueId}
   ipAddress: 10.1.2.3
   name: projects/${projectId}/locations/europe-north1/clusters/alloydbcluster${uniqueId}/instances/alloydbinstance${uniqueId}
   observedGeneration: 2
diff --git a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/readalloydbinstance/_generated_object_readalloydbinstance.golden.yaml b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/readalloydbinstance/_generated_object_readalloydbinstance.golden.yaml
index 66b3dd5bc2..7dfa2a97ae 100644
--- a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/readalloydbinstance/_generated_object_readalloydbinstance.golden.yaml
+++ b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/readalloydbinstance/_generated_object_readalloydbinstance.golden.yaml
@@ -30,7 +30,7 @@ status:
     status: "True"
     type: Ready
   createTime: "1970-01-01T00:00:00Z"
-  externalRef: //alloydb.googleapis.com/projects/${projectId}/locations/europe-southwest1/clusters/alloydbcluster${uniqueId}/instances/alloydbreadinstance${uniqueId}
+  externalRef: projects/${projectId}/locations/europe-southwest1/clusters/alloydbcluster${uniqueId}/instances/alloydbreadinstance${uniqueId}
   ipAddress: 10.1.2.3
   name: projects/${projectId}/locations/europe-southwest1/clusters/alloydbcluster${uniqueId}/instances/alloydbreadinstance${uniqueId}
   observedGeneration: 2
diff --git a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/zonalalloydbinstance/_generated_object_zonalalloydbinstance.golden.yaml b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/zonalalloydbinstance/_generated_object_zonalalloydbinstance.golden.yaml
index 7e77bd8fe7..68d9d461e1 100644
--- a/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/zonalalloydbinstance/_generated_object_zonalalloydbinstance.golden.yaml
+++ b/pkg/test/resourcefixture/testdata/basic/alloydb/v1beta1/alloydbinstance/zonalalloydbinstance/_generated_object_zonalalloydbinstance.golden.yaml
@@ -28,7 +28,7 @@ status:
     status: "True"
     type: Ready
   createTime: "1970-01-01T00:00:00Z"
-  externalRef: //alloydb.googleapis.com/projects/${projectId}/locations/europe-central2/clusters/alloydbcluster-${uniqueId}/instances/alloydbinstance-${uniqueId}
+  externalRef: projects/${projectId}/locations/europe-central2/clusters/alloydbcluster-${uniqueId}/instances/alloydbinstance-${uniqueId}
   ipAddress: 10.1.2.3
   name: projects/${projectId}/locations/europe-central2/clusters/alloydbcluster-${uniqueId}/instances/alloydbinstance-${uniqueId}
   observedGeneration: 1
diff --git a/scripts/generate-google3-docs/resource-reference/generated/resource-docs/alloydb/alloydbinstance.md b/scripts/generate-google3-docs/resource-reference/generated/resource-docs/alloydb/alloydbinstance.md
index f2bb9bac9d..4c46f1d94f 100644
--- a/scripts/generate-google3-docs/resource-reference/generated/resource-docs/alloydb/alloydbinstance.md
+++ b/scripts/generate-google3-docs/resource-reference/generated/resource-docs/alloydb/alloydbinstance.md
@@ -368,7 +368,7 @@ Use deletionPolicy = "FORCE" in the associated secondary cluster and delete the
         </td>
         <td>
             <p><code class="apitype">string</code></p>
-            <p>{% verbatim %}Immutable. Optional. The instanceId of the resource. If not given, the metadata.name will be used.{% endverbatim %}</p>
+            <p>{% verbatim %}Optional. The instanceId of the resource. If not given, the metadata.name will be used.{% endverbatim %}</p>
         </td>
     </tr>
 </tbody>