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 Computed flag set to true for Port property in ElastiCache Cluster #10017

Merged

Conversation

xsalazar
Copy link
Contributor

@xsalazar xsalazar commented Sep 5, 2019

Summary

Sets the Computed flag to true for the aws_elasticache_cluster resource. This property is already exposed as readable; however, there was a lack of identification to tell other resources to wait for the value to be set before using it. This is especially important when utilizing the default ports for both Memcached and Redis, since they will not be set explicitly in Terraform and only get a value after the resource is created.

The Existing Issue

Currently, the port property on the resource_aws_elasticache_cluster is allowed to be read externally and used throughout other resources; however, there was no explicit identifier to tell other resources when it would be safe to read this property.

For example, if you set up an aws_elasticache_cluster as follows:

resource "aws_elasticache_cluster" "redis_cluster" {
  cluster_id           = "redis"
  engine               = "redis"
  engine_version       = "5.0.4"
  node_type            = "cache.t2.micro"
  num_cache_nodes      = 1
  parameter_group_name = "default.redis5.0"
}

And you also wish to set up an aws_security_group to create an ingress rule using the port exposed by this cluster:

resource "aws_security_group" "redis_security_group" {
  description = "Redis Security Group"
  name        = "RedisSecurityGroup"

  ingress {
    from_port = aws_elasticache_cluster.redis_cluster.port
    protocol  = "tcp"
    to_port   = aws_elasticache_cluster.redis_cluster.port
  }
}

You get the following error on apply:

Error: "ingress.0.to_port": required field is not set

  on main.tf line 15, in resource "aws_security_group" "redis_security_group":
  15: resource "aws_security_group" "redis_security_group" {



Error: "ingress.0.from_port": required field is not set

  on main.tf line 15, in resource "aws_security_group" "redis_security_group":
  15: resource "aws_security_group" "redis_security_group" {

This property is eventually set; however, Terraform does not know to wait until the cluster resource is created to read this property

What's Fixed

You can see below that the new plan generated by Terraform is successful and also correctly identifies that the ingress rule will properly set the to- and from-port after the apply is done. Further, you can see the resource creation explicitly waits until the aws_elasticache_cluster is complete to start creating the aws_security_group.

Terraform will perform the following actions:

  # aws_elasticache_cluster.redis_cluster will be created
  + resource "aws_elasticache_cluster" "redis_cluster" {
      + apply_immediately      = (known after apply)
      + availability_zone      = (known after apply)
      + az_mode                = (known after apply)
      + cache_nodes            = (known after apply)
      + cluster_address        = (known after apply)
      + cluster_id             = "redis"
      + configuration_endpoint = (known after apply)
      + engine                 = "redis"
      + engine_version         = "5.0.4"
      + id                     = (known after apply)
      + maintenance_window     = (known after apply)
      + node_type              = "cache.t2.micro"
      + num_cache_nodes        = 1
      + parameter_group_name   = "default.redis5.0"
      + port                   = (known after apply)
      + replication_group_id   = (known after apply)
      + security_group_ids     = (known after apply)
      + security_group_names   = (known after apply)
      + snapshot_window        = (known after apply)
      + subnet_group_name      = (known after apply)
    }

  # aws_security_group.redis_security_group will be created
  + resource "aws_security_group" "redis_security_group" {
      + arn                    = (known after apply)
      + description            = "Redis Security Group"
      + egress                 = (known after apply)
      + id                     = (known after apply)
      + ingress                = [
          + {
              + cidr_blocks      = []
              + description      = ""
              + from_port        = (known after apply)
              + ipv6_cidr_blocks = []
              + prefix_list_ids  = []
              + protocol         = "tcp"
              + security_groups  = []
              + self             = false
              + to_port          = (known after apply)
            },
        ]
      + name                   = "RedisSecurityGroup"
      + owner_id               = (known after apply)
      + revoke_rules_on_delete = false
      + vpc_id                 = (known after apply)
    }

Plan: 2 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

aws_elasticache_cluster.redis_cluster: Creating...
...
aws_elasticache_cluster.redis_cluster: Still creating... [3m40s elapsed]
aws_elasticache_cluster.redis_cluster: Creation complete after 3m47s [id=redis]
aws_security_group.redis_security_group: Creating...
aws_security_group.redis_security_group: Creation complete after 2s [id=sg-0358a451871d9ced4]

Apply complete! Resources: 2 added, 0 changed, 0 destroyed.

Outputs:

redis_security_group = {
  "arn" = "arn:aws:ec2:us-west-1:368081326042:security-group/sg-0358a451871d9ced4"
  "description" = "Redis Security Group"
  "egress" = []
  "id" = "sg-0358a451871d9ced4"
  "ingress" = [
    {
      "cidr_blocks" = []
      "description" = ""
      "from_port" = 6379
      "ipv6_cidr_blocks" = []
      "prefix_list_ids" = []
      "protocol" = "tcp"
      "security_groups" = []
      "self" = false
      "to_port" = 6379
    },
  ]
  "name" = "RedisSecurityGroup"
  "owner_id" = "368081326042"
  "revoke_rules_on_delete" = false
  "vpc_id" = "vpc-708d8317"
}

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

Release Notes

Release note for CHANGELOG:

Fix bug with port property on elasticache_cluster being read before the resource is fully initialized

Test

Output from acceptance testing:

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

Forces proper order resource creation when reading this property externally
@xsalazar xsalazar requested a review from a team September 5, 2019 22:32
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/elasticache Issues and PRs that pertain to the elasticache service. labels Sep 5, 2019
@aeschright
Copy link
Contributor

Hi @xsalazar ! Thanks so much for the detailed explanation and use case. Can you have a look at the acceptance tests for the resource and add a new test to demonstrate the change? Let us know if you need pointers on where to start.

@ghost ghost added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Sep 10, 2019
@xsalazar
Copy link
Contributor Author

xsalazar commented Sep 10, 2019

@aeschright I went ahead and added a test around this scenario! The Travis CI check failed, but I'm not sure why; everything is passing on my local.

@xsalazar
Copy link
Contributor Author

Any update on this? @aeschright

Copy link
Contributor

@aeschright aeschright 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 at first read, thank you! Please run make fmt to clean up the linter errors, then I can verify the acceptance tests.

aws/resource_aws_elasticache_cluster_test.go Outdated Show resolved Hide resolved
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Jan 15, 2020
Copy link
Contributor

@aeschright aeschright left a comment

Choose a reason for hiding this comment

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

Looks like we're good to go! Thanks for taking care of this.

--- PASS: TestAccAWSElasticacheCluster_Port_Redis_Default (524.58s)

@aeschright aeschright added this to the v2.48.0 milestone Jan 31, 2020
@aeschright aeschright merged commit 2502ec3 into hashicorp:master Jan 31, 2020
aeschright added a commit that referenced this pull request Jan 31, 2020
@xsalazar
Copy link
Contributor Author

xsalazar commented Feb 1, 2020

@aeschright thanks for being responsive this long and helping get this in! 🚀

bflad added a commit that referenced this pull request Feb 5, 2020
@ghost
Copy link

ghost commented Feb 7, 2020

This has been released in version 2.48.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!

@xsalazar xsalazar deleted the resource_aws_elasticache_cluster/port branch February 10, 2020 18:11
@ghost
Copy link

ghost commented Mar 27, 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 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/elasticache Issues and PRs that pertain to the elasticache service. size/XL 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants