diff --git a/apis/alloydb/v1alpha1/instance_identity.go b/apis/alloydb/v1alpha1/instance_identity.go index 16542ff1ec..b0613fc4b3 100644 --- a/apis/alloydb/v1alpha1/instance_identity.go +++ b/apis/alloydb/v1alpha1/instance_identity.go @@ -17,7 +17,6 @@ package v1alpha1 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//locations//clusters//instances/)", 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 -} diff --git a/apis/alloydb/v1alpha1/instance_reference.go b/apis/alloydb/v1alpha1/instance_reference.go index e2b6055c6b..b07f519c40 100644 --- a/apis/alloydb/v1alpha1/instance_reference.go +++ b/apis/alloydb/v1alpha1/instance_reference.go @@ -17,6 +17,7 @@ package v1alpha1 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//locations//clusters//instances/)", 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 +} diff --git a/apis/alloydb/v1alpha1/instance_types.go b/apis/alloydb/v1alpha1/instance_types.go index 90987a7671..8f4c5c5daf 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"` - // +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"` diff --git a/apis/alloydb/v1beta1/instance_identity.go b/apis/alloydb/v1beta1/instance_identity.go index 9ee7c11a1a..20bae0d996 100644 --- a/apis/alloydb/v1beta1/instance_identity.go +++ b/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//locations//clusters//instances/)", 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 -} diff --git a/apis/alloydb/v1beta1/instance_reference.go b/apis/alloydb/v1beta1/instance_reference.go index 0518e62537..af24e27d1f 100644 --- a/apis/alloydb/v1beta1/instance_reference.go +++ b/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//locations//clusters//instances/)", 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 +} diff --git a/apis/alloydb/v1beta1/instance_types.go b/apis/alloydb/v1beta1/instance_types.go index 769952ecde..9d20b47bd9 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"` - // +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"` 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 72edcbdce0..bf78746d31 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 @@ -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 diff --git a/pkg/controller/direct/alloydb/instance_controller.go b/pkg/controller/direct/alloydb/instance_controller.go index 8ed840cb8e..5717cfcb79 100644 --- a/pkg/controller/direct/alloydb/instance_controller.go +++ b/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