From 908404282e81a7ba0c2c2b7271d7d9d9036f98b4 Mon Sep 17 00:00:00 2001 From: kimorris27 Date: Tue, 4 Jun 2024 15:39:50 -0500 Subject: [PATCH] Update static validator to return multiple validation issues at the same time where applicable for better UX --- pkg/api/admin/platformworkloadidentityroleset.go | 8 ++++---- ...tformworkloadidentityroleset_validatestatic.go | 15 +++++++++++---- ...in_platformworkloadidentityroleset_put_test.go | 8 +++----- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/pkg/api/admin/platformworkloadidentityroleset.go b/pkg/api/admin/platformworkloadidentityroleset.go index 9ed4680bceb..d2b26e9a6fb 100644 --- a/pkg/api/admin/platformworkloadidentityroleset.go +++ b/pkg/api/admin/platformworkloadidentityroleset.go @@ -33,14 +33,14 @@ type PlatformWorkloadIdentityRoleSetProperties struct { // PlatformWorkloadIdentityRole represents a mapping from a particular OCP operator to the built-in role that should be assigned to that operator's corresponding managed identity. type PlatformWorkloadIdentityRole struct { // OperatorName represents the name of the operator that this role is for. - OperatorName string `json:"operatorName,omitempty" mutable:"true"` + OperatorName string `json:"operatorName,omitempty" mutable:"true" validate:"required"` // RoleDefinitionName represents the name of the role. - RoleDefinitionName string `json:"roleDefinitionName,omitempty" mutable:"true"` + RoleDefinitionName string `json:"roleDefinitionName,omitempty" mutable:"true" validate:"required"` // RoleDefinitionID represents the resource ID of the role definition. - RoleDefinitionID string `json:"roleDefinitionId,omitempty" mutable:"true"` + RoleDefinitionID string `json:"roleDefinitionId,omitempty" mutable:"true" validate:"required"` // ServiceAccounts represents the set of service accounts associated with the given operator, since each service account needs its own federated credential. - ServiceAccounts []string `json:"serviceAccounts,omitempty" mutable:"true"` + ServiceAccounts []string `json:"serviceAccounts,omitempty" mutable:"true" validate:"required"` } diff --git a/pkg/api/admin/platformworkloadidentityroleset_validatestatic.go b/pkg/api/admin/platformworkloadidentityroleset_validatestatic.go index 000b61b11b6..d6fcfb9c1b6 100644 --- a/pkg/api/admin/platformworkloadidentityroleset_validatestatic.go +++ b/pkg/api/admin/platformworkloadidentityroleset_validatestatic.go @@ -3,6 +3,7 @@ package admin import ( "fmt" "net/http" + "strings" "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/api/util/immutable" @@ -42,24 +43,30 @@ func (sv platformWorkloadIdentityRoleSetStaticValidator) validate(new *PlatformW return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "properties.platformWorkloadIdentityRoles", "Must be provided and must be non-empty") } + missingProperties := []string{} + for i, r := range new.Properties.PlatformWorkloadIdentityRoles { if r.OperatorName == "" { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].operatorName", i), "Must be provided") + missingProperties = append(missingProperties, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].operatorName", i)) } if r.RoleDefinitionName == "" { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].roleDefinitionName", i), "Must be provided") + missingProperties = append(missingProperties, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].roleDefinitionName", i)) } if r.RoleDefinitionID == "" { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].roleDefinitionId", i), "Must be provided") + missingProperties = append(missingProperties, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].roleDefinitionId", i)) } if r.ServiceAccounts == nil || len(r.ServiceAccounts) == 0 { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].serviceAccounts", i), "Must be provided and must be non-empty") + missingProperties = append(missingProperties, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].serviceAccounts", i)) } } + if len(missingProperties) > 0 { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, strings.Join(missingProperties, ", "), "Must be provided") + } + return nil } diff --git a/pkg/frontend/admin_platformworkloadidentityroleset_put_test.go b/pkg/frontend/admin_platformworkloadidentityroleset_put_test.go index dbfb2ed5597..bc26a37d7e0 100644 --- a/pkg/frontend/admin_platformworkloadidentityroleset_put_test.go +++ b/pkg/frontend/admin_platformworkloadidentityroleset_put_test.go @@ -643,7 +643,7 @@ func TestPlatformWorkloadIdentityRoleSetPut(t *testing.T) { }, }, wantStatusCode: http.StatusBadRequest, - wantError: "400: InvalidParameter: properties.platformWorkloadIdentityRoles[0].serviceAccounts: Must be provided and must be non-empty", + wantError: "400: InvalidParameter: properties.platformWorkloadIdentityRoles[0].serviceAccounts: Must be provided", wantDocuments: []*api.PlatformWorkloadIdentityRoleSetDocument{ { ID: "08080808-0808-0808-0808-080808080001", @@ -674,7 +674,7 @@ func TestPlatformWorkloadIdentityRoleSetPut(t *testing.T) { }, }, { - name: "updating known version requires non-empty PlatformWorkloadIdentityRole.ServiceAccounts", + name: "updating known version requires PlatformWorkloadIdentityRole.RoleDefinitionId and PlatformWorkloadIdentityRole.ServiceAccounts (tests the case where multiple attributes are missing and error message consists of missing properties joined together)", fixture: func(f *testdatabase.Fixture) { f.AddPlatformWorkloadIdentityRoleSetDocuments( &api.PlatformWorkloadIdentityRoleSetDocument{ @@ -711,14 +711,12 @@ func TestPlatformWorkloadIdentityRoleSetPut(t *testing.T) { { OperatorName: "ClusterIngressOperator", RoleDefinitionName: "Azure RedHat OpenShift Cluster Ingress Operator Role", - RoleDefinitionID: "/providers/Microsoft.Authorization/roleDefinitions/a1f96423-95ce-4224-ab27-4e3dc72facd4", - ServiceAccounts: []string{}, }, }, }, }, wantStatusCode: http.StatusBadRequest, - wantError: "400: InvalidParameter: properties.platformWorkloadIdentityRoles[0].serviceAccounts: Must be provided and must be non-empty", + wantError: "400: InvalidParameter: properties.platformWorkloadIdentityRoles[0].roleDefinitionId, properties.platformWorkloadIdentityRoles[0].serviceAccounts: Must be provided", wantDocuments: []*api.PlatformWorkloadIdentityRoleSetDocument{ { ID: "08080808-0808-0808-0808-080808080001",