Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Dec 18, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion pkg/controller/external_nofork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
Loading