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

Tabulate renaming decisions #649

Merged
merged 1 commit into from Dec 6, 2022
Merged

Tabulate renaming decisions #649

merged 1 commit into from Dec 6, 2022

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Nov 18, 2022

On top of: #661

Introduce Renames type to tabulate name mappings from Pulumi to Terraform.

The motivation for this is translating names in bridged providers at runtime, specifically intended for use with Plugin Framework providers.

The alternative to having this data tabulated is using functions to compute the names:

This functions need data derived from MarshallableProviderInfo metadata passed into their inputs to work correctly. This I think is what happens in today's production providers. These functions also need form a bijection - this holds in practice but it seems like there might be corner cases where the names do not turn around.

Here is why I think explicit rename tables are an improvement (and I'd like to move forward with introducing this and using it for PF):

  • foolproof to interpret correctly

  • only TerraformToPulumiName function is eventually needed, PulumiToTerraformName is not needed and does not need to correctly form a bijection; tables are invertible

  • lets PF bridge to avoids shim dependency at runtime (though this may not work out)

A counter argument on why the tables are detrimental:

  • embedding the table into a provider binary increases its size; need to measure impact on pulumi-azure-native

@github-actions
Copy link

Diff for pulumi-random with merge commit 8e7c881

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 8e7c881

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 8e7c881

@github-actions
Copy link

Diff for pulumi-azure with merge commit 8e7c881

@github-actions
Copy link

Diff for pulumi-aws with merge commit 8e7c881

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 073c431

@github-actions
Copy link

Diff for pulumi-random with merge commit 073c431

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 073c431

@github-actions
Copy link

Diff for pulumi-azure with merge commit 073c431

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 7c67373

@github-actions
Copy link

Diff for pulumi-random with merge commit 7c67373

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 7c67373

@github-actions
Copy link

Diff for pulumi-aws with merge commit 073c431

@github-actions
Copy link

Diff for pulumi-azure with merge commit 7c67373

@github-actions
Copy link

Diff for pulumi-aws with merge commit 7c67373

@github-actions
Copy link

Diff for pulumi-random with merge commit fa41c25

@github-actions
Copy link

Diff for pulumi-azuread with merge commit fa41c25

@github-actions
Copy link

Diff for pulumi-gcp with merge commit fa41c25

@github-actions
Copy link

Diff for pulumi-azure with merge commit fa41c25

@github-actions
Copy link

Diff for pulumi-aws with merge commit fa41c25

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what consumes this table? It sounds like the table is only consumed by the provider that embeds it.

I'm a little concerned that this will prevent pulumi from round-tripping the schema. I'm not sure that we do, but it seems like a capability we will decide to want.

pkg/tfgen/renames.go Outdated Show resolved Hide resolved
@Frassle
Copy link
Member

Frassle commented Nov 20, 2022

What if we tabulated the renaming decisions and stored it in metadata, perhaps as an extension to PackageSpec? Exploring this in this draft PR, though the approach may not be final.

This should not be part of schema, it's only going to be used by two things.
One the provider itself, which is what defined the naming logic anyway so should be able to keep track of the renames however it likes (if a full rename table is the best way then fine do that)
Two the program conversion system, which would pull this data from the ProviderInfo struct, so the rename table should probably be thrown into there.

That second part is what would interact with the conversion mapper changes I'm making in pulumi/pulumi#11378, and is a good prompt that we probably need to version the data format there for plugins even though to the engine its just bytes.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 21, 2022

I don't mind not mixing this data into the schema, that is just a technical convenience to achieve the effect of precomputing the data and baking it into the provider for runtime access:

pulumi-tfgen-foo --> renames.json --[embed] --> pulumi-resource-foo

Can totally use another route to get that to happen.

Now extending ProviderInfo does not make sense to me at the moment, ProviderInfo seems to be authored by hand or some semi-automated process to configure tfgen. So ProviderInfo is an input to tfgen, while Renames are an output of tfgen. Renames are f(input, tfgen.logic).

program conversion system, which would pull this data from the ProviderInfo struct, so the rename table should probably be thrown into there.

The part I want to understand here is versioning/releases. Currently when we release a new version of pulumi-provider-aws say, we get a new schema built and as well as a new provider binary, and we track bw-compatibility there. I'd like to think TF renamings are published and versioned as part of this process.

If this is how it's versioned, any tool like tf2pulumi should be able to grab any version of pulumi-provider-aws and respect that version's renaming decisions, even if those are changing between versions.

So perhaps Program conversion system should instead query the provider for this? Or how does tf2pulumi read ProviderInfo, does it query the provider for it?

@t0yv0 t0yv0 requested a review from iwahbe November 21, 2022 15:17
@Frassle
Copy link
Member

Frassle commented Nov 21, 2022

Or how does tf2pulumi read ProviderInfo, does it query the provider for it?

Yes I'm adding a new GetMapping method to the provider interface for providers to implement so they can send data to tf2pulumi.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 2, 2022

I would like to make use of this on the t0yv0/tfpf-stubs branch right away, with a possible refactor of the current bridge later on. The new features include "structured types like objects". I cannot quite work out how existing code supports inflecting the properties of these at runtime, although tfgen does support a form of object types arising in nested blocks.

So for now rewriting how current tfbridge and tf2pulumi inflect names to use this table is out of scope of this PR. Presumably tf2pulumi is naming things right as implemented at the moment. But such a refactor would be directionally aligned with this PR. There's two options:

  1. continue with current approach to inflecting names; TerraformToPulumiName + MarshallableProviderInfo; drop this PR; adopt this approach also in the PluginFramework version.

Here is a demonstration on how this works for resource properties:

https://github.com/pulumi/pulumi-terraform-bridge/blob/b6bc4c4d8721895699b1b1d376e31b21a38a8b87/pkg/tfpfbridge/tests/schema_test.go#L37-#L37

It's not too bad for resources, but how does it work for named object types (which also may have inflected properties presumably)?

I found something like this but it does not seem to be used at the right place?

  1. move away from TerraformToPulumiName to tabulated renames eventually. Use it in PF branch first. Later refactor the bridge to use it. Expose the tables to tf2pulumi - extending MarshallablePluginInfo or the GetSchema() response all sounds reasonable, I'm not as familiar with the details yet.

2 is not immediately beneficial but I think much more fool-proof long term if we can afford storing these tables.

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-random with merge commit 4e02ab5

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-azuread with merge commit 4e02ab5

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-gcp with merge commit 4e02ab5

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-random with merge commit 3319aae

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-azuread with merge commit 3319aae

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-azure with merge commit 4e02ab5

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-gcp with merge commit 3319aae

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-azure with merge commit 3319aae

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

Diff for pulumi-aws with merge commit 4e02ab5

@@ -0,0 +1,331 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we will have this file now available in the repo and linked into the provider binary? Just a note that we will need to be mindful to support rolling this out across the providers as part of a rollout for tf bridge. Might involve some ci-mgmt changes etc.

Base automatically changed from t0yv0/more-precise-paths to master December 6, 2022 17:19
})
}

// Like Main but allows to customize the generation logic past the parsing of cmd-line arguments.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guineveresaenger and @viveklak I had questions about this in a few places, answering here. This annoying looking logic supports a divergent behavior between legacy bridged providers and TFPF providers around generating the renames.json file.

So that:

  • legacy bridged providers continue using Main that does not generate renames.json
  • whereas TFPF bridged providers use a MainWithCustomGenerate that goes ahead and generates the renames.json

SO the good news is that accepting this PR does not generate any downstream work in the bridged provider boilerplates, but the bad news perhaps is that this does not look particularly elegant or clear; if you have a better idea of where to make the option of generating renames configurable LMK I can clean this up in a follow-up PR.

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Diff for pulumi-random with merge commit 3218a7e

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Diff for pulumi-azuread with merge commit 3218a7e

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Diff for pulumi-gcp with merge commit 3218a7e

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Diff for pulumi-azure with merge commit 3218a7e

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Diff for pulumi-aws with merge commit 3218a7e

@t0yv0 t0yv0 merged commit 65a306c into master Dec 6, 2022
@t0yv0 t0yv0 deleted the t0yv0/rename-tables branch December 6, 2022 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants