adding missed lifecycles in elb code

[chrislovecnm]

Fixing missed lifecycles in two different places

1. additional sec groups on the ELB
2. additional sec groups on an ASG

[chrislovecnm]

/assign @robinpercy

[chrislovecnm]

@almariah we missed adding lifecycle to the ELB, and now I am getting odd errors with the unit test. Any ideas?

/hold

[almariah]

@chrislovecnm - The error is

--- FAIL: TestComplex (0.11s)
	integration_test.go:212: error running update cluster "complex.example.com": error building tasks: cannot add different task with same key "SecurityGroup/sg-exampleid3"

The integration test contains a nice specific case:
https://github.com/kubernetes/kops/blob/master/tests/integration/update_cluster/complex/in-v1alpha2.yaml#L11-L12
https://github.com/kubernetes/kops/blob/master/tests/integration/update_cluster/complex/in-v1alpha2.yaml#L63-L64

where you have additional and similar security groups for API ELB and instance group, which cause tasks with same name. If you noticed:
https://github.com/kubernetes/kops/blob/master/pkg/model/awsmodel/api_loadbalancer.go#L191
https://github.com/kubernetes/kops/blob/master/pkg/model/awsmodel/autoscalinggroup.go#L99

they have the same task name. I suggest changing the task name. For api_loadbalancer.go:

t := &awstasks.SecurityGroup{
				Name:      s("api-elb-" + id),
				ID:        fi.String(id),
				Shared:    fi.Bool(true),
				Lifecycle: b.SecurityLifecycle,
			}

and for autoscalinggroup.go

sgTask := &awstasks.SecurityGroup{
					Name:      s("ig-" + ig.ObjectMeta.Name + id),
					ID:        fi.String(id),
					Shared:    fi.Bool(true),
				}

Also AdditionalSecurityGroups in autoscalinggroup.go does not have Lifecycle: b.SecurityLifecycle

[chrislovecnm]

they have the same task name. I suggest changing the task name. For api_loadbalancer.go:

That is going to cause problems. I will make the autoscaling task an ensure task.

Thanks for the catch on the missing lifecycle. Will add that.

[chrislovecnm]

@almariah thanks for the assist btw!!

[almariah]

@chrislovecnm For both autoscaling group and ELB the additional security groups are ensure tasks. The problem happened because of same security group ID for ELB and ASG which cause the same task name (task name is the configured to be same as ID). But for either ELB or ASG you are not allowed to have multiple security group with the same ID and consequently no repeated task name for iteration over the list of security groups. This iteration happened twice because the test case was adding the same list for both ELB and ASG and that for example our case in our own deployment for the cluster.

[almariah]

@chrislovecnm I don't know why ensure task does not work in that case; may be because it uses reflect.DeepEqual and the objects are not deeply identical. In the PR you have modified one of them by adding the lifecycle for one of them and the other is left. That is why it was working before. So you can ensure that both are identical by adding the missing lifecycle and forget modifying the task name that I have mentioned.

[almariah]

@chrislovecnm I have tested it by adding Lifecycle: b.SecurityLifecycle, to AdditionalSecurityGroups of ELB and AutoscalingGroup. It works. But be aware to add SecurityLifecycle *fi.Lifecycle to AutoscalingGroupModelBuilder. It is missing https://github.com/kubernetes/kops/blob/master/pkg/model/awsmodel/autoscalinggroup.go#L36-L42.

Also you should add SecurityLifecycle: &securityLifecycle, to apply_cluster.go
https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/apply_cluster.go#L651-L655. If it is missing it will cause the same error because the SecurityLifecycle will be nil where in APILoadBalancerBuilder will be &securityLifecycle and will cause the same security group task to be not deeply identical.

[chrislovecnm]

Thanks, @almariah, exceptionally helpful! I am putting in the changes and testing.

[chrislovecnm]

/hold cancel

@justinsb @geojaz @andrewsykim PTAL

[justinsb]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[chrislovecnm]

/test pull-kops-e2e-kubernetes-aws