ARO-10859 sort required identities for consistent error messaging

Azure · Nov 26, 2024 · ec9136e · ec9136e
1 parent c6abcda
commit ec9136e
Show file tree

Hide file tree

Showing 2 changed files with 121 additions and 0 deletions.
diff --git a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"sort"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/database"
@@ -87,10 +88,14 @@ func (service *PlatformWorkloadIdentityRolesByVersionService) GetPlatformWorkloa
 }
 
 func GetPlatformWorkloadIdentityMismatchError(oc *api.OpenShiftCluster, platformWorkloadIdentityRolesByRoleName map[string]api.PlatformWorkloadIdentityRole) error {
+	if !oc.UsesWorkloadIdentity() {
+		return fmt.Errorf("GetPlatformWorkloadIdentityMismatchError called for a Cluster Service Principal cluster")
+	}
 	requiredOperatorIdentities := []string{}
 	for _, role := range platformWorkloadIdentityRolesByRoleName {
 		requiredOperatorIdentities = append(requiredOperatorIdentities, role.OperatorName)
 	}
+	sort.Strings(requiredOperatorIdentities)
 	currentOpenShiftVersion, err := version.ParseVersion(oc.Properties.ClusterProfile.Version)
 	if err != nil {
 		return err

diff --git a/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion_test.go b/pkg/util/platformworkloadidentity/platformworkloadidentityrolesbyversion_test.go
@@ -5,6 +5,7 @@ package platformworkloadidentity
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"k8s.io/utils/ptr"
@@ -291,3 +292,118 @@ func TestNewPlatformWorkloadIdentityRolesByVersion(t *testing.T) {
 		})
 	}
 }
+
+func TestGetPlatformWorkloadIdentityMismatchError(t *testing.T) {
+	invalidVersion := "4.1450"
+	for _, tt := range []struct {
+		name             string
+		oc               *api.OpenShiftCluster
+		wantErr          string
+		checkConsistency bool
+	}{
+		{
+			name: "Exit the func for non MIWI clusters that has ServicePrincipalProfile",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ServicePrincipalProfile: &api.ServicePrincipalProfile{
+						SPObjectID: "00000000-0000-0000-0000-000000000000",
+					},
+				},
+			},
+			wantErr: "GetPlatformWorkloadIdentityMismatchError called for a Cluster Service Principal cluster",
+		},
+		{
+			name: "Exit the func for non MIWI clusters that has no PlatformWorkloadIdentityProfile or ServicePrincipalProfile",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{},
+			},
+			wantErr: "GetPlatformWorkloadIdentityMismatchError called for a Cluster Service Principal cluster",
+		},
+		{
+			name: "Invalid Cluster version",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ClusterProfile: api.ClusterProfile{
+						Version: invalidVersion,
+					},
+					PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{
+						UpgradeableTo: ptr.To(api.UpgradeableTo("4.15.40")),
+					},
+				},
+			},
+			wantErr: fmt.Sprintf(`could not parse version "%s"`, invalidVersion),
+		},
+		{
+			name: "Invalid UpgradeableTo version",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ClusterProfile: api.ClusterProfile{
+						Version: "4.14.40",
+					},
+					PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{
+						UpgradeableTo: ptr.To(api.UpgradeableTo(invalidVersion)),
+					},
+				},
+			},
+			wantErr: fmt.Sprintf(`could not parse version "%s"`, invalidVersion),
+		},
+		{
+			name: "Unexpected Identites in the doc",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ClusterProfile: api.ClusterProfile{
+						Version: "4.14.40",
+					},
+					PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{},
+				},
+			},
+			wantErr:          fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[bar foo]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"),
+			checkConsistency: true,
+		},
+		{
+			name: "Unexpected Identites in the doc with UpgradeableTo",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ClusterProfile: api.ClusterProfile{
+						Version: "4.14.40",
+					},
+					PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{
+						UpgradeableTo: ptr.To(api.UpgradeableTo("4.15.40")),
+					},
+				},
+			},
+			wantErr: fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s or %s'. The required platform workload identities are '[bar foo]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14", "4.15"),
+		},
+		{
+			name: "Unexpected Identites in the doc with UpgradeableTo lower than cluster version",
+			oc: &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ClusterProfile: api.ClusterProfile{
+						Version: "4.14.40",
+					},
+					PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{
+						UpgradeableTo: ptr.To(api.UpgradeableTo("4.13.40")),
+					},
+				},
+			},
+			wantErr: fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[bar foo]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"),
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			pir := &PlatformWorkloadIdentityRolesByVersionService{
+				platformWorkloadIdentityRoles: []api.PlatformWorkloadIdentityRole{
+					{OperatorName: "foo"},
+					{OperatorName: "bar"},
+				},
+			}
+			iteration := 1
+			if tt.checkConsistency {
+				iteration = 5
+			}
+			for i := 0; i < iteration; i++ {
+				err := GetPlatformWorkloadIdentityMismatchError(tt.oc, pir.GetPlatformWorkloadIdentityRolesByRoleName())
+				utilerror.AssertErrorMessage(t, err, tt.wantErr)
+			}
+		})
+	}
+}