diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index 4f74556af36..d8169758fb9 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "hash/fnv" + "sort" "strconv" "strings" @@ -35,6 +36,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/loadbalancers" "sigs.k8s.io/cluster-api-provider-azure/azure/services/natgateways" "sigs.k8s.io/cluster-api-provider-azure/azure/services/privatedns" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" "sigs.k8s.io/cluster-api-provider-azure/azure/services/routetables" "sigs.k8s.io/cluster-api-provider-azure/azure/services/securitygroups" "sigs.k8s.io/cluster-api-provider-azure/azure/services/subnets" @@ -120,22 +122,29 @@ func (s *ClusterScope) Authorizer() autorest.Authorizer { } // PublicIPSpecs returns the public IP specs. -func (s *ClusterScope) PublicIPSpecs() []azure.PublicIPSpec { - var publicIPSpecs []azure.PublicIPSpec +func (s *ClusterScope) PublicIPSpecs() []azure.ResourceSpecGetter { + var publicIPSpecs []azure.ResourceSpecGetter // Public IP specs for control plane lb - var controlPlaneOutboundIPSpecs []azure.PublicIPSpec + var controlPlaneOutboundIPSpecs []azure.ResourceSpecGetter if s.IsAPIServerPrivate() { // Public IP specs for control plane outbound lb if s.ControlPlaneOutboundLB() != nil { controlPlaneOutboundIPSpecs = s.getOutboundLBPublicIPSpecs(s.ControlPlaneOutboundLB(), azure.GenerateControlPlaneOutboundIPName) } } else { - controlPlaneOutboundIPSpecs = []azure.PublicIPSpec{{ - Name: s.APIServerPublicIP().Name, - DNSName: s.APIServerPublicIP().DNSName, - IsIPv6: false, // currently azure requires a ipv4 lb rule to enable ipv6 - }} + controlPlaneOutboundIPSpecs = []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: s.APIServerPublicIP().Name, + ResourceGroup: s.ResourceGroup(), + DNSName: s.APIServerPublicIP().DNSName, + IsIPv6: false, // Currently azure requires an IPv4 lb rule to enable IPv6 + ClusterName: s.ClusterName(), + Location: s.Location(), + FailureDomains: s.FailureDomains(), + AdditionalTags: s.AdditionalTags(), + }, + } } publicIPSpecs = append(publicIPSpecs, controlPlaneOutboundIPSpecs...) @@ -146,22 +155,34 @@ func (s *ClusterScope) PublicIPSpecs() []azure.PublicIPSpec { } // Public IP specs for node NAT gateways - var nodeNatGatewayIPSpecs []azure.PublicIPSpec + var nodeNatGatewayIPSpecs []azure.ResourceSpecGetter for _, subnet := range s.NodeSubnets() { if subnet.IsNatGatewayEnabled() { - nodeNatGatewayIPSpecs = append(nodeNatGatewayIPSpecs, azure.PublicIPSpec{ - Name: subnet.NatGateway.NatGatewayIP.Name, - DNSName: subnet.NatGateway.NatGatewayIP.DNSName, + nodeNatGatewayIPSpecs = append(nodeNatGatewayIPSpecs, &publicips.PublicIPSpec{ + Name: subnet.NatGateway.NatGatewayIP.Name, + ResourceGroup: s.ResourceGroup(), + DNSName: subnet.NatGateway.NatGatewayIP.DNSName, + IsIPv6: false, // Public IP is IPv4 by default + ClusterName: s.ClusterName(), + Location: s.Location(), + FailureDomains: s.FailureDomains(), + AdditionalTags: s.AdditionalTags(), }) } publicIPSpecs = append(publicIPSpecs, nodeNatGatewayIPSpecs...) } - if s.AzureCluster.Spec.BastionSpec.AzureBastion != nil { + if azureBastion := s.AzureBastion(); azureBastion != nil { // public IP for Azure Bastion. - azureBastionPublicIP := azure.PublicIPSpec{ - Name: s.AzureCluster.Spec.BastionSpec.AzureBastion.PublicIP.Name, - DNSName: s.AzureCluster.Spec.BastionSpec.AzureBastion.PublicIP.DNSName, + azureBastionPublicIP := &publicips.PublicIPSpec{ + Name: azureBastion.PublicIP.Name, + ResourceGroup: s.ResourceGroup(), + DNSName: azureBastion.PublicIP.DNSName, + IsIPv6: false, // Public IP is IPv4 by default + ClusterName: s.ClusterName(), + Location: s.Location(), + FailureDomains: s.FailureDomains(), + AdditionalTags: s.AdditionalTags(), } publicIPSpecs = append(publicIPSpecs, azureBastionPublicIP) } @@ -775,6 +796,9 @@ func (s *ClusterScope) FailureDomains() []string { fds[i] = id i++ } + + sort.Strings(fds) + return fds } @@ -845,20 +869,34 @@ func (s *ClusterScope) SetDNSName() { } // getOutboundLBPublicIPSpecs returns the public ip specs for a LoadBalancerSpec based on the number of frontend ips configured. -func (s *ClusterScope) getOutboundLBPublicIPSpecs(outboundLB *infrav1.LoadBalancerSpec, generateOutboundIPName func(string) string) []azure.PublicIPSpec { - var outboundIPSpecs []azure.PublicIPSpec +func (s *ClusterScope) getOutboundLBPublicIPSpecs(outboundLB *infrav1.LoadBalancerSpec, generateOutboundIPName func(string) string) []azure.ResourceSpecGetter { + var outboundIPSpecs []azure.ResourceSpecGetter loadBalancerNodeOutboundIPs := outboundLB.FrontendIPsCount switch { case loadBalancerNodeOutboundIPs == nil || *loadBalancerNodeOutboundIPs == 0: // do nothing case *loadBalancerNodeOutboundIPs == 1: - outboundIPSpecs = append(outboundIPSpecs, azure.PublicIPSpec{ - Name: generateOutboundIPName(s.ClusterName()), + outboundIPSpecs = append(outboundIPSpecs, &publicips.PublicIPSpec{ + Name: generateOutboundIPName(s.ClusterName()), + ResourceGroup: s.ResourceGroup(), + ClusterName: s.ClusterName(), + DNSName: "", // Set to default value + IsIPv6: false, // Set to default value + Location: s.Location(), + FailureDomains: s.FailureDomains(), + AdditionalTags: s.AdditionalTags(), }) default: for i := 0; i < int(*loadBalancerNodeOutboundIPs); i++ { - outboundIPSpecs = append(outboundIPSpecs, azure.PublicIPSpec{ - Name: azure.WithIndex(generateOutboundIPName(s.ClusterName()), i+1), + outboundIPSpecs = append(outboundIPSpecs, &publicips.PublicIPSpec{ + Name: azure.WithIndex(generateOutboundIPName(s.ClusterName()), i+1), + ResourceGroup: s.ResourceGroup(), + ClusterName: s.ClusterName(), + DNSName: "", // Set to default value + IsIPv6: false, // Set to default value + Location: s.Location(), + FailureDomains: s.FailureDomains(), + AdditionalTags: s.AdditionalTags(), }) } } diff --git a/azure/scope/cluster_test.go b/azure/scope/cluster_test.go index 05ed0c85f89..bda468a087f 100644 --- a/azure/scope/cluster_test.go +++ b/azure/scope/cluster_test.go @@ -26,6 +26,7 @@ import ( "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/azure/auth" "github.com/Azure/go-autorest/autorest/to" + "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -33,6 +34,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/services/bastionhosts" "sigs.k8s.io/cluster-api-provider-azure/azure/services/natgateways" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" "sigs.k8s.io/cluster-api-provider-azure/azure/services/routetables" "sigs.k8s.io/cluster-api-provider-azure/azure/services/securitygroups" "sigs.k8s.io/cluster-api-provider-azure/azure/services/subnets" @@ -250,7 +252,7 @@ func TestPublicIPSpecs(t *testing.T) { tests := []struct { name string azureCluster *infrav1.AzureCluster - expectedPublicIPSpec []azure.PublicIPSpec + expectedPublicIPSpec []azure.ResourceSpecGetter }{ { name: "Azure cluster with internal type LB and nil frontend IP count", @@ -265,9 +267,22 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ APIServerLB: infrav1.LoadBalancerSpec{ @@ -293,9 +308,22 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ APIServerLB: infrav1.LoadBalancerSpec{ @@ -321,9 +349,22 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ ControlPlaneOutboundLB: &infrav1.LoadBalancerSpec{ @@ -338,11 +379,19 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, - expectedPublicIPSpec: []azure.PublicIPSpec{ - { - Name: "pip-my-cluster-controlplane-outbound", - DNSName: "", - IsIPv6: false, + expectedPublicIPSpec: []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: "pip-my-cluster-controlplane-outbound", + ResourceGroup: "my-rg", + DNSName: "", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, }, }, @@ -359,9 +408,22 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ ControlPlaneOutboundLB: &infrav1.LoadBalancerSpec{ @@ -376,21 +438,45 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, - expectedPublicIPSpec: []azure.PublicIPSpec{ - { - Name: "pip-my-cluster-controlplane-outbound-1", - DNSName: "", - IsIPv6: false, + expectedPublicIPSpec: []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: "pip-my-cluster-controlplane-outbound-1", + ResourceGroup: "my-rg", + DNSName: "", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, - { - Name: "pip-my-cluster-controlplane-outbound-2", - DNSName: "", - IsIPv6: false, + &publicips.PublicIPSpec{ + Name: "pip-my-cluster-controlplane-outbound-2", + ResourceGroup: "my-rg", + DNSName: "", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, - { - Name: "pip-my-cluster-controlplane-outbound-3", - DNSName: "", - IsIPv6: false, + &publicips.PublicIPSpec{ + Name: "pip-my-cluster-controlplane-outbound-3", + ResourceGroup: "my-rg", + DNSName: "", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, }, }, @@ -407,9 +493,22 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ ControlPlaneOutboundLB: &infrav1.LoadBalancerSpec{ @@ -429,11 +528,19 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, - expectedPublicIPSpec: []azure.PublicIPSpec{ - { - Name: "40.60.89.22", - DNSName: "fake-dns", - IsIPv6: false, + expectedPublicIPSpec: []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: "40.60.89.22", + ResourceGroup: "my-rg", + DNSName: "fake-dns", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, }, }, @@ -450,9 +557,22 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ ControlPlaneOutboundLB: &infrav1.LoadBalancerSpec{ @@ -475,11 +595,19 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, - expectedPublicIPSpec: []azure.PublicIPSpec{ - { - Name: "40.60.89.22", - DNSName: "fake-dns", - IsIPv6: false, + expectedPublicIPSpec: []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: "40.60.89.22", + ResourceGroup: "my-rg", + DNSName: "fake-dns", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, }, }, @@ -496,7 +624,15 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", BastionSpec: infrav1.BastionSpec{ AzureBastion: &infrav1.AzureBastion{ PublicIP: infrav1.PublicIPSpec{ @@ -507,6 +643,11 @@ func TestPublicIPSpecs(t *testing.T) { }, AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, NetworkSpec: infrav1.NetworkSpec{ Subnets: infrav1.Subnets{ @@ -542,16 +683,32 @@ func TestPublicIPSpecs(t *testing.T) { }, }, }, - expectedPublicIPSpec: []azure.PublicIPSpec{ - { - Name: "40.60.89.22", - DNSName: "fake-dns", - IsIPv6: false, + expectedPublicIPSpec: []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: "40.60.89.22", + ResourceGroup: "my-rg", + DNSName: "fake-dns", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, - { - Name: "fake-bastion-public-ip", - DNSName: "fake-bastion-dns-name", - IsIPv6: false, + &publicips.PublicIPSpec{ + Name: "fake-bastion-public-ip", + ResourceGroup: "my-rg", + DNSName: "fake-bastion-dns-name", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, }, }, @@ -584,8 +741,10 @@ func TestPublicIPSpecs(t *testing.T) { Client: fakeClient, }) g.Expect(err).NotTo(HaveOccurred()) - got := clusterScope.PublicIPSpecs() - g.Expect(tc.expectedPublicIPSpec).Should(Equal(got)) + + if got := clusterScope.PublicIPSpecs(); !reflect.DeepEqual(got, tc.expectedPublicIPSpec) { + t.Errorf("PublicIPSpecs() diff between expected result and actual result: %s", cmp.Diff(tc.expectedPublicIPSpec, got)) + } }) } } diff --git a/azure/scope/machine.go b/azure/scope/machine.go index be5b837705e..2dd1c98269c 100644 --- a/azure/scope/machine.go +++ b/azure/scope/machine.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/disks" "sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachineimages" @@ -186,14 +187,21 @@ func (m *MachineScope) TagsSpecs() []azure.TagsSpec { } // PublicIPSpecs returns the public IP specs. -func (m *MachineScope) PublicIPSpecs() []azure.PublicIPSpec { - var spec []azure.PublicIPSpec +func (m *MachineScope) PublicIPSpecs() []azure.ResourceSpecGetter { + var specs []azure.ResourceSpecGetter if m.AzureMachine.Spec.AllocatePublicIP { - spec = append(spec, azure.PublicIPSpec{ - Name: azure.GenerateNodePublicIPName(m.Name()), + specs = append(specs, &publicips.PublicIPSpec{ + Name: azure.GenerateNodePublicIPName(m.Name()), + ResourceGroup: m.ResourceGroup(), + ClusterName: m.ClusterName(), + DNSName: "", // Set to default value + IsIPv6: false, // Set to default value + Location: m.Location(), + FailureDomains: m.FailureDomains(), + AdditionalTags: m.ClusterScoper.AdditionalTags(), }) } - return spec + return specs } // InboundNatSpecs returns the inbound NAT specs. diff --git a/azure/scope/machine_test.go b/azure/scope/machine_test.go index 9195e2ff29c..ae97d821140 100644 --- a/azure/scope/machine_test.go +++ b/azure/scope/machine_test.go @@ -25,6 +25,7 @@ import ( "github.com/Azure/go-autorest/autorest/azure/auth" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" + "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" @@ -34,6 +35,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/disks" "sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachineimages" @@ -243,7 +245,7 @@ func TestMachineScope_PublicIPSpecs(t *testing.T) { tests := []struct { name string machineScope MachineScope - want []azure.PublicIPSpec + want []azure.ResourceSpecGetter }{ { name: "returns nil if AllocatePublicIP is false", @@ -270,10 +272,58 @@ func TestMachineScope_PublicIPSpecs(t *testing.T) { AllocatePublicIP: true, }, }, + ClusterScoper: &ClusterScope{ + Cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + // Note: m.ClusterName() takes the value from the Cluster object, not the AzureCluster object + }, + }, + AzureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + }, + Status: infrav1.AzureClusterStatus{ + FailureDomains: map[string]clusterv1.FailureDomainSpec{ + "failure-domain-id-1": {}, + "failure-domain-id-2": {}, + "failure-domain-id-3": {}, + }, + }, + Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + SubscriptionID: "123", + Location: "centralIndia", + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, + }, + NetworkSpec: infrav1.NetworkSpec{ + APIServerLB: infrav1.LoadBalancerSpec{ + LoadBalancerClassSpec: infrav1.LoadBalancerClassSpec{ + Type: infrav1.Internal, + }, + }, + }, + }, + }, + }, }, - want: []azure.PublicIPSpec{ - { - Name: "pip-machine-name", + want: []azure.ResourceSpecGetter{ + &publicips.PublicIPSpec{ + Name: "pip-machine-name", + ResourceGroup: "my-rg", + DNSName: "", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, }, }, }, @@ -281,7 +331,7 @@ func TestMachineScope_PublicIPSpecs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if got := tt.machineScope.PublicIPSpecs(); !reflect.DeepEqual(got, tt.want) { - t.Errorf("PublicIPSpecs() = %v, want %v", got, tt.want) + t.Errorf("PublicIPSpecs() expected but got: %s", cmp.Diff(tt.want, got)) } }) } diff --git a/azure/services/publicips/client.go b/azure/services/publicips/client.go index a82cd3f5573..25bfa0058b7 100644 --- a/azure/services/publicips/client.go +++ b/azure/services/publicips/client.go @@ -18,27 +18,23 @@ package publicips import ( "context" + "encoding/json" "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" "github.com/Azure/go-autorest/autorest" + azureautorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/pkg/errors" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -// Client wraps go-sdk. -type Client interface { - Get(context.Context, string, string) (network.PublicIPAddress, error) - CreateOrUpdate(context.Context, string, string, network.PublicIPAddress) error - Delete(context.Context, string, string) error -} - // AzureClient contains the Azure go-sdk Client. type AzureClient struct { publicips network.PublicIPAddressesClient } -var _ Client = &AzureClient{} - // NewClient creates a new public IP client from subscription ID. func NewClient(auth azure.Authorizer) *AzureClient { c := newPublicIPAddressesClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer()) @@ -53,43 +49,113 @@ func newPublicIPAddressesClient(subscriptionID string, baseURI string, authorize } // Get gets the specified public IP address in a specified resource group. -func (ac *AzureClient) Get(ctx context.Context, resourceGroupName, ipName string) (network.PublicIPAddress, error) { +func (ac *AzureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (result interface{}, err error) { ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.AzureClient.Get") defer done() - return ac.publicips.Get(ctx, resourceGroupName, ipName, "") + return ac.publicips.Get(ctx, spec.ResourceGroupName(), spec.ResourceName(), "") } // CreateOrUpdate creates or updates a static or dynamic public IP address. -func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName string, ipName string, ip network.PublicIPAddress) error { +// It sends a PUT request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing +// progress of the operation. +func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (result interface{}, future azureautorest.FutureAPI, err error) { ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.AzureClient.CreateOrUpdate") defer done() - future, err := ac.publicips.CreateOrUpdate(ctx, resourceGroupName, ipName, ip) + publicip, ok := parameters.(network.PublicIPAddress) + if !ok { + return nil, nil, errors.Errorf("%T is not a network.PublicIPAddress", parameters) + } + + createFuture, err := ac.publicips.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), publicip) if err != nil { - return err + return nil, nil, err } - err = future.WaitForCompletionRef(ctx, ac.publicips.Client) + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + + err = createFuture.WaitForCompletionRef(ctx, ac.publicips.Client) if err != nil { - return err + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return nil, &createFuture, err } - _, err = future.Result(ac.publicips) - return err + + result, err = createFuture.Result(ac.publicips) + // if the operation completed, return a nil future + return result, nil, err } -// Delete deletes the specified public IP address. -func (ac *AzureClient) Delete(ctx context.Context, resourceGroupName, ipName string) error { - ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.AzureClient.Delete") +// DeleteAsync deletes the specified public IP address asynchronously. DeleteAsync sends a DELETE +// request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing +// progress of the operation. +func (ac *AzureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (future azureautorest.FutureAPI, err error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.AzureClient.DeleteAsync") defer done() - future, err := ac.publicips.Delete(ctx, resourceGroupName, ipName) + deleteFuture, err := ac.publicips.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName()) + if err != nil { + return nil, err + } + + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() + + err = deleteFuture.WaitForCompletionRef(ctx, ac.publicips.Client) if err != nil { - return err + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return &deleteFuture, err } - err = future.WaitForCompletionRef(ctx, ac.publicips.Client) + _, err = deleteFuture.Result(ac.publicips) + // if the operation completed, return a nil future. + return nil, err +} + +// IsDone returns true if the long-running operation has completed. +func (ac *AzureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (isDone bool, err error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.AzureClient.IsDone") + defer done() + + isDone, err = future.DoneWithContext(ctx, ac.publicips) if err != nil { - return err + return false, errors.Wrap(err, "failed checking if the operation was complete") + } + + return isDone, nil +} + +// Result fetches the result of a long-running operation future. +func (ac *AzureClient) Result(ctx context.Context, future azureautorest.FutureAPI, futureType string) (result interface{}, err error) { + _, _, done := tele.StartSpanWithLogger(ctx, "publicips.AzureClient.Result") + defer done() + + if future == nil { + return nil, errors.Errorf("cannot get result from nil future") + } + + switch futureType { + case infrav1.PutFuture: + // Marshal and Unmarshal the future to put it into the correct future type so we can access the Result function. + // Unfortunately the FutureAPI can't be casted directly to PublicIPAddressesCreateOrUpdateFuture because it is a azureautorest.Future, which doesn't implement the Result function. See PR #1686 for discussion on alternatives. + // It was converted back to a generic azureautorest.Future from the CAPZ infrav1.Future type stored in Status: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/converters/futures.go#L49. + var createFuture *network.PublicIPAddressesCreateOrUpdateFuture + jsonData, err := future.MarshalJSON() + if err != nil { + return nil, errors.Wrap(err, "failed to marshal future") + } + if err := json.Unmarshal(jsonData, &createFuture); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal future data") + } + return createFuture.Result(ac.publicips) + + case infrav1.DeleteFuture: + // Delete does not return a result inbound NAT rule + return nil, nil + + default: + return nil, errors.Errorf("unknown future type %q", futureType) } - _, err = future.Result(ac.publicips) - return err } diff --git a/azure/services/publicips/mock_publicips/client_mock.go b/azure/services/publicips/mock_publicips/client_mock.go index 663831425f3..aebd34f60da 100644 --- a/azure/services/publicips/mock_publicips/client_mock.go +++ b/azure/services/publicips/mock_publicips/client_mock.go @@ -19,77 +19,3 @@ limitations under the License. // Package mock_publicips is a generated GoMock package. package mock_publicips - -import ( - context "context" - reflect "reflect" - - network "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" - gomock "github.com/golang/mock/gomock" -) - -// MockClient is a mock of Client interface. -type MockClient struct { - ctrl *gomock.Controller - recorder *MockClientMockRecorder -} - -// MockClientMockRecorder is the mock recorder for MockClient. -type MockClientMockRecorder struct { - mock *MockClient -} - -// NewMockClient creates a new mock instance. -func NewMockClient(ctrl *gomock.Controller) *MockClient { - mock := &MockClient{ctrl: ctrl} - mock.recorder = &MockClientMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockClient) EXPECT() *MockClientMockRecorder { - return m.recorder -} - -// CreateOrUpdate mocks base method. -func (m *MockClient) CreateOrUpdate(arg0 context.Context, arg1, arg2 string, arg3 network.PublicIPAddress) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdate", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(error) - return ret0 -} - -// CreateOrUpdate indicates an expected call of CreateOrUpdate. -func (mr *MockClientMockRecorder) CreateOrUpdate(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdate", reflect.TypeOf((*MockClient)(nil).CreateOrUpdate), arg0, arg1, arg2, arg3) -} - -// Delete mocks base method. -func (m *MockClient) Delete(arg0 context.Context, arg1, arg2 string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 -} - -// Delete indicates an expected call of Delete. -func (mr *MockClientMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockClient)(nil).Delete), arg0, arg1, arg2) -} - -// Get mocks base method. -func (m *MockClient) Get(arg0 context.Context, arg1, arg2 string) (network.PublicIPAddress, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2) - ret0, _ := ret[0].(network.PublicIPAddress) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Get indicates an expected call of Get. -func (mr *MockClientMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockClient)(nil).Get), arg0, arg1, arg2) -} diff --git a/azure/services/publicips/mock_publicips/publicips_mock.go b/azure/services/publicips/mock_publicips/publicips_mock.go index 3e2c9c22195..f9cbb21716c 100644 --- a/azure/services/publicips/mock_publicips/publicips_mock.go +++ b/azure/services/publicips/mock_publicips/publicips_mock.go @@ -27,6 +27,7 @@ import ( gomock "github.com/golang/mock/gomock" v1beta1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" azure "sigs.k8s.io/cluster-api-provider-azure/azure" + v1beta10 "sigs.k8s.io/cluster-api/api/v1beta1" ) // MockPublicIPScope is a mock of PublicIPScope interface. @@ -178,6 +179,18 @@ func (mr *MockPublicIPScopeMockRecorder) ClusterName() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClusterName", reflect.TypeOf((*MockPublicIPScope)(nil).ClusterName)) } +// DeleteLongRunningOperationState mocks base method. +func (m *MockPublicIPScope) DeleteLongRunningOperationState(arg0, arg1 string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DeleteLongRunningOperationState", arg0, arg1) +} + +// DeleteLongRunningOperationState indicates an expected call of DeleteLongRunningOperationState. +func (mr *MockPublicIPScopeMockRecorder) DeleteLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLongRunningOperationState", reflect.TypeOf((*MockPublicIPScope)(nil).DeleteLongRunningOperationState), arg0, arg1) +} + // FailureDomains mocks base method. func (m *MockPublicIPScope) FailureDomains() []string { m.ctrl.T.Helper() @@ -192,6 +205,20 @@ func (mr *MockPublicIPScopeMockRecorder) FailureDomains() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FailureDomains", reflect.TypeOf((*MockPublicIPScope)(nil).FailureDomains)) } +// GetLongRunningOperationState mocks base method. +func (m *MockPublicIPScope) GetLongRunningOperationState(arg0, arg1 string) *v1beta1.Future { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetLongRunningOperationState", arg0, arg1) + ret0, _ := ret[0].(*v1beta1.Future) + return ret0 +} + +// GetLongRunningOperationState indicates an expected call of GetLongRunningOperationState. +func (mr *MockPublicIPScopeMockRecorder) GetLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLongRunningOperationState", reflect.TypeOf((*MockPublicIPScope)(nil).GetLongRunningOperationState), arg0, arg1) +} + // HashKey mocks base method. func (m *MockPublicIPScope) HashKey() string { m.ctrl.T.Helper() @@ -221,10 +248,10 @@ func (mr *MockPublicIPScopeMockRecorder) Location() *gomock.Call { } // PublicIPSpecs mocks base method. -func (m *MockPublicIPScope) PublicIPSpecs() []azure.PublicIPSpec { +func (m *MockPublicIPScope) PublicIPSpecs() []azure.ResourceSpecGetter { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "PublicIPSpecs") - ret0, _ := ret[0].([]azure.PublicIPSpec) + ret0, _ := ret[0].([]azure.ResourceSpecGetter) return ret0 } @@ -248,6 +275,18 @@ func (mr *MockPublicIPScopeMockRecorder) ResourceGroup() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockPublicIPScope)(nil).ResourceGroup)) } +// SetLongRunningOperationState mocks base method. +func (m *MockPublicIPScope) SetLongRunningOperationState(arg0 *v1beta1.Future) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetLongRunningOperationState", arg0) +} + +// SetLongRunningOperationState indicates an expected call of SetLongRunningOperationState. +func (mr *MockPublicIPScopeMockRecorder) SetLongRunningOperationState(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLongRunningOperationState", reflect.TypeOf((*MockPublicIPScope)(nil).SetLongRunningOperationState), arg0) +} + // SubscriptionID mocks base method. func (m *MockPublicIPScope) SubscriptionID() string { m.ctrl.T.Helper() @@ -275,3 +314,39 @@ func (mr *MockPublicIPScopeMockRecorder) TenantID() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockPublicIPScope)(nil).TenantID)) } + +// UpdateDeleteStatus mocks base method. +func (m *MockPublicIPScope) UpdateDeleteStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdateDeleteStatus", arg0, arg1, arg2) +} + +// UpdateDeleteStatus indicates an expected call of UpdateDeleteStatus. +func (mr *MockPublicIPScopeMockRecorder) UpdateDeleteStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateDeleteStatus", reflect.TypeOf((*MockPublicIPScope)(nil).UpdateDeleteStatus), arg0, arg1, arg2) +} + +// UpdatePatchStatus mocks base method. +func (m *MockPublicIPScope) UpdatePatchStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePatchStatus", arg0, arg1, arg2) +} + +// UpdatePatchStatus indicates an expected call of UpdatePatchStatus. +func (mr *MockPublicIPScopeMockRecorder) UpdatePatchStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePatchStatus", reflect.TypeOf((*MockPublicIPScope)(nil).UpdatePatchStatus), arg0, arg1, arg2) +} + +// UpdatePutStatus mocks base method. +func (m *MockPublicIPScope) UpdatePutStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePutStatus", arg0, arg1, arg2) +} + +// UpdatePutStatus indicates an expected call of UpdatePutStatus. +func (mr *MockPublicIPScopeMockRecorder) UpdatePutStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePutStatus", reflect.TypeOf((*MockPublicIPScope)(nil).UpdatePutStatus), arg0, arg1, arg2) +} diff --git a/azure/services/publicips/publicips.go b/azure/services/publicips/publicips.go index 0b282cb956a..d4c9bea8e45 100644 --- a/azure/services/publicips/publicips.go +++ b/azure/services/publicips/publicips.go @@ -18,14 +18,13 @@ package publicips import ( "context" - "strings" "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" - "github.com/Azure/go-autorest/autorest/to" "github.com/pkg/errors" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/converters" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) @@ -33,21 +32,26 @@ const serviceName = "publicips" // PublicIPScope defines the scope interface for a public IP service. type PublicIPScope interface { + azure.Authorizer + azure.AsyncStatusUpdater azure.ClusterDescriber - PublicIPSpecs() []azure.PublicIPSpec + PublicIPSpecs() []azure.ResourceSpecGetter } // Service provides operations on Azure resources. type Service struct { Scope PublicIPScope - Client + async.Reconciler + async.Getter } // New creates a new service. func New(scope PublicIPScope) *Service { + client := NewClient(scope) return &Service{ - Scope: scope, - Client: NewClient(scope), + Scope: scope, + Getter: client, + Reconciler: async.New(scope, client, client), } } @@ -58,58 +62,28 @@ func (s *Service) Name() string { // Reconcile gets/creates/updates a public ip. func (s *Service) Reconcile(ctx context.Context) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "publicips.Service.Reconcile") + ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.Service.Reconcile") defer done() - for _, ip := range s.Scope.PublicIPSpecs() { - log.V(2).Info("creating public IP", "public ip", ip.Name) - - // only set DNS properties if there is a DNS name specified - addressVersion := network.IPVersionIPv4 - if ip.IsIPv6 { - addressVersion = network.IPVersionIPv6 - } + specs := s.Scope.PublicIPSpecs() + if len(specs) == 0 { + return nil + } - // only set DNS properties if there is a DNS name specified - var dnsSettings *network.PublicIPAddressDNSSettings - if ip.DNSName != "" { - dnsSettings = &network.PublicIPAddressDNSSettings{ - DomainNameLabel: to.StringPtr(strings.Split(ip.DNSName, ".")[0]), - Fqdn: to.StringPtr(ip.DNSName), + // We go through the list of PublicIPSpecs to reconcile each one, independently of the result of the previous one. + // If multiple errors occur, we return the most pressing one. + // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error creating) -> operationNotDoneError (i.e. creating in progress) -> no error (i.e. created) + var result error + for _, publicIPSpec := range specs { + if _, err := s.CreateResource(ctx, publicIPSpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || result == nil { + result = err } } - - err := s.Client.CreateOrUpdate( - ctx, - s.Scope.ResourceGroup(), - ip.Name, - network.PublicIPAddress{ - Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{ - ClusterName: s.Scope.ClusterName(), - Lifecycle: infrav1.ResourceLifecycleOwned, - Name: to.StringPtr(ip.Name), - Additional: s.Scope.AdditionalTags(), - })), - Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, - Name: to.StringPtr(ip.Name), - Location: to.StringPtr(s.Scope.Location()), - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - PublicIPAddressVersion: addressVersion, - PublicIPAllocationMethod: network.IPAllocationMethodStatic, - DNSSettings: dnsSettings, - }, - Zones: to.StringSlicePtr(s.Scope.FailureDomains()), - }, - ) - - if err != nil { - return errors.Wrap(err, "cannot create public IP") - } - - log.V(2).Info("successfully created public IP", "public ip", ip.Name) } - return nil + s.Scope.UpdatePutStatus(infrav1.PublicIPsReadyCondition, serviceName, result) + return result } // Delete deletes the public IP with the provided scope. @@ -117,43 +91,60 @@ func (s *Service) Delete(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "publicips.Service.Delete") defer done() - for _, ip := range s.Scope.PublicIPSpecs() { - managed, err := s.isIPManaged(ctx, ip.Name) + specs := s.Scope.PublicIPSpecs() + if len(specs) == 0 { + return nil + } + + hasManagedPublicIPs := false + + // We go through the list of VnetPeeringSpecs to delete each one, independently of the result of the previous one. + // If multiple errors occur, we return the most pressing one. + // Order of precedence (highest -> lowest) is: error that is not an operationNotDoneError (i.e. error deleting) -> operationNotDoneError (i.e. deleting in progress) -> no error (i.e. deleted) + var result error + for _, publicIPSpec := range specs { + managed, err := s.isIPManaged(ctx, publicIPSpec) if err != nil && !azure.ResourceNotFound(err) { return errors.Wrap(err, "could not get public IP management state") } if !managed { - log.V(2).Info("Skipping IP deletion for unmanaged public IP", "public ip", ip.Name) + log.V(2).Info("Skipping IP deletion for unmanaged public IP", "public ip", publicIPSpec.ResourceName()) continue } - log.V(2).Info("deleting public IP", "public ip", ip.Name) - err = s.Client.Delete(ctx, s.Scope.ResourceGroup(), ip.Name) - if err != nil && azure.ResourceNotFound(err) { - // already deleted - continue - } - if err != nil { - return errors.Wrapf(err, "failed to delete public IP %s in resource group %s", ip.Name, s.Scope.ResourceGroup()) + log.V(2).Info("deleting public IP", "public ip", publicIPSpec.ResourceName()) + hasManagedPublicIPs = true + if err := s.DeleteResource(ctx, publicIPSpec, serviceName); err != nil { + if !azure.IsOperationNotDoneError(err) || result == nil { + result = err + } } - log.V(2).Info("deleted public IP", "public ip", ip.Name) + log.V(2).Info("deleted public IP", "public ip", publicIPSpec.ResourceName()) } - return nil + + if hasManagedPublicIPs { + s.Scope.UpdateDeleteStatus(infrav1.PublicIPsReadyCondition, serviceName, result) + } + + return result } // isIPManaged returns true if the IP has an owned tag with the cluster name as value, // meaning that the IP's lifecycle is managed. -func (s *Service) isIPManaged(ctx context.Context, ipName string) (bool, error) { - ctx, _, done := tele.StartSpanWithLogger(ctx, "publicips.Service.isIPManaged") - defer done() - - ip, err := s.Client.Get(ctx, s.Scope.ResourceGroup(), ipName) +func (s *Service) isIPManaged(ctx context.Context, spec azure.ResourceSpecGetter) (bool, error) { + result, err := s.Get(ctx, spec) if err != nil { return false, err } - tags := converters.MapToTags(ip.Tags) + + publicIP, ok := result.(network.PublicIPAddress) + if !ok { + return false, errors.Errorf("%T is not a network.PublicIPAddress", publicIP) + } + + tags := converters.MapToTags(publicIP.Tags) return tags.HasOwned(s.Scope.ClusterName()), nil } diff --git a/azure/services/publicips/publicips_test.go b/azure/services/publicips/publicips_test.go index e26dff5b6e3..e746d40a35e 100644 --- a/azure/services/publicips/publicips_test.go +++ b/azure/services/publicips/publicips_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips/mock_publicips" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -38,127 +39,113 @@ func init() { _ = clusterv1.AddToScheme(scheme.Scheme) } +var ( + fakePublicIPSpec1 = PublicIPSpec{ + Name: "my-publicip", + ResourceGroup: "my-rg", + DNSName: "fakedns.mydomain.io", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, + } + fakePublicIPSpec2 = PublicIPSpec{ + Name: "my-publicip-2", + ResourceGroup: "my-rg", + DNSName: "fakedns2-52959.uksouth.cloudapp.azure.com", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, + } + fakePublicIPSpec3 = PublicIPSpec{ + Name: "my-publicip-3", + ResourceGroup: "my-rg", + DNSName: "", + IsIPv6: false, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + }, + } + fakePublicIPSpecIpv6 = PublicIPSpec{ + Name: "my-publicip-ipv6", + ResourceGroup: "my-rg", + DNSName: "fakename.mydomain.io", + IsIPv6: true, + ClusterName: "my-cluster", + Location: "centralIndia", + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + AdditionalTags: infrav1.Tags{ + "Name": "my-publicip-ipv6", + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": "owned", + "foo": "bar", + }, + } + + fakeUnmanagedPublicIP = network.PublicIPAddress{ + Name: to.StringPtr("my-publicip-1"), + Tags: map[string]*string{ + "foo": to.StringPtr("bar"), + }, + } + fakeManagedPublicIP = network.PublicIPAddress{ + Name: to.StringPtr("my-publicip-2"), + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), + "foo": to.StringPtr("bar"), + }, + } + + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") +) + func TestReconcilePublicIP(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) + expect func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { - name: "can create public IPs", + name: "noop if no public IPs", + expectedError: "", + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{}) + }, + }, + { + name: "successfully create public IPs", expectedError: "", - expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) { - s.PublicIPSpecs().Return([]azure.PublicIPSpec{ - { - Name: "my-publicip", - DNSName: "fakedns.mydomain.io", - }, - { - Name: "my-publicip-2", - DNSName: "fakedns2-52959.uksouth.cloudapp.azure.com", - }, - { - Name: "my-publicip-3", - }, - { - Name: "my-publicip-ipv6", - IsIPv6: true, - DNSName: "fakename.mydomain.io", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.ClusterName().AnyTimes().Return("my-cluster") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Location().AnyTimes().Return("testlocation") - s.FailureDomains().AnyTimes().Return([]string{"1,2,3"}) - gomock.InOrder( - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-publicip", gomockinternal.DiffEq(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip"), - Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, - Location: to.StringPtr("testlocation"), - Tags: map[string]*string{ - "Name": to.StringPtr("my-publicip"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - }, - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - PublicIPAddressVersion: network.IPVersionIPv4, - PublicIPAllocationMethod: network.IPAllocationMethodStatic, - DNSSettings: &network.PublicIPAddressDNSSettings{ - DomainNameLabel: to.StringPtr("fakedns"), - Fqdn: to.StringPtr("fakedns.mydomain.io"), - }, - }, - Zones: to.StringSlicePtr([]string{"1,2,3"}), - })).Times(1), - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-publicip-2", gomockinternal.DiffEq(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip-2"), - Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, - Location: to.StringPtr("testlocation"), - Tags: map[string]*string{ - "Name": to.StringPtr("my-publicip-2"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - }, - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - PublicIPAddressVersion: network.IPVersionIPv4, - PublicIPAllocationMethod: network.IPAllocationMethodStatic, - DNSSettings: &network.PublicIPAddressDNSSettings{ - DomainNameLabel: to.StringPtr("fakedns2-52959"), - Fqdn: to.StringPtr("fakedns2-52959.uksouth.cloudapp.azure.com"), - }, - }, - Zones: to.StringSlicePtr([]string{"1,2,3"}), - })).Times(1), - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-publicip-3", gomockinternal.DiffEq(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip-3"), - Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, - Location: to.StringPtr("testlocation"), - Tags: map[string]*string{ - "Name": to.StringPtr("my-publicip-3"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - }, - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - PublicIPAddressVersion: network.IPVersionIPv4, - PublicIPAllocationMethod: network.IPAllocationMethodStatic, - }, - Zones: to.StringSlicePtr([]string{"1,2,3"}), - })).Times(1), - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-publicip-ipv6", gomockinternal.DiffEq(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip-ipv6"), - Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, - Location: to.StringPtr("testlocation"), - Tags: map[string]*string{ - "Name": to.StringPtr("my-publicip-ipv6"), - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - }, - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - PublicIPAddressVersion: network.IPVersionIPv6, - PublicIPAllocationMethod: network.IPAllocationMethodStatic, - DNSSettings: &network.PublicIPAddressDNSSettings{ - DomainNameLabel: to.StringPtr("fakename"), - Fqdn: to.StringPtr("fakename.mydomain.io"), - }, - }, - Zones: to.StringSlicePtr([]string{"1,2,3"}), - })).Times(1), - ) + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{&fakePublicIPSpec1, &fakePublicIPSpec2, &fakePublicIPSpec3, &fakePublicIPSpecIpv6}) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpec1, serviceName).Return(nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpec2, serviceName).Return(nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpec3, serviceName).Return(nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpecIpv6, serviceName).Return(nil, nil) + s.UpdatePutStatus(infrav1.PublicIPsReadyCondition, serviceName, nil) }, }, { name: "fail to create a public IP", - expectedError: "cannot create public IP: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) { - s.PublicIPSpecs().Return([]azure.PublicIPSpec{ - { - Name: "my-publicip", - DNSName: "fakedns.mydomain.io", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.ClusterName().AnyTimes().Return("my-cluster") - s.AdditionalTags().AnyTimes().Return(infrav1.Tags{}) - s.Location().AnyTimes().Return("testlocation") - s.FailureDomains().Times(1) - m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-publicip", gomock.AssignableToTypeOf(network.PublicIPAddress{})).Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + expectedError: internalError.Error(), + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{&fakePublicIPSpec1, &fakePublicIPSpec2, &fakePublicIPSpec3, &fakePublicIPSpecIpv6}) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpec1, serviceName).Return(nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpec2, serviceName).Return(nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpec3, serviceName).Return(nil, internalError) + r.CreateResource(gomockinternal.AContext(), &fakePublicIPSpecIpv6, serviceName).Return(nil, nil) + s.UpdatePutStatus(infrav1.PublicIPsReadyCondition, serviceName, internalError) }, }, } @@ -171,14 +158,17 @@ func TestReconcilePublicIP(t *testing.T) { t.Parallel() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() + scopeMock := mock_publicips.NewMockPublicIPScope(mockCtrl) - clientMock := mock_publicips.NewMockClient(mockCtrl) + getterMock := mock_async.NewMockGetter(mockCtrl) + reconcilerMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), getterMock.EXPECT(), reconcilerMock.EXPECT()) s := &Service{ - Scope: scopeMock, - Client: clientMock, + Scope: scopeMock, + Getter: getterMock, + Reconciler: reconcilerMock, } err := s.Reconcile(context.TODO()) @@ -196,115 +186,81 @@ func TestDeletePublicIP(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) + expect func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { - name: "successfully delete two existing public IP", + name: "noop if no public IPs", expectedError: "", - expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) { - s.PublicIPSpecs().Return([]azure.PublicIPSpec{ - { - Name: "my-publicip", - }, - { - Name: "my-publicip-2", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.ClusterName().AnyTimes().Return("my-cluster") - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip").Return(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip"), - Tags: map[string]*string{ - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - "foo": to.StringPtr("bar"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "my-publicip") - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip-2").Return(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip-2"), - Tags: map[string]*string{ - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - "foo": to.StringPtr("buzz"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "my-publicip-2") + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{}) }, }, { - name: "public ip already deleted", + name: "successfully delete managed public IPs and ignore unmanaged public IPs", expectedError: "", - expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) { - s.PublicIPSpecs().Return([]azure.PublicIPSpec{ - { - Name: "my-publicip", - }, - { - Name: "my-publicip-2", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.ClusterName().AnyTimes().Return("my-cluster") - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip").Return(network.PublicIPAddress{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip-2").Return(network.PublicIPAddress{ - Name: to.StringPtr("my-public-ip-2"), - Tags: map[string]*string{ - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - "foo": to.StringPtr("buzz"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "my-publicip-2") + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{&fakePublicIPSpec1, &fakePublicIPSpec2, &fakePublicIPSpec3, &fakePublicIPSpecIpv6}) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec1).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpec1, serviceName).Return(nil) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec2).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpec2, serviceName).Return(nil) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec3).Return(fakeUnmanagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + + m.Get(gomockinternal.AContext(), &fakePublicIPSpecIpv6).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpecIpv6, serviceName).Return(nil) + + s.UpdateDeleteStatus(infrav1.PublicIPsReadyCondition, serviceName, nil) }, }, { - name: "public ip deletion fails", - expectedError: "failed to delete public IP my-publicip in resource group my-rg: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) { - s.PublicIPSpecs().Return([]azure.PublicIPSpec{ - { - Name: "my-publicip", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.ClusterName().AnyTimes().Return("my-cluster") - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip").Return(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip"), - Tags: map[string]*string{ - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - "foo": to.StringPtr("bar"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "my-publicip"). - Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + name: "noop if no managed public IPs", + expectedError: "", + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{&fakePublicIPSpec1, &fakePublicIPSpec2, &fakePublicIPSpec3, &fakePublicIPSpecIpv6}) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec1).Return(fakeUnmanagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec2).Return(fakeUnmanagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec3).Return(fakeUnmanagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + + m.Get(gomockinternal.AContext(), &fakePublicIPSpecIpv6).Return(fakeUnmanagedPublicIP, nil) + s.ClusterName().Return("my-cluster") }, }, { - name: "skip unmanaged public ip deletion", - expectedError: "", - expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_publicips.MockClientMockRecorder) { - s.PublicIPSpecs().Return([]azure.PublicIPSpec{ - { - Name: "my-publicip", - }, - { - Name: "my-publicip-2", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - s.ClusterName().AnyTimes().Return("my-cluster") - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip").Return(network.PublicIPAddress{ - Name: to.StringPtr("my-public-ip"), - Tags: map[string]*string{ - "foo": to.StringPtr("bar"), - }, - }, nil) - m.Get(gomockinternal.AContext(), "my-rg", "my-publicip-2").Return(network.PublicIPAddress{ - Name: to.StringPtr("my-publicip-2"), - Tags: map[string]*string{ - "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), - "foo": to.StringPtr("buzz"), - }, - }, nil) - m.Delete(gomockinternal.AContext(), "my-rg", "my-publicip-2") + name: "fail to delete managed public IP", + expectedError: internalError.Error(), + expect: func(s *mock_publicips.MockPublicIPScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.PublicIPSpecs().Return([]azure.ResourceSpecGetter{&fakePublicIPSpec1, &fakePublicIPSpec2, &fakePublicIPSpec3, &fakePublicIPSpecIpv6}) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec1).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpec1, serviceName).Return(nil) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec2).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpec2, serviceName).Return(nil) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpec3).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpec3, serviceName).Return(internalError) + + m.Get(gomockinternal.AContext(), &fakePublicIPSpecIpv6).Return(fakeManagedPublicIP, nil) + s.ClusterName().Return("my-cluster") + r.DeleteResource(gomockinternal.AContext(), &fakePublicIPSpecIpv6, serviceName).Return(nil) + + s.UpdateDeleteStatus(infrav1.PublicIPsReadyCondition, serviceName, internalError) }, }, } @@ -317,14 +273,17 @@ func TestDeletePublicIP(t *testing.T) { t.Parallel() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() + scopeMock := mock_publicips.NewMockPublicIPScope(mockCtrl) - clientMock := mock_publicips.NewMockClient(mockCtrl) + getterMock := mock_async.NewMockGetter(mockCtrl) + reconcilerMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), getterMock.EXPECT(), reconcilerMock.EXPECT()) s := &Service{ - Scope: scopeMock, - Client: clientMock, + Scope: scopeMock, + Getter: getterMock, + Reconciler: reconcilerMock, } err := s.Delete(context.TODO()) diff --git a/azure/services/publicips/spec.go b/azure/services/publicips/spec.go new file mode 100644 index 00000000000..6cc548095e5 --- /dev/null +++ b/azure/services/publicips/spec.go @@ -0,0 +1,97 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package publicips + +import ( + "strings" + + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" + "github.com/Azure/go-autorest/autorest/to" + "github.com/pkg/errors" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure/converters" +) + +// PublicIPSpec defines the specification for a Public IP. +type PublicIPSpec struct { + Name string + ResourceGroup string + ClusterName string + DNSName string + IsIPv6 bool + Location string + FailureDomains []string + AdditionalTags infrav1.Tags +} + +// ResourceName returns the name of the public IP. +func (s *PublicIPSpec) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the resource group. +func (s *PublicIPSpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName is a no-op for public IPs. +func (s *PublicIPSpec) OwnerResourceName() string { + return "" +} + +// Parameters returns the parameters for the public IP. +func (s *PublicIPSpec) Parameters(existing interface{}) (params interface{}, err error) { + if existing != nil { + if _, ok := existing.(network.PublicIPAddress); !ok { + return nil, errors.Errorf("%T is not a network.PublicIPAddress", existing) + } + // public IP already exists + return nil, nil + } + + addressVersion := network.IPVersionIPv4 + if s.IsIPv6 { + addressVersion = network.IPVersionIPv6 + } + + // only set DNS properties if there is a DNS name specified + var dnsSettings *network.PublicIPAddressDNSSettings + if s.DNSName != "" { + dnsSettings = &network.PublicIPAddressDNSSettings{ + DomainNameLabel: to.StringPtr(strings.Split(s.DNSName, ".")[0]), + Fqdn: to.StringPtr(s.DNSName), + } + } + + return network.PublicIPAddress{ + Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{ + ClusterName: s.ClusterName, + Lifecycle: infrav1.ResourceLifecycleOwned, + Name: to.StringPtr(s.Name), + Additional: s.AdditionalTags, + })), + Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, + Name: to.StringPtr(s.Name), + Location: to.StringPtr(s.Location), + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: addressVersion, + PublicIPAllocationMethod: network.IPAllocationMethodStatic, + DNSSettings: dnsSettings, + }, + Zones: to.StringSlicePtr(s.FailureDomains), + }, nil +} diff --git a/azure/services/publicips/spec_test.go b/azure/services/publicips/spec_test.go new file mode 100644 index 00000000000..ee68a68d6ff --- /dev/null +++ b/azure/services/publicips/spec_test.go @@ -0,0 +1,166 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package publicips + +import ( + "reflect" + "testing" + + "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" + "github.com/Azure/go-autorest/autorest/to" + "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" +) + +var ( + fakePublicIPSpecWithDNS = PublicIPSpec{ + Name: "my-publicip", + DNSName: "fakedns.mydomain.io", + Location: "centralIndia", + ClusterName: "my-cluster", + AdditionalTags: infrav1.Tags{ + "foo": "bar", + }, + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + } + + fakePublicIPSpecWithoutDNS = PublicIPSpec{ + Name: "my-publicip-2", + Location: "centralIndia", + ClusterName: "my-cluster", + AdditionalTags: infrav1.Tags{ + "foo": "bar", + }, + FailureDomains: []string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}, + } + + fakePublicIPWithDNS = network.PublicIPAddress{ + Name: to.StringPtr("my-publicip"), + Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, + Location: to.StringPtr("centralIndia"), + Tags: map[string]*string{ + "Name": to.StringPtr("my-publicip"), + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), + "foo": to.StringPtr("bar"), + }, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPVersionIPv4, + PublicIPAllocationMethod: network.IPAllocationMethodStatic, + DNSSettings: &network.PublicIPAddressDNSSettings{ + DomainNameLabel: to.StringPtr("fakedns"), + Fqdn: to.StringPtr("fakedns.mydomain.io"), + }, + }, + Zones: to.StringSlicePtr([]string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}), + } + + fakePublicIPWithoutDNS = network.PublicIPAddress{ + Name: to.StringPtr("my-publicip-2"), + Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, + Location: to.StringPtr("centralIndia"), + Tags: map[string]*string{ + "Name": to.StringPtr("my-publicip-2"), + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), + "foo": to.StringPtr("bar"), + }, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPVersionIPv4, + PublicIPAllocationMethod: network.IPAllocationMethodStatic, + }, + Zones: to.StringSlicePtr([]string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}), + } + + fakePublicIPIpv6 = network.PublicIPAddress{ + Name: to.StringPtr("my-publicip-ipv6"), + Sku: &network.PublicIPAddressSku{Name: network.PublicIPAddressSkuNameStandard}, + Location: to.StringPtr("centralIndia"), + Tags: map[string]*string{ + "Name": to.StringPtr("my-publicip-ipv6"), + "sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": to.StringPtr("owned"), + "foo": to.StringPtr("bar"), + }, + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + PublicIPAddressVersion: network.IPVersionIPv6, + PublicIPAllocationMethod: network.IPAllocationMethodStatic, + DNSSettings: &network.PublicIPAddressDNSSettings{ + DomainNameLabel: to.StringPtr("fakename"), + Fqdn: to.StringPtr("fakename.mydomain.io"), + }, + }, + Zones: to.StringSlicePtr([]string{"failure-domain-id-1", "failure-domain-id-2", "failure-domain-id-3"}), + } +) + +func TestParameters(t *testing.T) { + testCases := []struct { + name string + existing interface{} + spec PublicIPSpec + expected interface{} + expectedError string + }{ + { + name: "noop if public IP exists", + existing: fakePublicIPWithDNS, + spec: fakePublicIPSpecWithDNS, + expected: nil, + expectedError: "", + }, + { + name: "public ipv4 address with dns", + existing: nil, + spec: fakePublicIPSpecWithDNS, + expected: fakePublicIPWithDNS, + expectedError: "", + }, + { + name: "public ipv4 address without dns", + existing: nil, + spec: fakePublicIPSpecWithoutDNS, + expected: fakePublicIPWithoutDNS, + expectedError: "", + }, + { + name: "public ipv6 address with dns", + existing: nil, + spec: fakePublicIPSpecIpv6, // In publicips_test.go + expected: fakePublicIPIpv6, + expectedError: "", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + t.Parallel() + + result, err := tc.spec.Parameters(tc.existing) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + + if !reflect.DeepEqual(result, tc.expected) { + t.Errorf("Diff between expected result and actual result:\n%s", cmp.Diff(tc.expected, result)) + } + }) + } +} diff --git a/azure/services/virtualmachines/virtualmachines.go b/azure/services/virtualmachines/virtualmachines.go index 79b0ba5d5d8..f2240cb92a1 100644 --- a/azure/services/virtualmachines/virtualmachines.go +++ b/azure/services/virtualmachines/virtualmachines.go @@ -53,7 +53,7 @@ type Service struct { Scope VMScope async.Reconciler interfacesGetter async.Getter - publicIPsClient publicips.Client + publicIPsGetter async.Getter } // New creates a new service. @@ -62,7 +62,7 @@ func New(scope VMScope) *Service { return &Service{ Scope: scope, interfacesGetter: networkinterfaces.NewClient(scope), - publicIPsClient: publicips.NewClient(scope), + publicIPsGetter: publicips.NewClient(scope), Reconciler: async.New(scope, Client, Client), } } @@ -207,10 +207,19 @@ func (s *Service) getPublicIPAddress(ctx context.Context, publicIPAddressName st defer done() retAddress := corev1.NodeAddress{} - publicIP, err := s.publicIPsClient.Get(ctx, rgName, publicIPAddressName) + result, err := s.publicIPsGetter.Get(ctx, &publicips.PublicIPSpec{ + Name: publicIPAddressName, + ResourceGroup: rgName, + }) if err != nil { return retAddress, err } + + publicIP, ok := result.(network.PublicIPAddress) + if !ok { + return retAddress, errors.Errorf("%T is not a network.PublicIPAddress", result) + } + retAddress.Type = corev1.NodeExternalIP retAddress.Address = to.String(publicIP.IPAddress) diff --git a/azure/services/virtualmachines/virtualmachines_test.go b/azure/services/virtualmachines/virtualmachines_test.go index d3bb10a9d74..66e3d9a045c 100644 --- a/azure/services/virtualmachines/virtualmachines_test.go +++ b/azure/services/virtualmachines/virtualmachines_test.go @@ -31,7 +31,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips/mock_publicips" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines/mock_virtualmachines" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) @@ -84,6 +84,10 @@ var ( }, }, } + fakePublicIPSpec = publicips.PublicIPSpec{ + Name: "pip-1", + ResourceGroup: "test-group", + } fakePublicIPs = network.PublicIPAddress{ PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ IPAddress: to.StringPtr("10.0.0.6"), @@ -110,19 +114,19 @@ func TestReconcileVM(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) + expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "noop if no vm spec is found", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(nil) }, }, { name: "create vm succeeds", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(&fakeVMSpec) r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) @@ -130,7 +134,7 @@ func TestReconcileVM(t *testing.T) { s.SetProviderID("azure://test-vm-id") s.SetAnnotation("cluster-api-provider-azure", "true") mnic.Get(gomockinternal.AContext(), &fakeNetworkInterfaceGetterSpec).Return(fakeNetworkInterface, nil) - mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(fakePublicIPs, nil) + mpip.Get(gomockinternal.AContext(), &fakePublicIPSpec).Return(fakePublicIPs, nil) s.SetAddresses(fakeNodeAddresses) s.SetVMState(infrav1.Succeeded) }, @@ -138,7 +142,7 @@ func TestReconcileVM(t *testing.T) { { name: "creating vm fails", expectedError: "#: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(&fakeVMSpec) r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil, internalError) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, internalError) @@ -148,7 +152,7 @@ func TestReconcileVM(t *testing.T) { { name: "create vm succeeds but failed to get network interfaces", expectedError: "failed to fetch VM addresses: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(&fakeVMSpec) r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) @@ -161,7 +165,7 @@ func TestReconcileVM(t *testing.T) { { name: "create vm succeeds but failed to get public IPs", expectedError: "failed to fetch VM addresses: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_async.MockGetterMockRecorder, mpip *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.VMSpec().Return(&fakeVMSpec) r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) @@ -169,7 +173,7 @@ func TestReconcileVM(t *testing.T) { s.SetProviderID("azure://test-vm-id") s.SetAnnotation("cluster-api-provider-azure", "true") mnic.Get(gomockinternal.AContext(), &fakeNetworkInterfaceGetterSpec).Return(fakeNetworkInterface, nil) - mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(network.PublicIPAddress{}, internalError) + mpip.Get(gomockinternal.AContext(), &fakePublicIPSpec).Return(network.PublicIPAddress{}, internalError) }, }, } @@ -184,7 +188,7 @@ func TestReconcileVM(t *testing.T) { scopeMock := mock_virtualmachines.NewMockVMScope(mockCtrl) interfaceMock := mock_async.NewMockGetter(mockCtrl) - publicIPMock := mock_publicips.NewMockClient(mockCtrl) + publicIPMock := mock_async.NewMockGetter(mockCtrl) asyncMock := mock_async.NewMockReconciler(mockCtrl) tc.expect(scopeMock.EXPECT(), interfaceMock.EXPECT(), publicIPMock.EXPECT(), asyncMock.EXPECT()) @@ -192,7 +196,7 @@ func TestReconcileVM(t *testing.T) { s := &Service{ Scope: scopeMock, interfacesGetter: interfaceMock, - publicIPsClient: publicIPMock, + publicIPsGetter: publicIPMock, Reconciler: asyncMock, } diff --git a/azure/types.go b/azure/types.go index 3e0ad04199e..0a92c1e78d6 100644 --- a/azure/types.go +++ b/azure/types.go @@ -23,13 +23,6 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" ) -// PublicIPSpec defines the specification for a Public IP. -type PublicIPSpec struct { - Name string - DNSName string - IsIPv6 bool -} - // RoleAssignmentSpec defines the specification for a Role Assignment. type RoleAssignmentSpec struct { MachineName string