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.

Sign up for GitHub

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
Closed

Only accept lowercase names for aws_glue_catalog_database and aws_glue_catalog_table #8288

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ab2576d
Add validations for db name casing
jukie Apr 11, 2019
874c3d0
add regexp dependency
jukie Apr 11, 2019
d7e87b4
Move validate function to aws/validators.go
jukie Apr 12, 2019
10dfffe
add test case for validation error
teraken0509 Mar 27, 2019
a42300f
fix regex - all special characters allowed
jukie Apr 12, 2019
6047537
add testAcc functions for catalogtable
jukie Apr 12, 2019
9c73bad
Update webiste docs to use lowercase table names and database names i…
jukie Apr 14, 2019
6cacfbb
add validation tests for table name, add test case for valid table & …
jukie Apr 14, 2019
96ab7a9
add test case for valid database name
jukie Apr 14, 2019
2a54335
rebasing
jukie Sep 27, 2019
0e6a4f5
Fix typos in docs
jukie Sep 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions aws/resource_aws_glue_catalog_database.go
View file
Open in desktop
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
38 changes: 38 additions & 0 deletions aws/resource_aws_glue_catalog_database_test.go
View file
Open in desktop
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 @@ -33,6 +34,28 @@ func TestAccAWSGlueCatalogDatabase_importBasic(t *testing.T) {
})
}

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
},
})
}

func TestAccAWSGlueCatalogDatabase_full(t *testing.T) {
rInt := acctest.RandInt()
resource.ParallelTest(t, resource.TestCase{
Expand Down Expand Up @@ -222,6 +245,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
7 changes: 4 additions & 3 deletions aws/resource_aws_glue_catalog_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ 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,
Expand Down
32 changes: 32 additions & 0 deletions aws/resource_aws_glue_catalog_table_test.go
View file
Open in desktop
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 @@ -98,6 +99,28 @@ func TestAccAWSGlueCatalogTable_basic(t *testing.T) {
})
}

func TestAccAWSGlueCatalogTable_validation(t *testing.T) {
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGlueTableDestroy,
Steps: []resource.TestStep{
{
Config: testAccGlueCatalogTable_validation(acctest.RandString(256)),
ExpectError: regexp.MustCompile(`"database_name" cannot be more than 255 characters long`),
},
{
Config: testAccGlueCatalogTable_validation(""),
ExpectError: regexp.MustCompile(`"database_name" must be at least 1 character long`),
},
{
Config: testAccGlueCatalogTable_validation("ABCDEFG"),
ExpectError: regexp.MustCompile(`"database_name" can only contain lowercase alphanumeric characters`),
},
},
})
}

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

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

func testAccGlueCatalogTable_full(rInt int, desc string) string {
return fmt.Sprintf(`
resource "aws_glue_catalog_database" "test" {
Expand Down
14 changes: 14 additions & 0 deletions aws/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2424,3 +2424,17 @@ 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.

or even better use AWS's
https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-databases.html
https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-common.html#aws-glue-api-regex-oneLine

[\u0020-\uD7FF\uE000-\uFFFD\uD800\uDC00-\uDBFF\uDFFF\t]*

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 •
edited
Loading

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 👍

}