wip:fix: don't assign external

[acpana]

The canonical example for reference handling is that we either pass in:

• the external format OR
• the name of the reference

Consider this example

k8s-config-connector/pkg/test/resourcefixture/testdata/basic/bigqueryanalyticshub/v1alpha1/bigqueryanalyticshublisting-base/create.yaml

Lines 23 to 25 in bcf1e3e

	bigQueryDatasetSource:
	datasetRef:
	name: bigquerydataset${uniqueId}

If the NormlizedExternal checks that only one of r.External or r.Name is set BUT assigns r.External at the end, future calls to this function will error out. IOW, the function is not idempotent and I think it should be because we may be calling this function repeatedly throughout the reconciliation of a resource.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign maqiuyujoyce for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[acpana]

cc @yuwenma / @gemmahou

Let me know what you think! The apis changes were mechanical so let's not index on them for now.

[acpana]

@gemmahou let's use this comment thread. The problem persists in the new refactor too.

For instance, if a code block (or control flow) calls the function twice for a reference defined with r.Name:

ref.ResolveExternal(...) // this one assigns the External value
ref.ResolveExternal(...) // this one sees the External assigned and the Name used so errors out...

The error will be cannot specify both name and external on reference

[gemmahou]

Gotcha!

I'm thinking why we don't encounter this error during direct reconciliation of current resources: Since we always use a brand new object(code) when calling the resolver, and only resolve it once. It seems unnecessary to resolve it again unless there's a reason I'm not aware of?

We must have the resolved value assigned to External because we need it when converting KRM object proto(code). Here's another example in the mapper I wrote for compute resource.

[gemmahou]

If the NormlizedExternal checks that only one of r.External or r.Name is set BUT assigns r.External at the end, future calls to this function will error out.

Could you explain more on the error and maybe share the error message? I think it's intended to resolve and assign to r.External.

I'm working on some refactoring on the resource_references.go code. New function looks like:

func (r *ComputeBackendServiceRef) ResolveExternal(ctx context.Context, reader client.Reader, otherNamespace string) error {
	// Get value from spec.ComputeBackendServiceRef.external
	if r.External != "" {
		if r.Name != "" {
			return "", fmt.Errorf("cannot specify both name and external on reference")
		}
		return nil
	}

	if r.Name == "" {
		return fmt.Errorf("must specify either name or external on reference")
	}

	// Get value 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(ComputeBackendServiceGVK)
	if err := reader.Get(ctx, key, u); err != nil {
		if apierrors.IsNotFound(err) {
			return k8s.NewReferenceNotFoundError(u.GroupVersionKind(), key)
		}
		return fmt.Errorf("reading referenced %s %s: %w", ComputeBackendServiceGVK, key, err)
	}

	// Get value from object's status.externalRef(only support for objects created by direct controller). 
	// This is the most trustworthy place.
	actualExternalRef, _, err := unstructured.NestedString(u.Object, "status", "externalRef")
	if err != nil {
		return fmt.Errorf("reading status.externalRef: %w", err)
	}
	if actualExternalRef == "" {
		return k8s.NewReferenceNotReadyError(u.GroupVersionKind(), key)
	}
	r.External = actualExternalRef
	return nil
}