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

fix(secgroups): use standalone secgroup rules instead of in-line rules #109

Merged
merged 3 commits into from
Apr 25, 2019

Conversation

metral
Copy link
Contributor

@metral metral commented Apr 23, 2019

fixes: #106
rel: #69

@metral
Copy link
Contributor Author

metral commented Apr 24, 2019

Note: this PR currently sets the ingress and egress prop of both secgroups to an empty array []. This is necessary as there seems to be a bug when removing both props all-together and pulumi not triggering a removal of the rules as expected. e.g. in the changes below, neither set of ingress or egress rules that were in-line were set for removal when attempting to remove props entirely. The only work around is to set each prop to an empty array instead, along with the standalone secgroup rules as done in this PR.

I'll be opening a separate issue to track this props removal bug in pulumi/pulumi.
Update: issue has been opened.

Changes:
 
    Type                           Name                                   Operation
~   aws:ec2:SecurityGroup          kx-eks-cluster-eksClusterSecurityGroup updated
~   aws:ec2:SecurityGroup          kx-eks-cluster-nodeSecurityGroup       updated
~   pulumi-nodejs:dynamic:Resource kx-eks-cluster-vpc-cni                 updated
 
Resources:
    ~ updated 3
    17 unchanged
 
Duration: 4s

@metral
Copy link
Contributor Author

metral commented Apr 24, 2019

@lukehoban PTAL

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

LGTM.

This does make me really want some tests to ensure that this update can safely be performed from a stack deployed using previous versions of the library. Do you know exactly what happens during the update? Is there a very brief period of outage where the security groups are removed before they get added? If so, may want to add a note about the upgrade to the CHANGELOG?

@metral
Copy link
Contributor Author

metral commented Apr 24, 2019

@lukehoban agree on the fact that we should enable testing prev versions against the new commits.

Do you know exactly what happens during the update?

I'd imagine that there can be a brief period of unavailability given that the secgroup rules are against the worker instances themselves. That said, this patch update does happen in a matter of seconds (< 10 sec and closer to 5sec) so it isn't a prolonged period.

I'll make sure to mention this ^ as a part of the changelog.

For the sake of more data on this change:

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.

Move from secgroup in-line rules to standalone secgroup rules
2 participants