Align the module fetching logic with the OpenTofu registry v1 protocol (#901)

Signed-off-by: Dmitry Kisler <admin@dkisler.com>

internal/registry/client.go

@@ -227,24 +227,35 @@ func (c *Client) ModuleLocation(ctx context.Context, module *regsrc.Module, vers
 	}
 	defer resp.Body.Close()
 
-	// there should be no body, but save it for logging
 	body, err := io.ReadAll(resp.Body)
 	if err != nil {
 		return "", fmt.Errorf("error reading response body from registry: %w", err)
 	}
 
+	var location string
+
 	switch resp.StatusCode {
-	case http.StatusOK, http.StatusNoContent:
-		// OK
+	case http.StatusOK:
+		var v response.ModuleLocationRegistryResp
+		if err := json.Unmarshal(body, &v); err != nil {
+			return "", fmt.Errorf("module %q version %q failed to deserialize response body %s: %w",
+				module, version, body, err)
+		}
+
+		location = v.Location
+
+	case http.StatusNoContent:
+		// FALLBACK: set the found location from the header
+		location = resp.Header.Get(xTerraformGet)
+
 	case http.StatusNotFound:
 		return "", fmt.Errorf("module %q version %q not found", module, version)
+
 	default:
 		// anything else is an error:
 		return "", fmt.Errorf("error getting download location for %q: %s resp:%s", module, resp.Status, body)
 	}
 
-	// the download location is in the X-Terraform-Get header
-	location := resp.Header.Get(xTerraformGet)
 	if location == "" {
 		return "", fmt.Errorf("failed to get download URL for %q: %s resp:%s", module, resp.Status, body)
 	}

internal/registry/client_test.go

@@ -5,8 +5,11 @@ package registry
 
 import (
 	"context"
+	"errors"
+	"io"
 	"net/http"
 	"os"
+	"reflect"
 	"strings"
 	"testing"
 	"time"
@@ -370,3 +373,182 @@ func TestLookupModuleNetworkError(t *testing.T) {
 		t.Fatal("unexpected error, got:", err)
 	}
 }
+
+func TestModuleLocation_readRegistryResponse(t *testing.T) {
+	cases := map[string]struct {
+		src                  string
+		httpClient           *http.Client
+		registryFlags        []uint8
+		want                 string
+		wantErrorStr         string
+		wantToReadFromHeader bool
+		wantStatusCode       int
+	}{
+		"shall find the module location in the registry response body": {
+			src:            "exists-in-registry/identifier/provider",
+			want:           "file:///registry/exists",
+			wantStatusCode: http.StatusOK,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{},
+			},
+		},
+		"shall find the module location in the registry response header": {
+			src:                  "exists-in-registry/identifier/provider",
+			registryFlags:        []uint8{test.WithModuleLocationInHeader},
+			want:                 "file:///registry/exists",
+			wantToReadFromHeader: true,
+			wantStatusCode:       http.StatusNoContent,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{},
+			},
+		},
+		"shall read location from the registry response body even if the header with location address is also set": {
+			src:                  "exists-in-registry/identifier/provider",
+			want:                 "file:///registry/exists",
+			wantStatusCode:       http.StatusOK,
+			wantToReadFromHeader: false,
+			registryFlags:        []uint8{test.WithModuleLocationInBody, test.WithModuleLocationInHeader},
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{},
+			},
+		},
+		"shall fail to find the module": {
+			src: "not-exist/identifier/provider",
+			// note that the version is fixed in the mock
+			// see: /internal/registry/test/mock_registry.go:testMods
+			wantErrorStr:   `module "not-exist/identifier/provider" version "0.2.0" not found`,
+			wantStatusCode: http.StatusNotFound,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{},
+			},
+		},
+		"shall fail because of reading response body error": {
+			src:            "foo/bar/baz",
+			wantErrorStr:   "error reading response body from registry: foo",
+			wantStatusCode: http.StatusOK,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{
+					forwardResponse: &http.Response{
+						StatusCode: http.StatusOK,
+						Body:       mockErrorReadCloser{err: errors.New("foo")},
+					},
+				},
+			},
+		},
+		"shall fail to deserialize JSON response": {
+			src:            "foo/bar/baz",
+			wantErrorStr:   `module "foo/bar/baz" version "0.2.0" failed to deserialize response body {: unexpected end of JSON input`,
+			wantStatusCode: http.StatusOK,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{
+					forwardResponse: &http.Response{
+						StatusCode: http.StatusOK,
+						Body:       io.NopCloser(strings.NewReader("{")),
+					},
+				},
+			},
+		},
+		"shall fail because of unexpected protocol change - 422 http status": {
+			src:            "foo/bar/baz",
+			wantErrorStr:   `error getting download location for "foo/bar/baz": foo resp:bar`,
+			wantStatusCode: http.StatusUnprocessableEntity,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{
+					forwardResponse: &http.Response{
+						StatusCode: http.StatusUnprocessableEntity,
+						Status:     "foo",
+						Body:       io.NopCloser(strings.NewReader("bar")),
+					},
+				},
+			},
+		},
+		"shall fail because location is not found in the response": {
+			src:            "foo/bar/baz",
+			wantErrorStr:   `failed to get download URL for "foo/bar/baz": OK resp:{"foo":"git::https://github.com/foo/terraform-baz-bar?ref=v0.2.0"}`,
+			wantStatusCode: http.StatusOK,
+			httpClient: &http.Client{
+				Transport: &mockRoundTripper{
+					forwardResponse: &http.Response{
+						StatusCode: http.StatusOK,
+						Status:     "OK",
+						// note that the response emulates a contract change
+						Body: io.NopCloser(strings.NewReader(`{"foo":"git::https://github.com/foo/terraform-baz-bar?ref=v0.2.0"}`)),
+					},
+				},
+			},
+		},
+	}
+
+	t.Parallel()
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			server := test.Registry(tc.registryFlags...)
+			defer server.Close()
+
+			client := NewClient(test.Disco(server), tc.httpClient)
+
+			mod, err := regsrc.ParseModuleSource(tc.src)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			got, err := client.ModuleLocation(context.Background(), mod, "0.2.0")
+			if err != nil && tc.wantErrorStr == "" {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err != nil && err.Error() != tc.wantErrorStr {
+				t.Fatalf("unexpected error content: want=%s, got=%v", tc.wantErrorStr, err)
+			}
+			if got != tc.want {
+				t.Fatalf("unexpected location: want=%s, got=%v", tc.want, got)
+			}
+
+			gotStatusCode := tc.httpClient.Transport.(*mockRoundTripper).reverseResponse.StatusCode
+			if tc.wantStatusCode != gotStatusCode {
+				t.Fatalf("unexpected response status code: want=%d, got=%d", tc.wantStatusCode, gotStatusCode)
+			}
+
+			if tc.wantToReadFromHeader {
+				resp := tc.httpClient.Transport.(*mockRoundTripper).reverseResponse
+				if !reflect.DeepEqual(resp.Body, http.NoBody) {
+					t.Fatalf("expected no body")
+				}
+			}
+		})
+	}
+}
+
+type mockRoundTripper struct {
+	// response to return without calling the server
+	// SET TO USE AS A REVERSE PROXY
+	forwardResponse *http.Response
+	// the response from the server will be written here
+	// DO NOT SET
+	reverseResponse *http.Response
+	err             error
+}
+
+func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	if m.err != nil {
+		return nil, m.err
+	}
+	if m.forwardResponse != nil {
+		m.reverseResponse = m.forwardResponse
+		return m.forwardResponse, nil
+	}
+	resp, err := http.DefaultTransport.RoundTrip(req)
+	m.reverseResponse = resp
+	return resp, err
+}
+
+type mockErrorReadCloser struct {
+	err error
+}
+
+func (m mockErrorReadCloser) Read(_ []byte) (n int, err error) {
+	return 0, m.err
+}
+
+func (m mockErrorReadCloser) Close() error {
+	return m.err
+}

internal/registry/response/module_download.go

@@ -0,0 +1,11 @@
+// Copyright (c) OpenTofu
+// SPDX-License-Identifier: MPL-2.0
+
+package response
+
+// ModuleLocationRegistryResp defines the OpenTofu registry response
+// returned when calling the endpoint /v1/modules/:namespace/:name/:system/:version/download
+type ModuleLocationRegistryResp struct {
+	// The URL to download the module from.
+	Location string `json:"location"`
+}

internal/registry/test/mock_registry.go

@@ -131,7 +131,7 @@ func init() {
 	}
 }
 
-func mockRegHandler() http.Handler {
+func mockRegHandler(config map[uint8]struct{}) http.Handler {
 	mux := http.NewServeMux()
 
 	moduleDownload := func(w http.ResponseWriter, r *http.Request) {
@@ -167,9 +167,26 @@ func mockRegHandler() http.Handler {
 			location = fmt.Sprintf("file://%s/%s", wd, location)
 		}
 
-		w.Header().Set("X-Terraform-Get", location)
+		// the location will be returned in the response header
+		_, inHeader := config[WithModuleLocationInHeader]
+		// the location will be returned in the response body
+		_, inBody := config[WithModuleLocationInBody]
+
+		if inHeader {
+			w.Header().Set("X-Terraform-Get", location)
+		}
+
+		if inBody {
+			w.WriteHeader(http.StatusOK)
+			o, err := json.Marshal(response.ModuleLocationRegistryResp{Location: location})
+			if err != nil {
+				panic("mock error: " + err.Error())
+			}
+			_, _ = w.Write(o)
+			return
+		}
+
 		w.WriteHeader(http.StatusNoContent)
-		// no body
 	}
 
 	moduleVersions := func(w http.ResponseWriter, r *http.Request) {
@@ -244,9 +261,29 @@ func mockRegHandler() http.Handler {
 	return mux
 }
 
+const (
+	// WithModuleLocationInBody sets to return the module's location in the response body
+	WithModuleLocationInBody uint8 = iota
+	// WithModuleLocationInHeader sets to return the module's location in the response header
+	WithModuleLocationInHeader
+)
+
 // Registry returns an httptest server that mocks out some registry functionality.
-func Registry() *httptest.Server {
-	return httptest.NewServer(mockRegHandler())
+func Registry(flags ...uint8) *httptest.Server {
+	if len(flags) == 0 {
+		return httptest.NewServer(mockRegHandler(
+			map[uint8]struct{}{
+				// default setting
+				WithModuleLocationInBody: {},
+			},
+		))
+	}
+
+	cfg := map[uint8]struct{}{}
+	for _, flag := range flags {
+		cfg[flag] = struct{}{}
+	}
+	return httptest.NewServer(mockRegHandler(cfg))
 }
 
 // RegistryRetryableErrorsServer returns an httptest server that mocks out the

website/docs/internals/module-registry-protocol.mdx

@@ -191,25 +191,37 @@ This endpoint downloads the specified version of a module for a single target sy
 ### Sample Request
 
 ```text
-$ curl -i 'https://registry.example.io/v1/modules/hashicorp/consul/aws/0.0.1/download'
+$ curl -i 'https://registry.example.io/v1/modules/foo/bar/baz/0.0.1/download'
 ```
 
 ### Sample Response
 
+A successful response contains the location from which the module version's source can be downloaded.
+
+It is expected to be found in the JSON encoded body as the value for the key `location`:
+
 ```text
-HTTP/1.1 204 No Content
-Content-Length: 0
-X-Terraform-Get: https://api.github.com/repos/hashicorp/terraform-aws-consul/tarball/v0.0.1//*?archive=tar.gz
+HTTP/2 200
+Content-Length: 81
+
+{"location": "git::https://github.com/foo/terraform-baz-bar?ref=v0.0.1"}
 ```
 
-A successful response has no body, and includes the location from which the
-module version's source can be downloaded in the `X-Terraform-Get` header.
-The value of this header accepts the same values as the `source` argument
-in a `module` block in OpenTofu configuration, as described in
-[Module Sources](/docs/language/modules/sources),
-except that it may not recursively refer to another module registry address.
+In the absence of a response body, OpenTofu will use the `X-Terraform-Get` header as the module location:
 
-The value of `X-Terraform-Get` may instead be a relative URL, indicated by
-beginning with `/`, `./` or `../`, in which case it is resolved relative to
-the full URL of the download endpoint to produce
-[an HTTP URL module source](/docs/language/modules/sources#http-urls).
+```text
+HTTP/2 204 No Content
+Content-Length: 0
+X-Terraform-Get: git::https://github.com/foo/terraform-baz-bar?ref=v0.0.1
+```
+
+:::warning
+OpenTofu will prioritize reading the response body content if both, the body and the `X-Terraform-Get` header, are received from the registry server.
+:::
+
+The module location value accepts the same values as the `source` argument in a
+`module` block in OpenTofu configuration, as described in [Module Sources](/docs/language/modules/sources),
+except that it may not recursively refer to another module registry address.
+The value of the module location may instead be a relative URL, indicated by beginning with `/`, `./` or `../`,
+in which case it is resolved relative to the full URL of the download endpoint to
+produce [an HTTP URL module source](/docs/language/modules/sources#http-urls).