refactor: envgroup mappings and apigee organization ref

apis/apigee/v1alpha1/environmentgroup_identity.go

@@ -24,43 +24,49 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// GoogleCloudApigeeV1EnvironmentGroupIdentity defines the resource reference to ApigeeEnvgroup, which "External" field
+// EnvironmentGroupIdentity defines the resource reference to ApigeeEnvgroup, which "External" field
 // holds the GCP identifier for the KRM object.
-type GoogleCloudApigeeV1EnvironmentGroupIdentity struct {
-	parent *GoogleCloudApigeeV1EnvironmentGroupParent
+type EnvironmentGroupIdentity struct {
+	parent *EnvironmentGroupParent
 	id     string
 }
 
-func (i *GoogleCloudApigeeV1EnvironmentGroupIdentity) String() string {
-	return fmt.Sprintf("%s/envgroups/%s", i.parent.String(), i.id)
+func (i *EnvironmentGroupIdentity) String() string {
+	return fmt.Sprintf("%s/envgroups/%s", i.parent, i.id)
 }
 
-func (i *GoogleCloudApigeeV1EnvironmentGroupIdentity) ID() string {
+func (i *EnvironmentGroupIdentity) ID() string {
 	return i.id
 }
 
-func (i *GoogleCloudApigeeV1EnvironmentGroupIdentity) Parent() *GoogleCloudApigeeV1EnvironmentGroupParent {
+func (i *EnvironmentGroupIdentity) Parent() *EnvironmentGroupParent {
 	return i.parent
 }
 
-type GoogleCloudApigeeV1EnvironmentGroupParent struct {
+type EnvironmentGroupParent struct {
 	Organization string
 }
 
-func (p *GoogleCloudApigeeV1EnvironmentGroupParent) String() string {
+func (p *EnvironmentGroupParent) String() string {
 	return "organizations/" + p.Organization
 }
 
-// New builds a GoogleCloudApigeeV1EnvironmentGroupIdentity from the Config Connector GoogleCloudApigeeV1EnvironmentGroup object.
-func NewGoogleCloudApigeeV1EnvironmentGroupIdentity(ctx context.Context, reader client.Reader, obj *ApigeeEnvgroup) (*GoogleCloudApigeeV1EnvironmentGroupIdentity, error) {
+// New builds a NewEnvironmentGroupIdentity from the Config Connector GoogleCloudApigeeV1EnvironmentGroup object.
+func NewEnvironmentGroupIdentity(ctx context.Context, reader client.Reader, obj *ApigeeEnvgroup) (*EnvironmentGroupIdentity, error) {
 	// Get Parent
-	orgRef, err := refs.ResolveOrganization(ctx, reader, obj, obj.Spec.Parent.OrganizationRef)
+	orgRef := obj.Spec.Parent.OrganizationRef
+	if orgRef == nil {
+		return nil, fmt.Errorf("no parent organization")
+	}
+
+	orgExternal, err := orgRef.NormalizedExternal(ctx, reader, obj.Namespace)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("cannot resolve organization: %w", err)
 	}
-	orgName := orgRef.OrganizationName
-	if orgName == "" {
-		return nil, fmt.Errorf("cannot resolve organization")
+
+	org, err := refs.ParseOrganizationExternal(orgExternal)
+	if err != nil {
+		return nil, fmt.Errorf("cannot parse external organization: %w", err)
 	}
 
 	resourceID := direct.ValueOf(obj.Spec.ResourceID)
@@ -72,39 +78,38 @@ func NewGoogleCloudApigeeV1EnvironmentGroupIdentity(ctx context.Context, reader
 	}
 
 	externalRef := direct.ValueOf(obj.Status.ExternalRef)
-
 	if externalRef != "" {
 		// Validate desired with actual
-		actualParent, actualResourceID, err := ParseGoogleCloudApigeeV1EnvironmentGroupExternal(externalRef)
+		actualParent, actualResourceID, err := ParseEnvironmentGroupExternal(externalRef)
 		if err != nil {
 			return nil, err
 		}
 
-		if actualParent.Organization != orgName {
+		if actualParent.Organization != org {
 			return nil, fmt.Errorf("ApigeeEnvgroup %s/%s has Spec.Parent.OrganizationRef changed, expect %s, got %s",
-				obj.GetNamespace(), obj.GetName(), actualParent.Organization, orgRef.OrganizationName)
+				obj.GetNamespace(), obj.GetName(), actualParent.Organization, org)
 		}
 		if actualResourceID != resourceID {
 			return nil, fmt.Errorf("ApigeeEnvgroup %s/%s has metadata.name or spec.resourceID changed, expect %s, got %s",
 				obj.GetNamespace(), obj.GetName(), actualResourceID, resourceID)
 		}
 	}
 
-	return &GoogleCloudApigeeV1EnvironmentGroupIdentity{
-		parent: &GoogleCloudApigeeV1EnvironmentGroupParent{
-			Organization: orgName,
+	return &EnvironmentGroupIdentity{
+		parent: &EnvironmentGroupParent{
+			Organization: org,
 		},
 		id: resourceID,
 	}, nil
 }
 
-func ParseGoogleCloudApigeeV1EnvironmentGroupExternal(external string) (parent *GoogleCloudApigeeV1EnvironmentGroupParent, resourceID string, err error) {
+func ParseEnvironmentGroupExternal(external string) (parent *EnvironmentGroupParent, resourceID string, err error) {
 	tokens := strings.Split(external, "/")
 	if len(tokens) != 4 || tokens[0] != "organizations" || tokens[2] != "envgroups" {
 		return nil, "", fmt.Errorf("format of ApigeeEnvgroup external=%q was not known (use organizations/{{organization}}/envgroups/{{envgroup}})",
 			external)
 	}
-	parent = &GoogleCloudApigeeV1EnvironmentGroupParent{
+	parent = &EnvironmentGroupParent{
 		Organization: tokens[1],
 	}
 	resourceID = tokens[3]

apis/apigee/v1alpha1/environmentgroup_reference.go

@@ -51,7 +51,7 @@ func (r *GoogleCloudApigeeV1EnvironmentGroupRef) NormalizedExternal(ctx context.
 	}
 	// From given External
 	if r.External != "" {
-		if _, _, err := ParseGoogleCloudApigeeV1EnvironmentGroupExternal(r.External); err != nil {
+		if _, _, err := ParseEnvironmentGroupExternal(r.External); err != nil {
 			return "", err
 		}
 		return r.External, nil

apis/refs/v1alpha1/organizationref.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"strings"
 
+	refsv1beta1 "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -27,6 +28,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+var (
+	GroupVersion          = schema.GroupVersion{Group: "apigeeorganizations.apigee.cnrm.cloud.google.com", Version: "v1beta1"}
+	ApigeeOrganizationGVK = GroupVersion.WithKind("ApigeeOrganization")
+)
+
+var _ refsv1beta1.ExternalNormalizer = &OrganizationRef{}
+
 type OrganizationRef struct {
 	/* The Organization selfLink, when not managed by Config Connector. */
 	External string `json:"external,omitempty"`
@@ -40,61 +48,61 @@ type Organization struct {
 	OrganizationName string
 }
 
-func (s *Organization) FullyQualifiedName() string {
-	return "organizations/" + s.OrganizationName
+func (o *Organization) FullyQualifiedName() string {
+	return "organizations/" + o.OrganizationName
 }
-func ResolveOrganization(ctx context.Context, reader client.Reader, src client.Object, ref *OrganizationRef) (*Organization, error) {
-	if ref == nil {
-		return nil, nil
+
+func (r *OrganizationRef) NormalizedExternal(ctx context.Context, reader client.Reader, otherNamespace string) (string, error) {
+	if r.External != "" && r.Name != "" {
+		return "", fmt.Errorf("cannot specify both name and external on %s reference", ApigeeOrganizationGVK.Kind)
 	}
 
-	if ref.External != "" {
-		if ref.Name != "" {
-			return nil, fmt.Errorf("cannot specify both name and external on organization reference")
-		}
+	// From given External
+	if r.External != "" {
+		external := strings.TrimPrefix(r.External, "/")
+		tokens := strings.Split(external, "/")
 
-		tokens := strings.Split(ref.External, "/")
-		if len(tokens) == 1 {
-			return &Organization{OrganizationName: tokens[0]}, nil
-		}
 		if len(tokens) == 2 && tokens[0] == "organizations" {
-			return &Organization{OrganizationName: tokens[1]}, nil
+			return r.External, nil
 		}
-		return nil, fmt.Errorf("format of organization external=%q was not known (use organization/<orgId> or <orgId>)", ref.External)
-
+		return "", fmt.Errorf("format of Organization external=%q was not known (use organization/{{orgId}})", external)
 	}
 
-	if ref.Name == "" {
-		return nil, fmt.Errorf("must specify either name or external on organization reference")
+	// From the Config Connector object
+	if r.Namespace == "" {
+		r.Namespace = otherNamespace
 	}
+	key := types.NamespacedName{Name: r.Name, Namespace: r.Namespace}
+	u := &unstructured.Unstructured{}
+	u.SetGroupVersionKind(ApigeeOrganizationGVK)
 
-	key := types.NamespacedName{
-		Namespace: ref.Namespace,
-		Name:      ref.Name,
-	}
-	if key.Namespace == "" {
-		key.Namespace = src.GetNamespace()
-	}
-
-	organization := &unstructured.Unstructured{}
-	organization.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "apigee.cnrm.cloud.google.com",
-		Version: "v1alpha1",
-		Kind:    "ApigeeOrganization",
-	})
-
-	if err := reader.Get(ctx, key, organization); err != nil {
+	if err := reader.Get(ctx, key, u); err != nil {
 		if apierrors.IsNotFound(err) {
-			return nil, k8s.NewReferenceNotFoundError(organization.GroupVersionKind(), key)
+			return "", k8s.NewReferenceNotFoundError(u.GroupVersionKind(), key)
 		}
-		return nil, fmt.Errorf("error reading referenced Organization %v: %w", key, err)
+		return "", fmt.Errorf("error reading referenced %s %s: %w", ApigeeOrganizationGVK, key, err)
 	}
 
-	orgID, err := GetResourceID(organization)
-
+	// get external from status.externalRef. This is the most trustworthy place.
+	actualExternalRef, _, err := unstructured.NestedString(u.Object, "status", "externalRef")
 	if err != nil {
-		return nil, err
+		return "", fmt.Errorf("reading status.externalRef: %w", err)
+	}
+	if actualExternalRef == "" {
+		return "", k8s.NewReferenceNotReadyError(u.GroupVersionKind(), key)
 	}
 
-	return &Organization{OrganizationName: orgID}, nil
+	r.External = actualExternalRef
+	return r.External, nil
+}
+
+func ParseOrganizationExternal(external string) (resourceID string, err error) {
+	tokens := strings.Split(external, "/")
+
+	if len(tokens) != 2 || tokens[0] != "organizations" {
+		return "", fmt.Errorf("format of ApigeeOrganization external=%q was not known (use organizations/{{organization}})",
+			external)
+	}
+	resourceID = tokens[1]
+	return resourceID, nil
 }

pkg/controller/direct/apigee/envgroup_controller.go

@@ -37,10 +37,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-const (
-	serviceDomain = "//apigee.googleapis.com"
-	ctrlName      = "apigee-envgroup-controller"
-)
+const ctrlName = "apigee-envgroup-controller"
 
 func init() {
 	registry.RegisterModel(krm.ApigeeEnvgroupGVK, NewApigeeEnvgroupModel)
@@ -72,17 +69,19 @@ func (m *modelApigeeEnvgroup) AdapterForObject(ctx context.Context, reader clien
 		return nil, fmt.Errorf("error converting to %T: %w", obj, err)
 	}
 
-	id, err := krm.NewGoogleCloudApigeeV1EnvironmentGroupIdentity(ctx, reader, obj)
+	id, err := krm.NewEnvironmentGroupIdentity(ctx, reader, obj)
 	if err != nil {
 		return nil, err
 	}
 
 	mapCtx := &direct.MapContext{}
-	desired := ApigeeEnvgroup_ToApi(mapCtx, obj)
+	desired := ApigeeEnvgroupSpec_ToApi(mapCtx, &obj.Spec)
 	if mapCtx.Err() != nil {
 		return nil, mapCtx.Err()
 	}
 
+	desired.Name = id.ID()
+
 	return &Adapter{
 		id:                   id,
 		desired:              desired,
@@ -96,7 +95,7 @@ func (m *modelApigeeEnvgroup) AdapterForURL(ctx context.Context, url string) (di
 }
 
 type Adapter struct {
-	id                   *krm.GoogleCloudApigeeV1EnvironmentGroupIdentity
+	id                   *krm.EnvironmentGroupIdentity
 	desired              *api.GoogleCloudApigeeV1EnvironmentGroup
 	actual               *api.GoogleCloudApigeeV1EnvironmentGroup
 	apigeeEnvgroupClient *apigeeEnvgroupClient
@@ -106,7 +105,7 @@ var _ directbase.Adapter = &Adapter{}
 
 // Find retrieves the GCP resource.
 // Return true means the object is found. This triggers Adapter `Update` call.
-// Return true means the object is not found. This triggers Adapter `Create` call.
+// Return false means the object is not found. This triggers Adapter `Create` call.
 // Return a non-nil error requeues the requests.
 func (a *Adapter) Find(ctx context.Context) (bool, error) {
 	log := klog.FromContext(ctx).WithName(ctrlName)