Skip to content

Network-based Mirrors for Provider Installation#25999

Merged
apparentlymart merged 2 commits intomasterfrom
f-provider-network-mirrors
Aug 26, 2020
Merged

Network-based Mirrors for Provider Installation#25999
apparentlymart merged 2 commits intomasterfrom
f-provider-network-mirrors

Conversation

@apparentlymart
Copy link
Copy Markdown
Contributor

Our initial v0.13.0 release included support for mirroring providers in the local filesystem, as an alternative to retrieving them from the registry host indicated in the provider source address. We had intended to also support mirrors implemented as HTTP servers, thus allowing a mirror to potentially be set up once and shared across a number of different calling hosts, but that was a stretch goal for v0.13.0 and ultimately wasn't completed in time.

This changeset implements the network_mirror installation method, using a newly-defined protocol that is designed to be relatively easy to implement using a static website system, particularly if you upload to it the result of running terraform providers mirror.

An important use-case for network mirrors is in providing a different installation source for providers when running Terraform in an environment where upstream registries are not reachable, which brings with it a caveat: when installing from a network mirror, Terraform trusts that the mirror will host an equivalent distribution package and resulting executable as presented by the upstream registry, and does not reach out to the origin registry to verify that. It's therefore important to only use network mirrors you trust, such as one maintained internally within your organization.

Terraform uses network mirrors only when you explicitly enable them in the CLI configuration, as described in the documentation included in this PR.

Earlier we introduced a new package hashing mechanism that is compatible
with both packed and unpacked packages, because it's a hash of the
contents of the package rather than of the archive it's delivered in.
However, we were using that only for the local selections file and not
for any remote package authentication yet.

The provider network mirrors protocol includes new-style hashes as a step
towards transitioning over to the new hash format in all cases, so this
new authenticator is here in preparation for verifying the checksums of
packages coming from network mirrors, for mirrors that support them.

For now this leaves us in a kinda confusing situation where we have both
NewPackageHashAuthentication for the new style and
NewArchiveChecksumAuthentication for the old style, which for the moment
is represented only by a doc comment on the latter. Hopefully we can
remove NewArchiveChecksumAuthentication in a future commit, if we can
get the registry updated to use the new hashing format.
@apparentlymart apparentlymart added this to the v0.13.x milestone Aug 26, 2020
@apparentlymart apparentlymart requested a review from a team August 26, 2020 00:57
@apparentlymart apparentlymart self-assigned this Aug 26, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 26, 2020

Codecov Report

Merging #25999 into master will increase coverage by 0.07%.
The diff coverage is 66.98%.

Impacted Files Coverage Δ
internal/getproviders/errors.go 36.23% <0.00%> (-10.07%) ⬇️
provider_source.go 24.48% <0.00%> (ø)
internal/getproviders/http_mirror_source.go 71.42% <71.34%> (+71.42%) ⬆️
internal/getproviders/hash.go 58.33% <80.00%> (+58.33%) ⬆️
internal/getproviders/package_authentication.go 87.96% <84.61%> (-0.37%) ⬇️
backend/remote/backend_common.go 54.57% <0.00%> (-0.68%) ⬇️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️
internal/getproviders/registry_client.go 66.33% <0.00%> (+1.65%) ⬆️

Copy link
Copy Markdown
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

This looks great! I'm trusting the tests more than an exhaustive review, but everything looks good and my only comments were some nitpicky user documentation notes.

I did have a notion that it might be a good idea - in the future, not necessary today! - to extract the json parsing functions and write tests (maybe an e2e test) that run terraform providers mirror and then verifies that the output is properly formed. I suspect if we don't write those tests some developer (I'm looking at me here) surprise themselves by making a change in one spot that breaks the other.

therefore, if desired, publish human-readable usage documentation for your
network mirror at that URL.

The following sections describe the various operatinos that a providor
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following sections describe the various operatinos that a providor
The following sections describe the various operations that a provider


## List Available Installation Packages

This operation returns download URLs of and associated metadata about the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
This operation returns download URLs of and associated metadata about the
This operation returns download URLs and associated metadata about the

I think it's ok to leave off the 'of' here but it's more of a style suggestion, I will not argue if you ignore this one!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"download URLs about" feels weird to me, but both "download URLs for" and "associated metadata for" both seem to work, so I think I'm going to address this by removing the "of" and changing "about" to "for", so the new preposition will be shared between both of the noun phrases. Thanks!

(There's a similar passage to this in the Provider Registry Protocol page, which I used as the base here. However, to keep this already-rather-large PR focused I'm going to ignore that for now and focus on editing this new docs page.)

* `namespace` (required): the namespace portion of the address of the requested
provider.
* `type` (required): the type portion of the address of the requested provider.
* `version` (required): the version selected to download. This will exactly
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what do you think about "must" in place of "will"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment isn't clear, I meant in the last line, the version description: This must exactly blah blah blah

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the oddness here is because this is written from the perspective of a potential server implementer rather than a potential client implementer, so this is describing what the client (Terraform CLI) will do, rather than describing a requirement for the server implementer.

With that said, I do agree that it feels weird because protocol docs are normally written as instructions to future implementors rather than as descriptions of a particular implementation. I think I'm inclined to leave it as "will" for now, because other client implementations of this protocol feel unlikely to me, but maybe we could do another editorial pass later to ensure that the writing on this page is all written with a consistent audience in mind, because I expect there's also some holdovers here with content derived from the module registry docs, that were originally written as a description of how the Terraform Registry behaves, because they started off as the registry's API docs.

"h1:lCJCxf/LIowc2IGS9TPjWDyXY4nOmdGdfcwwDQCOURQ="
]
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}
}

We previously had this just stubbed out because it was a stretch goal for
the v0.13.0 release and it ultimately didn't make it in.

Here we fill out the existing stub -- with a minor change to its interface
so it can access credentials -- with a client implementation that is
compatible with the directory structure produced by the
"terraform providers mirror" subcommand, were the result to be published
on a static file server.
@apparentlymart apparentlymart force-pushed the f-provider-network-mirrors branch from 7fec560 to 5780888 Compare August 26, 2020 17:38
@apparentlymart apparentlymart merged commit 2bd2a9a into master Aug 26, 2020
@apparentlymart apparentlymart deleted the f-provider-network-mirrors branch August 26, 2020 20:18
@apparentlymart
Copy link
Copy Markdown
Contributor Author

Sorry I just realized that I neglected to respond to the note in the main review comment about an end-to-end test of verifying that HTTPMirrorSource is able to consume the output of terraform providers mirror...

I agree that this could make a good test in the e2etest package. I didn't do that here because I need to shift my focus onto a different project now and unfortunately it seems somewhat complex to implement due to the need for a HTTPS server that a real terraform executable would trust, but I think it'd a good thing to keep in mind to consider as part of whatever project next makes significant changes to either of those subsystems, so we can have some programmatic verification that both sides of this protocol are implementing it in a mutually-compatible way.

@ghost
Copy link
Copy Markdown

ghost commented Oct 13, 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 as resolved and limited conversation to collaborators Oct 13, 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