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
7 changes: 4 additions & 3 deletions aws/resource_aws_glue_catalog_database.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ func resourceAwsGlueCatalogDatabase() *schema.Resource {
Computed: true,
},
"name": {
Type: schema.TypeString,
ForceNew: true,
Required: true,
Type: schema.TypeString,
ForceNew: true,
Required: true,
ValidateFunc: validateGlueCatalogDatabaseName,
},
"description": {
Type: schema.TypeString,
Expand Down
64 changes: 64 additions & 0 deletions aws/resource_aws_glue_catalog_database_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"fmt"
"regexp"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand All @@ -12,6 +13,54 @@ import (
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSGlueCatalogDatabase_importBasic(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're in the process of removing import-only testing (#8944). Please add the ImportState testing to existing tests if its not present, although TestAccAWSGlueCatalogDatabase_full on master looks like it includes it already.

resourceName := "aws_glue_catalog_database.test"
rInt := acctest.RandInt()

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGlueDatabaseDestroy,
Steps: []resource.TestStep{
{
Config: testAccGlueCatalogDatabase_full(rInt, "A test catalog from terraform"),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccAWSGlueCatalogDatabase_validation(t *testing.T) {
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGlueDatabaseDestroy,
Steps: []resource.TestStep{
{
Config: testAccGlueCatalogDatabase_validation(acctest.RandString(256)),
ExpectError: regexp.MustCompile(`"name" cannot be more than 255 characters long`),
},
{
Config: testAccGlueCatalogDatabase_validation(""),
ExpectError: regexp.MustCompile(`"name" must be at least 1 character long`),
},
{
Config: testAccGlueCatalogDatabase_validation("ABCDEFG"),
ExpectError: regexp.MustCompile(`"name" can only contain lowercase alphanumeric characters`),
},
jukie marked this conversation as resolved.
Show resolved Hide resolved
{
Config: testAccGlueCatalogDatabase_validation("valid_dbname"),
ExpectNonEmptyPlan: true,
PlanOnly: true,
},
},
})
}

func TestAccAWSGlueCatalogDatabase_full(t *testing.T) {
rInt := acctest.RandInt()
resourceName := "aws_glue_catalog_database.test"
Expand Down Expand Up @@ -208,6 +257,21 @@ resource "aws_glue_catalog_database" "test" {
`, rInt, desc)
}

func testAccGlueCatalogDatabase_validation(rName string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, it might be better if this was named against the attribute its testing:

Suggested change
func testAccGlueCatalogDatabase_validation(rName string) string {
func testAccGlueCatalogDatabaseConfigName(rName string) string {

return fmt.Sprintf(`
resource "aws_glue_catalog_database" "test" {
name = "%s"
description = "validation test"
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the rest of the arguments are optional and we're only looking to test the name attribute, I'd suggest removing the other arguments from the test configuration.

location_uri = "my-location"
parameters = {
param1 = "value1"
param2 = true
param3 = 50
}
}
`, rName)
}

func testAccCheckGlueCatalogDatabaseExists(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[name]
Expand Down
14 changes: 8 additions & 6 deletions aws/resource_aws_glue_catalog_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,20 @@ func resourceAwsGlueCatalogTable() *schema.Resource {
Computed: true,
},
"database_name": {
Type: schema.TypeString,
ForceNew: true,
Required: true,
Type: schema.TypeString,
ForceNew: true,
Required: true,
ValidateFunc: validateGlueCatalogDatabaseName,
},
"description": {
Type: schema.TypeString,
Optional: true,
},
"name": {
Type: schema.TypeString,
ForceNew: true,
Required: true,
Type: schema.TypeString,
ForceNew: true,
Required: true,
ValidateFunc: validateGlueCatalogTableName,
},
"owner": {
Type: schema.TypeString,
Expand Down
51 changes: 51 additions & 0 deletions aws/resource_aws_glue_catalog_table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"fmt"
"regexp"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -82,6 +83,47 @@ 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(validTableName, acctest.RandString(256)),
ExpectError: regexp.MustCompile(`"database_name" cannot be more than 255 characters long`),
},
{
Config: testAccGlueCatalogTable_validation(validTableName, ""),
ExpectError: regexp.MustCompile(`"database_name" must be at least 1 character long`),
},
{
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,
},
},
})
}

func TestAccAWSGlueCatalogTable_full(t *testing.T) {
rInt := acctest.RandInt()
description := "A test table from terraform"
Expand Down Expand Up @@ -351,6 +393,15 @@ resource "aws_glue_catalog_table" "test" {
`, rInt, rInt)
}

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

func testAccGlueCatalogTable_full(rInt int, desc string) string {
return fmt.Sprintf(`
resource "aws_glue_catalog_database" "test" {
Expand Down
28 changes: 28 additions & 0 deletions aws/validators.go
Original file line number Diff line number Diff line change
Expand Up @@ -2371,3 +2371,31 @@ func validateRoute53ResolverName(v interface{}, k string) (ws []string, errors [

return
}

func validateGlueCatalogDatabaseName(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
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
}
2 changes: 1 addition & 1 deletion website/docs/r/glue_catalog_database.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Provides a Glue Catalog Database Resource. You can refer to the [Glue Developer

```hcl
resource "aws_glue_catalog_database" "aws_glue_catalog_database" {
name = "MyCatalogDatabase"
name = "my_catalog_database"
}
```

Expand Down
10 changes: 5 additions & 5 deletions website/docs/r/glue_catalog_table.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@ Provides a Glue Catalog Table Resource. You can refer to the [Glue Developer Gui

```hcl
resource "aws_glue_catalog_table" "aws_glue_catalog_table" {
name = "MyCatalogTable"
database_name = "MyCatalogDatabase"
name = "my_catalog_table"
database_name = "my_catalog_database"
}
```

### Parquet Table for Athena

```hcl
resource "aws_glue_catalog_table" "aws_glue_catalog_table" {
name = "MyCatalogTable"
database_name = "MyCatalogDatabase"
name = "my_catalog_table"
database_name = "my_catalog_database"

table_type = "EXTERNAL_TABLE"

Expand Down Expand Up @@ -140,5 +140,5 @@ The following arguments are supported:
Glue Tables can be imported with their catalog ID (usually AWS account ID), database name, and table name, e.g.

```
$ terraform import aws_glue_catalog_table.MyTable 123456789012:MyDatabase:MyTable
$ terraform import aws_glue_catalog_table.my_table 123456789012:my_database:my_table
```