Update managed vnet check logic #5012
Conversation
k8s-ci-robot
added
release-note
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @hrbasic! |
|
Hi @hrbasic. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
k8s-ci-robot
added
size/L
|
/easycla |
k8s-ci-robot
added
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5012 +/- ##
==========================================
+ Coverage 51.19% 51.23% +0.04%
==========================================
Files 274 274
Lines 24628 24637 +9
==========================================
+ Hits 12608 12623 +15
+ Misses 11234 11227 -7
- Partials 786 787 +1 ☔ View full report in Codecov by Sentry. |
|
/assign |
| return false | ||
| } | ||
|
|
||
| isManaged := infrav1.Tags(vnet.Status.Tags).HasOwned(s.ClusterName()) |
There was a problem hiding this comment.
Shouldn't this line also include the check to s.Vnet().ID from the previous function? https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/5012/files#diff-b74c6acb4629abe58f8286da9939563cc4404e09fe9966eb61dbde9b87274189L585
There was a problem hiding this comment.
As described in the issue #4930, when AzureCluster is created using BYO Vnet: https://capz.sigs.k8s.io/topics/custom-vnet#pre-existing-vnet-and-subnets, virtual network object will be created, but during creation of Vnet, ID will be empty which will mark Vnet as managed. This causes creation of network security groups and route tables which will not be deleted (unless they are created manually using ASO). After certain time Vnet ID will be populated and marked as not-managed. So I believe that check s.Vnet().ID shouldn't be used here.
I was expecting that managed status will never be changed due to cache, but that wasn't the case.
Do we need this ID check for backward compatibility? Because I saw that this is not used in managed control plane.
There was a problem hiding this comment.
That should be fine, thanks for the explanation!
| return false | ||
| } | ||
|
|
||
| isManaged := infrav1.Tags(vnet.Status.Tags).HasOwned(s.ClusterName()) |
There was a problem hiding this comment.
That should be fine, thanks for the explanation!
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 07d71e7b7097b2b4a4477f931cf5e935d92c4b01
|
|
/assign @nojnhuh |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mboersma 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
What type of PR is this?
/kind bug
What this PR does / why we need it:
In some cases, using a BYO (Bring Your Own) VNet can cause problems with the deletion of route tables and network security groups due to an issue with checking if the VNet is managed. More information can be found in the related issue.
Which issue(s) this PR fixes:
Fixes #4930
TODOs:
Release note: