ARO-9382 prevent updating existing platform identities

pkg/api/v20240812preview/openshiftcluster.go

@@ -41,6 +41,11 @@ type OpenShiftCluster struct {
 	Identity *Identity `json:"identity,omitempty"`
 }
 
+// UsesWorkloadIdentity checks whether a cluster is a Workload Identity cluster or a Service Principal cluster
+func (oc *OpenShiftCluster) UsesWorkloadIdentity() bool {
+	return oc.Properties.PlatformWorkloadIdentityProfile != nil && oc.Properties.ServicePrincipalProfile == nil
+}
+
 // Tags represents an OpenShift cluster's tags.
 type Tags map[string]string
 

pkg/api/v20240812preview/openshiftcluster_test.go

@@ -0,0 +1,67 @@
+package v20240812preview
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestIsWorkloadIdentity(t *testing.T) {
+	tests := []*struct {
+		name string
+		oc   OpenShiftCluster
+		want bool
+	}{
+		{
+			name: "Cluster is Workload Identity",
+			oc: OpenShiftCluster{
+				Properties: OpenShiftClusterProperties{
+					PlatformWorkloadIdentityProfile: &PlatformWorkloadIdentityProfile{},
+					ServicePrincipalProfile:         nil,
+				},
+			},
+			want: true,
+		},
+		{
+			name: "Cluster is Service Principal",
+			oc: OpenShiftCluster{
+				Properties: OpenShiftClusterProperties{
+					PlatformWorkloadIdentityProfile: nil,
+					ServicePrincipalProfile:         &ServicePrincipalProfile{},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "Cluster is Service Principal",
+			oc: OpenShiftCluster{
+				Properties: OpenShiftClusterProperties{
+					PlatformWorkloadIdentityProfile: nil,
+					ServicePrincipalProfile:         nil,
+				},
+			},
+			want: false,
+		},
+		{
+			name: "Cluster is Service Principal",
+			oc: OpenShiftCluster{
+				Properties: OpenShiftClusterProperties{
+					PlatformWorkloadIdentityProfile: &PlatformWorkloadIdentityProfile{},
+					ServicePrincipalProfile:         &ServicePrincipalProfile{},
+				},
+			},
+			want: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := test.oc.UsesWorkloadIdentity()
+			if got != test.want {
+				t.Error(fmt.Errorf("got != want: %v != %v", got, test.want))
+			}
+		})
+	}
+}

pkg/api/v20240812preview/openshiftcluster_validatestatic.go

@@ -446,6 +446,29 @@ func (sv openShiftClusterStaticValidator) validateDelta(oc, current *OpenShiftCl
 		return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodePropertyChangeNotAllowed, err.Target, err.Message)
 	}
 
+	if current.UsesWorkloadIdentity() {
+		currentIdentities := map[string]PlatformWorkloadIdentity{}
+		for _, i := range current.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
+			currentIdentities[i.OperatorName] = i
+		}
+
+		updateIdentities := map[string]PlatformWorkloadIdentity{}
+		for _, i := range oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities {
+			updateIdentities[i.OperatorName] = i
+		}
+
+		for name, currentIdentity := range currentIdentities {
+			updateIdentity, present := updateIdentities[name]
+			// this also validates that existing identities' names haven't changed
+			if !present {
+				return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodePropertyChangeNotAllowed, "properties.platformWorkloadIdentityProfile.platformWorkloadIdentities", "Operator identity cannot be removed or have its name changed.")
+			}
+			if currentIdentity.ResourceID != updateIdentity.ResourceID {
+				return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodePropertyChangeNotAllowed, "properties.platformWorkloadIdentityProfile.platformWorkloadIdentities", "Operator identity resource ID cannot be changed.")
+			}
+		}
+	}
+
 	return nil
 }
 

pkg/api/v20240812preview/openshiftcluster_validatestatic_test.go

@@ -43,7 +43,19 @@ const (
 )
 
 var (
-	subscriptionID = "00000000-0000-0000-0000-000000000000"
+	subscriptionID    = "00000000-0000-0000-0000-000000000000"
+	platformIdentity1 = PlatformWorkloadIdentity{
+		OperatorName: "FAKE-OPERATOR",
+		ResourceID:   "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name",
+	}
+	platformIdentity2 = PlatformWorkloadIdentity{
+		OperatorName: "ANOTHER-FAKE-OPERATOR",
+		ResourceID:   "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name-two",
+	}
+	clusterIdentity1 = ClusterUserAssignedIdentity{
+		ClientID:    "11111111-1111-1111-1111-111111111111",
+		PrincipalID: "SOMETHING",
+	}
 )
 
 func getResourceID(clusterName string) string {
@@ -1234,10 +1246,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 			modify: func(oc *OpenShiftCluster) {
 				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
 					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
-						{
-							OperatorName: "FAKE-OPERATOR",
-							ResourceID:   "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name",
-						},
+						platformIdentity1,
 					},
 				}
 				oc.Identity = &Identity{
@@ -1288,10 +1297,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 				oc.Properties.ServicePrincipalProfile = nil
 				oc.Identity = &Identity{
 					UserAssignedIdentities: UserAssignedIdentities{
-						"first": {
-							ClientID:    "11111111-1111-1111-1111-111111111111",
-							PrincipalID: "SOMETHING",
-						},
+						"first": clusterIdentity1,
 					},
 				}
 			},
@@ -1302,18 +1308,12 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 			modify: func(oc *OpenShiftCluster) {
 				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
 					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
-						{
-							OperatorName: "FAKE-OPERATOR",
-							ResourceID:   "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name",
-						},
+						platformIdentity1,
 					},
 				}
 				oc.Identity = &Identity{
 					UserAssignedIdentities: UserAssignedIdentities{
-						"first": {
-							ClientID:    "11111111-1111-1111-1111-111111111111",
-							PrincipalID: "SOMETHING",
-						},
+						"first": clusterIdentity1,
 					},
 				}
 				oc.Properties.ServicePrincipalProfile = &ServicePrincipalProfile{
@@ -1328,10 +1328,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 			modify: func(oc *OpenShiftCluster) {
 				oc.Identity = &Identity{
 					UserAssignedIdentities: UserAssignedIdentities{
-						"first": {
-							ClientID:    "11111111-1111-1111-1111-111111111111",
-							PrincipalID: "SOMETHING",
-						},
+						"first": clusterIdentity1,
 					},
 				}
 			},
@@ -1356,10 +1353,7 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 			modify: func(oc *OpenShiftCluster) {
 				oc.Identity = &Identity{
 					UserAssignedIdentities: UserAssignedIdentities{
-						"first": {
-							ClientID:    "11111111-1111-1111-1111-111111111111",
-							PrincipalID: "SOMETHING",
-						},
+						"first": clusterIdentity1,
 					},
 				}
 				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
@@ -1405,5 +1399,135 @@ func TestOpenShiftClusterStaticValidatePlatformWorkloadIdentityProfile(t *testin
 		},
 	}
 
+	updateTests := []*validateTest{
+		{
+			name: "addition of operator identity",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
+						platformIdentity1,
+					},
+				}
+				oc.Identity = &Identity{
+					UserAssignedIdentities: UserAssignedIdentities{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = append(oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities, platformIdentity2)
+			},
+		},
+		{
+			name: "invalid change of operator identity name",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
+						platformIdentity1,
+					},
+				}
+				oc.Identity = &Identity{
+					UserAssignedIdentities: UserAssignedIdentities{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].OperatorName = "FAKE-OPERATOR-OTHER"
+			},
+			wantErr: "400: PropertyChangeNotAllowed: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: Operator identity cannot be removed or have its name changed.",
+		},
+		{
+			name: "invalid change of operator identity resource ID",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
+						platformIdentity1,
+					},
+				}
+				oc.Identity = &Identity{
+					UserAssignedIdentities: UserAssignedIdentities{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].ResourceID = platformIdentity2.ResourceID
+			},
+			wantErr: "400: PropertyChangeNotAllowed: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: Operator identity resource ID cannot be changed.",
+		},
+		{
+			name: "change of operator identity order",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
+						platformIdentity1,
+						platformIdentity2,
+					},
+				}
+				oc.Identity = &Identity{
+					UserAssignedIdentities: UserAssignedIdentities{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = []PlatformWorkloadIdentity{
+					platformIdentity2,
+					platformIdentity1,
+				}
+			},
+		},
+		{
+			name: "invalid change of operator identity name and resource ID",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
+						platformIdentity1,
+					},
+				}
+				oc.Identity = &Identity{
+					UserAssignedIdentities: UserAssignedIdentities{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].OperatorName = platformIdentity2.OperatorName
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities[0].ResourceID = platformIdentity2.ResourceID
+			},
+			wantErr: "400: PropertyChangeNotAllowed: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: Operator identity cannot be removed or have its name changed.",
+		},
+		{
+			name: "invalid removal of identity",
+			current: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile = &PlatformWorkloadIdentityProfile{
+					PlatformWorkloadIdentities: []PlatformWorkloadIdentity{
+						platformIdentity1,
+						platformIdentity2,
+					},
+				}
+				oc.Identity = &Identity{
+					UserAssignedIdentities: UserAssignedIdentities{
+						"first": clusterIdentity1,
+					},
+				}
+				oc.Properties.ServicePrincipalProfile = nil
+			},
+			modify: func(oc *OpenShiftCluster) {
+				oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities = []PlatformWorkloadIdentity{
+					platformIdentity1,
+				}
+			},
+			wantErr: "400: PropertyChangeNotAllowed: properties.platformWorkloadIdentityProfile.platformWorkloadIdentities: Operator identity cannot be removed or have its name changed.",
+		},
+	}
+
 	runTests(t, testModeCreate, createTests)
+	runTests(t, testModeUpdate, updateTests)
 }