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.

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

Validation-aware defaults application for Plugin Framework based providers #1095

Open
t0yv0 opened this issue May 9, 2023 · 5 comments
Open
Labels
area/plugin-framework Support for Plugin Framework based providers kind/enhancement Improvements or new features

Comments

@t0yv0
Copy link
Member

t0yv0 commented May 9, 2023

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

The v3/tfbrdige supports awareness of ExactlyOneOf and ConflictsWith properties in defaults application. That is it will not apply a default value even if it is specified if that would cause validation to fail.

Unfortunately with migration to Plugin Framework there is no easy-to-parse information on the presence of ExactlyOneOf and ConflictsWith. Instead there are validators, something like this:

                                boolvalidator.ConflictsWith(
                                    path.MatchRelative().AtParent().AtName("example_attribute_one"),
                                    path.MatchRelative().AtParent().AtName("example_attribute_two"),
                                    path.MatchRelative().AtParent().AtName("example_attribute_three"),
                                ),

The bridge code is not well positioned to infer these validators, but perhaps there could still be a way forward in iteratively unapplying defaults until all the validators pass, to respect these validators.

Note that as of now the scope of default value application for plugin framework code is smaller than the v3 tfbridge code - it only applies DefaultInfo derived defaults and does not try to replicate or mimic or interact with the way TF-defined default values are applied (plan modifiers or other mechanisms).

Affected area/feature

@t0yv0 t0yv0 added kind/enhancement Improvements or new features area/plugin-framework Support for Plugin Framework based providers labels May 9, 2023
@lukehoban
Copy link
Contributor

@t0yv0 I'm trying to parse this:

Unfortunately with migration to Plugin Framework there is no easy-to-parse information on the presence of ExactlyOneOf and ConflictsWith. Instead there are validators, something like this:

Does this mean there is a way for Pulumi providers to opt in to the correct behaviour for ConflictsWith + Defaults, but that it requires modifications in the Pulumi provider resources.go? Or that there is no support at all, and that's what this issue is tracking?

If the former, where is there an example of how provider authors can apply this today?

For example, it sounds like pulumi/pulumi-azuredevops#38 is an example of the need for this. What is required to fix that issue? (Or is it a dupe of this?)

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 20, 2023

Currently https://github.com/pulumi/pulumi-terraform-bridge/blob/main/pf/tfbridge/provider_check.go#L65 proactively applies Pulumi-level defaults and if these defaults lead to a conflict, there's no good way that I know for the provider author to stop that from happening, short of removing the defaults from schema, and perhaps hand-coding how to apply the defaults intelligently into PreCheckCallback.

The issue is tracking making this better so that Pulumi stops applying defaults when that leads to a conflict directly. At the time of writing there wasn't a quick and easy way to do this, that is infer conflict metadata from the PF code. Perhaps something changed here with recent versions of Plugin Framework, that's worth another look.

Another option is to try to to detect conflicts iteratively by applying pulumi defaults, running validations, detecting conflicts and attempting to automatically correct and retry by backing off default application.

Another option is to ask the user to double-state the property conflicts again in the bridge, if we totally cannot infer them.

@lukehoban
Copy link
Contributor

Another option is to ask the user to double-state the property conflicts again in the bridge, if we totally cannot infer them.

I suspect if there is not a simple way to address this otherwise, we should do this so that provider at least have some recourse for fixing bugs like this without forking.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 15, 2023

CC @VenelinMartinov

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 15, 2023

Looking at this again with Venelin. The azure-devops example was not using plugin framework. Do we have some specific examples that do use plugin framework?

We're having some insights here that pulumi defaults and TF defaults are treated differently. In PF bridge case the sequence is:

  • first apply pulumi defaults
  • then run TF validators
  • then apply TF defaults (as part of PlanResourceChange)

This may be.. reasonable except as the ticket says pulumi defaults do not stop themselves from applying when that would cause a validation failure. Would be curious to see practical examples.

IN contrast, for SDKv2 bridge the situation is a lot less tenable:

  • first we apply pulumi and TF defaults as part of MakeTerraformInputs
  • then we run TF validators

This order is clearly different as Venelin pointed out from the canonical TF order. Now contemplating fixing that to a PF-like operation ordering, as it seems that Pulumi defaults such as autonames still need to intentionally apply themselves before TF validation pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/plugin-framework Support for Plugin Framework based providers kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

2 participants