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

Correct validations for AWS Cloud Map service discovery information for App Mesh virtual nodes #9788

Merged
merged 2 commits into from
Nov 20, 2019
Merged

Correct validations for AWS Cloud Map service discovery information for App Mesh virtual nodes #9788

merged 2 commits into from
Nov 20, 2019

Conversation

ewbankkit
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #9764.

Release note for CHANGELOG:

BUG FIX: resource/aws_appmesh_virtual_node: Correct validations for AWS Cloud Map service discovery information

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/VirtualNode'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAppmesh/VirtualNode -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/VirtualNode
=== RUN   TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery
=== RUN   TestAccAWSAppmesh/VirtualNode/listenerHealthChecks
=== RUN   TestAccAWSAppmesh/VirtualNode/logging
=== RUN   TestAccAWSAppmesh/VirtualNode/tags
=== RUN   TestAccAWSAppmesh/VirtualNode/basic
--- PASS: TestAccAWSAppmesh (277.61s)
    --- PASS: TestAccAWSAppmesh/VirtualNode (277.61s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery (113.18s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/listenerHealthChecks (41.33s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/logging (40.46s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/tags (57.66s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/basic (24.99s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	277.675s

@ewbankkit ewbankkit requested a review from a team August 15, 2019 16:34
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/appmesh Issues and PRs that pertain to the appmesh service. labels Aug 15, 2019
@ghost ghost added service/servicediscovery Issues and PRs that pertain to the servicediscovery service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 4, 2019
@ewbankkit
Copy link
Contributor Author

Rebased.

While re-running the acceptance tests I got:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/VirtualNode'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSAppmesh/VirtualNode -timeout 120m
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/VirtualNode
=== RUN   TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery
=== RUN   TestAccAWSAppmesh/VirtualNode/listenerHealthChecks
=== RUN   TestAccAWSAppmesh/VirtualNode/logging
=== RUN   TestAccAWSAppmesh/VirtualNode/tags
=== RUN   TestAccAWSAppmesh/VirtualNode/basic
--- FAIL: TestAccAWSAppmesh (142.17s)
    --- FAIL: TestAccAWSAppmesh/VirtualNode (142.17s)
        --- FAIL: TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery (1.86s)
            testing.go:569: Step 0 error: config is invalid: last character of "name" must be a letter
        --- PASS: TestAccAWSAppmesh/VirtualNode/listenerHealthChecks (35.71s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/logging (34.57s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/tags (48.78s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/basic (21.25s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	142.194s
FAIL
GNUmakefile:24: recipe for target 'testacc' failed
make: *** [testacc] Error 1

which comes from validateServiceDiscoveryHttpNamespaceName().
The random service discovery namespace name has a 10-in-36 chance of ending in a numeric if acctest.CharSetAlphaNum is used so I changed to acctest.CharSetAlpha.
The same error could happen in TestAccAWSServiceDiscoveryService_http() where I also corrected the character set of the random name.

Re-ran acceptance tests:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/VirtualNode'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSAppmesh/VirtualNode -timeout 120m
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/VirtualNode
=== RUN   TestAccAWSAppmesh/VirtualNode/basic
=== RUN   TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery
=== RUN   TestAccAWSAppmesh/VirtualNode/listenerHealthChecks
=== RUN   TestAccAWSAppmesh/VirtualNode/logging
=== RUN   TestAccAWSAppmesh/VirtualNode/tags
--- PASS: TestAccAWSAppmesh (247.29s)
    --- PASS: TestAccAWSAppmesh/VirtualNode (247.29s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/basic (21.80s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery (106.85s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/listenerHealthChecks (34.98s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/logging (34.57s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/tags (49.09s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	247.356s
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSServiceDiscoveryService_' 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSServiceDiscoveryService_ -timeout 120m
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
go: finding github.com/terraform-providers/terraform-provider-tls v2.1.1+incompatible
=== RUN   TestAccAWSServiceDiscoveryService_private
=== PAUSE TestAccAWSServiceDiscoveryService_private
=== RUN   TestAccAWSServiceDiscoveryService_public
=== PAUSE TestAccAWSServiceDiscoveryService_public
=== RUN   TestAccAWSServiceDiscoveryService_http
=== PAUSE TestAccAWSServiceDiscoveryService_http
=== CONT  TestAccAWSServiceDiscoveryService_private
=== CONT  TestAccAWSServiceDiscoveryService_http
=== CONT  TestAccAWSServiceDiscoveryService_public
--- PASS: TestAccAWSServiceDiscoveryService_http (90.99s)
--- PASS: TestAccAWSServiceDiscoveryService_public (113.10s)
--- PASS: TestAccAWSServiceDiscoveryService_private (143.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	143.543s

Kit Ewbank and others added 2 commits October 11, 2019 10:15
@ewbankkit
Copy link
Contributor Author

Rebased and verified that all the checks go green after #10460 was merged.
Re-ran acceptance tests:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/VirtualNode'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSAppmesh/VirtualNode -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/VirtualNode
=== RUN   TestAccAWSAppmesh/VirtualNode/tags
=== RUN   TestAccAWSAppmesh/VirtualNode/basic
=== RUN   TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery
=== RUN   TestAccAWSAppmesh/VirtualNode/listenerHealthChecks
=== RUN   TestAccAWSAppmesh/VirtualNode/logging
--- PASS: TestAccAWSAppmesh (253.86s)
    --- PASS: TestAccAWSAppmesh/VirtualNode (253.86s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/tags (51.12s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/basic (22.22s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery (108.34s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/listenerHealthChecks (35.81s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/logging (36.36s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	253.921s

@tomaszdudek7
Copy link

Any ETA on the merge?

This is blocking people from using App Mesh with AWS Cloud Map(which is the most common usecase). :(

@ewbankkit ewbankkit mentioned this pull request Oct 18, 2019
Closed
@MLDMoritz
Copy link

@bflad @cgbaker @chrisarcand I really don't want to be rude, but this PR is essential to re-gain functionality.

This PR should be merged asap.

@tomaszdudek7
Copy link

@MLDMoritz Fortunately, as long as you don't use any dots in your Route 53 Private Namespace name(so that your ECS Services are available at service1.mynamespace, service2.mynamespace) then it does work correctly, because it passes the validation.

As soon as your name contains dots (service2.mynamespace.myorg) then it fails.

This is a "workaround" for now.

@MLDMoritz
Copy link

@spaszek Thanks for the hint, however accepting FQDN should be working :-)

@bflad bflad added the bug Addresses a defect in current functionality. label Nov 20, 2019
@bflad bflad self-assigned this Nov 20, 2019
@bflad bflad added this to the v2.39.0 milestone Nov 20, 2019
bflad
bflad approved these changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Looks good according to the AppMesh API Reference, sorry for the delayed review/merge, @ewbankkit 🚀

Output from acceptance testing:

--- PASS: TestAccAWSAppmesh (243.64s)
    --- PASS: TestAccAWSAppmesh/VirtualNode (243.64s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/basic (21.29s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/cloudMapServiceDiscovery (105.57s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/listenerHealthChecks (35.61s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/logging (34.10s)
        --- PASS: TestAccAWSAppmesh/VirtualNode/tags (47.07s)

@bflad bflad merged commit f504ac2 into hashicorp:master Nov 20, 2019
@ewbankkit ewbankkit deleted the issue-9764 branch November 20, 2019 17:14
@ghost
Copy link

ghost commented Nov 21, 2019

This has been released in version 2.39.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
bug Addresses a defect in current functionality. service/appmesh Issues and PRs that pertain to the appmesh service. service/servicediscovery Issues and PRs that pertain to the servicediscovery service. size/XS Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v2.39.0
Development

Successfully merging this pull request may close these issues.

Not able to use valid FQDN as namespace in app mesh virtual nodes
4 participants
@ewbankkit @tomaszdudek7 @MLDMoritz @bflad