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

Jump to bottom

Bug - Fix regex for aws_transfer_user #8304

Merged
merged 4 commits into from
Apr 24, 2019
Merged

Bug - Fix regex for aws_transfer_user #8304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06325c4
Change regex to allow hyphens, underscores, and uppercase characters
jukie Apr 12, 2019
0261323
Add test function for user_name validation
jukie Apr 12, 2019
0714b28
add test case for valid username
jukie Apr 14, 2019
7345bd8
Use single regex and add valid resource configs in validation tests
jukie Apr 15, 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
65 changes: 65 additions & 0 deletions aws/resource_aws_transfer_user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,37 @@ func TestAccAWSTransferUser_disappears(t *testing.T) {
})
}

func TestAccAWSTransferUserName_validation(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.

Nit: The acceptance test naming format should match: TestAccAWSTransferUser_XXX so all testing for the resource can be run with the same prefix and underscore, e.g. TestAccAWSTransferUser_UserName_Validation

Otherwise we have this situation (note lack of this new test being run):

$ TF_ACC=1 go test ./aws -v -timeout 120m -parallel 20 -run='TestAccAWSTransferUser_'
=== RUN   TestAccAWSTransferUser_basic
=== PAUSE TestAccAWSTransferUser_basic
=== RUN   TestAccAWSTransferUser_modifyWithOptions
=== PAUSE TestAccAWSTransferUser_modifyWithOptions
=== RUN   TestAccAWSTransferUser_disappears
=== PAUSE TestAccAWSTransferUser_disappears
=== CONT  TestAccAWSTransferUser_basic
=== CONT  TestAccAWSTransferUser_modifyWithOptions
=== CONT  TestAccAWSTransferUser_disappears
--- PASS: TestAccAWSTransferUser_disappears (11.24s)
--- PASS: TestAccAWSTransferUser_basic (12.36s)
--- PASS: TestAccAWSTransferUser_modifyWithOptions (26.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	27.993s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll update and make another PR. Thanks for the review!

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSTransferUserDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSTransferUserName_validation("!@#$%^"),
ExpectError: regexp.MustCompile(`Invalid "user_name": must be between 3 and 32 alphanumeric or special characters hyphen and underscore. However, "user_name" cannot begin with a hyphen`),
},
{
Config: testAccAWSTransferUserName_validation(acctest.RandString(2)),
ExpectError: regexp.MustCompile(`Invalid "user_name": must be between 3 and 32 alphanumeric or special characters hyphen and underscore. However, "user_name" cannot begin with a hyphen`),
},
{
Config: testAccAWSTransferUserName_validation(acctest.RandString(33)),
ExpectError: regexp.MustCompile(`Invalid "user_name": must be between 3 and 32 alphanumeric or special characters hyphen and underscore. However, "user_name" cannot begin with a hyphen`),
},
{
Config: testAccAWSTransferUserName_validation("-abcdef"),
ExpectError: regexp.MustCompile(`Invalid "user_name": must be between 3 and 32 alphanumeric or special characters hyphen and underscore. However, "user_name" cannot begin with a hyphen`),
},
{
Config: testAccAWSTransferUserName_validation("valid_username"),
ExpectNonEmptyPlan: true,
PlanOnly: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

That's cool! You never test that your validation function itself "passes" though.
You might want to add that step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand, is that not what I had already done via below?

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransferUserName_validation'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSTransferUserName_validation -timeout 120m
=== RUN   TestAccAWSTransferUserName_validation
=== PAUSE TestAccAWSTransferUserName_validation
=== CONT  TestAccAWSTransferUserName_validation
--- PASS: TestAccAWSTransferUserName_validation (1.64s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.681s

...

I'll be sure to add whatever else is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that you've tested all the errors but you have no test saying that the name is correct.
They're all expecting errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, I'll take care of that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charlyx could you take another look at this?

},
})
}

func testAccCheckAWSTransferUserExists(n string, res *transfer.DescribedUser) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand Down Expand Up @@ -260,6 +291,40 @@ resource "aws_transfer_user" "foo" {
`, rName, rName)
}

func testAccAWSTransferUserName_validation(rName string) string {
return fmt.Sprintf(`
resource "aws_transfer_server" "foo" {
identity_provider_type = "SERVICE_MANAGED"
tags = {
NAME = "tf-acc-test-transfer-server"
}
}
resource "aws_transfer_user" "foo" {
server_id = "${aws_transfer_server.foo.id}"
user_name = "%s"
role = "${aws_iam_role.foo.arn}"
}
resource "aws_iam_role" "foo" {
name = "tf-test-transfer-user-iam-role"

assume_role_policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Principal": {
"Service": "transfer.amazonaws.com"
},
"Action": "sts:AssumeRole"
}
]
}
EOF
}
`, rName)
}

func testAccAWSTransferUserConfig_options(rName string) string {
return fmt.Sprintf(`
resource "aws_transfer_server" "foo" {
Expand Down
9 changes: 2 additions & 7 deletions aws/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,10 @@ func validateTransferServerID(v interface{}, k string) (ws []string, errors []er

func validateTransferUserName(v interface{}, k string) (ws []string, errors []error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In the future this can be simplified to use Terraform Provider SDK validation functions to show multiple validation errors easier:

ValidateFunc: validation.All(
	validation.StringLenBetween(3, 32),
	validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9_]`), "must begin with alphanumeric or underscore character"),
	validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9-_]+$`), "must contain only alphanumeric, hyphen, and underscore characters"),
),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I wasn't aware of this. Is there a tech-debt effort to change other validation functions as well in validators.go? I'd be happy to help with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought I had created the technical debt issue for that, but apparently not. I'll try to get that created later today.

For now, this is a good start on a similar effort: #7906

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an initial proposal issue for fixing up validators.go: #8424 We should wait until other maintainers submit input to it before beginning any implementation.

value := v.(string)

// https://docs.aws.amazon.com/transfer/latest/userguide/API_CreateUser.html
pattern := `^[a-z0-9]{3,32}$`
if !regexp.MustCompile(pattern).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q isn't a valid transfer user name (only lowercase alphanumeric characters are allowed): %q",
k, value))
if !regexp.MustCompile(`^[a-zA-Z0-9_][a-zA-Z0-9_-]{2,31}$`).MatchString(value) {
errors = append(errors, fmt.Errorf("Invalid %q: must be between 3 and 32 alphanumeric or special characters hyphen and underscore. However, %q cannot begin with a hyphen", k, k))
}

return
}

Expand Down