Skip to content

Finish up support for built-in providers#24530

Merged
apparentlymart merged 2 commits intof-init-new-installerfrom
f-internal-providers
Apr 2, 2020
Merged

Finish up support for built-in providers#24530
apparentlymart merged 2 commits intof-init-new-installerfrom
f-internal-providers

Conversation

@apparentlymart
Copy link
Copy Markdown
Contributor

An earlier changeset established the addressing scheme for "built-in" providers in the addrs package, which was intended to represent the special providers that are built in to Terraform itself, rather than separately installable, which we defined to live in the namespace terraform.io/builtin/. The terraform provider is currently the only built-in provider, and it was made available in the set of provider factories passed to terraform.NewContext already in an earlier change.

This PR introduces that concept into the provider installer too, both so that it knows not to try to install them the "normal" way and so it can verify that the requested built-in providers are valid and thus we retain the characteristic that Installer.EnsureProviderVersions will return without error only once it's ensured that all of the requested providers are available.


This PR also introduces a special case: an unqualified reference to the name "terraform" is implied to mean terraform.io/builtin/terraform rather than registry.terraform.io/hashicorp/terraform. This is important because the one on registry.terraform.io is an old release we've retained to preserve compatibility with older versions of Terraform that didn't have this provider built-in, but existing configurations that lack a specific source declaration ought to be selecting the built-in one rather than this stale separate one.

This special case is handled entirely inside the configs and internal/earlyconfig packages (using addrs.ImpliedProviderForUnqualifiedType to encapsulate the actual logic), so the rest of Terraform doesn't need to worry about it and can just deal generically in addrs.Provider as before.


This is ongoing work targeting our integration branch represented as #24477, so the full test suite doesn't pass here but the ones related to internal providers in terraform init do now pass, having been updated here. More test fixes will follow in later commits, and improved test coverage for internal/providercache itself will follow once we get the already-existing tests back in good shape again.

@apparentlymart apparentlymart requested a review from a team April 2, 2020 00:07
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Apr 2, 2020
Copy link
Copy Markdown
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

These changes make sense to me! 👍 Thanks as always for the excellent commentary.

I found a couple of other direct calls to NewDefaultProvider which haven't been updated, and I'm not sure why not:

terraform/configs/module.go

Lines 509 to 514 in ddd6382

func (m *Module) ProviderForLocalConfig(pc addrs.LocalProviderConfig) addrs.Provider {
if provider, exists := m.ProviderRequirements[pc.LocalName]; exists {
return provider.Type
}
return addrs.NewDefaultProvider(pc.LocalName)
}

// GraphNodeProviderConsumer
func (n *NodeAbstractResource) Provider() addrs.Provider {
if n.Config != nil {
return n.Config.Provider
}
return addrs.NewDefaultProvider(n.Addr.Resource.ImpliedProvider())
}

Was this intentional?

@apparentlymart
Copy link
Copy Markdown
Contributor Author

It does look like both of those ought to be updated too, as far as I can tell. It's kinda unfortunate that we have a fallback there in the terraform package too cause I think we'd originally hoped to keep all of that sort of thing in the configs and earlyconfig packages, but I'm sure it's there for a good reason I don't fully understand yet so I'll update it where it is and save discussions about why for another day...

(The one in configs was purely an oversight though, and I'll fix it.)

This encapsulates the logic for selecting an implied FQN for an
unqualified type name, which could either come from a local name used in
a module without specifying an explicit source for it or from the prefix
of a resource type on a resource that doesn't explicitly set "provider".

This replaces the previous behavior of just directly calling
NewDefaultProvider everywhere so that we can use a different implication
for the local name "terraform", to refer to the built-in terraform
provider rather than the stale one that's on registry.terraform.io for
compatibility with other Terraform versions.
Built-in providers are special providers that are distributed as part of
Terraform CLI itself, rather than being installed separately. They always
live in the terraform.io/builtin/... namespace so it's easier to see that
they are special, and currently there is only one built-in provider named
"terraform".

Previous commits established the addressing scheme for built-in providers.
This commit makes the installer aware of them to the extent that it knows
not to try to install them the usual way and it's able to report an error
if the user requests a built-in provider that doesn't exist or tries to
impose a particular version constraint for a built-in provider.

For the moment the tests for this are the ones in the "command" package
because that's where the existing testing infrastructure for this
functionality lives. A later commit should add some more focused unit
tests here in the internal/providercache package, too.
@mildwonkey
Copy link
Copy Markdown
Contributor

I will look into that remaining call in terraform: it's very likely a holdout that isn't necessary any more!

@apparentlymart apparentlymart merged commit ab867f1 into f-init-new-installer Apr 2, 2020
@apparentlymart apparentlymart deleted the f-internal-providers branch April 2, 2020 17:43
@ghost
Copy link
Copy Markdown

ghost commented May 3, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cli enhancement sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants