Address comments

apis/alloydb/v1alpha1/instance_types.go

@@ -30,7 +30,6 @@ type AlloyDBInstanceSpec struct {
 	// +required
 	ClusterRef *refs.AlloyDBClusterRef `json:"clusterRef,omitempty"`
 
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ResourceID field is immutable"
 	// Immutable.
 	// Optional. The instanceId of the resource. If not given, the metadata.name will be used.
 	ResourceID *string `json:"resourceID,omitempty"`

apis/alloydb/v1beta1/instance_identity.go

@@ -17,7 +17,6 @@ package v1beta1
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-config-connector/apis/common"
 	refsv1beta1 "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
@@ -101,23 +100,3 @@ func NewInstanceIdentity(ctx context.Context, reader client.Reader, obj *AlloyDB
 		id: resourceID,
 	}, 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]),
-	}
-	resourceID = tokens[7]
-	return parent, resourceID, nil
-}

apis/alloydb/v1beta1/instance_reference.go

@@ -17,6 +17,7 @@ package v1beta1
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	refsv1beta1 "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s"
@@ -81,3 +82,23 @@ func (r *InstanceRef) NormalizedExternal(ctx context.Context, reader client.Read
 	r.External = actualExternalRef
 	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]),
+	}
+	resourceID = tokens[7]
+	return parent, resourceID, nil
+}

apis/alloydb/v1beta1/instance_types.go

@@ -30,7 +30,6 @@ type AlloyDBInstanceSpec struct {
 	// +required
 	ClusterRef *refs.AlloyDBClusterRef `json:"clusterRef,omitempty"`
 
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ResourceID field is immutable"
 	// Immutable.
 	// Optional. The instanceId of the resource. If not given, the metadata.name will be used.
 	ResourceID *string `json:"resourceID,omitempty"`

config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_alloydbinstances.alloydb.cnrm.cloud.google.com.yaml

@@ -222,9 +222,6 @@ spec:
                 description: Immutable. Optional. The instanceId of the resource.
                   If not given, the metadata.name will be used.
                 type: string
-                x-kubernetes-validations:
-                - message: ResourceID field is immutable
-                  rule: self == oldSelf
             required:
             - clusterRef
             type: object
@@ -512,9 +509,6 @@ spec:
                 description: Immutable. Optional. The instanceId of the resource.
                   If not given, the metadata.name will be used.
                 type: string
-                x-kubernetes-validations:
-                - message: ResourceID field is immutable
-                  rule: self == oldSelf
             required:
             - clusterRef
             type: object

pkg/controller/direct/alloydb/instance_controller.go

@@ -64,40 +64,44 @@ func (m *instanceModel) client(ctx context.Context) (*gcp.AlloyDBAdminClient, er
 	return gcpClient, err
 }
 
-func (m *instanceModel) AdapterForObject(ctx context.Context, reader client.Reader, u *unstructured.Unstructured) (directbase.Adapter, error) {
-	obj := &krm.AlloyDBInstance{}
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &obj); err != nil {
-		return nil, fmt.Errorf("error converting to %T: %w", obj, err)
-	}
-
-	id, err := krm.NewInstanceIdentity(ctx, reader, obj)
-	if err != nil {
-		return nil, err
-	}
-
+func validateRequiredFields(ctx context.Context, reader client.Reader, obj *krm.AlloyDBInstance) error {
 	if obj.Spec.InstanceType != nil && obj.Spec.InstanceTypeRef != nil {
-		return nil, fmt.Errorf("one and only one of 'spec.InstanceTypeRef' " +
+		return fmt.Errorf("one and only one of 'spec.InstanceTypeRef' " +
 			"and 'spec.InstanceType' should be configured: both are configured")
 	}
 	if obj.Spec.InstanceType == nil && obj.Spec.InstanceTypeRef == nil {
-		return nil, fmt.Errorf("one and only one of 'spec.InstanceTypeRef' " +
+		return fmt.Errorf("one and only one of 'spec.InstanceTypeRef' " +
 			"and 'spec.InstanceType' should be configured: neither is configured")
 	}
 
 	var instanceType *string
 	if obj.Spec.InstanceType != nil {
 		if *instanceType == "" {
-			return nil, fmt.Errorf("'spec.InstanceType' should be configured with a non-empty string")
+			return fmt.Errorf("'spec.InstanceType' should be configured with a non-empty string")
 		}
 		instanceType = obj.Spec.InstanceType
 	}
 	if obj.Spec.InstanceTypeRef != nil {
+		var err error
 		instanceType, err = refsv1beta1.ResolveAlloyDBClusterType(ctx, reader, obj, obj.Spec.InstanceTypeRef)
 		if err != nil {
-			return nil, fmt.Errorf("cannot resolve `spec.InstanceTypeRef`: %w", err)
+			return fmt.Errorf("cannot resolve `spec.InstanceTypeRef`: %w", err)
 		}
 	}
 	obj.Spec.InstanceType = instanceType
+	return nil
+}
+
+func (m *instanceModel) AdapterForObject(ctx context.Context, reader client.Reader, u *unstructured.Unstructured) (directbase.Adapter, error) {
+	obj := &krm.AlloyDBInstance{}
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &obj); err != nil {
+		return nil, fmt.Errorf("error converting to %T: %w", obj, err)
+	}
+
+	id, err := krm.NewInstanceIdentity(ctx, reader, obj)
+	if err != nil {
+		return nil, err
+	}
 
 	// Get alloydb GCP client
 	gcpClient, err := m.client(ctx)
@@ -107,6 +111,7 @@ func (m *instanceModel) AdapterForObject(ctx context.Context, reader client.Read
 	return &instanceAdapter{
 		id:        id,
 		gcpClient: gcpClient,
+		reader:    reader,
 		desired:   obj,
 	}, nil
 }
@@ -119,6 +124,7 @@ func (m *instanceModel) AdapterForURL(ctx context.Context, url string) (directba
 type instanceAdapter struct {
 	id        *krm.InstanceIdentity
 	gcpClient *gcp.AlloyDBAdminClient
+	reader    client.Reader
 	desired   *krm.AlloyDBInstance
 	actual    *alloydbpb.Instance
 }
@@ -154,6 +160,10 @@ 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 {
+		return err
+	}
+
 	desired := a.desired.DeepCopy()
 	resource := AlloyDBInstanceSpec_ToProto(mapCtx, &desired.Spec)
 	if mapCtx.Err() != nil {
@@ -217,11 +227,16 @@ 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 {
+		return err
+	}
+
 	parsedActual := AlloyDBInstanceSpec_FromProto(mapCtx, a.actual)
 	if mapCtx.Err() != nil {
 		return mapCtx.Err()
 	}
 
+	// TODO: Change to CompareProtoMessage once we support all the files in the instance pb.
 	updatePaths, err := compareInstance(ctx, parsedActual, &a.desired.Spec)
 	if err != nil {
 		return err