Add ability to enter subnets for ELBs and RDS DB #65
Conversation
Switch S3 paths to use virtual-style pathing (#60)
|
This is filed to resolve #61 |
There was a problem hiding this comment.
Thank you so much @jeffj254 for putting in the effort here! I really like this solution in general because it largely maintains compatibility for existing users (no breaking changes). I added a couple of comments that need to be addressed.
| @@ -712,7 +722,7 @@ Resources: | |||
| TemplateURL: !Sub https://${AutomationBucket}.s3.amazonaws.com/${TemplateVersion}/chef_rds.yaml | |||
| Parameters: | |||
| VPC: !Sub ${VPC} | |||
| ChefServerSubnets: !Join [ ",", !Ref ChefServerSubnets ] | |||
| ChefServerSubnets: !If [UseServerSubnetsForDatabase, !Join [ ",", !Ref ChefServerSubnets ], !Join [ ",", !Ref DatabaseSubnets ] ] | |||
There was a problem hiding this comment.
hey @jeffj254 I think we'd also want to follow the same convention for Elasticsearch subnets, right?
There was a problem hiding this comment.
In our particular use case, Elasticsearch will go in the same subnet as the Chef servers, but yeah I agree for completeness it should probably also be configurable in the same way as the other services. I'll update the PR with this change
There was a problem hiding this comment.
PR updated with code for ElasticSearch
| @@ -573,4 +571,4 @@ Outputs: | |||
| Name: !Sub "${AWS::StackName}-DNSName" | |||
| ServerIP: | |||
| Description: The IP Address of the Automate Server | |||
| Value: !If [UsePublicIp, !GetAtt AutomateServer.PublicIp, !GetAtt AutomateServer.PrivateIp] | |||
| Value: !GetAtt AutomateServer.PrivateIp | |||
There was a problem hiding this comment.
hey @jeffj254 what's the rationale on this change? I'm worried it may break existing users who currently need to access via the PublicIp
There was a problem hiding this comment.
Right now it doesn't look like there's code to ever configure the Automate EC2 instance with a public IP. The AssociatePublicIpAddress parameter in main.yaml isn't passed to automate.yaml and there's no NetworkInterface section in the Automate EC2::Instance resource to receive it if it was. Even if there was a way to configure the Automate EC2 instance with a public IP, the UsePublicIp conditional used by this output should probably use the AssociatePublicIpAddress parameter for it's check, rather than the load balancer scheme, since it more directly correlates to if the Automate EC2 instance will have a public IP or not.
Leaving this code in the template will actually cause all stack deployments to fail when using the internet-facing load balancer scheme since there will never be a public IP on the EC2 instance for this Output to retrieve. This is what I ran into since we are using internet-facing ELBs with private EC2s.
There was a problem hiding this comment.
this is where I really wish CF had an equivalent of Terraform's coalesce or compact functions to pick the Public or Private IP. We're kinda stuck here aren't we?
These changes add additional parameters to the necessary templates to allow specifying different subnets for the ELBs and/or the database. If the parameters are left empty the original behavior of using the ServerSubnets for all resources is maintained.
These parameters must be added as type
CommaDelimitedListbecause it is not possible to leave AWS-specific parameters likeList<AWS::EC2::Subnet::Id>blank.This change also removes the
UsePublicIpcondition and the associated logic in the Outputs section fromautomate.yaml. This logic seems unnecessary since even when using theinternet-facingELB scheme, the Automate instance itself will never have a public IP address (and the logic actually fails wheninternet-facingis specified since there is no public IP address on the EC2 instance to retrieve). TheAssociatePublicIpAddressparameter in main.yaml is not passed to automate.yaml and the Automate EC2 resource does not include any configuration for using a public IP.