Skip to content

Commit 51f5ec4

Browse files
bryan-coxbryan-cox
committed
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 <[email protected]>
1 parent e695d87 commit 51f5ec4

File tree

5 files changed

+294
-4
lines changed

5 files changed

+294
-4
lines changed

‎controllers/azurejson_machine_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func (r *AzureJSONMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req
235235
}
236236
}
237237

238-
if azureMachine.Spec.Identity == infrav1.VMIdentityNone {
238+
if azureMachine.Spec.Identity == infrav1.VMIdentityNone && isUsingSPCredentials(ctx, r.Client, azureCluster) {
239239
log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning))
240240
r.Recorder.Eventf(azureMachine, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning)
241241
}

‎controllers/azurejson_machinepool_controller.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,10 @@ func (r *AzureJSONMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl
177177
}
178178

179179
if azureMachinePool.Spec.Identity == infrav1.VMIdentityNone {
180-
log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning))
181-
r.Recorder.Eventf(azureMachinePool, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning)
180+
if azureCluster := getAzureClusterFromCluster(ctx, r.Client, cluster); azureCluster != nil && isUsingSPCredentials(ctx, r.Client, azureCluster) {
181+
log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning))
182+
r.Recorder.Eventf(azureMachinePool, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning)
183+
}
182184
}
183185

184186
newSecret, err := GetCloudProviderSecret(

‎controllers/azurejson_machinetemplate_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func (r *AzureJSONTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Re
195195
}
196196
}
197197

198-
if azureMachineTemplate.Spec.Template.Spec.Identity == infrav1.VMIdentityNone {
198+
if azureMachineTemplate.Spec.Template.Spec.Identity == infrav1.VMIdentityNone && isUsingSPCredentials(ctx, r.Client, azureCluster) {
199199
log.Info(fmt.Sprintf("WARNING, %s", spIdentityWarning))
200200
r.Recorder.Eventf(azureMachineTemplate, corev1.EventTypeWarning, "VMIdentityNone", spIdentityWarning)
201201
}

‎controllers/helpers.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,3 +1152,43 @@ func RemoveBlockMoveAnnotation(obj metav1.Object) {
11521152
delete(azClusterAnnotations, clusterctlv1.BlockMoveAnnotation)
11531153
obj.SetAnnotations(azClusterAnnotations)
11541154
}
1155+
1156+
// getAzureClusterFromCluster returns the AzureCluster for a Cluster if the infraRef is an AzureCluster.
1157+
// Returns nil if the infraRef is not an AzureCluster or if the AzureCluster cannot be fetched.
1158+
func getAzureClusterFromCluster(ctx context.Context, c client.Client, cluster *clusterv1.Cluster) *infrav1.AzureCluster {
1159+
if cluster.Spec.InfrastructureRef == nil || cluster.Spec.InfrastructureRef.Kind != infrav1.AzureClusterKind {
1160+
return nil
1161+
}
1162+
azureCluster := &infrav1.AzureCluster{}
1163+
key := client.ObjectKey{
1164+
Namespace: cluster.Spec.InfrastructureRef.Namespace,
1165+
Name: cluster.Spec.InfrastructureRef.Name,
1166+
}
1167+
if err := c.Get(ctx, key, azureCluster); err != nil {
1168+
return nil
1169+
}
1170+
return azureCluster
1171+
}
1172+
1173+
// isUsingSPCredentials checks if the cluster is using Service Principal credentials.
1174+
func isUsingSPCredentials(ctx context.Context, c client.Client, azureCluster *infrav1.AzureCluster) bool {
1175+
if azureCluster.Spec.IdentityRef == nil {
1176+
return false
1177+
}
1178+
identity := &infrav1.AzureClusterIdentity{}
1179+
key := client.ObjectKey{
1180+
Name: azureCluster.Spec.IdentityRef.Name,
1181+
Namespace: azureCluster.Spec.IdentityRef.Namespace,
1182+
}
1183+
if key.Namespace == "" {
1184+
key.Namespace = azureCluster.Namespace
1185+
}
1186+
if err := c.Get(ctx, key, identity); err != nil {
1187+
return false
1188+
}
1189+
switch identity.Spec.Type {
1190+
case infrav1.ServicePrincipal, infrav1.ManualServicePrincipal, infrav1.ServicePrincipalCertificate:
1191+
return true
1192+
}
1193+
return false
1194+
}

‎controllers/helpers_test.go‎

Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1655,3 +1655,251 @@ func TestRemoveBlockMoveAnnotation(t *testing.T) {
16551655
})
16561656
}
16571657
}
1658+
1659+
func TestIsUsingSPCredentials(t *testing.T) {
1660+
tests := []struct {
1661+
name string
1662+
azureCluster *infrav1.AzureCluster
1663+
identity *infrav1.AzureClusterIdentity
1664+
expected bool
1665+
}{
1666+
{
1667+
name: "ServicePrincipal returns true",
1668+
azureCluster: &infrav1.AzureCluster{
1669+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1670+
Spec: infrav1.AzureClusterSpec{
1671+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1672+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"},
1673+
},
1674+
},
1675+
},
1676+
identity: &infrav1.AzureClusterIdentity{
1677+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"},
1678+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ServicePrincipal},
1679+
},
1680+
expected: true,
1681+
},
1682+
{
1683+
name: "ManualServicePrincipal returns true",
1684+
azureCluster: &infrav1.AzureCluster{
1685+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1686+
Spec: infrav1.AzureClusterSpec{
1687+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1688+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"},
1689+
},
1690+
},
1691+
},
1692+
identity: &infrav1.AzureClusterIdentity{
1693+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"},
1694+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ManualServicePrincipal},
1695+
},
1696+
expected: true,
1697+
},
1698+
{
1699+
name: "ServicePrincipalCertificate returns true",
1700+
azureCluster: &infrav1.AzureCluster{
1701+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1702+
Spec: infrav1.AzureClusterSpec{
1703+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1704+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"},
1705+
},
1706+
},
1707+
},
1708+
identity: &infrav1.AzureClusterIdentity{
1709+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"},
1710+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ServicePrincipalCertificate},
1711+
},
1712+
expected: true,
1713+
},
1714+
{
1715+
name: "WorkloadIdentity returns false",
1716+
azureCluster: &infrav1.AzureCluster{
1717+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1718+
Spec: infrav1.AzureClusterSpec{
1719+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1720+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"},
1721+
},
1722+
},
1723+
},
1724+
identity: &infrav1.AzureClusterIdentity{
1725+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"},
1726+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.WorkloadIdentity},
1727+
},
1728+
expected: false,
1729+
},
1730+
{
1731+
name: "UserAssignedMSI returns false",
1732+
azureCluster: &infrav1.AzureCluster{
1733+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1734+
Spec: infrav1.AzureClusterSpec{
1735+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1736+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"},
1737+
},
1738+
},
1739+
},
1740+
identity: &infrav1.AzureClusterIdentity{
1741+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"},
1742+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.UserAssignedMSI},
1743+
},
1744+
expected: false,
1745+
},
1746+
{
1747+
name: "UserAssignedIdentityCredential returns false",
1748+
azureCluster: &infrav1.AzureCluster{
1749+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1750+
Spec: infrav1.AzureClusterSpec{
1751+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1752+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: "default"},
1753+
},
1754+
},
1755+
},
1756+
identity: &infrav1.AzureClusterIdentity{
1757+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "default"},
1758+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.UserAssignedIdentityCredential},
1759+
},
1760+
expected: false,
1761+
},
1762+
{
1763+
name: "nil IdentityRef returns false",
1764+
azureCluster: &infrav1.AzureCluster{
1765+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1766+
Spec: infrav1.AzureClusterSpec{},
1767+
},
1768+
identity: nil,
1769+
expected: false,
1770+
},
1771+
{
1772+
name: "empty IdentityRef namespace falls back to cluster namespace",
1773+
azureCluster: &infrav1.AzureCluster{
1774+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "my-namespace"},
1775+
Spec: infrav1.AzureClusterSpec{
1776+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1777+
IdentityRef: &corev1.ObjectReference{Name: "identity", Namespace: ""},
1778+
},
1779+
},
1780+
},
1781+
identity: &infrav1.AzureClusterIdentity{
1782+
ObjectMeta: metav1.ObjectMeta{Name: "identity", Namespace: "my-namespace"},
1783+
Spec: infrav1.AzureClusterIdentitySpec{Type: infrav1.ServicePrincipal},
1784+
},
1785+
expected: true,
1786+
},
1787+
{
1788+
name: "identity not found returns false",
1789+
azureCluster: &infrav1.AzureCluster{
1790+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1791+
Spec: infrav1.AzureClusterSpec{
1792+
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
1793+
IdentityRef: &corev1.ObjectReference{Name: "missing-identity", Namespace: "default"},
1794+
},
1795+
},
1796+
},
1797+
identity: nil,
1798+
expected: false,
1799+
},
1800+
}
1801+
1802+
for _, tc := range tests {
1803+
t.Run(tc.name, func(t *testing.T) {
1804+
g := NewWithT(t)
1805+
scheme := setupScheme(g)
1806+
1807+
var initObjects []runtime.Object
1808+
if tc.identity != nil {
1809+
initObjects = append(initObjects, tc.identity)
1810+
}
1811+
1812+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(initObjects...).Build()
1813+
result := isUsingSPCredentials(t.Context(), fakeClient, tc.azureCluster)
1814+
g.Expect(result).To(Equal(tc.expected))
1815+
})
1816+
}
1817+
}
1818+
1819+
func TestGetAzureClusterFromCluster(t *testing.T) {
1820+
tests := []struct {
1821+
name string
1822+
cluster *clusterv1.Cluster
1823+
azureCluster *infrav1.AzureCluster
1824+
expectNil bool
1825+
}{
1826+
{
1827+
name: "returns AzureCluster when infraRef is AzureCluster",
1828+
cluster: &clusterv1.Cluster{
1829+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1830+
Spec: clusterv1.ClusterSpec{
1831+
InfrastructureRef: &corev1.ObjectReference{
1832+
Kind: infrav1.AzureClusterKind,
1833+
Name: "azure-cluster",
1834+
Namespace: "default",
1835+
},
1836+
},
1837+
},
1838+
azureCluster: &infrav1.AzureCluster{
1839+
ObjectMeta: metav1.ObjectMeta{Name: "azure-cluster", Namespace: "default"},
1840+
},
1841+
expectNil: false,
1842+
},
1843+
{
1844+
name: "returns nil when infraRef is nil",
1845+
cluster: &clusterv1.Cluster{
1846+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1847+
Spec: clusterv1.ClusterSpec{},
1848+
},
1849+
azureCluster: nil,
1850+
expectNil: true,
1851+
},
1852+
{
1853+
name: "returns nil when infraRef is AzureManagedCluster",
1854+
cluster: &clusterv1.Cluster{
1855+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1856+
Spec: clusterv1.ClusterSpec{
1857+
InfrastructureRef: &corev1.ObjectReference{
1858+
Kind: infrav1.AzureManagedClusterKind,
1859+
Name: "azure-managed-cluster",
1860+
Namespace: "default",
1861+
},
1862+
},
1863+
},
1864+
azureCluster: nil,
1865+
expectNil: true,
1866+
},
1867+
{
1868+
name: "returns nil when AzureCluster not found",
1869+
cluster: &clusterv1.Cluster{
1870+
ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "default"},
1871+
Spec: clusterv1.ClusterSpec{
1872+
InfrastructureRef: &corev1.ObjectReference{
1873+
Kind: infrav1.AzureClusterKind,
1874+
Name: "missing-cluster",
1875+
Namespace: "default",
1876+
},
1877+
},
1878+
},
1879+
azureCluster: nil,
1880+
expectNil: true,
1881+
},
1882+
}
1883+
1884+
for _, tc := range tests {
1885+
t.Run(tc.name, func(t *testing.T) {
1886+
g := NewWithT(t)
1887+
scheme := setupScheme(g)
1888+
1889+
var initObjects []runtime.Object
1890+
if tc.azureCluster != nil {
1891+
initObjects = append(initObjects, tc.azureCluster)
1892+
}
1893+
1894+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(initObjects...).Build()
1895+
result := getAzureClusterFromCluster(t.Context(), fakeClient, tc.cluster)
1896+
1897+
if tc.expectNil {
1898+
g.Expect(result).To(BeNil())
1899+
} else {
1900+
g.Expect(result).NotTo(BeNil())
1901+
g.Expect(result.Name).To(Equal(tc.azureCluster.Name))
1902+
}
1903+
})
1904+
}
1905+
}

0 commit comments

Comments
 (0)