-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 mismatch in SecurityGroups handling with launch templates #9288
Fix mismatch in SecurityGroups handling with launch templates #9288
Conversation
/retest |
@johngmyers This change was intentional, as best practice. I think for Mixed instance policies is the only possibility. |
@hakman Every |
@hakman Notice I'm changing |
Thanks. Looks pretty good. Let me know what you think about the other comment and we can get this in. |
@@ -208,6 +208,10 @@ func (t *LaunchTemplate) Find(c *fi.Context) (*LaunchTemplate, error) { | |||
actual.SecurityGroups = append(actual.SecurityGroups, &SecurityGroup{ID: id}) | |||
} | |||
} | |||
// In older Kops versions, security groups were added to LaunchTemplateData.SecurityGroupIds | |||
for _, id := range lt.LaunchTemplateData.SecurityGroupIds { | |||
actual.SecurityGroups = append(actual.SecurityGroups, &SecurityGroup{ID: fi.String("legacy-" + *id)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the "legacy-" prefix?
I thought we just want to move the SGs from LaunchTemplateData.SecurityGroupIds
to LaunchTemplateData.NetworkInterfaces[0].Groups
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find()
builds a representation of the "actual" object, the state as it currently exists. The caller then diffs that against the representation of the "expected" object, the state that was built from the config in the state store. Only if those two differ does it call RenderAWS
, which would build a new launch template to attach to the ASG.
If we did not add the "legacy-" prefix then the actual could equal the expected and thus the caller would believe that no changes were necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, if SG is in the legacy place, I think the LT should not change. Only on next change the SG will be moved together with something else to the new location.
I guess it is debatable if we want this or not. Your choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the change should happen when kops is upgraded, not when some unrelated configuration is changed days or months later. To assist diagnosis the effect should be close to the cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Then this should be merged as is.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…88-upstream-release-1.18 Automated cherry pick of #9288: Fix mismatch in SecurityGroups handling with launch
When launch templates are enabled, they will be unnecessarily updated when they have a security group and are in a private subnet.
/kind bug
Targeted for 1.18