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

Support for ASG scaling notification configuration (SNS) #1419

Closed
willmcg opened this issue Apr 7, 2015 · 23 comments
Closed

Support for ASG scaling notification configuration (SNS) #1419

willmcg opened this issue Apr 7, 2015 · 23 comments

Comments

@willmcg
Copy link

willmcg commented Apr 7, 2015

It would be great if the aws_autoscaling_group implemented support for scaling notifications via SNS as would normally be done with the autoscaling: PutNotificationConfiguration API. Still requires some external setup for the SNS topic, but when Terraform supports SNS/SQS the SNS topic ARN could simply be a aws_sns_topic resource.

Here is a straw man for a possible format for the notification:

resource "aws_autoscaling_group" "bar" {
  availability_zones = ["us-east-1a", "us-east-1b", "us-east-1c"]
  name = "testasg"
  max_size = 3
  min_size = 5
  desired_capacity = 4
  health_check_grace_period = 300
  health_check_type = "EC2"
  launch_configuration = "${aws_launch_configuration.mylc.name}"

  notification {
    topic = "arn:aws:sns:us-west-2:123456789012:my-sns-topic"
    types = [
      "EC2_INSTANCE_LAUNCH",
      "EC2_INSTANCE_LAUNCH_ERROR",
      "EC2_INSTANCE_TERMINATE",
      "EC2_INSTANCE_TERMINATE_ERROR",
      "TEST_NOTIFICATION"
    ]
  }
}

I'm not sure whether autoscaling groups support multiple notification configurations being attached to a single group.

@matthewford
Copy link

+1

2 similar comments
@CpuID
Copy link
Contributor

CpuID commented Apr 24, 2015

+1

@mzupan
Copy link
Contributor

mzupan commented Apr 25, 2015

👍

@ctiwald
Copy link
Contributor

ctiwald commented May 7, 2015

I'm working on this now. Technically I'm adding sns_topic resources, but will be rolling up the AS group change as well. Expect a PR in a couple days or early next week.

@clstokes
Copy link
Contributor

+1

@catsby
Copy link
Contributor

catsby commented May 18, 2015

#1974 is currently the leading PR for this. Please subscribe to that issue if you'd like to keep informed or see when it's merged.

Closing this, thanks!

@catsby catsby closed this as completed May 18, 2015
@clstokes
Copy link
Contributor

@catsby, unless I'm mistaken, it looks like #1974 only includes the SNS support, but not the support for configuring notifications on the autoscaling group, which is a part of the ASG API.

Should a separate issue be opened just for PutNotificationConfiguration support?

@clstokes
Copy link
Contributor

Or maybe that PR is still a work in progress... 😧

@nelg
Copy link

nelg commented May 21, 2015

+1

1 similar comment
@ojongerius
Copy link
Contributor

+1

@stack72
Copy link
Contributor

stack72 commented Jun 1, 2015

I have been trying to follow all the alternative threads for ASG notifications. I know that new features (SNS_TOPIC and SNS_TOPIC_DESCRIPTION) have been added but the support for ASG notifications still doesn't seem to exist

I think this issue should be reopened

@catsby
Copy link
Contributor

catsby commented Jun 2, 2015

Hey all – yeah looks like I closed this prematurely. For ASG notifications, I'm envisioning a new resource:

resource "aws_autoscaling_notification" "example" {
  group_name     = "${aws_autoscaling_group.asg_example.name}"
  notifications  = ["EC2_INSTANCE_LAUNCH", "EC2_INSTANCE_TERMINATE"]
  topic_arn      = "${aws_sns_topic.some_topic.arn}"
}

Does that sound in-line with what you're thinking?

@catsby catsby reopened this Jun 2, 2015
@ctiwald
Copy link
Contributor

ctiwald commented Jun 2, 2015

@catsby -- why not just embed it within the aws_autoscaling_group like an SG's ingress / egress rules or the like? I'm not sure there's really any utility with having it outside the resource. Since you have to map it to an ASG anyway, it's not like you could use it for multiple ASG's.

It just feels a little weird to do this in the config:

resource "some_thing" "my_thing" {
   name = "my_thing"
}

resource "other_thing_that_only_exists_in_the_context_of_some_thing" "another_thing" {
  some_thing = "${some_thing.my_thing.name}"
}

rather than:

resource "some_thing" "my_thing" {
   name = "my_thing"
   other_thing {
        ...
    }
}

@catsby
Copy link
Contributor

catsby commented Jun 2, 2015

@ctiwald we're kind of trying to move away from that model (sub resources). They get hairy as they're currently handled. We have plans to re-work it and make them a better citizen, but it's a ways off.

For example, we recently broke out security group rules into their own top level resource, aws_security_group_rule.

Since you have to map it to an ASG anyway, it's not like you could use it for multiple ASG's.

My suggested syntax does not support multiple ASGs, but it's not immediately apparent that we couldn't manage it that way in the future. There is no limitation (that I can see) on AWS that enforces a 1 to 1 here. I just created two ASGs and applied identical notifications to them.

@catsby
Copy link
Contributor

catsby commented Jun 2, 2015

It just feels a little weird to do this in the config:

I would do it that way, for the reason of opening up to apply to multiple ASGs in the future, primarily. Unless you see something in the API docs that limit this that I'm not seeing? 😄

I welcome other opinions here as well, if anyone has them. The plan isn't set in stone, but I'm still leaning towards a separate resource.

@phinze
Copy link
Contributor

phinze commented Jun 2, 2015

The other benefit of a top level resource is that it doesn't dictate that all notifications be known at ASG definition time - it opens up the possibility for a module author to yield and ASG and for a module consumer to add notifications to said ASG.

I'm hoping that we'll be able to circle back and reimplement the sub-resource config style as syntactic sugar, but in order to do that we'll need to lean towards top level resources for now.

@catsby
Copy link
Contributor

catsby commented Jun 2, 2015

I opened #2197 as a proposal , and to track development

@ctiwald
Copy link
Contributor

ctiwald commented Jun 2, 2015

Nah that makes sense to me. Having grappled with the code around sub-resources I'm not particularly envious of you extending them that way anyway. I thought I saw 1:1 mapping between them in some doc somewhere @catsby, but now that I look around I can't seem to find it.

All of that makes sense to me.

@catsby
Copy link
Contributor

catsby commented Jun 2, 2015

Thanks @ctiwald , I've come to value your opinion and I'm glad we have some congruence 😄

I'll start on #2197 tomorrow-ish, unless I wake up and found someone else implemented it.. :P

@ctiwald
Copy link
Contributor

ctiwald commented Jun 2, 2015

Haha, thanks @catsby. I'll leave this one for you guys. Truth be told I actually looked to implement this a few weeks back, cracked open resource_aws_autoscaling_group.go, stared down the idea of implementing the sub-resource and then... I dunno. I think went hiking or to the bar or something.

From my limited "programming on terraform" perspective, +1 for top level resources.

@catsby
Copy link
Contributor

catsby commented Jun 5, 2015

Hey all – #2197 has been implemented, tested, and now documented.
If you can, please review and test out. Thanks!

@catsby
Copy link
Contributor

catsby commented Jun 5, 2015

I just merged #2197 to add this feature.
Let me know if you need anything else!

@ghost
Copy link

ghost commented May 2, 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 May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests