Merge pull request #313 from ulucinar/capture-id

Cache the Terraform instance state returned from schema.Resource.Apply even if the returned diagnostics contain errors

pkg/controller/external_nofork.go

@@ -564,8 +564,36 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man
 	start := time.Now()
 	newState, diag := n.resourceSchema.Apply(ctx, n.opTracker.GetTfState(), n.instanceDiff, n.ts.Meta)
 	metrics.ExternalAPITime.WithLabelValues("create").Observe(time.Since(start).Seconds())
-	// diag := n.resourceSchema.CreateWithoutTimeout(ctx, n.resourceData, n.ts.Meta)
 	if diag != nil && diag.HasError() {
+		// we need to store the Terraform state from the downstream create call if
+		// one is available even if the diagnostics has reported errors.
+		// The downstream create call comprises multiple external API calls such as
+		// the external resource create call, expected state assertion calls
+		// (external resource state reads) and external resource state refresh
+		// calls, etc. Any of these steps can fail and if the initial
+		// external resource create call succeeds, then the TF plugin SDK makes the
+		// state (together with the TF ID associated with the external resource)
+		// available reporting any encountered issues in the returned diagnostics.
+		// If we don't record the returned state from the successful create call,
+		// then we may hit issues for resources whose Crossplane identifiers cannot
+		// be computed solely from spec parameters and provider configs, i.e.,
+		// those that contain a random part generated by the CSP. Please see:
+		// https://github.com/upbound/provider-aws/issues/1010, or
+		// https://github.com/upbound/provider-aws/issues/1018, which both involve
+		// MRs with config.IdentifierFromProvider external-name configurations.
+		// NOTE: The safe (and thus the proper) thing to do in this situation from
+		// the Crossplane provider's perspective is to set the MR's
+		// `crossplane.io/external-create-failed` annotation because the provider
+		// does not know the exact state the external resource is in and a manual
+		// intervention may be required. But at the time we are introducing this
+		// fix, we believe associating the external-resource with the MR will just
+		// provide a better UX although the external resource may not be in the
+		// expected/desired state yet. We are also planning for improvements on the
+		// crossplane-runtime's managed reconciler to better support upjet's async
+		// operations in this regard.
+		if !n.opTracker.HasState() { // we do not expect a previous state here but just being defensive
+			n.opTracker.SetTfState(newState)
+		}
 		return managed.ExternalCreation{}, errors.Errorf("failed to create the resource: %v", diag)
 	}