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

Unable to use relative path to local Git module #25488

Open
henrytk opened this issue Jul 6, 2020 · 3 comments · May be fixed by #26056
Open

Unable to use relative path to local Git module #25488

henrytk opened this issue Jul 6, 2020 · 3 comments · May be fixed by #26056
Labels
config enhancement v0.12 Issues (primarily bugs) reported against v0.12 releases

Comments

@henrytk
Copy link

henrytk commented Jul 6, 2020

Context/use case

When operating in an environment with no external network access, I want to use a module accessed via the file system, but still leverage the versioning ability of a Git repository. In other words, I want to use a Terraform module which is inside a Git submodule on the filesystem. This works with an absolute path:

module "a" {
  source = "git::/Users/myusername/repos/repository-name//sub-folder?ref=version-ref"
}

but fails with a relative path (see below).

Terraform Version

$ terraform --version
Terraform v0.12.9

Terraform Configuration Files

I'll give a distilled example.

Say I have a Git repository containing a piece of Terraform intended to be used as a module:

output "value" {
  value = "some value"
}

All it does is output some value.

I then have a repository which includes the above as a Git submodule.:

module "a" {
  # Assume this path correctly traverses to where the Git submodule is located
  source = "git::../terraform-repo/module-path//module-sub-directory"
}

output "a" {
  value = module.a.value
}

Expected Behavior

I expect terraform init to work with absolute or relative file paths.

Actual Behavior

The absolute file paths work, but relative paths always yields:

Error: Failed to download module

Could not download module "a" (main.tf:1) source code from
"git::../../../myusername/dev/test-terraform/test-modules": error downloading
'file:///myusername/dev/test-terraform/test-modules': /usr/bin/git exited with
128: Cloning into '.terraform/modules/a'...
fatal: '/myusername/dev/test-terraform/test-modules' does not appear to be a
git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Steps to Reproduce

Reference a Git repository using a local URL, such as "git::/Users/myusername/repos/repository-name//sub-folder?ref=version-ref", then change it to a relative URL.

Bonus fact

If you use a relative URL which goes up to the root directory it does work:

module "a" {
  source = "git::../../../../Users/myusername/repos/repository-name//sub-folder?ref=version-ref"
}
@alisdair alisdair added enhancement v0.12 Issues (primarily bugs) reported against v0.12 releases labels Jul 6, 2020
@henrytk
Copy link
Author

henrytk commented Jul 6, 2020

@alisdair this was intended as a bug report. Use case only provided for context.

@salewski
Copy link
Contributor

Hi Folks,

I developed a fix for this independently for my own needs in the past couple of days, but am just now seeing this GH issue. I'll write-up my approach and submit a PR for review in the next day or so. I started with the use case of supporting git:: forced local repos for my modules (basically the same as @henrytk's use case), but then generalized it to work with absoulte paths, too (for symmetry).

More soon,
-Al

@salewski
Copy link
Contributor

I'll write-up my approach and submit a PR for review in the next day or so.

That write-up is in hashicorp/go-getter#268, as that's the library that provides the Git repo cloning support for Terraform. The string value of a Terraform module call's source parameter is parsed and handled by the go-getter lib.

Assuming the pull request, hashicorp/go-getter#269, (or something like it) gets merged, then getting the support for this feature in Terraform will require updating the dependency on go-getter to a version that includes the change.

salewski added a commit to salewski/go-getter that referenced this issue Aug 31, 2020
…nd relative

This series of changesets introduces a feature that allows the 'git::'
forcing token to be used on local file system paths to reference Git
repositories. Both absolute paths and relative paths are supported. For
example:
    git::./some/relative/path/to/a/git-repo//some-subdir?ref=v1.2.3
or:
    git::../../some/relative/path/to/a/git-repo//some-subdir?ref=v1.2.3
or:
    git::/some/absolute/path/to/a/git-repo//some-subdir?ref=v4.5.6

Only filepaths that are prefixed with the 'git::' forcing token are
considered for processing.

Internally, go-getter transforms the provided string into a 'file://'
URI with an absolute filepath, with query string params and subdirectory
retained.

The rationale for using a 'file://' URI internally is that the Git clone
operation can already work with 'file://' URIs, and using them for this
feature allows us to leverage the existing go-getter URI-handling
machinery. That gets us support for query params (to clone a specific
git ref (tag, commit hash, ...)) "for free".

The rationale for using an absolute filepath (even when the provided
string is a relative filepath) is that (per RFC 1738 and RFC 8089) only
absolute filepaths are legitimate in 'file://' URIs. But more
importantly here, the Git clone operation only supports 'file://' URIs
with absolute paths.

Q: Why support this functionality at all?

   Why not just require that a source location use an absolute path in a
   'file://' URI explicitly if that's what is needed?

A: The primary reason is to allow support for relative filepaths to Git
   repos.

   There are use cases in which the absolute path cannot be known in
   advance, but a relative path to a Git repo is known.

   For example, when a Terraform project (or any Git-based project) uses
   Git submodules, it will know the relative location of the Git
   submodule repos, but cannot know the absolute path in advance because
   it will vary based on where the "superproject" repo is
   cloned. Nevertheless, those relative paths should be usable as
   clonable Git repos, and this mechanism would allow for that.

   Support for filepaths that are already absolute is provided mainly
   for symmetry. It would be surprising for the feature to work with
   relative file paths, but not for absolute filepaths.

For projects using Terraform, in particular, this feature (along with a
small change in the Terraform code to leverage it) enables the
non-fragile use of relative paths in a module "call" block, when
combined with Git submodules:

    module "my_module" {
        source = "git::../git-submodules/tf-modules/some-tf-module?ref=v0.1.0"
        // ...
    }

In the above example "superproject" Git repo (the one "calling" the
terraform module) knows the relative path to its own Git submodules
because they are embedded in a subdirectory beneath the top-level of the
"superproject" repo.

Two downstream Terraform issues that would require go-getter support for
this feature (or something like it) are at [0] and [1].

This first changeset in the series updates the README.md documentation
to note the new feature and provide examples.

[0] "Unable to use relative path to local Git module"
    hashicorp/terraform#25488

[1] "In 0.12, modules can no longer be installed from local git repositories at relative paths"
    hashicorp/terraform#21107

Design Notes
------------
In order for this feature to work, additional contextual information is
needed by the Git detector than can be provided using the existing
Detector API.

Internally, the Detector's Detect method does not pass along to the
Detector implementations all of the contextual information that it has
available. In particular, the forcing token and go-getter subdir
component are stripped out of the source string before invoking the
implementation's Detect method. In the particular case of the Git
detector, that means it cannot know that a 'git::' forcing token was
provided on an input string that otherwise looks like a file system
path. And /that/ means that it is not correct or safe for it to identify
any filepath string value as a Git repository.

Externally, callers (such as Terraform) already provide a value for the
'pwd' parameter of Detect, but it is not (necessarily) the location from
which a relative path in a 'git::' string should be resolved. In a
Terraform module (which may be in an arbitrary subdirectory from the
process current working directory), module "source" references that
contain relative paths must be interpreted relative to the location of
the module source file. Terraform has that information available, but in
the existing Detect API there is no way to convey it to go-getter.

Constraints
-----------
Additional Detector methods cannot be added without burdening all
existing detectors (both internal and in the wild) with the need to
support them.

Additional Detect method params cannot be added without breaking all
existing Detector implementations (internal, wild).

Additional parameters cannot be added to the Detect dispatching function
without affecting all callers.

Approach
--------
The goal is to provide the feature in a way that is as minimally
invasive as possible. But above all else it needs to avoid breaking
backward compatibility in any way.

Given that, the approach taken by this changeset series is to introduce
the concept of a "Contextual Detector". It is structured in the same way
as the current Detector framework, but works through a new CtxDetector
interface that is not constrained by the existing API.

The only callers affected by this change would be those that wish to take
advantage of the additional capabilities. And for those, the migration path
straight-forward because the new API is structured like the existing one.

In particular, this changeset series introduces four new elements:

    1. CtxDetector interface

    2. CtxDetect dispatching function

    3. CtxDetect method on the CtxDetector interface

    4. Full suite of CtxDetector implementations that are analogues of
       the existing detectors (most of which (currently) just delegate
       to the existing Detector implementations).

There is also a global 'ContextualDetectors' list that serves a function
analogous to the existing 'Detectors' list.
salewski added a commit to salewski/terraform that referenced this issue Aug 31, 2020
The go-getters library now has support for local file system paths to
Git repositories, specified with the 'git::' forcing token. The feature
works for both absolute and relative filepaths, and supports all the
usual go-getter goodies including '//' delimited subdirs and URI-style
query parameters.[0][1]

We incorporate that capability into Terraform, which allows users to
specify paths to locally present Git repositories from which to clone
other Terrform modules on which they are dependent. When coupled with
Git submodules, this creates a powerful way to manage Terraform modules
at specific versions without requiring those modules to be available on
the network (e.g., on GitHub):

    module "my_module" {
        source = "git::../git-submodules/tf-modules/some-tf-module?ref=v0.1.0"
        // ...
    }

From the perspective of Terraform, such Git repositories are "remote" in
the same way that repositories on GitHub are.

Note that within a Terraform module "call" block, the filepaths
specified are relative to the directory in which the *.tf file lives,
not relative to the current working directory of the Terraform
process. In order to support this feature, Terraform needs to supply
that contextual information to go-getter to allow relative filepath
resolution to work. In order to do so, we needed to switch over to using
go-getter's new "Contextual Detector" API. It works in the same basic
way as the traditional Detector API, but allows us to provide this
additional information.

In keeping with the "keep things simple" comment in the commit message
of 2b2ac1f, we are here maintaining our custom go-getter detectors
in two places. Only now each is called FooCtxDetector rather than
FooDetector. Nevertheless, all except the GitCtxDetector do little more
than "pass through" delegation to its analogous FooDetector counterpart.

Fixes hashicorp#25488
Fixes hashicorp#21107

[0] hashicorp/go-getter#268
[1] hashicorp/go-getter#269
salewski added a commit to salewski/go-getter that referenced this issue Jun 5, 2023
…nd relative

This series of changesets introduces a feature that allows the 'git::'
forcing token to be used on local file system paths to reference Git
repositories. Both absolute paths and relative paths are supported. For
example:
    git::./some/relative/path/to/a/git-repo//some-subdir?ref=v1.2.3
or:
    git::../../some/relative/path/to/a/git-repo//some-subdir?ref=v1.2.3
or:
    git::/some/absolute/path/to/a/git-repo//some-subdir?ref=v4.5.6

Only filepaths that are prefixed with the 'git::' forcing token are
considered for processing.

Internally, go-getter transforms the provided string into a 'file://'
URI with an absolute filepath, with query string params and subdirectory
retained.

The rationale for using a 'file://' URI internally is that the Git clone
operation can already work with 'file://' URIs, and using them for this
feature allows us to leverage the existing go-getter URI-handling
machinery. That gets us support for query params (to clone a specific
git ref (tag, commit hash, ...)) "for free".

The rationale for using an absolute filepath (even when the provided
string is a relative filepath) is that (per RFC 1738 and RFC 8089) only
absolute filepaths are legitimate in 'file://' URIs. But more
importantly here, the Git clone operation only supports 'file://' URIs
with absolute paths.

Q: Why support this functionality at all?

   Why not just require that a source location use an absolute path in a
   'file://' URI explicitly if that's what is needed?

A: The primary reason is to allow support for relative filepaths to Git
   repos.

   There are use cases in which the absolute path cannot be known in
   advance, but a relative path to a Git repo is known.

   For example, when a Terraform project (or any Git-based project) uses
   Git submodules, it will know the relative location of the Git
   submodule repos, but cannot know the absolute path in advance because
   it will vary based on where the "superproject" repo is
   cloned. Nevertheless, those relative paths should be usable as
   clonable Git repos, and this mechanism would allow for that.

   Support for filepaths that are already absolute is provided mainly
   for symmetry. It would be surprising for the feature to work with
   relative file paths, but not for absolute filepaths.

For projects using Terraform, in particular, this feature (along with a
small change in the Terraform code to leverage it) enables the
non-fragile use of relative paths in a module "call" block, when
combined with Git submodules:

    module "my_module" {
        source = "git::../git-submodules/tf-modules/some-tf-module?ref=v0.1.0"
        // ...
    }

In the above example "superproject" Git repo (the one "calling" the
terraform module) knows the relative path to its own Git submodules
because they are embedded in a subdirectory beneath the top-level of the
"superproject" repo.

Two downstream Terraform issues that would require go-getter support for
this feature (or something like it) are at [0] and [1].

This first changeset in the series updates the README.md documentation
to note the new feature and provide examples.

[0] "Unable to use relative path to local Git module"
    hashicorp/terraform#25488

[1] "In 0.12, modules can no longer be installed from local git repositories at relative paths"
    hashicorp/terraform#21107

Design Notes
------------
In order for this feature to work, additional contextual information is
needed by the Git detector than can be provided using the existing
Detector API.

Internally, the Detector's Detect method does not pass along to the
Detector implementations all of the contextual information that it has
available. In particular, the forcing token and go-getter subdir
component are stripped out of the source string before invoking the
implementation's Detect method. In the particular case of the Git
detector, that means it cannot know that a 'git::' forcing token was
provided on an input string that otherwise looks like a file system
path. And /that/ means that it is not correct or safe for it to identify
any filepath string value as a Git repository.

Externally, callers (such as Terraform) already provide a value for the
'pwd' parameter of Detect, but it is not (necessarily) the location from
which a relative path in a 'git::' string should be resolved. In a
Terraform module (which may be in an arbitrary subdirectory from the
process current working directory), module "source" references that
contain relative paths must be interpreted relative to the location of
the module source file. Terraform has that information available, but in
the existing Detect API there is no way to convey it to go-getter.

Constraints
-----------
Additional Detector methods cannot be added without burdening all
existing detectors (both internal and in the wild) with the need to
support them.

Additional Detect method params cannot be added without breaking all
existing Detector implementations (internal, wild).

Additional parameters cannot be added to the Detect dispatching function
without affecting all callers.

Approach
--------
The goal is to provide the feature in a way that is as minimally
invasive as possible. But above all else it needs to avoid breaking
backward compatibility in any way.

Given that, the approach taken by this changeset series is to introduce
the concept of a "Contextual Detector". It is structured in the same way
as the current Detector framework, but works through a new CtxDetector
interface that is not constrained by the existing API.

The only callers affected by this change would be those that wish to take
advantage of the additional capabilities. And for those, the migration path
straight-forward because the new API is structured like the existing one.

In particular, this changeset series introduces four new elements:

    1. CtxDetector interface

    2. CtxDetect dispatching function

    3. CtxDetect method on the CtxDetector interface

    4. Full suite of CtxDetector implementations that are analogues of
       the existing detectors (most of which (currently) just delegate
       to the existing Detector implementations).

There is also a global 'ContextualDetectors' list that serves a function
analogous to the existing 'Detectors' list.
salewski added a commit to salewski/go-getter that referenced this issue Jun 6, 2023
…nd relative

This series of changesets introduces a feature that allows the 'git::'
forcing token to be used on local file system paths to reference Git
repositories. Both absolute paths and relative paths are supported. For
example:
    git::./some/relative/path/to/a/git-repo//some-subdir?ref=v1.2.3
or:
    git::../../some/relative/path/to/a/git-repo//some-subdir?ref=v1.2.3
or:
    git::/some/absolute/path/to/a/git-repo//some-subdir?ref=v4.5.6

Only filepaths that are prefixed with the 'git::' forcing token are
considered for processing.

Internally, go-getter transforms the provided string into a 'file://'
URI with an absolute filepath, with query string params and subdirectory
retained.

The rationale for using a 'file://' URI internally is that the Git clone
operation can already work with 'file://' URIs, and using them for this
feature allows us to leverage the existing go-getter URI-handling
machinery. That gets us support for query params (to clone a specific
git ref (tag, commit hash, ...)) "for free".

