Weaken provider name segment validation to match real Terraform registries

go.mod

@@ -5,5 +5,5 @@ go 1.14
 require (
 	github.com/google/go-cmp v0.5.9
 	github.com/hashicorp/terraform-svchost v0.0.0-20200729002733-f050f53b9734
-	golang.org/x/net v0.0.0-20210119194325-5f4716e94777
+	golang.org/x/net v0.0.0-20210119194325-5f4716e94777 // indirect
 )

module_test.go

@@ -51,6 +51,18 @@ func TestParseModuleSource_simple(t *testing.T) {
 				Subdir: "",
 			},
 		},
+		"custom registry, name/namespace not purely alphanumeric": {
+			input: "example.com/example_corp/network-ing/happycloud9",
+			want: Module{
+				Package: ModulePackage{
+					Host:         svchost.Hostname("example.com"),
+					Namespace:    "example_corp",
+					Name:         "network-ing",
+					TargetSystem: "happycloud9",
+				},
+				Subdir: "",
+			},
+		},
 		"custom registry, subdir": {
 			input: "example.com/awesomecorp/network/happycloud//examples/foo",
 			want: Module{

provider.go

@@ -2,10 +2,10 @@ package tfaddr
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	svchost "github.com/hashicorp/terraform-svchost"
-	"golang.org/x/net/idna"
 )
 
 // Provider encapsulates a single provider type. In the future this will be
@@ -48,6 +48,8 @@ const UnknownProviderNamespace = "?"
 // FQN. This may be produced by Terraform 0.13.
 const LegacyProviderNamespace = "-"
 
+var providerNamePartPattern = regexp.MustCompile("^[0-9A-Za-z](?:[0-9A-Za-z-_]{0,62}[0-9A-Za-z])?$")
+
 // String returns an FQN string, indended for use in machine-readable output.
 func (pt Provider) String() string {
 	if pt.IsZero() {
@@ -179,9 +181,10 @@ func (pt Provider) Equals(other Provider) bool {
 // terraform-config-inspect.
 //
 // The following are valid source string formats:
-// 		name
-// 		namespace/name
-// 		hostname/namespace/name
+//
+//	name
+//	namespace/name
+//	hostname/namespace/name
 //
 // "name"-only format is parsed as -/name (i.e. legacy namespace)
 // requiring further identification of the namespace via Registry API
@@ -217,7 +220,7 @@ func ParseProviderSource(str string) (Provider, error) {
 			if err != nil {
 				return Provider{}, &ParserError{
 					Summary: "Invalid provider namespace",
-					Detail:  fmt.Sprintf(`Invalid provider namespace %q in source %q: %s"`, namespace, str, err),
+					Detail:  fmt.Sprintf(`Invalid provider namespace %q in source %q: %s"`, givenNamespace, str, err),
 				}
 			}
 			ret.Namespace = namespace
@@ -227,11 +230,12 @@ func ParseProviderSource(str string) (Provider, error) {
 	// Final Case: 3 parts
 	if len(parts) == 3 {
 		// the namespace is always the first part in a three-part source string
-		hn, err := svchost.ForComparison(parts[0])
+		givenHostname := parts[0]
+		hn, err := svchost.ForComparison(givenHostname)
 		if err != nil {
 			return Provider{}, &ParserError{
 				Summary: "Invalid provider source hostname",
-				Detail:  fmt.Sprintf(`Invalid provider source hostname namespace %q in source %q: %s"`, hn, str, err),
+				Detail:  fmt.Sprintf(`Invalid provider source hostname %q in source %q: %s"`, givenHostname, str, err),
 			}
 		}
 		ret.Hostname = hn
@@ -293,7 +297,7 @@ func ParseProviderSource(str string) (Provider, error) {
 
 // MustParseProviderSource is a wrapper around ParseProviderSource that panics if
 // it returns an error.
-func MustParseProviderSource(raw string) (Provider) {
+func MustParseProviderSource(raw string) Provider {
 	p, err := ParseProviderSource(raw)
 	if err != nil {
 		panic(err)
@@ -374,31 +378,22 @@ func parseSourceStringParts(str string) ([]string, error) {
 }
 
 // ParseProviderPart processes an addrs.Provider namespace or type string
-// provided by an end-user, producing a normalized version if possible or
+// provided by an end-user, returning the same string if validation succeeds or
 // an error if the string contains invalid characters.
 //
-// A provider part is processed in the same way as an individual label in a DNS
-// domain name: it is transformed to lowercase per the usual DNS case mapping
-// and normalization rules and may contain only letters, digits, and dashes.
-// Additionally, dashes may not appear at the start or end of the string.
-//
-// These restrictions are intended to allow these names to appear in fussy
-// contexts such as directory/file names on case-insensitive filesystems,
-// repository names on GitHub, etc. We're using the DNS rules in particular,
-// rather than some similar rules defined locally, because the hostname part
-// of an addrs.Provider is already a hostname and it's ideal to use exactly
-// the same case folding and normalization rules for all of the parts.
-//
-// In practice a provider type string conventionally does not contain dashes
-// either. Such names are permitted, but providers with such type names will be
-// hard to use because their resource type names will not be able to contain
-// the provider type name and thus each resource will need an explicit provider
-// address specified. (A real-world example of such a provider is the
-// "google-beta" variant of the GCP provider, which has resource types that
-// start with the "google_" prefix instead.)
+// As per the implementation of HashiCorp's public and private Terraform
+// registries, a provider part may be up to 64 characters long, can contain
+// letters, digits, underscores, and dashes, must start with a letter, and must
+// end with a letter or digit.
 //
-// It's valid to pass the result of this function as the argument to a
-// subsequent call, in which case the result will be identical.
+// In practice, these name segments are often more restricted. Most notably, the
+// public registry requires namespaces to match a GitHub username (ruling out
+// underscores). Also, some permitted provider names are hard to use because
+// their resource type names will not be able to contain the provider type name
+// and thus each resource will need an explicit provider address specified. (A
+// real-world example of such a provider is the "google-beta" variant of the GCP
+// provider, which has resource types that start with the "google_" prefix
+// instead.)
 func ParseProviderPart(given string) (string, error) {
 	if len(given) == 0 {
 		return "", fmt.Errorf("must have at least one character")
@@ -415,21 +410,11 @@ func ParseProviderPart(given string) (string, error) {
 		return "", fmt.Errorf("dots are not allowed")
 	}
 
-	// We don't allow names containing multiple consecutive dashes, just as
-	// a matter of preference: they look weird, confusing, or incorrect.
-	// This also, as a side-effect, prevents the use of the "punycode"
-	// indicator prefix "xn--" that would cause the IDNA library to interpret
-	// the given name as punycode, because that would be weird and unexpected.
-	if strings.Contains(given, "--") {
-		return "", fmt.Errorf("cannot use multiple consecutive dashes")
-	}
-
-	result, err := idna.Lookup.ToUnicode(given)
-	if err != nil {
-		return "", fmt.Errorf("must contain only letters, digits, and dashes, and may not use leading or trailing dashes")
+	if !providerNamePartPattern.MatchString(given) {
+		return "", fmt.Errorf("must contain only letters, digits, dashes, and underscores, and may not use leading or trailing dashes or underscores")
 	}
 
-	return result, nil
+	return given, nil
 }
 
 // MustParseProviderPart is a wrapper around ParseProviderPart that panics if

provider_test.go

@@ -268,8 +268,8 @@ func TestParseProviderSource(t *testing.T) {
 		},
 		"registry.Terraform.io/HashiCorp/AWS": {
 			Provider{
-				Type:      "aws",
-				Namespace: "hashicorp",
+				Type:      "AWS",
+				Namespace: "HashiCorp",
 				Hostname:  DefaultProviderRegistryHost,
 			},
 			false,
@@ -304,8 +304,8 @@ func TestParseProviderSource(t *testing.T) {
 		},
 		"HashiCorp/AWS": {
 			Provider{
-				Type:      "aws",
-				Namespace: "hashicorp",
+				Type:      "AWS",
+				Namespace: "HashiCorp",
 				Hostname:  DefaultProviderRegistryHost,
 			},
 			false,
@@ -320,7 +320,7 @@ func TestParseProviderSource(t *testing.T) {
 		},
 		"AWS": {
 			Provider{
-				Type:      "aws",
+				Type:      "AWS",
 				Namespace: UnknownProviderNamespace,
 				Hostname:  DefaultProviderRegistryHost,
 			},
@@ -342,6 +342,14 @@ func TestParseProviderSource(t *testing.T) {
 			},
 			false,
 		},
+		"example.com/foo_bar/baz_boop": {
+			Provider{
+				Type:      "baz_boop",
+				Namespace: "foo_bar",
+				Hostname:  svchost.Hostname("example.com"),
+			},
+			false,
+		},
 		"localhost:8080/foo/bar": {
 			Provider{
 				Type:      "bar",
@@ -374,9 +382,13 @@ func TestParseProviderSource(t *testing.T) {
 			Provider{},
 			true,
 		},
-		"example.com/bad--namespace/aws": {
-			Provider{},
-			true,
+		"example.com/okay--namespace/aws": {
+			Provider{
+				Type:      "aws",
+				Namespace: "okay--namespace",
+				Hostname:  svchost.Hostname("example.com"),
+			},
+			false,
 		},
 		"example.com/-badnamespace/aws": {
 			Provider{},
@@ -386,18 +398,30 @@ func TestParseProviderSource(t *testing.T) {
 			Provider{},
 			true,
 		},
-		"example.com/bad.namespace/aws": {
+		"example.com/badnamespace_/aws": {
 			Provider{},
 			true,
 		},
-		"example.com/hashicorp/badtype!": {
+		"example.com/_badnamespace/aws": {
 			Provider{},
 			true,
 		},
-		"example.com/hashicorp/bad--type": {
+		"example.com/bad.namespace/aws": {
+			Provider{},
+			true,
+		},
+		"example.com/hashicorp/badtype!": {
 			Provider{},
 			true,
 		},
+		"example.com/hashicorp/okay--type": {
+			Provider{
+				Type:      "okay--type",
+				Namespace: "hashicorp",
+				Hostname:  svchost.Hostname("example.com"),
+			},
+			false,
+		},
 		"example.com/hashicorp/-badtype": {
 			Provider{},
 			true,
@@ -406,6 +430,14 @@ func TestParseProviderSource(t *testing.T) {
 			Provider{},
 			true,
 		},
+		"example.com/hashicorp/_badtype": {
+			Provider{},
+			true,
+		},
+		"example.com/hashicorp/badtype_": {
+			Provider{},
+			true,
+		},
 		"example.com/hashicorp/bad.type": {
 			Provider{},
 			true,
@@ -453,48 +485,48 @@ func TestParseProviderPart(t *testing.T) {
 			``,
 		},
 		`FOO`: {
-			`foo`,
+			`FOO`,
 			``,
 		},
 		`Foo`: {
-			`foo`,
+			`Foo`,
 			``,
 		},
 		`abc-123`: {
 			`abc-123`,
 			``,
 		},
 		`Испытание`: {
-			`испытание`,
 			``,
+			`must contain only letters, digits, dashes, and underscores, and may not use leading or trailing dashes or underscores`,
 		},
 		`münchen`: { // this is a precomposed u with diaeresis
-			`münchen`, // this is a precomposed u with diaeresis
 			``,
+			`must contain only letters, digits, dashes, and underscores, and may not use leading or trailing dashes or underscores`,
 		},
 		`münchen`: { // this is a separate u and combining diaeresis
-			`münchen`, // this is a precomposed u with diaeresis
 			``,
+			`must contain only letters, digits, dashes, and underscores, and may not use leading or trailing dashes or underscores`,
 		},
 		`abc--123`: {
+			`abc--123`,
 			``,
-			`cannot use multiple consecutive dashes`,
 		},
 		`xn--80akhbyknj4f`: { // this is the punycode form of "испытание", but we don't accept punycode here
+			`xn--80akhbyknj4f`,
 			``,
-			`cannot use multiple consecutive dashes`,
 		},
 		`abc.123`: {
 			``,
 			`dots are not allowed`,
 		},
 		`-abc123`: {
 			``,
-			`must contain only letters, digits, and dashes, and may not use leading or trailing dashes`,
+			`must contain only letters, digits, dashes, and underscores, and may not use leading or trailing dashes or underscores`,
 		},
 		`abc123-`: {
 			``,
-			`must contain only letters, digits, and dashes, and may not use leading or trailing dashes`,
+			`must contain only letters, digits, dashes, and underscores, and may not use leading or trailing dashes or underscores`,
 		},
 		``: {
 			``,