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.

Sign up for GitHub

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

Jump to bottom

fix(core): Add security group property to HealthMonitor #408

Merged
merged 1 commit into from
May 4, 2021
Merged

fix(core): Add security group property to HealthMonitor #408

merged 1 commit into from
May 4, 2021

Conversation

kozlove-aws
Copy link
Contributor

@kozlove-aws kozlove-aws commented May 3, 2021 •
edited
Loading

Problem

The health monitor construct creates a application load balancer. This construct creates a new security group(SG) implicitly if it is not provided in constructor properties. This means, a new SG is always created and ingress and egress rules are added automatically based on the target groups created.

Solution

Extended HealthMonitorProps with new optional property securityGroup.
Passing this new property to ApplicationLoadBalancer construct.

Testing

Added unit test.
Deployed fleet with defined security group and tested that:

  • instance was raised and running at least 30 minutes
  • after terminating launcher on instance it received status unhealthy
  • new instance was invoked to reach target capacity
  • target capacity set to 0 when both instances had status unhealthy

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@jusiskin jusiskin linked an issue May 3, 2021 that may be closed by this pull request
Allow setting pre-defined security group in Health Monitor construct #407
Closed
2 tasks
@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label May 3, 2021
@kozlove-aws kozlove-aws changed the title [WIP] Add security group property to HealthMonitor Add security group property to HealthMonitor May 3, 2021
@kozlove-aws kozlove-aws changed the title Add security group property to HealthMonitor fix(core): Add security group property to HealthMonitor May 3, 2021
jusiskin
jusiskin requested changes May 4, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin 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! just minor feedback for the documentation and test

packages/aws-rfdk/lib/core/test/health-monitor.test.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/core/lib/health-monitor.ts Outdated Show resolved Hide resolved
yashda
yashda reviewed May 4, 2021
View reviewed changes
Copy link
Contributor

@yashda yashda left a comment

Choose a reason for hiding this comment

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

Changes looks good to me. Thanks Eugene!

@kozlove-aws kozlove-aws merged commit c2ed9e7 into aws:mainline May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yashda yashda yashda approved these changes

@jusiskin jusiskin jusiskin approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting pre-defined security group in Health Monitor construct
3 participants
@kozlove-aws @jusiskin @yashda