The rationale for using an absolute filepath (even when the provided
string is a relative filepath) is that (per RFC 1738 and RFC 8089) only
absolute filepaths are legitimate in 'file://' URIs. But more
importantly here, the Git clone operation only supports 'file://' URIs
with absolute paths.

Q: Why support this functionality at all?

   Why not just require that a source location use an absolute path in a
   'file://' URI explicitly if that's what is needed?

A: The primary reason is to allow support for relative filepaths to Git
   repos.

   There are use cases in which the absolute path cannot be known in
   advance, but a relative path to a Git repo is known.

   For example, when a Terraform project (or any Git-based project) uses
   Git submodules, it will know the relative location of the Git
   submodule repos, but cannot know the absolute path in advance because
   it will vary based on where the "superproject" repo is
   cloned. Nevertheless, those relative paths should be usable as
   clonable Git repos, and this mechanism would allow for that.

   Support for filepaths that are already absolute is provided mainly
   for symmetry. It would be surprising for the feature to work with
   relative file paths, but not for absolute filepaths.

For projects using Terraform, in particular, this feature (along with a
small change in the Terraform code to leverage it) enables the
non-fragile use of relative paths in a module "call" block, when
combined with Git submodules:

    module "my_module" {
        source = "git::../git-submodules/tf-modules/some-tf-module?ref=v0.1.0"
        // ...
    }

In the above example "superproject" Git repo (the one "calling" the
terraform module) knows the relative path to its own Git submodules
because they are embedded in a subdirectory beneath the top-level of the
"superproject" repo.

Two downstream Terraform issues that would require go-getter support for
this feature (or something like it) are at [0] and [1].

This first changeset in the series updates the README.md documentation
to note the new feature and provide examples.

[0] "Unable to use relative path to local Git module"
    hashicorp/terraform#25488

[1] "In 0.12, modules can no longer be installed from local git repositories at relative paths"
    hashicorp/terraform#21107

Design Notes
------------
In order for this feature to work, additional contextual information is
needed by the Git detector than can be provided using the existing
Detector API.

Internally, the Detector's Detect method does not pass along to the
Detector implementations all of the contextual information that it has
available. In particular, the forcing token and go-getter subdir
component are stripped out of the source string before invoking the
implementation's Detect method. In the particular case of the Git
detector, that means it cannot know that a 'git::' forcing token was
provided on an input string that otherwise looks like a file system
path. And /that/ means that it is not correct or safe for it to identify
any filepath string value as a Git repository.

Externally, callers (such as Terraform) already provide a value for the
'pwd' parameter of Detect, but it is not (necessarily) the location from
which a relative path in a 'git::' string should be resolved. In a
Terraform module (which may be in an arbitrary subdirectory from the
process current working directory), module "source" references that
contain relative paths must be interpreted relative to the location of
the module source file. Terraform has that information available, but in
the existing Detect API there is no way to convey it to go-getter.

Constraints
-----------
Additional Detector methods cannot be added without burdening all
existing detectors (both internal and in the wild) with the need to
support them.

Additional Detect method params cannot be added without breaking all
existing Detector implementations (internal, wild).

Additional parameters cannot be added to the Detect dispatching function
without affecting all callers.

Approach
--------
The goal is to provide the feature in a way that is as minimally
invasive as possible. But above all else it needs to avoid breaking
backward compatibility in any way.

Given that, the approach taken by this changeset series is to introduce
the concept of a "Contextual Detector". It is structured in the same way
as the current Detector framework, but works through a new CtxDetector
interface that is not constrained by the existing API.

The only callers affected by this change would be those that wish to take
advantage of the additional capabilities. And for those, the migration path
straight-forward because the new API is structured like the existing one.

In particular, this changeset series introduces four new elements:

    1. CtxDetector interface

    2. CtxDetect dispatching function

    3. CtxDetect method on the CtxDetector interface

    4. Full suite of CtxDetector implementations that are analogues of
       the existing detectors (most of which (currently) just delegate
       to the existing Detector implementations).

There is also a global 'ContextualDetectors' list that serves a function
analogous to the existing 'Detectors' list.

Signed-off-by: Alan D. Salewski <ads@salewski.email>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config enhancement v0.12 Issues (primarily bugs) reported against v0.12 releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants