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

Explicit provider installation source configuration #24728

Merged
merged 14 commits into from
Apr 23, 2020

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented Apr 21, 2020

Until now, the set of places where Terraform's automatic provider installer (part of terraform init) has looked for available providers was generated based on a set of rules inside Terraform CLI, not directly customizable by the end-user.

The goal of this PR is to allow the user to optionally override those implied defaults in the CLI config, to allow for explicit control of source locations in special situations such as when Terraform is running in automation.

For example, the following configuration achieves the same effect as setting -plugin-dir=/tmp/example on the terraform init command line, where Terraform will only search in the given directory:

provider_installation {
  filesystem_mirror {
    path = "/tmp/example"
  }
}

The following configuration disables all of the local directory search locations and forces all providers to be installed solely from their origin provider registries:

provider_installation {
  direct {}
}

If the user knows that in-house providers are not actually available at an origin provider registry and can be installed only from a local directory, they might say something like this:

provider_installation {
  filesystem_mirror {
    include = ["tf.example.com/awesomecorp/*"]
  }
  direct {
    exclude = ["tf.example.com/awesomecorp/*"]
  }
}

This also includes some syntax for specifying network-based mirrors (on HTTP servers), but the actual implementation of that will follow in a later PR.


This configuration structure of having a sequence of blocks of different types where the total order is significant is not something HCL 1's decoder can represent, so unlike the rest of the cliconfig package this part is implemented directly using HCL 1's AST API where we can have more control over how we interpret the physica configuration structure.

That has the nice side-effect of allowing us to replicate some of the strictness we'd normally expect from the HCL 2 decoder, which I did here mainly to ease the later migration of the CLI config to HCL 2 at least for this new part of the CLI configuration. (The rest remains rather lax, as before.)


This includes an initial draft of documentation for this new feature of the CLI configuration. This is mainly intended as a placeholder for now, because there are other documentation updates pending for the new provider namespacing and installation scheme and we'll likely want to revise these docs to better complement the broader documentation once it's written.


While working on this I noticed that the TF_CLI_CONFIG_FILE environment variable we implemented a while back to allow the user to temporarily override the CLI config search location was not disabling the CLI configuration directory, and so it was ending up making a strange blend of overridden and standard options.

There's a commit in this set that fixes that bug, so that TF_CLI_CONFIG_FILE completely disables all of the normal CLI configuration file search locations. That makes TF_CLI_CONFIG_FILE more useful as a way to isolate wrapper script behavior from ambient CLI configuration, but it is a breaking change we'll need to note in the changelog once we merge this.

This new CLI config block type allows explicitly specifying where
Terraform should look to find provider plugins for installation. This is
not used anywhere as of this commit, but in a future commit we'll change
package main to treat the presence of a block of this type as a request
to disable the default set of provider sources and use these explicitly-
specified ones instead.
This is a placeholder for later implementation of a mirror source that
talks to a particular remote HTTP server and expects it to implement the
provider mirror protocol.
If the CLI configuration contains a provider_installation block then we'll
use the source configuration it describes instead of the implied one we'd
build otherwise.
@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #24728 into master will decrease coverage by 0.03%.
The diff coverage is 45.93%.

Impacted Files Coverage Δ
internal/getproviders/http_mirror_source.go 0.00% <0.00%> (ø)
provider_source.go 22.44% <4.54%> (-39.32%) ⬇️
main.go 38.62% <9.09%> (-2.16%) ⬇️
command/cliconfig/cliconfig.go 49.39% <41.66%> (+1.39%) ⬆️
command/cliconfig/provider_installation.go 69.23% <69.23%> (ø)
dag/marshal.go 52.27% <0.00%> (-1.14%) ⬇️

@apparentlymart apparentlymart requested a review from a team April 23, 2020 01:44
@apparentlymart apparentlymart self-assigned this Apr 23, 2020
@apparentlymart apparentlymart marked this pull request as ready for review April 23, 2020 01:45
}
} else {
log.Printf("Not reading CLI config directory because config location is overridden by environment variable")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a log level? "[INFO]" (or whatever you deem appropriate) at the start of the string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like some other log lines in this area don't tend to have it, but I suppose I don't really know why that is, so I will add levels to all the new ones I've added around here.

@@ -136,6 +153,13 @@ func loadConfigFile(path string) (*Config, tfdiags.Diagnostics) {
return result, diags
}

// Deal with the provider_installation block, which is not handled using
// DecodeObject because its structure is not compatible with the
Copy link
Contributor

Choose a reason for hiding this comment

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

I started to write a comment that I'd like to see more detail here for future readers (ie, me) but found it in the function instead so 🎉 thank you!

// decoder would be in terms of exactly what configuration shape it is
// expecting.
//
// Note that this function wants the top-level file object which might or
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 SUCH GOOD DOCUMENTATION thank you

main.go Outdated
@@ -168,7 +169,22 @@ func wrappedMain() int {
// direct from a registry. In future there should be a mechanism to
// configure providers sources from the CLI config, which will then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment can be updated now.

may not be able to access an origin registry due to firewall restrictions
within your organization or your locality.

To allow using Terraform in these situations, there are some alternative
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nitpick (and totally ok if you don't agree): it's more specific and accurate to say this features allows installing/using terraform providers in these situations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! I think I was assuming that Terraform is useless if you can't install providers, but no need to be vague about it.

@mildwonkey
Copy link
Contributor

This looks great! I left a couple of small comments, nothing that needs to block you from merging this.

I have two larger questions about this new feature that are out of scope for this PR, but I want to write them down somewhere before I forget:

  1. Do you think we should check for (relatively) obvious conflicts in the provider_installation block? Something like the example below, where the include and exclude attributes are the same:
  network_mirror {
    url     = "https://tf-Mirror.example.com/"
    include = ["registry.terraform.io/*/*"]
    exclude = ["registry.Terraform.io/*/*"]
  }

I can imagine that there are too many possible permutations to make checking this for the user onerous - at the very least we can wait and see what kind of edge cases and issues pop up.

  1. We've talked about the provider developer experience and a few different ideas for simplifying the process. What do you think about adding a specific attribute (exclusive to filesystem_mirrors) to let provider developers point directly to a binary, forgoing the need to create a directory structure?

I'm making this up as I go and have zero attachment to the wording or syntax, but something that conveys my intent:

  filesystem_mirror {
    include = ["tf.example.com/awesomecorp/*"]

    # this is my awkward attempt to only use attributes instead of blocks
    exact = ["{provider_source}:{/path/to/binary}"]

    # for example:  
    exact = ["example.com/myorg/happcloud":"$HOME/go/bin"]
  }

We don't have to discuss these questions here!

When we originally introduced this environment variable it was intended to
solve for the use-case where a particular invocation of Terraform needs
a different CLI configuration than usual, such as if Terraform is being
run as part of an automated test suite or other sort of automated
situation with different needs than normal use.

However, we accidentally had it only override the original singleton CLI
config file, while leaving the CLI configuration directory still enabled.
Now we'll take the CLI configuration out of the equation too, so that only
the single specified configuration file and any other environment-sourced
settings will be included.
An earlier commit added a redundant stub for a new network mirror source
that was already previously stubbed as HTTPMirrorSource.

This commit removes the unnecessary extra stub and changes the CLI config
handling to use it instead. Along the way this also switches to using a
full base URL rather than just a hostname for the mirror, because using
the usual "Terraform-native service discovery" protocol here doesn't isn't
as useful as in the places we normally use it (the mirror mechanism is
already serving as an indirection over the registry protocol) and using
a direct base URL will make it easier to deploy an HTTP mirror under
a path prefix on an existing static file server.
…ation

In the first pass of implementing this it was strict about what arguments
are allowed inside source blocks, but that was counter to our usual design
principles for CLI config where we tend to ignore unrecognized things to
allow for some limited kinds of future expansion without breaking
compatibility with older versions of Terraform that will be sharing the
same CLI configuration files with newer versions.

However, I'd removed the tracking of that prior to the initial commit. I
missed some leftover parts when doing that removal, so this cleans up the
rest of it.
Unfortunately in the user model the noun "source" is already used for the
argument in the required_providers block to specify which provider to use,
so it's confusing to use the same noun to also refer to the method used to
obtain that provider.

In the hope of mitigating that confusion, here we use the noun "method",
as in "installation method", to talk about the decision between getting
a provider directly from its origin registry or getting it from some
mirror. This is distinct from the provider's "source", which is the
location where a provider _originates_ (prior to mirroring).

This noun is also not super awesome, but better than overloading an
existing term in the same feature.
The CLI config can be written in both native HCL and HCL JSON syntaxes, so
the provider_installation block must be expressible using JSON too. Our
previous checks to approximate HCL 2-level strictness were too strict for
HCL JSON where things are more ambiguous even in HCL 2, so this includes
some additional relaxations if we detect that we're decoding an AST
produced from a JSON file.

This is still subject to the quirky ways HCL 1 handles JSON though, so
the JSON value must be structured in a way that doesn't trigger HCL's
heuristics that try to guess what is a block and what is an attribute.
(This is the issue that HCL 2 fixes by always decoding using a schema;
there's more context on this in:
  https://log.martinatkins.me/2019/04/25/hcl-json/ )
This exercises the ability to customize the installation methods used by
the provider plugin installer, in this case forcing the use of a custom
local directory with a result essentially the same as what happens when
you pass -plugin-dir to "terraform init".
Previously we were incorrectly using the Include configuration for both
the include and exclude list, making the include portion totally
ineffective.
…onfig

This is an initial draft of documentation for this new feature of the
CLI configuration. This is mainly intended as a placeholder for now,
because there are other documentation updates pending for the new provider
namespacing and installation scheme and we'll likely want to revise these
docs to better complement the broader documentation once it's written.
These were being used in an earlier iteration of the provider installation
configuration but it was all collapsed down into a single
ProviderInstallationMethod type later, making these redundant.
@apparentlymart apparentlymart merged commit 1ce3c60 into master Apr 23, 2020
@apparentlymart apparentlymart deleted the f-explicit-provider-source branch April 23, 2020 17:58
@apparentlymart
Copy link
Contributor Author

@mildwonkey and I discussed the follow-on questions above in an offline discussion today. I just wanted to capture the high-level conclusions here for posterity:

  1. We will not have any explicit checks for overlapping/conflicting declarations at first because the wildcard support makes that tricky to do robustly and we want to see what sorts of problems tend to arise in practice before spending the time to research and implement such checks.

  2. We will be having a follow-up discussion with provider developers about a few different options for how to approximate the old provider development workflow while giving Terraform the metadata it needs to understand how to use a locally-built provider. A CLI configuration option for this is one of the ideas on the table, but it's not clear that this would be the most ergonomic answer because it could make it easy to accidentally leave temporary development configuration in your main CLI config and then be confused later when terraform init installs something other than what it would do by default. We'll review a few different options and see which one seems best to implement for the 0.13 release.

@ghost
Copy link

ghost commented May 24, 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 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants