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

Auto Scaling Group - Turn UserData into rich object #623

Closed
moofish32 opened this issue Aug 24, 2018 · 7 comments · Fixed by #6029
Closed

Auto Scaling Group - Turn UserData into rich object #623

moofish32 opened this issue Aug 24, 2018 · 7 comments · Fixed by #6029
Assignees
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on.

Comments

@moofish32
Copy link
Contributor

I am having a few challenges with the new ASG (will be 0.8.3?).

  1. I have a simple pattern using cfn-signal for a basic rolling update. The issue is I need the logical ID of the ASG for the command. If I use CDK ASG construct (not the CloudFormation one). I have no means to get the LogicalId of the ASG. There might be some tricks in the rename functions, but I'm pretty sure we want an elegant solution here. I added a method to the ASG to get logical ID and that will work. I think I would prefer to model the cfn-* functions actually in the OperatingSystem implementations, similar to createUserData(..).

  2. The security group pattern via Connections has a lot of great features, but I think I might still need access to either assign additional security groups or export the default one assigned. The primary use case is a bastion pattern. We have exported the bastion node security group and other instances use cross stack references to allow inbound port 22 from the security group on the bastion node. We could add an export function for the security group to solve this. Alternatively we could enable users to insert the initial security group or add security groups to the configuration. As teams migrate to CDK I think many users may have legacy Security Groups managed by central teams (or stacksets) for IP whitelisting patterns (sadly even with Private Link we still have a few use cases that don't fit).

Are there solutions to these problems that I missed? Reasons we wouldn't want to add them?

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 24, 2018

I think I would prefer to model the cfn-* functions actually in the OperatingSystem implementations, similar to createUserData(..).

I think that would be a great idea. I'd even like to keep the logicalId out of that. Can we not do something like this:

asg.userData.addCommand('echo Hello')
asg.userData.addCommand('rm -rf /')
asg.userData.addSignalCommand()     // Will do the right thing

Don't know if that makes sense, and in how many use cases you'd really need the logicalId (we might be shooting ourselves in the foot if we hide it too much because we'd end up special-casing functions every time we need it). But I would like to hide it as much as possible from users.

We could add an export function for the security group to solve this.

Exists: https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-ec2/lib/security-group.ts#L64 (might have been added in a later version than the one you're using?)

So you should totally be able to do:

const sg = SecurityGroupRef.import(this, 'SG', {
    securityGroupId: new SecurityGroupId('sg-12345')
});

asg.connections.allowFrom(sg, new TcpPort(22), 'SSH access');

(My apologies for upcoming API breakage in advance--the import() function should have been SecurityGroup.import(), not SecurityGroupRef.import(), so we're going to have to change this some time in the near future... 🙍‍♀️ )

@moofish32
Copy link
Contributor Author

asg.userData.addSignalCommand()     // Will do the right thing

Yes. I think the underlying call attached to operating system will need to support some parameters and some may have to to be exposed in ASG. For AmazonLinux we know where cfn-signal is in the path. For other distros we might need to enable providing a { path: string} parameter or inform the user they must add it to path prior to this call (we just can't check until run time 👎 .

Re: Security Group

If my security group is all CidrIPv4 objects and the bastion node is the edge, doesn't the example above fail? The problem is I need to put those rules on the bastion SG. I can do that, but now I want to export the SG from the ASG which is not exposed, so that I can add that connection to my hosts?

I can put some code up here to clarify, but I need to sanitize this a bit and figure out where my HEAD is on the git tree.

@moofish32
Copy link
Contributor Author

moofish32 commented Aug 24, 2018

Also I think we might need to expose the AssociatePublicIpAddress. We avoid this by defaulting Public Subnets correctly, but as users bring their own VPC they might not have correctly configured this. I am in favor of exposing this, but defaulting it based on the VpcPlacementStrategy. Does this make sense?

        const vpcPlacement = props.vpcPlacement || {usePublicSubnets: false};
        const associatePublicIpAddress = vpcPlacement.usePublicSubnets; // this will change too
        // const associatePublicIpAddress = vpcPlacement.subnetsToUse === SubnetType.Public ? true : false;

I added this little block to the constructor of the newer ASG.

@skinny85
Copy link
Contributor

(My apologies for upcoming API breakage in advance--the import() function should have been SecurityGroup.import(), not SecurityGroupRef.import(), so we're going to have to change this some time in the near future... 🙍 )

Really? The import method is on BucketRef, not Bucket, in S3, and on ProjectRef, not Project, in CodeBuild... are we sure it should be on SecurityGroup here?

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 26, 2018

Hmm, good point, it's on the XxxRef in more places.

Just seems to make more sense to me on the main resource. But okay, the problem might be with SecurityGroup instead.

@rix0rrr rix0rrr changed the title Auto Scaling Group - Logical ID not available (cfn-signal) & Security Group not Exposed Auto Scaling Group - Turn UserData into rich object Sep 17, 2018
rix0rrr pushed a commit that referenced this issue Sep 27, 2018
Make a new construct to manager User Data, which should make it easier
to apply the features correctly.

Fixes #623 and 777.
@srchase srchase added feature-request A feature should be added or improved. and removed enhancement labels Jan 3, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 1, 2019

The signal command btw. shouldn't be a manual action. If signaling is enabled for the securitygroup, appropriate signaling commands should automatically be added in such a way that they trigger both for success and for failure, while the rest of the script should probably behave as set -u. So something like:

set -xeuo pipefail 
trap 'cfn-signal -e $? ...' EXIT

command1
command2

Should work.

@eladb eladb added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Sep 8, 2019
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Jan 23, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 23, 2020

While doing this -- the automatic configuration of the right type of (unconfigured) UserData should come from the MachineImage, not the consuming construct (EC2 instance or AutoScalingGroup)

grbartel added a commit to grbartel/aws-cdk that referenced this issue Jan 30, 2020
User Data objects currently only supports adding commands by providing the full command as a string.  This commit hopes to address this by adding the following functionality:
* On Exit Commands - Both bash and powershell have the concepts of trap functions which can be used to force a function to run when a an exception is run.  Using this we are able to set up a script block that will always run at the end of the script.
* add Signal Command - Using the above on Exit commands we are able to make it so the User data will send a signal to a specific resource (eg. Instance/Auto scaling group) with the results of the last command.
* Download and Execute a file from S3 - This writes a function into the user data which can be used for repeated calls to download and execute a file from s3 with a list of arguments.

This was tested by launching instances which pulled a script from an S3 asset then signaling on completion.

Fixes aws#623
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jan 30, 2020
rix0rrr pushed a commit that referenced this issue Feb 27, 2020
User Data objects currently only supports adding commands by providing the full command as a string.  This commit hopes to address this by adding the following functionality:
* On Exit Commands - Both bash and powershell have the concepts of trap functions which can be used to force a function to run when a an exception is run.  Using this we are able to set up a script block that will always run at the end of the script.
* add Signal Command - Using the above on Exit commands we are able to make it so the User data will send a signal to a specific resource (eg. Instance/Auto scaling group) with the results of the last command.
* Download S3 File Command - This adds commands to download the specified file using the aws cli on linux and AWS powershell  utility on windows
* Execute File Command - This adds commands to ensure that the specified file is executable then executes the file with specified arguments.

Fixes #623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on.
Projects
None yet
6 participants