Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use owner refs to determine ownership for ASO resources #4499

Merged
merged 1 commit into from
Jan 24, 2024
Merged

use owner refs to determine ownership for ASO resources #4499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/v1beta1/consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ const (
// OwnedByClusterLabelKey communicates CAPZ's ownership of an ASO resource
// independently of its ownership of the underlying Azure resource. The
// value for the label is the CAPI Cluster Name.
//
// Deprecated: OwnerReferences now determine ownership.
Comment on lines +161 to +162
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax makes sure the linter flags any additional usages of this constant.

OwnedByClusterLabelKey = NameAzureProviderPrefix + string(ResourceLifecycleOwned)
)

Expand Down
5 changes: 3 additions & 2 deletions azure/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,9 @@ type ResourceSpecGetterWithHeaders interface {

// ASOResourceSpecGetter is an interface for getting all the required information to create/update/delete an Azure resource.
type ASOResourceSpecGetter[T genruntime.MetaObject] interface {
// ResourceRef returns a concrete, named (and namespaced if applicable) ASO
// resource type to facilitate a strongly-typed GET.
// ResourceRef returns a concrete, named ASO resource type to facilitate a
// strongly-typed GET. Namespace is not read if set here and is instead
// derived from OwnerReferences.
Comment on lines +155 to +157
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OwnerReferences can't span namespaces, so this is why namespace is being removed in lots of places throughout this PR.

ResourceRef() T
// Parameters returns a modified object if it points to a non-nil resource.
// Otherwise it returns an unmodified object if no updates are needed.
Expand Down
16 changes: 5 additions & 11 deletions azure/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@
return s.Cluster.DeletionTimestamp
}

// ASOOwner implements aso.Scope.
func (s *ClusterScope) ASOOwner() client.Object {
return s.AzureCluster

Check warning on line 143 in azure/scope/cluster.go

View check run for this annotation

Codecov / codecov/patch

azure/scope/cluster.go#L142-L143 

Added lines #L142 - L143 were not covered by tests
}

// PublicIPSpecs returns the public IP specs.
func (s *ClusterScope) PublicIPSpecs() []azure.ResourceSpecGetter {
var publicIPSpecs []azure.ResourceSpecGetter
Expand Down Expand Up @@ -334,7 +339,6 @@
natGatewaySet[subnet.NatGateway.Name] = struct{}{} // empty struct to represent hash set
natGateways = append(natGateways, &natgateways.NatGatewaySpec{
Name: subnet.NatGateway.Name,
Namespace: s.Namespace(),
ResourceGroup: s.ResourceGroup(),
SubscriptionID: s.SubscriptionID(),
Location: s.Location(),
Expand Down Expand Up @@ -383,7 +387,6 @@
for _, subnet := range s.AzureCluster.Spec.NetworkSpec.Subnets {
subnetSpec := &subnets.SubnetSpec{
Name: subnet.Name,
Namespace: s.Namespace(),
ResourceGroup: s.ResourceGroup(),
SubscriptionID: s.SubscriptionID(),
CIDRs: subnet.CIDRBlocks,
Expand All @@ -402,7 +405,6 @@
azureBastionSubnet := s.AzureCluster.Spec.BastionSpec.AzureBastion.Subnet
subnetSpecs = append(subnetSpecs, &subnets.SubnetSpec{
Name: azureBastionSubnet.Name,
Namespace: s.Namespace(),
ResourceGroup: s.ResourceGroup(),
SubscriptionID: s.SubscriptionID(),
CIDRs: azureBastionSubnet.CIDRBlocks,
Expand All @@ -420,25 +422,20 @@

// GroupSpecs returns the resource group spec.
func (s *ClusterScope) GroupSpecs() []azure.ASOResourceSpecGetter[*asoresourcesv1.ResourceGroup] {
owner := *metav1.NewControllerRef(s.AzureCluster, infrav1.GroupVersion.WithKind(infrav1.AzureClusterKind))
specs := []azure.ASOResourceSpecGetter[*asoresourcesv1.ResourceGroup]{
&groups.GroupSpec{
Name: s.ResourceGroup(),
Namespace: s.Namespace(),
Location: s.Location(),
ClusterName: s.ClusterName(),
AdditionalTags: s.AdditionalTags(),
Owner: owner,
},
}
if s.Vnet().ResourceGroup != s.ResourceGroup() {
specs = append(specs, &groups.GroupSpec{
Name: s.Vnet().ResourceGroup,
Namespace: s.Namespace(),
Location: s.Location(),
ClusterName: s.ClusterName(),
AdditionalTags: s.AdditionalTags(),
Owner: owner,
})
}
return specs
Expand Down Expand Up @@ -484,7 +481,6 @@
return &virtualnetworks.VNetSpec{
ResourceGroup: s.Vnet().ResourceGroup,
Name: s.Vnet().Name,
Namespace: s.Namespace(),
CIDRs: s.Vnet().CIDRBlocks,
ExtendedLocation: s.ExtendedLocation(),
Location: s.Location(),
Expand Down Expand Up @@ -561,7 +557,6 @@

return &bastionhosts.AzureBastionSpec{
Name: s.AzureBastion().Name,
Namespace: s.Namespace(),
ResourceGroup: s.ResourceGroup(),
Location: s.Location(),
ClusterName: s.ClusterName(),
Expand Down Expand Up @@ -1100,7 +1095,6 @@
for _, privateEndpoint := range subnet.PrivateEndpoints {
privateEndpointSpec := &privateendpoints.PrivateEndpointSpec{
Name: privateEndpoint.Name,
Namespace: s.Namespace(),
ResourceGroup: s.ResourceGroup(),
Location: privateEndpoint.Location,
CustomNetworkInterfaceName: privateEndpoint.CustomNetworkInterfaceName,
Expand Down
3 changes: 0 additions & 3 deletions azure/scope/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3820,7 +3820,6 @@ func TestPrivateEndpointSpecs(t *testing.T) {
want: []azure.ASOResourceSpecGetter[*asonetworkv1api20220701.PrivateEndpoint]{
&privateendpoints.PrivateEndpointSpec{
Name: "my-private-endpoint",
Namespace: "dummy-ns",
ResourceGroup: "dummy-rg",
Location: "westus2",
CustomNetworkInterfaceName: "my-custom-nic",
Expand Down Expand Up @@ -3848,7 +3847,6 @@ func TestPrivateEndpointSpecs(t *testing.T) {
},
&privateendpoints.PrivateEndpointSpec{
Name: "my-private-endpoint-2",
Namespace: "dummy-ns",
ResourceGroup: "dummy-rg",
Location: "westus2",
CustomNetworkInterfaceName: "my-custom-nic-2",
Expand Down Expand Up @@ -3876,7 +3874,6 @@ func TestPrivateEndpointSpecs(t *testing.T) {
},
&privateendpoints.PrivateEndpointSpec{
Name: "my-private-endpoint-3",
Namespace: "dummy-ns",
ResourceGroup: "dummy-rg",
Location: "westus2",
CustomNetworkInterfaceName: "my-custom-nic-3",
Expand Down
13 changes: 6 additions & 7 deletions azure/scope/managedcontrolplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ func (s *ManagedControlPlaneScope) GetClient() client.Client {
return s.Client
}

// ASOOwner implements aso.Scope.
func (s *ManagedControlPlaneScope) ASOOwner() client.Object {
return s.ControlPlane
}

// GetDeletionTimestamp returns the deletion timestamp of the cluster.
func (s *ManagedControlPlaneScope) GetDeletionTimestamp() *metav1.Time {
return s.Cluster.DeletionTimestamp
Expand Down Expand Up @@ -266,11 +271,9 @@ func (s *ManagedControlPlaneScope) GroupSpecs() []azure.ASOResourceSpecGetter[*a
return []azure.ASOResourceSpecGetter[*asoresourcesv1.ResourceGroup]{
&groups.GroupSpec{
Name: s.ResourceGroup(),
Namespace: s.Cluster.Namespace,
Location: s.Location(),
ClusterName: s.ClusterName(),
AdditionalTags: s.AdditionalTags(),
Owner: *metav1.NewControllerRef(s.ControlPlane, infrav1.GroupVersion.WithKind(infrav1.AzureManagedControlPlaneKind)),
},
}
}
Expand All @@ -280,7 +283,6 @@ func (s *ManagedControlPlaneScope) VNetSpec() azure.ASOResourceSpecGetter[*asone
return &virtualnetworks.VNetSpec{
ResourceGroup: s.Vnet().ResourceGroup,
Name: s.Vnet().Name,
Namespace: s.ControlPlane.Namespace,
CIDRs: s.Vnet().CIDRBlocks,
Location: s.Location(),
ClusterName: s.ClusterName(),
Expand All @@ -295,7 +297,6 @@ func (s *ManagedControlPlaneScope) AzureFleetsMemberSpec() []azure.ASOResourceSp
}
return []azure.ASOResourceSpecGetter[*asocontainerservicev1preview.FleetsMember]{&fleetsmembers.AzureFleetsMemberSpec{
Name: s.AzureFleetMembership().Name,
Namespace: s.Cluster.Namespace,
ClusterName: s.ClusterName(),
ClusterResourceGroup: s.ResourceGroup(),
Group: s.AzureFleetMembership().Group,
Expand Down Expand Up @@ -325,7 +326,6 @@ func (s *ManagedControlPlaneScope) SubnetSpecs() []azure.ASOResourceSpecGetter[*
return []azure.ASOResourceSpecGetter[*asonetworkv1api20201101.VirtualNetworksSubnet]{
&subnets.SubnetSpec{
Name: s.NodeSubnet().Name,
Namespace: s.ControlPlane.Namespace,
ResourceGroup: s.ResourceGroup(),
SubscriptionID: s.SubscriptionID(),
CIDRs: s.NodeSubnet().CIDRBlocks,
Expand Down Expand Up @@ -422,6 +422,7 @@ func (s *ManagedControlPlaneScope) IsVnetManaged() bool {
defer done()

vnet := s.VNetSpec().ResourceRef()
vnet.SetNamespace(s.ASOOwner().GetNamespace())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the namespace set here explicitly? I see that other resources have the namespace set in the generic aso resource, but I could be misunderstanding how that works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on the interface definition for ResourceRef() now explains that it no longer reads the namespace from there and instead uses the namespace of the owner. This is to account for that change. On the plus side, we won't have to add a namespace field to the Spec types for all the services we have yet to convert to ASO.

err := s.Client.Get(ctx, client.ObjectKeyFromObject(vnet), vnet)
if err != nil {
log.Error(err, "Unable to determine if ManagedControlPlaneScope VNET is managed by capz, assuming unmanaged", "AzureManagedCluster", s.ClusterName())
Expand Down Expand Up @@ -503,7 +504,6 @@ func (s *ManagedControlPlaneScope) IsAADEnabled() bool {
func (s *ManagedControlPlaneScope) ManagedClusterSpec() azure.ASOResourceSpecGetter[*asocontainerservicev1.ManagedCluster] {
managedClusterSpec := managedclusters.ManagedClusterSpec{
Name: s.ControlPlane.Name,
Namespace: s.ControlPlane.Namespace,
ResourceGroup: s.ControlPlane.Spec.ResourceGroupName,
NodeResourceGroup: s.ControlPlane.Spec.NodeResourceGroupName,
ClusterName: s.ClusterName(),
Expand Down Expand Up @@ -876,7 +876,6 @@ func (s *ManagedControlPlaneScope) PrivateEndpointSpecs() []azure.ASOResourceSpe
for _, privateEndpoint := range s.ControlPlane.Spec.VirtualNetwork.Subnet.PrivateEndpoints {
privateEndpointSpec := &privateendpoints.PrivateEndpointSpec{
Name: privateEndpoint.Name,
Namespace: s.Cluster.Namespace,
ResourceGroup: s.Vnet().ResourceGroup,
Location: privateEndpoint.Location,
CustomNetworkInterfaceName: privateEndpoint.CustomNetworkInterfaceName,
Expand Down
6 changes: 0 additions & 6 deletions azure/scope/managedcontrolplane_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ func TestManagedControlPlaneScope_PoolVersion(t *testing.T) {
&agentpools.AgentPoolSpec{
Name: "pool0",
AzureName: "pool0",
Namespace: "default",
SKU: "Standard_D2s_v3",
Replicas: 1,
Mode: "System",
Expand Down Expand Up @@ -222,7 +221,6 @@ func TestManagedControlPlaneScope_PoolVersion(t *testing.T) {
&agentpools.AgentPoolSpec{
Name: "pool0",
AzureName: "pool0",
Namespace: "default",
SKU: "Standard_D2s_v3",
Mode: "System",
Replicas: 1,
Expand Down Expand Up @@ -474,7 +472,6 @@ func TestManagedControlPlaneScope_OSType(t *testing.T) {
&agentpools.AgentPoolSpec{
Name: "pool0",
AzureName: "pool0",
Namespace: "default",
SKU: "Standard_D2s_v3",
Mode: "System",
Replicas: 1,
Expand All @@ -484,7 +481,6 @@ func TestManagedControlPlaneScope_OSType(t *testing.T) {
&agentpools.AgentPoolSpec{
Name: "pool1",
AzureName: "pool1",
Namespace: "default",
SKU: "Standard_D2s_v3",
Mode: "User",
Replicas: 1,
Expand All @@ -495,7 +491,6 @@ func TestManagedControlPlaneScope_OSType(t *testing.T) {
&agentpools.AgentPoolSpec{
Name: "pool2",
AzureName: "pool2",
Namespace: "default",
SKU: "Standard_D2s_v3",
Mode: "User",
Replicas: 1,
Expand Down Expand Up @@ -1390,7 +1385,6 @@ func TestManagedControlPlaneScope_PrivateEndpointSpecs(t *testing.T) {
Expected: []azure.ASOResourceSpecGetter[*asonetworkv1.PrivateEndpoint]{
&privateendpoints.PrivateEndpointSpec{
Name: "my-private-endpoint",
Namespace: "dummy-ns",
ResourceGroup: "dummy-rg",
Location: "westus2",
CustomNetworkInterfaceName: "my-custom-nic",
Expand Down
6 changes: 5 additions & 1 deletion azure/scope/managedmachinepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@
return s.Client
}

// ASOOwner implements aso.Scope.
func (s *ManagedMachinePoolScope) ASOOwner() client.Object {
return s.InfraMachinePool

Check warning on line 134 in azure/scope/managedmachinepool.go

View check run for this annotation

Codecov / codecov/patch

azure/scope/managedmachinepool.go#L133-L134 

Added lines #L133 - L134 were not covered by tests
}

// Name returns the name of the infra machine pool.
func (s *ManagedMachinePoolScope) Name() string {
return s.InfraMachinePool.Name
Expand Down Expand Up @@ -167,7 +172,6 @@

agentPoolSpec := &agentpools.AgentPoolSpec{
Name: managedMachinePool.Name,
Namespace: managedMachinePool.Namespace,
AzureName: ptr.Deref(managedMachinePool.Spec.Name, ""),
ResourceGroup: managedControlPlane.Spec.ResourceGroupName,
Cluster: managedControlPlane.Name,
Expand Down
Loading