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 Route 53 Resolver endpoint resource #6574

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@ewbankkit
Copy link
Contributor

ewbankkit commented Nov 24, 2018

Fixes #6563.
Includes:

Acceptance tests (so far):

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverEndpoint_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverEndpoint_ -timeout 120m
=== RUN   TestAccAwsRoute53ResolverEndpoint_importBasic
=== PAUSE TestAccAwsRoute53ResolverEndpoint_importBasic
=== RUN   TestAccAwsRoute53ResolverEndpoint_basicInbound
=== PAUSE TestAccAwsRoute53ResolverEndpoint_basicInbound
=== RUN   TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== PAUSE TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== CONT  TestAccAwsRoute53ResolverEndpoint_importBasic
=== CONT  TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== CONT  TestAccAwsRoute53ResolverEndpoint_basicInbound
--- PASS: TestAccAwsRoute53ResolverEndpoint_importBasic (108.99s)
--- PASS: TestAccAwsRoute53ResolverEndpoint_basicInbound (129.35s)
--- PASS: TestAccAwsRoute53ResolverEndpoint_updateOutbound (503.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	503.819s
@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Nov 24, 2018

This is still a WIP as I'm having problems supporting more than one "IP address" (subnet ID and optional specified IP) per subnet.
The issue is that I want to use a TypeSet for the ip_address attribute (using TypeList has the problem that the order of IP addresses returned from ListResolverEndpointIpAddresses is non-deterministic and state diffs will keep showing) but there are a couple of wrinkles:

  • ip is computed - If it's blank when creating or updating the Resolver endpoint then AWS assigns a value, so it can't be used in the set's hash function
  • More than one ip_address per subnet_id is allowed so I can't just use the hash of the subnet ID as the set's hash value although that is what I am doing right now to get the tests working
@gazoakley

This comment has been minimized.

Copy link
Contributor

gazoakley commented Nov 24, 2018

You might be able to use TypeList with DiffSuppressFunc and match things up by ip_id. That sounds like a pain, but it might work.

@gazoakley

This comment has been minimized.

Copy link
Contributor

gazoakley commented Nov 25, 2018

Your DiffSuppressFunc might:

  • Check the ip_address entries where ip is specified all exist and are associated with the correct subnet_id
  • Check there are the correct number of entries per subnet_id for the remaining ip_address entries
  • Suppress the diff if all the above pass

@ewbankkit ewbankkit force-pushed the ewbankkit:issue-6563 branch from 350fbf4 to 65fb362 Nov 27, 2018

@bflad bflad added new-resource and removed dependencies labels Nov 30, 2018

@ewbankkit ewbankkit force-pushed the ewbankkit:issue-6563 branch from a2501a0 to 7d24122 Dec 7, 2018

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Dec 7, 2018

Not having much luck with the ip_address diff issue and won't have time to work on this for the next week or so.
I've reverted to a set for now with still the same concerns:

	// TODO
	// TODO * "ip" may be computed - Blank on input, set by API on output; How to handle?
	// TODO * Multiple "ip_address" values per "subnet_id" are allowed in the API
	// TODO   but a List (instead of a Set) won't work because of non-deterministic order
	// TODO   on listing ip_address, plus how to determine diffs for update?
	// TODO

Acceptance tests:

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

@ewbankkit ewbankkit force-pushed the ewbankkit:issue-6563 branch from 7d24122 to 038c0bf Dec 16, 2018

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Dec 16, 2018

Rebased to fix conflicts.
Re-ran acceptance tests:

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

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Dec 16, 2018

@bflad Looking for advice on the best way of dealing with the set/list conundrum for the ip_address attribute. It best fits the requirements for a set - Order is unimportant, but there are problems with determining the correct hash function.
Maybe we leave as I have it now - Just use subnet_id, which is the one Required field, as the hash value?

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Dec 18, 2018

TODO: Update tag handling code to reflect comments here.

@rbadillo

This comment has been minimized.

Copy link

rbadillo commented Jan 4, 2019

Hi team, any updates when this will be available ?

@ewbankkit

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Jan 7, 2019

@bflad Any thoughts on my stumbling block - #6574 (comment)?

@aar6ncai

This comment has been minimized.

Copy link

aar6ncai commented Jan 9, 2019

hi guys, any update for this feature become available ? thanks

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Jan 9, 2019

@aar6ncai I'm going to need this quite soon too 😄.
I'll remove WIP and submit for formal review with the caveat that there is the known concern around ip in the ip_address block.

@ewbankkit ewbankkit changed the title [WIP] Add Route 53 Resolver endpoint resource Add Route 53 Resolver endpoint resource Jan 9, 2019

@kmcquade

This comment has been minimized.

Copy link

kmcquade commented Jan 22, 2019

@ewbankkit @gazoakley - any word? We need this pretty soon as well.

@ewbankkit ewbankkit force-pushed the ewbankkit:issue-6563 branch from f4d40c5 to 02b26ee Jan 23, 2019

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Jan 23, 2019

  • Rebased to remove conflict
  • Correct acceptance tests to use tags = {} syntax for TF 0.12

Acceptance tests:

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

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Jan 23, 2019

/nudge @bflad

@hashibot hashibot bot added the service/ec2 label Jan 25, 2019

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Jan 25, 2019

Added sweeper (otherwise dangling resources will give you a nasty surprise on your AWS bill 😄).

$ go test ./aws -v -sweep=us-west-2 -sweep-run=aws_route53_resolver_endpoint -timeout 10h
2019/01/25 18:34:13 [DEBUG] Running Sweepers for region (us-west-2):
2019/01/25 18:34:13 [INFO] Building AWS region structure
2019/01/25 18:34:13 [INFO] Building AWS auth structure
2019/01/25 18:34:13 [INFO] Setting AWS metadata API timeout to 100ms
2019/01/25 18:34:13 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2019/01/25 18:34:13 [INFO] AWS Auth provider used: "EnvProvider"
2019/01/25 18:34:13 [INFO] Initializing DeviceFarm SDK connection
2019/01/25 18:34:13 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2019/01/25 18:34:14 Sweeper Tests ran:
	- aws_route53_resolver_endpoint
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.862s
@kmcquade

This comment has been minimized.

Copy link

kmcquade commented Feb 5, 2019

@bflad any updates? thanks.
CC: @ewbankkit

@ewbankkit ewbankkit force-pushed the ewbankkit:issue-6563 branch from 6318ca6 to c91cafe Feb 6, 2019

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Feb 6, 2019

Rebased to resolve conflicts.

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Feb 10, 2019

Removed sweeper prefix check (#7484).

)

const (
Route53ResolverEndpointStatusDeleted = "DELETED"

This comment has been minimized.

@paultyng

paultyng Feb 13, 2019

Contributor

this probably doesn't need to be exported

This comment has been minimized.

@ewbankkit

ewbankkit Feb 14, 2019

Author Contributor

correct, it doesn't

@ewbankkit ewbankkit force-pushed the ewbankkit:issue-6563 branch from fb5373c to 625e34b Feb 15, 2019

@ewbankkit

This comment has been minimized.

Copy link
Contributor Author

ewbankkit commented Feb 15, 2019

Re-ran acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverEndpoint_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverEndpoint_ -timeout 120m
=== RUN   TestAccAwsRoute53ResolverEndpoint_basicInbound
=== PAUSE TestAccAwsRoute53ResolverEndpoint_basicInbound
=== RUN   TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== PAUSE TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== CONT  TestAccAwsRoute53ResolverEndpoint_basicInbound
=== CONT  TestAccAwsRoute53ResolverEndpoint_updateOutbound
--- PASS: TestAccAwsRoute53ResolverEndpoint_basicInbound (101.54s)
--- PASS: TestAccAwsRoute53ResolverEndpoint_updateOutbound (551.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	551.218s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment