Don't try to delete ElasticIPs of NatGateway is shared #7525
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @dzoeteman. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/XS
|
/assign @mikesplain |
| @@ -1339,7 +1339,13 @@ func FindNatGateways(cloud fi.Cloud, routeTables map[string]*resources.Resource, | |||
| natGatewayId := aws.StringValue(ngw.NatGatewayId) | |||
|
|
|||
| forceShared := !ownedNatGatewayIds.Has(natGatewayId) | |||
| resourceTrackers = append(resourceTrackers, buildNatGatewayResource(ngw, forceShared, clusterName)) | |||
| ngwResource := buildNatGatewayResource(ngw, forceShared, clusterName) | |||
| resourceTrackers = append(resourceTrackers, ngwResource) | |||
There was a problem hiding this comment.
did you test this? Now you are skipping only elastic ip deletion? Are you sure that nat gateway is not going to be deleted
There was a problem hiding this comment.
did you test this? Now you are skipping only elastic ip deletion? Are you sure that nat gateway is not going to be deleted
Yes, because the nat gateway is correctly tagged with ownership tag. I tested both scenarios:
1: Nat gateway owned: nat gateway deleted, elastic IP deleted
2: Nat gateway shared: nat gateway not deleted, elastic IP not deleted
k8s-ci-robot
added
ok-to-test
|
Makes sense to me, nice approach @dzoeteman! Thanks for the PR! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dzoeteman, mikesplain 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 |
k8s-ci-robot
added
the
approved
|
Thanks @mikesplain & @zetaab! Is it possible to get this into 1.14 too? Would allow us to get our kops pipeline automated - that would be fantastic :) |
|
Please make cherry pick PR? |
Done :) |
Cherry-pick #7525 into 1.14
Right now, if a NatGateway is shared, kops will still try to remove ElasticIPs associated to the NatGateway when trying to delete a cluster. These ElasticIPs don't have any ownership tags.
This will never work, as kops is not deleting the NatGateway, and it shouldn't anyway, since kops doesn't own the ElasticIP.
Fixed this by not checking for ElasticIPs when trying to delete a shared NatGateway.
Tested that with an owned NatGateway, the associated Elastic IP is still deleted.