From bc6bb640fad989600ddfa9d1b61db9b7cbf4ce76 Mon Sep 17 00:00:00 2001 From: camilabustos Date: Fri, 20 Dec 2024 20:42:36 +0000 Subject: [PATCH] refactor: envgroup mappings and apigee organization ref --- .../v1alpha1/environmentgroup_identity.go | 55 ++++++------ .../v1alpha1/environmentgroup_reference.go | 2 +- apis/apigee/v1alpha1/zz_generated.deepcopy.go | 18 ++-- apis/refs/v1alpha1/helper.go | 43 --------- apis/refs/v1alpha1/organizationref.go | 88 ++++++++++--------- .../direct/apigee/envgroup_controller.go | 15 ++-- .../direct/apigee/envgroup_mappings.go | 9 +- 7 files changed, 97 insertions(+), 133 deletions(-) delete mode 100644 apis/refs/v1alpha1/helper.go diff --git a/apis/apigee/v1alpha1/environmentgroup_identity.go b/apis/apigee/v1alpha1/environmentgroup_identity.go index 0cb2b286e0..961e699ccc 100644 --- a/apis/apigee/v1alpha1/environmentgroup_identity.go +++ b/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,17 +78,16 @@ 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", @@ -90,21 +95,21 @@ func NewGoogleCloudApigeeV1EnvironmentGroupIdentity(ctx context.Context, reader } } - 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] diff --git a/apis/apigee/v1alpha1/environmentgroup_reference.go b/apis/apigee/v1alpha1/environmentgroup_reference.go index b6faa3a87c..91ce96517d 100644 --- a/apis/apigee/v1alpha1/environmentgroup_reference.go +++ b/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 diff --git a/apis/apigee/v1alpha1/zz_generated.deepcopy.go b/apis/apigee/v1alpha1/zz_generated.deepcopy.go index d7295ec862..f49e1ebde6 100644 --- a/apis/apigee/v1alpha1/zz_generated.deepcopy.go +++ b/apis/apigee/v1alpha1/zz_generated.deepcopy.go @@ -180,36 +180,36 @@ func (in *ApigeeEnvgroupStatus) DeepCopy() *ApigeeEnvgroupStatus { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *GoogleCloudApigeeV1EnvironmentGroupIdentity) DeepCopyInto(out *GoogleCloudApigeeV1EnvironmentGroupIdentity) { +func (in *EnvironmentGroupIdentity) DeepCopyInto(out *EnvironmentGroupIdentity) { *out = *in if in.parent != nil { in, out := &in.parent, &out.parent - *out = new(GoogleCloudApigeeV1EnvironmentGroupParent) + *out = new(EnvironmentGroupParent) **out = **in } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GoogleCloudApigeeV1EnvironmentGroupIdentity. -func (in *GoogleCloudApigeeV1EnvironmentGroupIdentity) DeepCopy() *GoogleCloudApigeeV1EnvironmentGroupIdentity { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EnvironmentGroupIdentity. +func (in *EnvironmentGroupIdentity) DeepCopy() *EnvironmentGroupIdentity { if in == nil { return nil } - out := new(GoogleCloudApigeeV1EnvironmentGroupIdentity) + out := new(EnvironmentGroupIdentity) in.DeepCopyInto(out) return out } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *GoogleCloudApigeeV1EnvironmentGroupParent) DeepCopyInto(out *GoogleCloudApigeeV1EnvironmentGroupParent) { +func (in *EnvironmentGroupParent) DeepCopyInto(out *EnvironmentGroupParent) { *out = *in } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GoogleCloudApigeeV1EnvironmentGroupParent. -func (in *GoogleCloudApigeeV1EnvironmentGroupParent) DeepCopy() *GoogleCloudApigeeV1EnvironmentGroupParent { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EnvironmentGroupParent. +func (in *EnvironmentGroupParent) DeepCopy() *EnvironmentGroupParent { if in == nil { return nil } - out := new(GoogleCloudApigeeV1EnvironmentGroupParent) + out := new(EnvironmentGroupParent) in.DeepCopyInto(out) return out } diff --git a/apis/refs/v1alpha1/helper.go b/apis/refs/v1alpha1/helper.go deleted file mode 100644 index 30973f70f8..0000000000 --- a/apis/refs/v1alpha1/helper.go +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright 2024 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package v1alpha1 - -import ( - "fmt" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" -) - -func GetResourceID(u *unstructured.Unstructured) (string, error) { - resourceID, _, err := unstructured.NestedString(u.Object, "spec", "resourceID") - if err != nil { - return "", fmt.Errorf("reading spec.resourceID from %v %v/%v: %w", u.GroupVersionKind().Kind, u.GetNamespace(), u.GetName(), err) - } - if resourceID == "" { - resourceID = u.GetName() - } - return resourceID, nil -} - -func GetLocation(u *unstructured.Unstructured) (string, error) { - location, _, err := unstructured.NestedString(u.Object, "spec", "location") - if err != nil { - return "", fmt.Errorf("reading spec.location from %v %v/%v: %w", u.GroupVersionKind().Kind, u.GetNamespace(), u.GetName(), err) - } - if location == "" { - return "", fmt.Errorf("spec.location not set in %v %v/%v: %w", u.GroupVersionKind().Kind, u.GetNamespace(), u.GetName(), err) - } - return location, nil -} diff --git a/apis/refs/v1alpha1/organizationref.go b/apis/refs/v1alpha1/organizationref.go index 92963e6bb5..14008ba63c 100644 --- a/apis/refs/v1alpha1/organizationref.go +++ b/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/ or )", 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 } diff --git a/pkg/controller/direct/apigee/envgroup_controller.go b/pkg/controller/direct/apigee/envgroup_controller.go index da1a6ae972..bf9131bc68 100644 --- a/pkg/controller/direct/apigee/envgroup_controller.go +++ b/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) diff --git a/pkg/controller/direct/apigee/envgroup_mappings.go b/pkg/controller/direct/apigee/envgroup_mappings.go index 4be5347f8e..958581b826 100644 --- a/pkg/controller/direct/apigee/envgroup_mappings.go +++ b/pkg/controller/direct/apigee/envgroup_mappings.go @@ -54,17 +54,12 @@ func ApigeeEnvgroupSpec_FromApi(mapCtx *direct.MapContext, in *api.GoogleCloudAp return out } -func ApigeeEnvgroup_ToApi(mapCtx *direct.MapContext, in *krm.ApigeeEnvgroup) *api.GoogleCloudApigeeV1EnvironmentGroup { +func ApigeeEnvgroupSpec_ToApi(mapCtx *direct.MapContext, in *krm.ApigeeEnvgroupSpec) *api.GoogleCloudApigeeV1EnvironmentGroup { if in == nil { return nil } out := &api.GoogleCloudApigeeV1EnvironmentGroup{} - out.Hostnames = in.Spec.Hostnames + out.Hostnames = in.Hostnames - if in.Spec.ResourceID != nil { - out.Name = *in.Spec.ResourceID - } else { - out.Name = in.Name - } return out }