Prevent unintended resource updates to LB attatchments

[rdrgmnzs]

For Terraform when using aws_autoscaling_attachment a situation can happen where the LB is attached/detached from it's associated ASG (more details can be found on the Note section of https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/autoscaling_attachment ).

This change should prevent this from happening. To simplify the code, I also moved cloudformantion and kops managed to the new attachment format.

Fixes #9891, fixes #9913

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rdrgmnzs

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:

• OWNERS [rdrgmnzs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rifelpet]

If someone has other attachment resources in their terraform state, this change would conflict. We may need to expose a way to provide additional ELB names or TG ARNs to be included in these lists.

[rdrgmnzs]

@rifelpet not sure I follow. Isn't the external LB resource in Kops providing a way to attach other LB resources already? From what I understand the changes above don't prevent other LBs from being attached with aws_autoscaling_attachment it only gives a initial list of LBs that are associated with the ASG. Or am I missing something?

[rifelpet]

Ah I forgot that InstanceGroupSpec already has an ExternalLoadBalancers field. That note in the terraform docs suggests you should use either separate attachment resources or the field in the ASG resource but not both (hence the need to use ignore_changes).

The externallb integration test has the externalLoadBalancers field set, I would expect this PR to be updating its terraform output to include those additional load balancers / TGs, but that test doesnt have any changes at all. Is that expected?

[rdrgmnzs]

Ah, see I was reading that note in the docs as you could use aws_autoscaling_attachment and inline load_balancers. However if you were using aws_autoscaling_attachment you would need to set the ignore_changes.

That's sort of how I ended up here, while using aws_autoscaling_attachment as it previously was (no inline load_balancers) I kept seeing the LBs being attached and detached between different terraform runs with no changes to the actual terraform file.

Either way, moving ExternalLoadBalancers to be inline load_balancers as well seems to be an easy enough change so let me update the diff to take care of that.

[rdrgmnzs]

I created the TargetGroup struct mostly as a skeleton struct so that if someone decides to implement TargetGroup for uses in other places (such as API) they won't then have to figure out how to convert a list of string in AutoscalingGroup to an actual struct.

[rifelpet]

We'll want a release note instructing anyone using terraform or cloudformation that is also defining LB attachments not using kops (in a separate .tf file in their terraform state, for example) to pass those TGs or ELBs through the ClusterSpec.

[rdrgmnzs]

/test pull-kops-e2e-k8s-containerd
/test pull-kops-e2e-cni-cilium
/test pull-kops-verify-staticcheck

[rifelpet]

I'm wondering if the migration from aws_autoscaling_attachment to inline aws_autoscaling_group fields will be seamless or if there will be a period of time during the terraform apply in which the ASG is not registered with the ELB / TG which would result in downtime for the service.

[rdrgmnzs]

Terraform plan shows it as a NOOP using terraform 0.12.29 and I saw no downtime while testing.

[rifelpet]

lgtm other than one nit 👍

[rdrgmnzs]

/hold

[rifelpet]

@rdrgmnzs any update on this? looks like its becoming a pretty common issue for terraform users.

[rdrgmnzs]

Hey @rifelpet I’ve had this on hold because I found instances where the switch causes the API endpoint to have a downtime when migrating from the old format to the new one. Unfortunately I haven’t been able to find a way around that.

What do you think about having a note in the release that indicates folks will most likely experience that when migrating? I’m also open to other suggestions on how to handle this.

[rifelpet]

downtime could be problematic to anyone running a nodeport service and a load balancer defined through terraform.

I wonder if we could use the terraform state commands to modify the resource states. For example, if the upgrade switches from aws_autoscaling_attachment to the load_balancers field of the aws_autoscaling_group, terraform state rm aws_autoscaling_attachment.foo might do the trick.

Similar to the release notes we had for terraform 0.12 support in 1.17 and 1.18.

[bmelbourne]

I came across this issue whilst working on #9940 and I like this solution provided users are using Terraform 0.12+, so I'd suggest merging in Kops v1.20 when Terraform 0.11 is deprecated.

[bmelbourne]

@rdrgmnzs
Can you add Fixes #9891, fixes #9913 to the PR description to give the other contributors an opportunity to review this PR?

[rdrgmnzs]

/hold cancel

Ok, added instructions on switching over without downtime & fixed nit.

[rifelpet]

two minor comments but overall lgtm!

[rifelpet]

/lgtm