convert subnets and virtualnetworks to ASO #4300
Conversation
k8s-ci-robot
added
do-not-merge/hold
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4300 +/- ##
==========================================
+ Coverage 61.61% 61.90% +0.28%
==========================================
Files 190 188 -2
Lines 18994 18725 -269
==========================================
- Hits 11704 11591 -113
+ Misses 6641 6496 -145
+ Partials 649 638 -11 ☔ View full report in Codecov by Sentry. |
|
/test pull-cluster-api-provider-azure-e2e-optional |
|
/test pull-cluster-api-provider-azure-e2e-optional |
| } | ||
|
|
||
| if !s.IsVNetManaged { | ||
| // TODO: change this to terminal error once we add support for handling them | ||
| return nil, errors.Errorf("custom vnet was provided but subnet %s is missing", s.Name) |
There was a problem hiding this comment.
This check enforcing subnets already exist in user-managed vnets has been removed because implementing an equivalent in the ASO framework would get messier than it'd be worth I think.
This check worked before because Parameters used to only be called when CAPZ determined it should manage a resource and was ready to make an API call to Azure, but with ASO Parameters is called even when an ASO resource maps to a user-managed Azure resource. That means this same !s.IsVNetManaged check would be expected to be true for a subnet that already exists in Azure outside of ASO, in which case we should not error because CAPZ still needs to create the ASO proxy resource. I can see if we can rearrange the framework a bit to make this check possible, but I think that will still be net-positive complexity.
As a result, it is now valid not to pre-create all of the subnets in an unmanaged vnet and CAPZ will create and manage missing subnets instead of throwing an error.
There was a problem hiding this comment.
I think that was actually a requested user feature in the past (there might even be an issue for it somewhere)
How do we handle delete for those vnets/subnets?
There was a problem hiding this comment.
My archaeology skills led me here, where it seems this was added as a part of a general effort to support BYO vnet: https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/340/files#diff-6d4bea1a244164bf3d16b025868b878dffcf0c78963b59bad9c189bcee04d35eR87-R90
CAPZ would take full responsibility for any subnet it creates, whether it's part of a managed or unmanaged vnet. So a managed subnet in an unmanaged vnet can be deleted by CAPZ.
There was a problem hiding this comment.
^ But in that same scenario, the unmanaged vnet would not be deleted.
|
/test pull-cluster-api-provider-azure-e2e-optional |
|
/retest |
|
Ready for review! /retitle convert subnets and virtualnetworks to ASO |
k8s-ci-robot
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
k8s-ci-robot
added
needs-rebase
|
/test pull-cluster-api-provider-azure-e2e-optional |
|
/test pull-cluster-api-provider-azure-e2e |
|
/assign @willie-yao @nawazkh |
|
/retest |
1 similar comment
|
/retest |
|
/test pull-cluster-api-provider-azure-e2e-optional |
| @@ -504,7 +492,7 @@ func (s *ManagedControlPlaneScope) ManagedClusterSpec() azure.ASOResourceSpecGet | |||
| DNSServiceIP: s.ControlPlane.Spec.DNSServiceIP, | |||
| VnetSubnetID: azure.SubnetID( | |||
| s.ControlPlane.Spec.SubscriptionID, | |||
| s.VNetSpec().ResourceGroupName(), | |||
| s.Vnet().ResourceGroup, | |||
| s.ControlPlane.Spec.VirtualNetwork.Name, | |||
| s.ControlPlane.Spec.VirtualNetwork.Subnet.Name, | |||
There was a problem hiding this comment.
Should these be also using s.Vnet()? Just to keep things consistent with the line above.
There was a problem hiding this comment.
s.Vnet() doesn't define the Subnet, and all the other places using the vnet resource group refer to it like this, so it seems like there's some inconsistency necessary here without some wider refactoring.
| Location: privateEndpoint.Location, | ||
| CustomNetworkInterfaceName: privateEndpoint.CustomNetworkInterfaceName, | ||
| PrivateIPAddresses: privateEndpoint.PrivateIPAddresses, | ||
| SubnetID: azure.SubnetID( | ||
| s.ControlPlane.Spec.SubscriptionID, | ||
| s.VNetSpec().ResourceGroupName(), | ||
| s.Vnet().ResourceGroup, | ||
| s.ControlPlane.Spec.VirtualNetwork.Name, | ||
| s.ControlPlane.Spec.VirtualNetwork.Subnet.Name, |
There was a problem hiding this comment.
Same comment as above ^^
|
/lgtm |
|
@CecileRobertMichon This should be ready for a final review. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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
|
squashed! |
k8s-ci-robot
added
needs-rebase
k8s-ci-robot
requested review from
CecileRobertMichon,
nawazkh and
willie-yao
k8s-ci-robot
removed
the
needs-rebase
|
@willie-yao @CecileRobertMichon Resolved conflicts, PTAL. /hold for squash |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 04942c0e876eb09cb9087c50d42cadc4e48002b2
|
|
squashed! |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
1 similar comment
|
/lgtm |
|
@nojnhuh: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it: This PR updates the subnets and virtualnetworks services to use ASO.
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 #3528
Special notes for your reviewer:
This is split into two commits with one for each service. I kept these in the same PR though because converting subnets to ASO before virtualnetworks would have required some gymnastics to get the unit tests in ./controllers to pass that I didn't think were worth ironing out. I intend to squash this into one commit before merging since the unit tests don't pass for the first commit that only updates subnets.
/hold for squash
TODOs:
Release note: