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

Make KeyName optional #444

Merged
merged 4 commits into from
Jul 9, 2018
Merged

Make KeyName optional #444

merged 4 commits into from
Jul 9, 2018

Conversation

zsims
Copy link
Contributor

@zsims zsims commented Jul 7, 2018

This closes #413, by making the KeyName parameter optional. The parameter type has to be changed to String as the AWS::EC2::KeyPair::KeyName typed parameter means a valid key name must be specified.

If no KeyName is specified, then the SSH ingress rule will not be created.

Some tests:

With a new stack

$ aws cloudformation create-stack --stack-name optional-ssh-zac --template-body file://build/aws-stack.json --parameters "ParameterKey=BuildkiteAgentToken,ParameterValue=secret" --capabilities CAPABILITY_NAMED_IAM
# ... 
CREATE_COMPLETE

Against an existing 3.2.1 stack

$ aws cloudformation create-change-set --stack-name buildkite --change-set-name optional-ssh-key-test --template-body file://build/aws-stack.json --parameters "ParameterKey=BuildkiteAgentToken,ParameterValue=secret" --capabilities CAPABILITY_NAMED_IAM
# ...
$ aws cloudformation describe-change-set --stack-name buildkite --change-set-name optional-ssh-key-test --query 'Changes[].ResourceChange.{LogicalResourceId:LogicalResourceId,Replacement:Replacement,Action:Action}'
[
    {
        "LogicalResourceId": "AgentAutoScaleGroup",
        "Replacement": "Conditional",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "AgentLaunchConfiguration",
        "Replacement": "True",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "AgentLifecycleHook",
        "Replacement": "Conditional",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "AgentScaleDownPolicy",
        "Replacement": "Conditional",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "AgentScaleUpPolicy",
        "Replacement": "Conditional",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "AgentUtilizationAlarmHigh",
        "Replacement": "False",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "AgentUtilizationAlarmLow",
        "Replacement": "False",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "SecurityGroup",
        "Replacement": "True",
        "Action": "Modify"
    },
    {
        "LogicalResourceId": "SecurityGroupSshIngress",
        "Replacement": null,
        "Action": "Add"
    }
]

zsims added 2 commits July 7, 2018 15:14
If this is left blank, then SSH will be disabled on agents. This
provides an additional level of agent security, and the existing
CloudWatch logs provide a good level of debuggability/diagnostics.
@tbenade
Copy link

tbenade commented Jul 8, 2018

Awesome 1000

@lox
Copy link
Contributor

lox commented Jul 9, 2018

This is looking great, thanks for this @zsims. My only feedback is that the conditional SSH ingress needs to also take into account AuthorizedUsersUrl which lets you provide a url for downloading authorized_keys from.

@zsims
Copy link
Contributor Author

zsims commented Jul 9, 2018

Thanks for the review @lox, have updated the PR with your suggestion

- { Condition: HasKeyName }
- { Condition : CreateSecurityGroup }
# Enable ingress if authorized users (keys) can be specified another way
- "Fn::Not": [ "Fn::Equals": [ { Ref: AuthorizedUsersUrl }, "" ] ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs to be in an Fn::Or, or am I booleaning wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that's embarrassing, good catch! I've pushed up a fix

@lox
Copy link
Contributor

lox commented Jul 9, 2018

Cool, works for me!

@lox lox merged commit f3c471e into buildkite:master Jul 9, 2018
@zsims zsims deleted the make-keyname-optional branch July 10, 2018 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is the key / keyname needed. Would you consider not making the keyname mandatory
3 participants