cluster-api-provider-aws icon indicating copy to clipboard operation
cluster-api-provider-aws copied to clipboard

Remove AWSCloudProvider tags from unmanaged subnets when deleting a cluster

Open pydctw opened this issue 3 years ago • 6 comments
trafficstars

What type of PR is this? /kind bug

What this PR does / why we need it: CAPA adds AWSCloudProvider tags to unmanaged subnets but didn't remove the those tags when deleting the cluster. This PR fixes it.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #3074

Special notes for your reviewer: In addition to adding unit tests, I tested creating a cluster, observed the tags, deleted the cluster and confirmed the tags are removed. See CAPA log showing tag deletion.

I0307 22:51:28.693067      16 subnets.go:279] controller/awscluster "msg"="Deleting AWSCloudProvider tags from unmanaged subnets" "cluster"="ec2-cluster" "name"="ec2-cluster" "namespace"="default" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="AWSCluster" 
I0307 22:51:28.853169      16 subnets.go:251] controller/awscluster "msg"="Deleted AWSCloudProvider tags from unmanaged subnets" "cluster"="ec2-cluster" "name"="ec2-cluster" "namespace"="default" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="AWSCluster"

Checklist:

  • [ ] squashed commits
  • [ ] includes documentation
  • [x] adds unit tests
  • [ ] adds or updates e2e tests

Release note:

Remove CAPA-added AWSCloudProvider tags from unmanaged subnets when deleting a cluster

pydctw avatar Mar 10 '22 19:03 pydctw

@pydctw: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 avatar Mar 10 '22 19:03 k8s-ci-robot

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign neolit123 after the PR has been reviewed. You can assign the PR to them by writing /assign @neolit123 in a comment when ready.

The full list of commands accepted by this bot can be found 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 avatar Mar 10 '22 19:03 k8s-ci-robot

As a side note, we allow users to specify their own subnets not only in clusterNetwork but also in controlPlaneLoadbalancer.subnets.

I noticed that we only tag the machine subnets when the VPC is unmanaged and hence not cleaning here. Might worth to consider if we want to be consistent about tagging all BYO subnets.

sedefsavas avatar Mar 16 '22 04:03 sedefsavas

We have a new e2e test for unmanaged infra, we can add a check there to confirm cloud provider tags we add are gone after the cluster is deleted right before deleting the subnets.

sedefsavas avatar Mar 16 '22 04:03 sedefsavas

(Sorry if I missed it in the PR) I wonder if this should also remove tags provided with additionalTags, restoring the Subnetss tags back to before a cluster was created?

dkoshkin avatar May 17 '22 19:05 dkoshkin

@pydctw What is the status of this PR?

sedefsavas avatar Aug 11 '22 10:08 sedefsavas

@pydctw: PR needs rebase.

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 avatar Sep 21 '22 23:09 k8s-ci-robot

No bandwidth to work on this.

pydctw avatar Oct 11 '22 20:10 pydctw