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

Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

Closed
wants to merge 11 commits into from
Prev Previous commit
Next Next commit
add validation tests for table name, add test case for valid table & …
…db name
  • Loading branch information
jukie committed Apr 14, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 6cacfbb2925421d5db0cd2cb9da9f57ccdbf886a
7 changes: 4 additions & 3 deletions aws/resource_aws_glue_catalog_table.go
Original file line number Diff line number Diff line change
@@ -38,9 +38,10 @@ func resourceAwsGlueCatalogTable() *schema.Resource {
Optional: true,
},
"name": {
Type: schema.TypeString,
ForceNew: true,
Required: true,
Type: schema.TypeString,
ForceNew: true,
Required: true,
ValidateFunc: validateGlueCatalogTableName,
},
"owner": {
Type: schema.TypeString,
31 changes: 25 additions & 6 deletions aws/resource_aws_glue_catalog_table_test.go
Original file line number Diff line number Diff line change
@@ -100,23 +100,42 @@ func TestAccAWSGlueCatalogTable_basic(t *testing.T) {
}

func TestAccAWSGlueCatalogTable_validation(t *testing.T) {
validDbName := "my_database"
validTableName := "my_table"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGlueTableDestroy,
Steps: []resource.TestStep{
{
Config: testAccGlueCatalogTable_validation(acctest.RandString(256)),
Config: testAccGlueCatalogTable_validation(validTableName, acctest.RandString(256)),
ExpectError: regexp.MustCompile(`"database_name" cannot be more than 255 characters long`),
},
{
Config: testAccGlueCatalogTable_validation(""),
Config: testAccGlueCatalogTable_validation(validTableName, ""),
ExpectError: regexp.MustCompile(`"database_name" must be at least 1 character long`),
},
{
Config: testAccGlueCatalogTable_validation("ABCDEFG"),
Config: testAccGlueCatalogTable_validation(validTableName, "ABCDEFG"),
ExpectError: regexp.MustCompile(`"database_name" can only contain lowercase alphanumeric characters`),
},
{
Config: testAccGlueCatalogTable_validation(acctest.RandString(256), validDbName),
ExpectError: regexp.MustCompile(`"name" cannot be more than 255 characters long`),
},
{
Config: testAccGlueCatalogTable_validation("", validDbName),
ExpectError: regexp.MustCompile(`"name" must be at least 1 character long`),
},
{
Config: testAccGlueCatalogTable_validation("ABCDEFG", validDbName),
ExpectError: regexp.MustCompile(`"name" can only contain lowercase alphanumeric characters`),
},
{
Config: testAccGlueCatalogTable_validation(validTableName, validDbName),
ExpectNonEmptyPlan: true,
PlanOnly: true,
},
},
})
}
@@ -375,13 +394,13 @@ resource "aws_glue_catalog_table" "test" {
`, rInt, rInt)
}

func testAccGlueCatalogTable_validation(rName string) string {
func testAccGlueCatalogTable_validation(tableName string, dbName string) string {
return fmt.Sprintf(`
resource "aws_glue_catalog_table" "test" {
name = "MyCatalogTable"
name = "%s"
database_name = "%s"
}
`, rName)
`, tableName, dbName)
}

func testAccGlueCatalogTable_full(rInt int, desc string) string {
14 changes: 14 additions & 0 deletions aws/validators.go
Original file line number Diff line number Diff line change
@@ -2438,3 +2438,17 @@ func validateGlueCatalogDatabaseName(v interface{}, k string) (ws []string, erro
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also validate the whole string against a validity regex to stop special characters getting through. e.g.

^[a-z0-9_-]*$

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that regex is valid, \uDC00-\uDBFF is an invalid range. I only have minimal experience using unicode expressions but as long as I've converted to go notation correctly I think we've found a bug.
https://play.golang.org/p/jhV8pEEjPrT

Copy link
Contributor Author

@jukie jukie Sep 28, 2019

Choose a reason for hiding this comment

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

Additionally, special characters are in fact allowed in both the table and database names. Technically, Uppercase is allowed in the api call as well but it's just converted to lowercase after.
This proposal for only accepting lowercase in terraform is because otherwise terraform would always attempt to reapply changes. I think my validator works but if you'd like me to make changes I can.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think your right 👍

}

func validateGlueCatalogTableName(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
if regexp.MustCompile(`[A-Z]`).MatchString(value) {
errors = append(errors, fmt.Errorf("%q can only contain lowercase alphanumeric characters", k))
}
if len(value) < 1 {
errors = append(errors, fmt.Errorf("%q must be at least 1 character long", k))
}
if len(value) > 255 {
errors = append(errors, fmt.Errorf("%q cannot be more than 255 characters long", k))
}
return
}