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

Add state filter to aws_availability_zones data source. #7965

Conversation

kwilczynski
Copy link
Contributor

@kwilczynski kwilczynski commented Aug 4, 2016

This commit adds an ability to filter Availability Zones based on state.

Be advised that this does not always works reliably for an older accounts which
have been created in the pre-VPC era of EC2. These accounts tends to retrieve
availability zones that are not VPC-enabled, thus creation of a custom subnet
within such Availability Zone would result in a failure.

Signed-off-by: Krzysztof Wilczynski krzysztof.wilczynski@linux.com

This commit adds an ability to filter Availability Zones based on state, where
by default it would only list available zones.

Be advised that this does not always works reliably for an older accounts which
have been created in the pre-VPC era of EC2. These accounts tends to retrieve
availability zones that are not VPC-enabled, thus creation of a custom subnet
within such Availability Zone would result in a failure.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
@kwilczynski
Copy link
Contributor Author

Resolves #7959.

@kwilczynski
Copy link
Contributor Author

Test is passing:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAvailabilityZones_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/04 10:52:29 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAvailabilityZones_basic -timeout 120m
=== RUN   TestAccAWSAvailabilityZones_basic
--- PASS: TestAccAWSAvailabilityZones_basic (23.28s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    23.303s

@kwilczynski
Copy link
Contributor Author

The debug output confirms that state is set:

data.aws_availability_zones.availability_zones:
  ID = 2016-08-04 01:35:06.34062534 +0000 UTC
  names.# = 3
  names.0 = us-west-2a
  names.1 = us-west-2b
  names.2 = us-west-2c
  state = available

@kwilczynski kwilczynski changed the title Add state filter to aws_availability_zones data source. [WIP] Add state filter to aws_availability_zones data source. Aug 4, 2016
Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
@kwilczynski
Copy link
Contributor Author

@stack72 over to you 🚀

@kwilczynski kwilczynski changed the title [WIP] Add state filter to aws_availability_zones data source. Add state filter to aws_availability_zones data source. Aug 4, 2016
"state": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Default: "available",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think setting this default would make it impossible to retrieve an unfiltered list, and is a backward-compatibility break.

Could we instead default to no filter, and have the user explicitly set state = "available" to get the filtered list? This does make the user do a little more work, but has the advantage of being consistent with how some of our other data sources are working (each new attribute acts as an additional filter constraint) and makes things a little more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apparentlymart makes sense! I will push an update shortly.

This commit makes the state filter applicable only when set.

Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
@kwilczynski
Copy link
Contributor Author

Test is passing:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAvailabilityZones_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/05 06:30:40 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAvailabilityZones_basic -timeout 120m

=== RUN   TestAccAWSAvailabilityZones_basic
--- PASS: TestAccAWSAvailabilityZones_basic (35.80s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    35.822s

@kwilczynski
Copy link
Contributor Author

@stack72 @apparentlymart over to you 🚀


## Attributes Reference

The following attributes are exported:

* `names` - A list of the availability zone names available to the account.
* `names` - A list of the Availability Zone names available to the account.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little weird to capitalize non-proper nouns in English... what was the motivation for this change 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.

Just to reflect what AWS does in their documentation, which might not be correct convention (since I am not an English language native speaker, etc., I have assumed that their style is somewhat acceptable). For example, as per: Regions and Availability Zones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm interesting... I'd not noticed that before. I think we've generally treated "availability zones" as a general noun rather than a proper noun elsewhere in our docs, so it might be a bit late for us to switch to the AWS docs style now without a lot of updates on other doc pages. Not a big deal though, certainly.

Copy link
Contributor Author

@kwilczynski kwilczynski Aug 4, 2016

Choose a reason for hiding this comment

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

Sorry for causing troubles!

In terms of consistency in documentation we have, it does vary:

(not a very scientific comparison, though)

krzysztof@apple ~/Development/Projects/Go/src/github.com/hashicorp/terraform (master %)$ ack --no-smart-case availability website/source/ -l | wc -l
      45
krzysztof@apple ~/Development/Projects/Go/src/github.com/hashicorp/terraform (master %)$ ack --no-smart-case Availability website/source/ -l | wc -l
      11

Would you like me to revert this back?

Copy link
Contributor

Choose a reason for hiding this comment

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

@apparentlymart we can merge this one for now and then follow up on the docs later :)

@stack72
Copy link
Contributor

stack72 commented Aug 5, 2016

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAvailabilityZones_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAvailabilityZones_ -timeout 120m
=== RUN   TestAccAWSAvailabilityZones_basic
--- PASS: TestAccAWSAvailabilityZones_basic (109.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    109.018s

@stack72 stack72 merged commit 19800b8 into hashicorp:master Aug 5, 2016
@cmusser
Copy link

cmusser commented Oct 10, 2016

With 0.7.5, I tried using the following, hoping to use the second AZ in the us-west-1 region in a subnet:

data "aws_availability_zones" "available" {
  state = "available"
}

resource "aws_subnet" "subnet-public-2" {
  vpc_id = "${aws_vpc.vpc-standalone.id}"
  availability_zone = "${data.aws_availability_zones.available.names[1]}"
  cidr_block = "${var.vpc_public_subnet_2}"
  tags {
    "Name" = "${var.vpc_name} Public 2"
    "VPC" = "${var.vpc_name}"
  }
}

It tried to insert "us-west-1b" (index 1), which isn't available. Did I do this incorrectly or is this fix not available in the version I'm using?

@kwilczynski
Copy link
Contributor Author

@cmusser hi there! I am sorry that you are having issues! Let me have a look what is going on here.

@kwilczynski
Copy link
Contributor Author

@cmusser hi there again!

I have had a look, and for example, for my account Terraform returns the right set of results:

$ terraform show
data.aws_availability_zones.available:
  id = 2016-10-10 23:04:29.858510486 +0000 UTC
  names.# = 2
  names.0 = us-west-1b
  names.1 = us-west-1c

$ aws ec2 describe-availability-zones --region us-west-1
{
    "AvailabilityZones": [
        {
            "State": "available",
            "RegionName": "us-west-1",
            "Messages": [],
            "ZoneName": "us-west-1b"
        },
        {
            "State": "available",
            "RegionName": "us-west-1",
            "Messages": [],
            "ZoneName": "us-west-1c"
        }
    ]
}

Which is consistent with the AWSCLI output for the given (same) set of credentials. Basically, this is all my IAM user sees in terms of what my account has an access to.

Also, us-west-1b seem to be available.

Is there something I am missing? I am not sure if I got the gist of the problem right.

@cmusser
Copy link

cmusser commented Oct 11, 2016

There is something funny going on, perhaps with AWS itself. I see the same thing you did with AWS CLI: 1b appears available. So it's reasonable that Terraform places it in the list. The specific error when creating the subnet with the list of available AZs is:

* aws_subnet.subnet-public-2: Error creating subnet: InvalidParameterValue: Value (us-west-1b) for parameter availabilityZone is invalid. Subnets can currently only be created in the following availability zones: us-west-1a, us-west-1c.

Maybe its somehow possible for them to return a zone as available, when it really isn't for subnets.

@kwilczynski
Copy link
Contributor Author

@cmusser the Availability Zones are very elusive. From the AWS documentation (excerpt):

As Availability Zones grow over time, our ability to expand them can become constrained. If this happens, we might restrict you from launching an instance in a constrained Availability Zone unless you already have an instance in that Availability Zone. Eventually, we might also remove the constrained Availability Zone from the list of Availability Zones for new customers. Therefore, your account might have a different number of available Availability Zones in a region than another account.

See: http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-regions-availability-zones.html

I have to accounts and both have different availability zones reported. The issue with subnets is quite a common one. Sadly, there is no reliable way to detect this (or at least I am not familiar with a possible solution and/or workaround), especially as there is no API from Amazon which would provide such details e.g. which Availability Zones are VPC-enabled and allow subnets creation, etc.

Related reading material:

I am sorry that I have no better solution for you yet.

We could, in theory, have a "VPC subnet probe" which would make an attempt to create a VPC then create a subnet in each of the Availability Zones, and catch failure/success in turn assembling a list of ones which support subnets creation. But this would be rather slow.

@pmoust
Copy link
Contributor

pmoust commented Dec 8, 2016

Hitting the same thing, and I am afraid the VPC subnet probe is the only way to actually determine if an AZ is available.

Is there a Github issue for this?

@kwilczynski
Copy link
Contributor Author

@pmoust hi there!

I agree. And it is rather annoying to stumble into. I proposed the VPC probe, but was seen as not necessarily the right thing to do (albeit, there isn't any other that works 100% successfully), and I can see why as it would require to attempt to provision a VPC with a subnet which might not necessarily be the best thing to do e.g. can be affected by account limits, etc.

What do you think, @stack72 and @radeksimko?

@pmoust
Copy link
Contributor

pmoust commented Dec 8, 2016

@kwilczynski I have contacted AWS via our TAMs, and can confirm there is no other sane way at the moment.
Perhaps adding another parameter like a boolean ensure_usable (naming is hard, heh) that does the dummy VPC probe and catches that specific exception only?

@ghost
Copy link

ghost commented Apr 19, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants