Cache registry module errors (#1258)

* Add custom registry client error

* Change module registry index to allow caching errors

Errors don't have a version, so we update the compound index
to allow missing indices

* Cache modules on 404 errors

* Test RegistryModuleStore CacheError

* Test caching of 404 module registry errors

* Feeback: naming

* Cache non transient module registry errors

internal/registry/module.go

@@ -51,6 +51,15 @@ type ModuleVersion struct {
 	Version string `json:"version"`
 }
 
+type ClientError struct {
+	StatusCode int
+	Body       string
+}
+
+func (rce ClientError) Error() string {
+	return fmt.Sprintf("%d: %s", rce.StatusCode, rce.Body)
+}
+
 func (c Client) GetModuleData(ctx context.Context, addr tfaddr.Module, cons version.Constraints) (*ModuleResponse, error) {
 	var response ModuleResponse
 
@@ -77,14 +86,15 @@ func (c Client) GetModuleData(ctx context.Context, addr tfaddr.Module, cons vers
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
+
 	if resp.StatusCode != 200 {
 		bodyBytes, err := ioutil.ReadAll(resp.Body)
 		if err != nil {
 			return nil, err
 		}
-		defer resp.Body.Close()
 
-		return nil, fmt.Errorf("unexpected response %s: %s", resp.Status, string(bodyBytes))
+		return nil, ClientError{StatusCode: resp.StatusCode, Body: string(bodyBytes)}
 	}
 
 	err = json.NewDecoder(resp.Body).Decode(&response)
@@ -128,14 +138,15 @@ func (c Client) GetModuleVersions(ctx context.Context, addr tfaddr.Module) (vers
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
+
 	if resp.StatusCode != 200 {
 		bodyBytes, err := ioutil.ReadAll(resp.Body)
 		if err != nil {
 			return nil, err
 		}
-		defer resp.Body.Close()
 
-		return nil, fmt.Errorf("unexpected response %s: %s", resp.Status, string(bodyBytes))
+		return nil, ClientError{StatusCode: resp.StatusCode, Body: string(bodyBytes)}
 	}
 
 	var response ModuleVersionsResponse

internal/state/module.go

@@ -528,6 +528,10 @@ func (s *ModuleStore) RegistryModuleMeta(addr tfaddr.Module, cons version.Constr
 	for item := it.Next(); item != nil; item = it.Next() {
 		mod := item.(*RegistryModuleData)
 
+		if mod.Error {
+			continue
+		}
+
 		if cons.Check(mod.Version) {
 			return &registry.ModuleData{
 				Version: mod.Version,

internal/state/registry_modules.go

@@ -14,6 +14,7 @@ import (
 type RegistryModuleData struct {
 	Source  tfaddr.Module
 	Version *version.Version
+	Error   bool
 	Inputs  []registry.Input
 	Outputs []registry.Output
 }
@@ -28,6 +29,13 @@ func (s *RegistryModuleStore) Exists(sourceAddr tfaddr.Module, constraint versio
 
 	for obj := iter.Next(); obj != nil; obj = iter.Next() {
 		p := obj.(*RegistryModuleData)
+		// There was an error fetching the module, so we can't compare
+		// any versions
+		if p.Error {
+			return true, nil
+		}
+
+		// Check if there an entry with a matching version
 		if constraint.Check(p.Version) {
 			return true, nil
 		}
@@ -67,3 +75,31 @@ func (s *RegistryModuleStore) Cache(sourceAddr tfaddr.Module, modVer *version.Ve
 	txn.Commit()
 	return nil
 }
+
+func (s *RegistryModuleStore) CacheError(sourceAddr tfaddr.Module) error {
+	txn := s.db.Txn(true)
+	defer txn.Abort()
+
+	obj, err := txn.First(s.tableName, "id", sourceAddr, nil)
+	if err != nil {
+		return err
+	}
+	if obj != nil {
+		return &AlreadyExistsError{
+			Idx: sourceAddr.String(),
+		}
+	}
+
+	modData := &RegistryModuleData{
+		Source: sourceAddr,
+		Error:  true,
+	}
+
+	err = txn.Insert(s.tableName, modData)
+	if err != nil {
+		return err
+	}
+
+	txn.Commit()
+	return nil
+}

internal/state/registry_modules_test.go

@@ -118,3 +118,40 @@ func TestModule_DeclaredModuleMeta(t *testing.T) {
 		t.Fatalf("mismatch chached metadata: %s", diff)
 	}
 }
+
+func TestStateStore_cache_error(t *testing.T) {
+	s, err := NewStateStore()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	source, err := tfaddr.ParseModuleSource("terraform-aws-modules/eks/aws")
+	if err != nil {
+		t.Fatal(err)
+	}
+	c := version.MustConstraints(version.NewConstraint(">= 3.0"))
+
+	// should be false
+	exists, err := s.RegistryModules.Exists(source, c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if exists == true {
+		t.Fatal("should not exist")
+	}
+
+	// store an error for a moudle
+	err = s.RegistryModules.CacheError(source)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// should be true
+	exists, err = s.RegistryModules.Exists(source, c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if exists != true {
+		t.Fatal("should exist")
+	}
+}

internal/state/state.go

@@ -154,6 +154,7 @@ var dbSchema = &memdb.DBSchema{
 							&StringerFieldIndexer{Field: "Source"},
 							&VersionFieldIndexer{Field: "Version"},
 						},
+						AllowMissing: true,
 					},
 				},
 				"source_addr": {

internal/terraform/module/module_ops.go

@@ -638,6 +638,18 @@ func GetModuleDataFromRegistry(ctx context.Context, regClient registry.Client, m
 		metaData, err := regClient.GetModuleData(ctx, sourceAddr, declaredModule.Version)
 		if err != nil {
 			errs = multierror.Append(errs, err)
+
+			clientError := registry.ClientError{}
+			if errors.As(err, &clientError) &&
+				((clientError.StatusCode >= 400 && clientError.StatusCode < 408) ||
+					(clientError.StatusCode > 408 && clientError.StatusCode < 429)) {
+				// Still cache the module
+				err = modRegStore.CacheError(sourceAddr)
+				if err != nil {
+					errs = multierror.Append(errs, err)
+				}
+			}
+
 			continue
 		}
 

internal/terraform/module/module_ops_test.go

@@ -164,7 +164,6 @@ func TestGetModuleDataFromRegistry_moduleNotFound(t *testing.T) {
 
 	// Verify that 2nd module is still cached even if
 	// obtaining data for the other one errored out
-
 	addr, err := tfaddr.ParseModuleSource("puppetlabs/deployment/ec")
 	if err != nil {
 		t.Fatal(err)
@@ -186,6 +185,28 @@ func TestGetModuleDataFromRegistry_moduleNotFound(t *testing.T) {
 	if diff := cmp.Diff(expectedModuleData, meta, ctydebug.CmpOptions); diff != "" {
 		t.Fatalf("metadata mismatch: %s", diff)
 	}
+
+	// Verify that the third module is still cached even if
+	// it returns a not found error
+	addr, err = tfaddr.ParseModuleSource("terraform-aws-modules/eks/aws")
+	if err != nil {
+		t.Fatal(err)
+	}
+	cons = version.MustConstraints(version.NewConstraint("0.0.8"))
+
+	exists, err = ss.RegistryModules.Exists(addr, cons)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !exists {
+		t.Fatalf("expected cached metadata to exist for %q %q", addr, cons)
+	}
+
+	// But it shouldn't return any module data
+	_, err = ss.Modules.RegistryModuleMeta(addr, cons)
+	if err == nil {
+		t.Fatal("expected module to be not found")
+	}
 }
 
 func TestGetModuleDataFromRegistry_apiTimeout(t *testing.T) {