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

d/aws_prefix_list: fixes and enhancements #14109

Closed
wants to merge 4 commits into from
Closed

d/aws_prefix_list: fixes and enhancements #14109

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
37c49d9
d/aws_prefix_list: fix name filter override and lack of error when ma…
roberth-k Jul 9, 2020
f259a03
d/aws_prefix_list: sort cidrs
roberth-k Jul 9, 2020
3d2ca8e
d/aws_prefix_list: fix doc type and add example about finding a prefi…
roberth-k Jul 9, 2020
a35c88c
d/aws_prefix_list: error check when setting cidr_blocks
roberth-k Jul 9, 2020
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
28 changes: 16 additions & 12 deletions aws/data_source_aws_prefix_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package aws
import (
"fmt"
"log"
"sort"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
Expand Down Expand Up @@ -45,31 +46,34 @@ func dataSourceAwsPrefixListRead(d *schema.ResourceData, meta interface{}) error
if prefixListID := d.Get("prefix_list_id"); prefixListID != "" {
req.PrefixListIds = aws.StringSlice([]string{prefixListID.(string)})
}
req.Filters = buildEC2AttributeFilterList(
map[string]string{
"prefix-list-name": d.Get("name").(string),
},
)
if prefixListName := d.Get("name"); prefixListName.(string) != "" {
req.Filters = append(req.Filters, &ec2.Filter{
Name: aws.String("prefix-list-name"),
Values: aws.StringSlice([]string{prefixListName.(string)}),
})
}

log.Printf("[DEBUG] Reading Prefix List: %s", req)
resp, err := conn.DescribePrefixLists(req)
if err != nil {
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

The predominate coding style for the codebase is if conditionals with early returns rather than bare switch conditionals. Can you please change this back rather than introducing a different coding style? Thanks!

case err != nil:
return err
}
if resp == nil || len(resp.PrefixLists) == 0 {
case resp == nil || len(resp.PrefixLists) == 0:
return fmt.Errorf("no matching prefix list found; the prefix list ID or name may be invalid or not exist in the current region")
case len(resp.PrefixLists) > 1:
return fmt.Errorf("more than one prefix list matched the given set of criteria")
Comment on lines +63 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

While this is a perfectly valid change for consistency with most other data sources, we should only perform this change during a major version upgrade to align with practitioner versioning expectations, which the next one will be a few months away. It would be best to remove this change here and create a separate GitHub bug report so we can mark it for that future milestone. 👍 You're more than welcome to submit this change as well separately.

}

pl := resp.PrefixLists[0]

d.SetId(*pl.PrefixListId)
d.Set("name", pl.PrefixListName)

cidrs := make([]string, len(pl.Cidrs))
for i, v := range pl.Cidrs {
cidrs[i] = *v
cidrs := aws.StringValueSlice(pl.Cidrs)
sort.Strings(cidrs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here with respect to introducing a breaking change which should be removed for now.

In this case though, if the ordering of the CIDR Blocks is not significant (which it likely is not), the cidr_blocks attribute should be changed to TypeSet instead of attempting to add ordering that is inconsistent with the semantics of the upstream API. Terraform configurations can and should use built in language support such as resource for_each for handling all elements of an attribute like this to avoid ordering concerns.

At some point in the future we may go back through many of the TypeList attributes and convert them to TypeSet to ensure that Terraform configurations are not depending on ordering where its not guaranteed or semantically correct.

if err := d.Set("cidr_blocks", cidrs); err != nil {
return fmt.Errorf("failed to set cidr blocks of prefix list %s: %s", d.Id(), err)
}
d.Set("cidr_blocks", cidrs)

return nil
}
45 changes: 45 additions & 0 deletions aws/data_source_aws_prefix_list_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"
"strconv"
"testing"

Expand Down Expand Up @@ -98,3 +99,47 @@ data "aws_prefix_list" "s3_by_id" {
}
}
`

func TestAccDataSourceAwsPrefixList_matchesTooMany(t *testing.T) {
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccDataSourceAwsPrefixListConfig_matchesTooMany,
ExpectError: regexp.MustCompile(`more than one prefix list matched the given set of criteria`),
},
},
})
}

const testAccDataSourceAwsPrefixListConfig_matchesTooMany = `
data "aws_prefix_list" "test" {}
`

func TestAccDataSourceAwsPrefixList_nameDoesNotOverrideFilter(t *testing.T) {
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
{
// The vanilla DescribePrefixLists API only supports filtering by
// id and name. In this case, the `name` attribute and `prefix-list-id`
// filter have been set up such that they conflict, thus proving
// that both criteria took effect.
Config: testAccDataSourceAwsPrefixListConfig_nameDoesNotOverrideFilter,
ExpectError: regexp.MustCompile(`no matching prefix list found`),
},
},
})
}

const testAccDataSourceAwsPrefixListConfig_nameDoesNotOverrideFilter = `
data "aws_prefix_list" "test" {
name = "com.amazonaws.us-west-2.s3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of our newer codebase linting will likely pick this up on rebase/merge for not being AWS Region agnostic. Similar to the website example you added (👍 ) we should use the aws_region data source here so the configuration can be applied in anywhere. This also means we'll need to remove the hardcoded EC2 Prefix List identifier. Something like this should get you close:

data "aws_region" "current" {}

data "aws_prefix_list" "dynamodb" {
  name = "com.amazonaws.${data.aws_region.current.name}.dynamodb"
}

data "aws_prefix_list" "test" {
  name = "com.amazonaws.${data.aws_region.current.name}.s3"

  filter {
    name = "prefix-list-id"
    values = [data.aws_prefix_list.dynamodb.id]
  }
}

filter {
name = "prefix-list-id"
values = ["pl-00a54069"] # com.amazonaws.us-west-2.dynamodb
}
}
`
11 changes: 10 additions & 1 deletion website/docs/d/prefix_list.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
subcategory: "VPC"
layout: "aws"
page_title: "AWS: aws_prefix-list"
page_title: "AWS: aws_prefix_list"
description: |-
Provides details about a specific prefix list
---
Expand Down Expand Up @@ -44,6 +44,15 @@ resource "aws_network_acl_rule" "private_s3" {
}
```

### Find the regional DynamoDB prefix list

```hcl
data "aws_region" "current" {}
data "aws_prefix_list" "dynamo" {
name = "com.amazonaws.${data.aws_region.current.name}.dynamodb"
}
```

### Filter

```hcl
Expand Down