From 51f5ec40d891aab771b70ec510998e99664515aa Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 3 Dec 2025 13:34:54 -0500 Subject: [PATCH] fix(controllers): only warn about SP credentials when actually using Service Principal The VMIdentityNone warning was incorrectly shown for clusters using WorkloadIdentity, UserAssignedMSI, or other non-SP authentication methods. This caused confusing warnings about "Service Principal credentials being written to disk" when no such credentials exist. Add isUsingSPCredentials helper to check the AzureClusterIdentity type before emitting the warning. Only ServicePrincipal, ManualServicePrincipal, and ServicePrincipalCertificate identity types now trigger the warning. Signed-off-by: Bryan Cox --- controllers/azurejson_machine_controller.go | 2 +- .../azurejson_machinepool_controller.go | 6 +- .../azurejson_machinetemplate_controller.go | 2 +- controllers/helpers.go | 40 +++ controllers/helpers_test.go | 248 ++++++++++++++++++ 5 files changed, 294 insertions(+), 4 deletions(-) diff --git a/controllers/azurejson_machine_controller.go b/controllers/azurejson_machine_controller.go index 7df5345ca6a..c510364911c 100644 --- a/controllers/azurejson_machine_controller.go +++ b/controllers/azurejson_machine_controller.go @@ -235,7 +235,7 @@ func (r *AzureJSONMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req } } - if azureMachine.Spec.Identity == infrav1.VMIdentityNone { + if azureMachine.Spec.Identity == infrav1.VMIdentityNone && isUsingSPCredentials(ctx, r.Client, azureCluster) { log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning)) r.Recorder.Eventf(azureMachine, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning) } diff --git a/controllers/azurejson_machinepool_controller.go b/controllers/azurejson_machinepool_controller.go index 3cf418680ba..27d3ef24b30 100644 --- a/controllers/azurejson_machinepool_controller.go +++ b/controllers/azurejson_machinepool_controller.go @@ -177,8 +177,10 @@ func (r *AzureJSONMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl } if azureMachinePool.Spec.Identity == infrav1.VMIdentityNone { - log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning)) - r.Recorder.Eventf(azureMachinePool, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning) + if azureCluster := getAzureClusterFromCluster(ctx, r.Client, cluster); azureCluster != nil && isUsingSPCredentials(ctx, r.Client, azureCluster) { + log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning)) + r.Recorder.Eventf(azureMachinePool, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning) + } } newSecret, err := GetCloudProviderSecret( diff --git a/controllers/azurejson_machinetemplate_controller.go b/controllers/azurejson_machinetemplate_controller.go index 2c1e4184bec..c629f4d55a3 100644 --- a/controllers/azurejson_machinetemplate_controller.go +++ b/controllers/azurejson_machinetemplate_controller.go @@ -195,7 +195,7 @@ func (r *AzureJSONTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - if azureMachineTemplate.Spec.Template.Spec.Identity == infrav1.VMIdentityNone { + if azureMachineTemplate.Spec.Template.Spec.Identity == infrav1.VMIdentityNone && isUsingSPCredentials(ctx, r.Client, azureCluster) { log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning)) r.Recorder.Eventf(azureMachineTemplate, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning) } diff --git a/controllers/helpers.go b/controllers/helpers.go index bfdadf3b0f8..e8001772cd0 100644 --- a/controllers/helpers.go +++ b/controllers/helpers.go @@ -1152,3 +1152,43 @@ func RemoveBlockMoveAnnotation(obj metav1.Object) { delete(azClusterAnnotations, clusterctlv1.BlockMoveAnnotation) obj.SetAnnotations(azClusterAnnotations) } + +// getAzureClusterFromCluster returns the AzureCluster for a Cluster if the infraRef is an AzureCluster. +// Returns nil if the infraRef is not an AzureCluster or if the AzureCluster cannot be fetched. +func getAzureClusterFromCluster(ctx context.Context, c client.Client, cluster *clusterv1.Cluster) *infrav1.AzureCluster { + if cluster.Spec.InfrastructureRef == nil || cluster.Spec.InfrastructureRef.Kind != infrav1.AzureClusterKind { + return nil + } + azureCluster := &infrav1.AzureCluster{} + key := client.ObjectKey{ + Namespace: cluster.Spec.InfrastructureRef.Namespace, + Name: cluster.Spec.InfrastructureRef.Name, + } + if err := c.Get(ctx, key, azureCluster); err != nil { + return nil + } + return azureCluster +} + +// isUsingSPCredentials checks if the cluster is using Service Principal credentials. +func isUsingSPCredentials(ctx context.Context, c client.Client, azureCluster *infrav1.AzureCluster) bool { + if azureCluster.Spec.IdentityRef == nil { + return false + } + identity := &infrav1.AzureClusterIdentity{} + key := client.ObjectKey{ + Name: azureCluster.Spec.IdentityRef.Name, + Namespace: azureCluster.Spec.IdentityRef.Namespace, + } + if key.Namespace == "" { + key.Namespace = azureCluster.Namespace + } + if err := c.Get(ctx, key, identity); err != nil { + return false + } + switch identity.Spec.Type { + case infrav1.ServicePrincipal, infrav1.ManualServicePrincipal, infrav1.ServicePrincipalCertificate: + return true + } + return false +} diff --git a/controllers/helpers_test.go b/controllers/helpers_test.go index 0e37e74f21c..7a7bf60652d 100644 --- a/controllers/helpers_test.go +++ b/controllers/helpers_test.go @@ -1655,3 +1655,251 @@ func TestRemoveBlockMoveAnnotation(t *testing.T) { }) } } + +func TestIsUsingSPCredentials(t *testing.T) { + tests := []struct { + name string + azureCluster *infrav1.AzureCluster + identity *infrav1.AzureClusterIdentity + expected bool + }{ + { + name: "ServicePrincipal returns true", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ServicePrincipal}, + }, + expected: true, + }, + { + name: "ManualServicePrincipal returns true", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ManualServicePrincipal}, + }, + expected: true, + }, + { + name: "ServicePrincipalCertificate returns true", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ServicePrincipalCertificate}, + }, + expected: true, + }, + { + name: "WorkloadIdentity returns false", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.WorkloadIdentity}, + }, + expected: false, + }, + { + name: "UserAssignedMSI returns false", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.UserAssignedMSI}, + }, + expected: false, + }, + { + name: "UserAssignedIdentityCredential returns false", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.UserAssignedIdentityCredential}, + }, + expected: false, + }, + { + name: "nil IdentityRef returns false", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{}, + }, + identity: nil, + expected: false, + }, + { + name: "empty IdentityRef namespace falls back to cluster namespace", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "my-namespace"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: ""}, + }, + }, + }, + identity: &infrav1.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "my-namespace"}, + Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ServicePrincipal}, + }, + expected: true, + }, + { + name: "identity not found returns false", + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: infrav1.AzureClusterSpec{ + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + IdentityRef: &corev1.ObjectReference{Name: "missing-identity", Namespace: "default"}, + }, + }, + }, + identity: nil, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + scheme := setupScheme(g) + + var initObjects []runtime.Object + if tc.identity != nil { + initObjects = append(initObjects, tc.identity) + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(initObjects...).Build() + result := isUsingSPCredentials(t.Context(), fakeClient, tc.azureCluster) + g.Expect(result).To(Equal(tc.expected)) + }) + } +} + +func TestGetAzureClusterFromCluster(t *testing.T) { + tests := []struct { + name string + cluster *clusterv1.Cluster + azureCluster *infrav1.AzureCluster + expectNil bool + }{ + { + name: "returns AzureCluster when infraRef is AzureCluster", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + Kind: infrav1.AzureClusterKind, + Name: "azure-cluster", + Namespace: "default", + }, + }, + }, + azureCluster: &infrav1.AzureCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "azure-cluster", Namespace: "default"}, + }, + expectNil: false, + }, + { + name: "returns nil when infraRef is nil", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: clusterv1.ClusterSpec{}, + }, + azureCluster: nil, + expectNil: true, + }, + { + name: "returns nil when infraRef is AzureManagedCluster", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + Kind: infrav1.AzureManagedClusterKind, + Name: "azure-managed-cluster", + Namespace: "default", + }, + }, + }, + azureCluster: nil, + expectNil: true, + }, + { + name: "returns nil when AzureCluster not found", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"}, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + Kind: infrav1.AzureClusterKind, + Name: "missing-cluster", + Namespace: "default", + }, + }, + }, + azureCluster: nil, + expectNil: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + scheme := setupScheme(g) + + var initObjects []runtime.Object + if tc.azureCluster != nil { + initObjects = append(initObjects, tc.azureCluster) + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(initObjects...).Build() + result := getAzureClusterFromCluster(t.Context(), fakeClient, tc.cluster) + + if tc.expectNil { + g.Expect(result).To(BeNil()) + } else { + g.Expect(result).NotTo(BeNil()) + g.Expect(result.Name).To(Equal(tc.azureCluster.Name)) + } + }) + } +}