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

ELB/TargetGroup/ASG attachment fixes #10138

Merged
merged 3 commits into from
Oct 29, 2020
Merged

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Oct 29, 2020

ref: #10134 (comment)

This addresses three issues:

  • External TargetGroupARNs werent being attached to ASGs at creation-time with --target direct
  • Kops was passing the name of the API ELB task to attach to the ASG rather than the resulting AWS LoadBalancerName which gets truncated and suffixed. This was resulting in AWS errors when creating the ASG, reporting that the ELB is not found.
  • Subsequent update clusters were reporting changes because when Finding the LBs attached to the ASG, we couldn't tell which was the API ELB and which were external ELBs passed in through the InstanceGroupSpec. Find now looks for the API ELB to decide whether an ELB attached to the ASG should be the API ELB task or an external ELB task.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 29, 2020
@rifelpet
Copy link
Member Author

@hakman can you test this commit out?

@hakman
Copy link
Member

hakman commented Oct 29, 2020

@hakman can you test this commit out?

No luck. Same thing.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2020
@rifelpet
Copy link
Member Author

rifelpet commented Oct 29, 2020

@hakman did some troubleshooting and discovered that we used to be correctly passing the AWS LoadBalancerName when attaching LBs to ASGs. the LoadBalancerName is the truncated name we pass in CreateLoadBalancer and gets a random hashed suffix.

if aws.StringValue(e.LoadBalancer.LoadBalancerName) == "" {
return nil, fmt.Errorf("LoadBalancer did not have LoadBalancerName set")
}

#9794 changed that such that we now try to pass the original Name (the kops task name) to the Attach call which fails.

if b.UseLoadBalancerForAPI() && ig.Spec.Role == kops.InstanceGroupRoleMaster {
t.LoadBalancers = append(t.LoadBalancers, b.LinkToELB("api"))
}

kops/pkg/model/names.go

Lines 88 to 91 in b7f66a6

func (b *KopsModelContext) LinkToELB(prefix string) *awstasks.LoadBalancer {
name := b.ELBName(prefix)
return &awstasks.LoadBalancer{Name: &name}
}

for _, k := range e.LoadBalancers {
request.LoadBalancerNames = append(request.LoadBalancerNames, k.GetName())
}

So we need to update the ASG task to try to use the LoadBalancerName field of the LB task, fetching it from AWS if we don't yet know it. External load balancers coming from the InstanceGroupSpec are the LoadBalancerName so we set that field when setting up the ASG task, but only the task name is known for the API load balancer so LinkToELB("api") only sets the Name field of the task, leaving LoadBalancerName blank. We then lookup the LoadBalancerName for the given Name, failing the ASG task if the LB doesn't yet exist.

My last commit in this PR aims to accomplish that.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2020
@rifelpet rifelpet changed the title WIP Attach/detach TGs from ASGs and setup new ASG task dependencies WIP Attach/detach TGs from ASGs and avoid LB attachment changes reported during subsequent updates Oct 29, 2020
@rifelpet rifelpet changed the title WIP Attach/detach TGs from ASGs and avoid LB attachment changes reported during subsequent updates WIP ELB/ASG attachment fixes Oct 29, 2020
@rifelpet rifelpet force-pushed the asg-tg branch 2 times, most recently from abd323d to 8e626cc Compare October 29, 2020 17:22
@rifelpet rifelpet changed the title WIP ELB/ASG attachment fixes WIP ELB/TargetGroup/ASG attachment fixes Oct 29, 2020
Usually this is an "actual.Foo = e.Foo" one-liner but we don't know which LB attached to an ASG is the API ELB so it's a bit more complicated
@rifelpet rifelpet marked this pull request as ready for review October 29, 2020 17:34
@rifelpet rifelpet changed the title WIP ELB/TargetGroup/ASG attachment fixes ELB/TargetGroup/ASG attachment fixes Oct 29, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2020
@rifelpet
Copy link
Member Author

/cc @rdrgmnzs

@hakman
Copy link
Member

hakman commented Oct 29, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2020
@rdrgmnzs
Copy link
Contributor

/lgtm

Thanks for the fix @rifelpet and for catching the issue @hakman

@k8s-ci-robot k8s-ci-robot merged commit 1ed5af0 into kubernetes:master Oct 29, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Oct 29, 2020
k8s-ci-robot added a commit that referenced this pull request Oct 29, 2020
Automated cherry pick of #9794 and #10138 onto release-1.19
k8s-ci-robot added a commit that referenced this pull request Nov 16, 2020
@rifelpet rifelpet deleted the asg-tg branch May 5, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants