Merge pull request #3362 from willie-yao/cherry-pick-3322-to-release-1.8

[release-1.8] Fetch AzureCluster name from OwnerCluster instead of assuming ClusterName = AzureCluster.Name
kubernetes-sigs · Mar 24, 2023 · 5af14ff · 5af14ff
2 parents dcd004c + 3dcdc99
commit 5af14ff
Show file tree

Hide file tree

Showing 5 changed files with 160 additions and 31 deletions.
diff --git a/api/v1beta1/azuremachine_default.go b/api/v1beta1/azuremachine_default.go
@@ -174,29 +174,51 @@ func (s *AzureMachineSpec) SetNetworkInterfacesDefaults() {
 	}
 }
 
-// GetSubscriptionID returns the subscription ID for the given cluster name and namespace.
-func GetSubscriptionID(cli client.Client, clusterName string, namespace string, maxAttempts int) (string, error) {
+// GetOwnerAzureClusterNameAndNamespace returns the owner azure cluster's name and namespace for the given cluster name and namespace.
+func GetOwnerAzureClusterNameAndNamespace(cli client.Client, clusterName string, namespace string, maxAttempts int) (azureClusterName string, azureClusterNamespace string, err error) {
 	ctx := context.Background()
 
-	ownerCluster := &AzureCluster{}
+	ownerCluster := &clusterv1.Cluster{}
 	key := client.ObjectKey{
 		Namespace: namespace,
 		Name:      clusterName,
 	}
 
 	for i := 1; ; i++ {
-		err := cli.Get(ctx, key, ownerCluster)
-		if err != nil {
+		if err := cli.Get(ctx, key, ownerCluster); err != nil {
 			if i > maxAttempts {
-				return "", errors.Wrapf(err, "failed to find owner cluster for %s/%s", namespace, clusterName)
+				return "", "", errors.Wrapf(err, "failed to find owner cluster for %s/%s", namespace, clusterName)
+			}
+			time.Sleep(1 * time.Second)
+			continue
+		}
+		break
+	}
+
+	return ownerCluster.Spec.InfrastructureRef.Name, ownerCluster.Spec.InfrastructureRef.Namespace, nil
+}
+
+// GetSubscriptionID returns the subscription ID for the AzureCluster given the cluster name and namespace.
+func GetSubscriptionID(cli client.Client, ownerAzureClusterName string, ownerAzureClusterNamespace string, maxAttempts int) (string, error) {
+	ctx := context.Background()
+
+	ownerAzureCluster := &AzureCluster{}
+	key := client.ObjectKey{
+		Namespace: ownerAzureClusterNamespace,
+		Name:      ownerAzureClusterName,
+	}
+	for i := 1; ; i++ {
+		if err := cli.Get(ctx, key, ownerAzureCluster); err != nil {
+			if i >= maxAttempts {
+				return "", errors.Wrapf(err, "failed to find AzureCluster for owner cluster %s/%s", ownerAzureClusterNamespace, ownerAzureClusterName)
 			}
 			time.Sleep(1 * time.Second)
 			continue
 		}
 		break
 	}
 
-	return ownerCluster.Spec.SubscriptionID, nil
+	return ownerAzureCluster.Spec.SubscriptionID, nil
 }
 
 // SetDefaults sets to the defaults for the AzureMachineSpec.
@@ -211,7 +233,12 @@ func (m *AzureMachine) SetDefaults(client client.Client) {
 		ctrl.Log.WithName("SetDefault").Error(errors.Errorf("failed to fetch owner ClusterName for AzureMachine %s/%s", m.Namespace, m.Name), "failed to fetch ClusterName")
 	}
 
-	subscriptionID, err := GetSubscriptionID(client, clusterName, m.Namespace, 5)
+	ownerAzureClusterName, ownerAzureClusterNamespace, err := GetOwnerAzureClusterNameAndNamespace(client, clusterName, m.Namespace, 5)
+	if err != nil {
+		ctrl.Log.WithName("SetDefault").Error(err, "failed to fetch owner cluster for AzureMachine %s/%s", m.Namespace, m.Name)
+	}
+
+	subscriptionID, err := GetSubscriptionID(client, ownerAzureClusterName, ownerAzureClusterNamespace, 5)
 	if err != nil {
 		ctrl.Log.WithName("SetDefault").Error(err, "failed to fetch subscription ID for AzureMachine %s/%s", m.Namespace, m.Name)
 	}

diff --git a/api/v1beta1/azuremachine_default_test.go b/api/v1beta1/azuremachine_default_test.go
@@ -26,7 +26,9 @@ import (
 	"github.com/google/uuid"
 	. "github.com/onsi/gomega"
 	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/utils/pointer"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -399,39 +401,101 @@ func TestAzureMachineSpec_SetNetworkInterfacesDefaults(t *testing.T) {
 	}
 }
 
+func TestAzureMachineSpec_GetOwnerCluster(t *testing.T) {
+	tests := []struct {
+		name            string
+		maxAttempts     int
+		wantedName      string
+		wantedNamespace string
+		wantErr         bool
+	}{
+		{
+			name:            "ownerCluster is returned",
+			maxAttempts:     1,
+			wantedName:      "test-cluster",
+			wantedNamespace: "default",
+			wantErr:         false,
+		},
+		{
+			name:            "ownerCluster is returned after 2 attempts",
+			maxAttempts:     2,
+			wantedName:      "test-cluster",
+			wantedNamespace: "default",
+			wantErr:         false,
+		},
+		{
+			name:            "ownerCluster is not returned after 5 attempts",
+			maxAttempts:     5,
+			wantedName:      "test-cluster",
+			wantedNamespace: "default",
+			wantErr:         true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			client := mockClient{ReturnError: tc.wantErr}
+			name, namespace, err := GetOwnerAzureClusterNameAndNamespace(client, "test-cluster", "default", tc.maxAttempts)
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(name).To(Equal(tc.wantedName))
+				g.Expect(namespace).To(Equal(tc.wantedNamespace))
+			}
+		})
+	}
+}
+
 func TestAzureMachineSpec_GetSubscriptionID(t *testing.T) {
 	g := NewWithT(t)
 
 	tests := []struct {
-		name        string
-		maxAttempts int
-		want        string
-		wantErr     bool
+		name                       string
+		maxAttempts                int
+		ownerAzureClusterName      string
+		ownerAzureClusterNamespace string
+		want                       string
+		wantErr                    bool
 	}{
 		{
-			name:        "subscription ID is returned",
-			maxAttempts: 1,
-			want:        "test-subscription-id",
-			wantErr:     false,
+			name:                  "empty owner cluster name returns error",
+			maxAttempts:           1,
+			ownerAzureClusterName: "",
+			want:                  "test-subscription-id",
+			wantErr:               true,
 		},
 		{
-			name:        "subscription ID is returned after 2 attempts",
-			maxAttempts: 2,
-			want:        "test-subscription-id",
-			wantErr:     false,
+			name:                       "subscription ID is returned",
+			maxAttempts:                1,
+			ownerAzureClusterName:      "test-cluster",
+			ownerAzureClusterNamespace: "default",
+			want:                       "test-subscription-id",
+			wantErr:                    false,
 		},
 		{
-			name:        "subscription ID is not returned after 5 attempts",
-			maxAttempts: 5,
-			want:        "",
-			wantErr:     true,
+			name:                       "subscription ID is returned after 2 attempts",
+			maxAttempts:                2,
+			ownerAzureClusterName:      "test-cluster",
+			ownerAzureClusterNamespace: "default",
+			want:                       "test-subscription-id",
+			wantErr:                    false,
+		},
+		{
+			name:                       "subscription ID is not returned after 5 attempts",
+			maxAttempts:                5,
+			ownerAzureClusterName:      "test-cluster",
+			ownerAzureClusterNamespace: "default",
+			want:                       "",
+			wantErr:                    true,
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			client := mockClient{ReturnError: tc.wantErr}
-			result, err := GetSubscriptionID(client, "test-cluster", "default", tc.maxAttempts)
+			result, err := GetSubscriptionID(client, tc.ownerAzureClusterName, tc.ownerAzureClusterNamespace, tc.maxAttempts)
 			if tc.wantErr {
 				g.Expect(err).To(HaveOccurred())
 			} else {
@@ -451,9 +515,19 @@ func (m mockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Ob
 	if m.ReturnError {
 		return errors.New("AzureCluster not found: failed to find owner cluster for test-cluster")
 	}
-	ac := &AzureCluster{}
-	ac.Spec.SubscriptionID = "test-subscription-id"
-	obj.(*AzureCluster).Spec.SubscriptionID = ac.Spec.SubscriptionID
+	// Check if we're calling Get on an AzureCluster or a Cluster
+	switch obj := obj.(type) {
+	case *AzureCluster:
+		obj.Spec.SubscriptionID = "test-subscription-id"
+	case *clusterv1.Cluster:
+		obj.Spec.InfrastructureRef = &corev1.ObjectReference{
+			Kind:      "AzureCluster",
+			Name:      "test-cluster",
+			Namespace: "default",
+		}
+	default:
+		return errors.New("unexpected object type")
+	}
 
 	return nil
 }

diff --git a/api/v1beta1/azuremachine_webhook_test.go b/api/v1beta1/azuremachine_webhook_test.go
@@ -22,8 +22,11 @@ import (
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute"
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/utils/pointer"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -712,7 +715,17 @@ type mockDefaultClient struct {
 }
 
 func (m mockDefaultClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	obj.(*AzureCluster).Spec.SubscriptionID = m.SubscriptionID
+	switch obj := obj.(type) {
+	case *AzureCluster:
+		obj.Spec.SubscriptionID = m.SubscriptionID
+	case *clusterv1.Cluster:
+		obj.Spec.InfrastructureRef = &corev1.ObjectReference{
+			Kind: "AzureCluster",
+			Name: "test-cluster",
+		}
+	default:
+		return errors.New("invalid object type")
+	}
 	return nil
 }
 

diff --git a/exp/api/v1beta1/azuremachinepool_default.go b/exp/api/v1beta1/azuremachinepool_default.go
@@ -40,7 +40,12 @@ func (amp *AzureMachinePool) SetDefaults(client client.Client) {
 		ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "findParentMachinePool failed")
 	}
 
-	subscriptionID, err := infrav1.GetSubscriptionID(client, machinePool.Spec.ClusterName, machinePool.Namespace, 5)
+	ownerAzureClusterName, ownerAzureClusterNamespace, err := infrav1.GetOwnerAzureClusterNameAndNamespace(client, machinePool.Spec.ClusterName, machinePool.Namespace, 5)
+	if err != nil {
+		ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "failed to get owner cluster")
+	}
+
+	subscriptionID, err := infrav1.GetSubscriptionID(client, ownerAzureClusterName, ownerAzureClusterNamespace, 5)
 	if err != nil {
 		ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "getSubscriptionID failed")
 	}

diff --git a/exp/api/v1beta1/azuremachinepool_webhook_test.go b/exp/api/v1beta1/azuremachinepool_webhook_test.go
@@ -261,7 +261,17 @@ type mockDefaultClient struct {
 }
 
 func (m mockDefaultClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	obj.(*infrav1.AzureCluster).Spec.SubscriptionID = m.SubscriptionID
+	switch obj := obj.(type) {
+	case *infrav1.AzureCluster:
+		obj.Spec.SubscriptionID = m.SubscriptionID
+	case *clusterv1.Cluster:
+		obj.Spec.InfrastructureRef = &corev1.ObjectReference{
+			Kind: "AzureCluster",
+			Name: "test-cluster",
+		}
+	default:
+		return errors.New("invalid object type")
+	}
 	return nil
 }