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

data-source/aws_region: Remove EC2 API call, default to current if no endpoint or name specified #3157

Merged
merged 3 commits into from
Feb 9, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
89 changes: 57 additions & 32 deletions aws/data_source_aws_region.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ package aws

import (
"fmt"
"log"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/aws/aws-sdk-go/aws/endpoints"
"github.com/hashicorp/terraform/helper/schema"
)

Expand Down Expand Up @@ -36,48 +35,74 @@ func dataSourceAwsRegion() *schema.Resource {
}

func dataSourceAwsRegionRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn
currentRegion := meta.(*AWSClient).region

req := &ec2.DescribeRegionsInput{}
var matchingRegion *endpoints.Region

req.RegionNames = make([]*string, 0, 2)
if name := d.Get("name").(string); name != "" {
req.RegionNames = append(req.RegionNames, aws.String(name))
if v, ok := d.GetOk("endpoint"); ok {
endpoint := v.(string)
for _, partition := range endpoints.DefaultPartitions() {
for _, region := range partition.Regions() {
regionEndpointEc2, err := region.ResolveEndpoint(endpoints.Ec2ServiceID)
if err != nil {
return err
}
if strings.TrimPrefix(regionEndpointEc2.URL, "https://") == endpoint {
matchingRegion = &region
break
}
}
}
if matchingRegion == nil {
return fmt.Errorf("region not found for endpoint: %s", endpoint)
}
Copy link
Member

Choose a reason for hiding this comment

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

It took me a while to understand what this block of code does. 😅 Would you mind extracting it into something like matchingRegion, err := findRegionByEndpoint(endpoint)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea! Let me implement that.

}

if d.Get("current").(bool) {
req.RegionNames = append(req.RegionNames, aws.String(currentRegion))
if v, ok := d.GetOk("name"); ok {
name := v.(string)
for _, partition := range endpoints.DefaultPartitions() {
for _, region := range partition.Regions() {
if region.ID() == name {
if matchingRegion != nil && (*matchingRegion).ID() != name {
return fmt.Errorf("multiple regions matched; use additional constraints to reduce matches to a single region")
}
matchingRegion = &region
break
}
}
}
if matchingRegion == nil {
return fmt.Errorf("region not found for name: %s", name)
}
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - how do you feel about extracting this into matchingRegion, err := findRegionByName(name)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 👍

}

req.Filters = buildEC2AttributeFilterList(
map[string]string{
"endpoint": d.Get("endpoint").(string),
},
)
if len(req.Filters) == 0 {
// Don't send an empty filters list; the EC2 API won't accept it.
req.Filters = nil
current := d.Get("current").(bool)
for _, partition := range endpoints.DefaultPartitions() {
for _, region := range partition.Regions() {
if region.ID() == currentRegion {
if matchingRegion == nil {
matchingRegion = &region
break
}
if current && (*matchingRegion).ID() != currentRegion {
return fmt.Errorf("multiple regions matched; use additional constraints to reduce matches to a single region")
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason we can't just error out here regardless of current?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, I think I was trying to make it so it could support current being false or something. I'm on board with deprecating the current attribute completely (making it have no effect in the interim period) so this extra logic is moot and I'll remove it now.

}
}
}
}

log.Printf("[DEBUG] Reading Region: %s", req)
resp, err := conn.DescribeRegions(req)
region := *matchingRegion

d.SetId(region.ID())
d.Set("current", region.ID() == currentRegion)

regionEndpointEc2, err := region.ResolveEndpoint(endpoints.Ec2ServiceID)
if err != nil {
return err
}
if resp == nil || len(resp.Regions) == 0 {
return fmt.Errorf("no matching regions found")
}
if len(resp.Regions) > 1 {
return fmt.Errorf("multiple regions matched; use additional constraints to reduce matches to a single region")
}

region := resp.Regions[0]
d.Set("endpoint", strings.TrimPrefix(regionEndpointEc2.URL, "https://"))
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity: What is the reason for the trimming here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the EC2 API response was returning just the hostname (see example here: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeRegions.html), however the SDK ResolvedEndpoint.URL prepends https:// to the front. We weren't previously testing the old behavior, but the acceptance testing shows it doing the required trimming (I think).


d.SetId(*region.RegionName)
d.Set("name", region.RegionName)
d.Set("endpoint", region.Endpoint)
d.Set("current", *region.RegionName == currentRegion)
d.Set("name", region.ID())

return nil
}
183 changes: 157 additions & 26 deletions aws/data_source_aws_region_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,63 +2,194 @@ package aws

import (
"fmt"
"os"
"regexp"
"testing"

"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccDataSourceAwsRegion(t *testing.T) {
func TestAccDataSourceAwsRegion_basic(t *testing.T) {
// Ensure we always get a consistent result
oldvar := os.Getenv("AWS_DEFAULT_REGION")
os.Setenv("AWS_DEFAULT_REGION", "us-east-1")
defer os.Setenv("AWS_DEFAULT_REGION", oldvar)

resourceName := "data.aws_region.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig,
Config: testAccDataSourceAwsRegionConfig_empty,
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "true"),
resource.TestCheckResourceAttr(resourceName, "endpoint", "ec2.us-east-1.amazonaws.com"),
resource.TestCheckResourceAttr(resourceName, "name", "us-east-1"),
),
},
},
})
}

func TestAccDataSourceAwsRegion_endpoint(t *testing.T) {
// Ensure we always get a consistent result
oldvar := os.Getenv("AWS_DEFAULT_REGION")
os.Setenv("AWS_DEFAULT_REGION", "us-east-1")
defer os.Setenv("AWS_DEFAULT_REGION", oldvar)

endpoint1 := "ec2.us-east-1.amazonaws.com"
endpoint2 := "ec2.us-east-2.amazonaws.com"
name1 := "us-east-1"
name2 := "us-east-2"
resourceName := "data.aws_region.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_endpoint(endpoint1),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "true"),
resource.TestCheckResourceAttr(resourceName, "endpoint", endpoint1),
resource.TestCheckResourceAttr(resourceName, "name", name1),
),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_endpoint(endpoint2),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck("data.aws_region.by_name_current", "us-west-2", "true"),
testAccDataSourceAwsRegionCheck("data.aws_region.by_name_other", "us-west-1", "false"),
testAccDataSourceAwsRegionCheck("data.aws_region.by_current", "us-west-2", "true"),
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "false"),
resource.TestCheckResourceAttr(resourceName, "endpoint", endpoint2),
resource.TestCheckResourceAttr(resourceName, "name", name2),
),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_currentAndEndpoint(endpoint1),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "true"),
resource.TestCheckResourceAttr(resourceName, "endpoint", endpoint1),
resource.TestCheckResourceAttr(resourceName, "name", name1),
),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_endpoint("does-not-exist"),
ExpectError: regexp.MustCompile(`region not found for endpoint: does-not-exist`),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_currentAndEndpoint(endpoint2),
ExpectError: regexp.MustCompile(`multiple regions matched`),
},
},
})
}

func testAccDataSourceAwsRegionCheck(name, region, current string) resource.TestCheckFunc {
func TestAccDataSourceAwsRegion_name(t *testing.T) {
// Ensure we always get a consistent result
oldvar := os.Getenv("AWS_DEFAULT_REGION")
os.Setenv("AWS_DEFAULT_REGION", "us-east-1")
defer os.Setenv("AWS_DEFAULT_REGION", oldvar)

endpoint1 := "ec2.us-east-1.amazonaws.com"
endpoint2 := "ec2.us-east-2.amazonaws.com"
name1 := "us-east-1"
name2 := "us-east-2"
resourceName := "data.aws_region.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_name(name1),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "true"),
resource.TestCheckResourceAttr(resourceName, "endpoint", endpoint1),
resource.TestCheckResourceAttr(resourceName, "name", name1),
),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_name(name2),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "false"),
resource.TestCheckResourceAttr(resourceName, "endpoint", endpoint2),
resource.TestCheckResourceAttr(resourceName, "name", name2),
),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_currentAndName(name1),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsRegionCheck(resourceName),
resource.TestCheckResourceAttr(resourceName, "current", "true"),
resource.TestCheckResourceAttr(resourceName, "endpoint", endpoint1),
resource.TestCheckResourceAttr(resourceName, "name", name1),
),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_name("does-not-exist"),
ExpectError: regexp.MustCompile(`region not found for name: does-not-exist`),
},
resource.TestStep{
Config: testAccDataSourceAwsRegionConfig_currentAndName(name2),
ExpectError: regexp.MustCompile(`multiple regions matched`),
},
},
})
}

func testAccDataSourceAwsRegionCheck(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[name]
_, ok := s.RootModule().Resources[name]
if !ok {
return fmt.Errorf("root module has no resource called %s", name)
}

attr := rs.Primary.Attributes

if attr["name"] != region {
return fmt.Errorf("bad name %s", attr["name"])
}
if attr["current"] != current {
return fmt.Errorf("bad current %s; want %s", attr["current"], current)
}

return nil
}
}

const testAccDataSourceAwsRegionConfig = `
provider "aws" {
region = "us-west-2"
const testAccDataSourceAwsRegionConfig_empty = `
data "aws_region" "test" {}
`

func testAccDataSourceAwsRegionConfig_currentAndEndpoint(endpoint string) string {
return fmt.Sprintf(`
data "aws_region" "test" {
current = true
endpoint = "%s"
}
`, endpoint)
}

data "aws_region" "by_name_current" {
name = "us-west-2"
func testAccDataSourceAwsRegionConfig_currentAndName(name string) string {
return fmt.Sprintf(`
data "aws_region" "test" {
current = true
name = "%s"
}
`, name)
}

data "aws_region" "by_name_other" {
name = "us-west-1"
func testAccDataSourceAwsRegionConfig_endpoint(endpoint string) string {
return fmt.Sprintf(`
data "aws_region" "test" {
endpoint = "%s"
}
`, endpoint)
}

data "aws_region" "by_current" {
current = true
func testAccDataSourceAwsRegionConfig_name(name string) string {
return fmt.Sprintf(`
data "aws_region" "test" {
name = "%s"
}
`, name)
}
`
21 changes: 8 additions & 13 deletions website/docs/d/region.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,18 @@ description: |-

`aws_region` provides details about a specific AWS region.

As well as validating a given region name (and optionally obtaining its
endpoint) this resource can be used to discover the name of the region
configured within the provider. The latter can be useful in a child module
which is inheriting an AWS provider configuration from its parent module.
As well as validating a given region name this resource can be used to
discover the name of the region configured within the provider. The latter
can be useful in a child module which is inheriting an AWS provider
configuration from its parent module.

## Example Usage

The following example shows how the resource might be used to obtain
the name of the AWS region configured on the provider.

```hcl
data "aws_region" "current" {
current = true
}
data "aws_region" "current" {}
```

## Argument Reference
Expand All @@ -35,12 +33,9 @@ exported as attributes.
* `name` - (Optional) The full name of the region to select.

* `current` - (Optional) Set to `true` to match only the region configured
in the provider. (It is not meaningful to set this to `false`.)
in the provider. Defaults to `true` if `endpoint` or `name` is not given.
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what is the value of current = true still. 🤔 Can't we just assume it's always true in such conditions and deprecate the field?


* `endpoint` - (Optional) The endpoint of the region to select.

At least one of the above attributes should be provided to ensure that only
one region is matched.
* `endpoint` - (Optional) The EC2 endpoint of the region to select.

## Attributes Reference

Expand All @@ -51,4 +46,4 @@ The following attributes are exported:
* `current` - `true` if the selected region is the one configured on the
provider, or `false` otherwise.

* `endpoint` - The endpoint for the selected region.
* `endpoint` - The EC2 endpoint for the selected region.
Copy link
Member

Choose a reason for hiding this comment

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

Thank you! 👍