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

Use EnsureTask instead of prepending IG names to external ELB tasks #10754

Merged
merged 3 commits into from
Feb 7, 2021
Merged

Use EnsureTask instead of prepending IG names to external ELB tasks #10754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7ede2b
Use EnsureTask instead of prepending IG names to external ELB tasks
rifelpet Feb 7, 2021
0ef3f5a
Use LoadBalancerName field of CLB task when attaching CLBs to existin…
rifelpet Feb 7, 2021
9f5e225
Add integration test for one external CLB being attached to multiple …
rifelpet Feb 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions pkg/model/awsmodel/autoscalinggroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,17 +450,13 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil

for _, extLB := range ig.Spec.ExternalLoadBalancers {
if extLB.LoadBalancerName != nil {
loadBalancerName := fi.StringValue(extLB.LoadBalancerName)
if loadBalancerName != awsup.GetResourceName32(b.Cluster.Name, "api") && loadBalancerName != awsup.GetResourceName32(b.Cluster.Name, "bastion") {
loadBalancerName = name + "-" + loadBalancerName
}
lb := &awstasks.ClassicLoadBalancer{
Name: fi.String(loadBalancerName),
Name: extLB.LoadBalancerName,
LoadBalancerName: extLB.LoadBalancerName,
Shared: fi.Bool(true),
}
t.LoadBalancers = append(t.LoadBalancers, lb)
c.AddTask(lb)
c.EnsureTask(lb)
Copy link
Member Author

Choose a reason for hiding this comment

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

note for reviewers: EnsureTask returns an error if the second invocation provides a task that isn't DeepEqual:

kops/upup/pkg/fi/task.go

Lines 70 to 74 in 0aa9cf1

// EnsureTask ensures that the specified task is configured.
// It adds the task if it does not already exist.
// If it does exist, it verifies that the existing task reflect.DeepEqual the new task,
// if they are different an error is returned.
func (c *ModelBuilderContext) EnsureTask(task Task) error {

here, the CLB tasks should always be equal if a CLB is in multiple IGs' externaLoadBalancers because the only task fields that are set use the LoadBalancerName and shared=true, so this should never return an error.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't know about this. May be a good idea to simplify the NLB side as well someday.

}

if extLB.TargetGroupARN != nil {
Expand Down
6 changes: 6 additions & 0 deletions tests/integration/update_cluster/complex/cloudformation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@
]
}
],
"LoadBalancerNames": [
"my-external-lb-1"
],
"TargetGroupARNs": [
{
"Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq"
Expand Down Expand Up @@ -182,6 +185,9 @@
"GroupTotalInstances"
]
}
],
"LoadBalancerNames": [
"my-external-lb-1"
]
}
},
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ spec:
- sg-exampleid3
- sg-exampleid4
associatePublicIp: true
externalLoadBalancers:
- loadBalancerName: my-external-lb-1
suspendProcesses:
- AZRebalance
image: kope.io/k8s-1.4-debian-jessie-amd64-hvm-ebs-2016-10-21
Expand Down Expand Up @@ -120,6 +122,8 @@ metadata:
kops.k8s.io/cluster: complex.example.com
spec:
associatePublicIp: true
externalLoadBalancers:
- loadBalancerName: my-external-lb-1
image: kope.io/k8s-1.4-debian-jessie-amd64-hvm-ebs-2016-10-21
machineType: m3.medium
maxSize: 1
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/update_cluster/complex/in-v1alpha2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ spec:
- sg-exampleid3
- sg-exampleid4
associatePublicIp: true
externalLoadBalancers:
- loadBalancerName: my-external-lb-1
suspendProcesses:
- AZRebalance
image: kope.io/k8s-1.4-debian-jessie-amd64-hvm-ebs-2016-10-21
Expand Down Expand Up @@ -120,6 +122,8 @@ metadata:
kops.k8s.io/cluster: complex.example.com
spec:
associatePublicIp: true
externalLoadBalancers:
- loadBalancerName: my-external-lb-1
image: kope.io/k8s-1.4-debian-jessie-amd64-hvm-ebs-2016-10-21
machineType: m3.medium
maxSize: 1
Expand Down
2 changes: 2 additions & 0 deletions tests/integration/update_cluster/complex/kubernetes.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ resource "aws_autoscaling_group" "master-us-test-1a-masters-complex-example-com"
id = aws_launch_template.master-us-test-1a-masters-complex-example-com.id
version = aws_launch_template.master-us-test-1a-masters-complex-example-com.latest_version
}
load_balancers = ["my-external-lb-1"]
max_size = 1
metrics_granularity = "1Minute"
min_size = 1
Expand Down Expand Up @@ -150,6 +151,7 @@ resource "aws_autoscaling_group" "nodes-complex-example-com" {
id = aws_launch_template.nodes-complex-example-com.id
version = aws_launch_template.nodes-complex-example-com.latest_version
}
load_balancers = ["my-external-lb-1"]
max_size = 2
metrics_granularity = "1Minute"
min_size = 2
Expand Down
8 changes: 2 additions & 6 deletions upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,8 @@ func (e *AutoscalingGroup) Find(c *fi.Context) (*AutoscalingGroup, error) {

actual.LoadBalancers = []*ClassicLoadBalancer{}
for _, lb := range g.LoadBalancerNames {
loadBalancerName := fi.StringValue(lb)
if loadBalancerName != awsup.GetResourceName32(c.Cluster.Name, "api") && loadBalancerName != awsup.GetResourceName32(c.Cluster.Name, "bastion") {
loadBalancerName = fi.StringValue(g.AutoScalingGroupName) + "-" + loadBalancerName
}
actual.LoadBalancers = append(actual.LoadBalancers, &ClassicLoadBalancer{
Name: aws.String(loadBalancerName),
Name: aws.String(*lb),
LoadBalancerName: aws.String(*lb),
})
}
Expand Down Expand Up @@ -721,7 +717,7 @@ func (e *AutoscalingGroup) AutoscalingLoadBalancers() []*string {
var list []*string

for _, v := range e.LoadBalancers {
list = append(list, v.Name)
list = append(list, v.LoadBalancerName)
}

return list
Expand Down