Validation errors not actionable during resource import

[t0yv0]

What happened?

Lifting pulumi/pulumi-gitlab#293 issue to the bridge. It appears that a resource import on a bridged provider can generate code that returns TF validation errors that are not really actionable as the user cannot edit the code as suggested, as the code is abbreviated and only pointing to the imported resource.

import * as pulumi_gitlab from "@pulumi/gitlab";
new pulumi_gitlab.Project("import_test", {}, {import: "qerub/import_test", ignoreChanges: ["containerExpirationPolicy"]});

error: gitlab:index/project:Project resource 'import_test' has a problem: Conflicting configuration arguments: "container_expiration_policy.0.name_regex": conflicts with container_expiration_policy.0.name_regex_delete. Examine values at 'Project.ContainerExpirationPolicy.NameRegex'.
error: gitlab:index/project:Project resource 'import_test' has a problem: Conflicting configuration arguments: "container_expiration_policy.0.name_regex_delete": conflicts with container_expiration_policy.0.name_regex. Examine values at 'Project.ContainerExpirationPolicy.NameRegexDelete'.
error: Preview failed: one or more inputs failed to validate
error: preview failed

Expected Behavior

Importing a resource should not work harder to not generate code that's invalid (fails validators) on pulumi up; if this cannot be accomplished then an escape hatch needs to be provided so the user can make progress in this situation.

Steps to reproduce

See above.

Output of pulumi about

See original ticket.

Additional context

N/A

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

[t0yv0]

Duplicate of pulumi/pulumi-gitlab#293

[t0yv0]

Ah this was duplicated on purpose. Let me see if the issue persists on latest bridge.

[t0yv0]

Still reproduces on 6.7.0 unfortunately - this remains a problem on latest bridge.

[t0yv0]

Possibly similar issues with warnings on import:

pulumi/pulumi-aws#2318
pulumi/pulumi-gcp#844
pulumi/pulumi-linode#373

[iwahbe]

pulumi/pulumi-github#642 is also related. There is a bug in the upstream provider that manifests as a hard failure for Pulumi users.

[t0yv0]

Likely more:

pulumi/pulumi-aws#3670
pulumi/pulumi-aws#3158

[t0yv0]

I'm curious since import/refresh is under scrutiny again and pulumi/pulumi-aws#2318 is an absolutely impactful AWS import issue, let me try to repro this quickly again and see if there's any quick wins. I'm thinking that PlanResourceChange flag has also changed the import and refresh sequencing so worth seeing if it repros there.

[t0yv0]

pulumi/pulumi-aws#2318 (comment) I've got some really good information from debugging 2318 further! This works fine in TF because it doesn't do import the same way Pulumi does at all.

https://pulumi-developer-docs.readthedocs.io/en/latest/architecture/import.html is the algorithm involved here.

In the case of 2318 Check fails - check is called by the algorithm above. TF does not assume that Read results will pass Check validation. Note also that in 2318 refresh already works perfectly fine because it correctly decides that these fields are outputs, not inputs.

I'd love to fix this surgically for import only. I think we need to enable Read to return inputs that can pass Check. This essentially involves changing the behavior of https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tfbridge/schema.go#L1757

We can:

• teach this function an automated mechanism to respect conflicts and at random (alphabetically biased to be deterministic?) remove entries that would conflict; bridge decides this
• add a hook to this function so that affected resources can override or augment the behavior, say never copy both availability_zone_id and availability_zone to inputs during import; pick one - but now provider decides
• add a PropertyInfo property that would inform the behavior of this function, say never copy availability_zone_id to an input but always copy availability_zone

I think it would be great to lineup a few more issues that we're confident we can fix here to decide which route we take, but also welcoming comments/ideas here.

[t0yv0]

@Frassle @VenelinMartinov @iwahbe

[t0yv0]

@flostadler

[t0yv0]

@VenelinMartinov mentioned there's precedent for "teach this function an automated mechanism to respect conflicts and at random" so we could do this, it could be very nice. Platform may consider a generic fix pulumi/pulumi#16048 but it's not going to be as nice of an experience anyway.

[VenelinMartinov]

I was referring to our logic here around applying defaults - we look into the ConflictsWith and ExactlyOneOf constraints and if a default value would violate them, we don't apply it.

I'm not 100% we should be applying defaults there at all but the logic might also be applied in the case here.

[Frassle]

Feels like this is gonna be a similar shape of problems to what I commented on pulumi/pulumi#16048 (comment) with.

Import Read really does need to do enough work to tell the engine what the minimal valid program inputs will be. But there's probably scope for TF bridge to do quite a lot of cleanup here relative to what a normal refresh read would do.

[t0yv0]

There is indeed that possibility, but we need to be careful here; the best methods of minimizing the import property-set in the bridge given TF APIs are iterative but they can slow down the import, so there's that constraint.

[VenelinMartinov]

I believe these would all be fixed by this:

• userDataBase64 not respected for AutoScalingLaunchConfiguration pulumi-awsx#519
• Importing azuread.Application produces an invalid resource pulumi-azuread#506
• Import generates conflicting keys pulumi-converter-terraform#78
• pulumi import showing deprecated field name pulumi-github#208 - closed due to other things being fixed but the import issue is still there prob.

[t0yv0]

I did a pass on this today after we fixed ConflictsWith warnings on import specifically, as the most prominent category, I think we can close this out though there is perhaps some further work that can be done for other kinds of validation warnings. In detail below.

Closed these:
pulumi/pulumi-azuread#506
pulumi/pulumi-converter-terraform#78
pulumi/pulumi-gcp#844
pulumi/pulumi-gitlab#293

These are confirmed to be still open:

pulumi/pulumi-linode#373
pulumi/pulumi-github#642

This AWSX issue is an unrelated classic (1.0) and not a security issue so not fixing it:

pulumi/pulumi-awsx#519

This AWS issue I suspect is unrelated to the bridge and needs an AWS-level second look:
pulumi/pulumi-aws#3158

Remaining import issues that may be fixable with bridge heuristics are further tracked in:
#2028