return all conflicting rms

master/internal/rm/multirm/multirm.go

@@ -18,10 +18,8 @@ import (
 
 // ErrRMConflict returns a detailed error if multiple resource managers define a resource pool
 // with the same name.
-func ErrRMConflict(rm1, rm2, rp string) error {
-	clause1 := fmt.Sprintf("resource pool %s exists for both resource managers %s and %s,", rp, rm1, rm2)
-	clause2 := fmt.Sprintf("please set resources.resource_manager = %s or resources.resource.manager = %s", rm1, rm2)
-	return fmt.Errorf(clause1 + clause2)
+func ErrRMConflict(rmNames []string, rp string) error {
+	return fmt.Errorf("resource pool %s exists for both resource managers %v,", rp, rmNames)
 }
 
 // ErrRMNotDefined returns a detailed error if a resource manager isn't found in the MultiRMRouter map.
@@ -395,29 +393,27 @@ func (m *MultiRMRouter) getRM(rmName string, rpName string) (string, error) {
 	}
 
 	// If just given the RP name, search through all resource managers for a single match.
-	var resolvedRMName string
+	rmMatches := []string{}
 	for name, r := range m.rms {
 		rps, err := r.GetResourcePools()
 		if err != nil {
 			return name, fmt.Errorf("could not get resource pools for %s", r)
 		}
 		for _, p := range rps.ResourcePools {
 			if p.Name == rpName {
-				// If the resolvedRMName is already set, we assume there is a conflict.
-				if resolvedRMName != "" {
-					return "", ErrRMConflict(resolvedRMName, name, rpName)
-				}
-				resolvedRMName = name
+				rmMatches = append(rmMatches, name)
 			}
 		}
 	}
 
-	// If the resolvedRMName isn't set, then the RP was not found.
-	if resolvedRMName == "" {
+	if len(rmMatches) == 0 {
+		// If the resolvedRMName isn't set, then the RP was not found.
 		return rmName, ErrRPNotDefined(rpName)
+	} else if len(rmMatches) > 1 {
+		// If the resolvedRMName is already set, we assume there is a conflict.
+		return "", ErrRMConflict(rmMatches, rpName)
 	}
-
-	return resolvedRMName, nil
+	return rmMatches[0], nil
 }
 
 func fanOutRMCall[TReturn any](m *MultiRMRouter, f func(rm.ResourceManager) (TReturn, error)) ([]TReturn, error) {

master/internal/rm/multirm/multirm_intg_test.go

@@ -683,7 +683,11 @@ func TestDisableSlot(t *testing.T) {
 
 func TestGetRMName(t *testing.T) {
 	def := mocks.ResourceManager{}
-	def.On("GetResourcePools").Return(&apiv1.GetResourcePoolsResponse{}, nil)
+	def.On("GetResourcePools").Return(&apiv1.GetResourcePoolsResponse{
+		ResourcePools: []*resourcepoolv1.ResourcePool{
+			{Name: "gcp2"},
+		},
+	}, nil)
 
 	gcp := mocks.ResourceManager{}
 	gcp.On("GetResourcePools").Return(&apiv1.GetResourcePoolsResponse{
@@ -709,43 +713,27 @@ func TestGetRMName(t *testing.T) {
 		syslog: logrus.WithField("component", "resource-router"),
 	}
 
-	// If the RM/RP are not named, return default RM name.
-	rmName, err := mockMultiRM.getRM("", "")
-	require.Nil(t, err)
-	require.Equal(t, mockMultiRM.defaultRMName, rmName)
-
-	// If the RP isn't named, return the explicitly named RM, if it exists.
-	rmName, err = mockMultiRM.getRM("aws", "")
-	require.Nil(t, err)
-	require.Equal(t, "aws", rmName)
-
-	// If the RP isn't named, return the explicitly named RM, if it exists, or an error.
-	rmName, err = mockMultiRM.getRM("aws123", "")
-	require.Equal(t, ErrRMNotDefined("aws123"), err)
-	require.Equal(t, "aws123", rmName)
-
-	// If the RP doesn't exist in the named RM, return the explicitly named RM anyway.
-	rmName, err = mockMultiRM.getRM("aws", "awsa")
-	require.Nil(t, err)
-	require.Equal(t, "aws", rmName)
-
-	// If the RP exists in the named RM, return the explicitly named RM anyway.
-	rmName, err = mockMultiRM.getRM("aws", "aws1")
-	require.Nil(t, err)
-	require.Equal(t, "aws", rmName)
-
-	// If the RP does exist but in multiple RMs, return blank name and error.
-	rmName, err = mockMultiRM.getRM("", "gcp2")
-	require.ErrorContains(t, err, "resource pool gcp2 exists for both resource managers")
-	require.Equal(t, "", rmName)
-
-	// If the RP does exist, but RM is not named, return correct RM name.
-	rmName, err = mockMultiRM.getRM("", "aws1")
-	require.Nil(t, err)
-	require.Equal(t, "aws", rmName)
-
-	// If the RP doesn't exist in any RM, return an error and blank name.
-	rmName, err = mockMultiRM.getRM("", "gcp3")
-	require.Equal(t, ErrRPNotDefined("gcp3"), err)
-	require.Equal(t, "", rmName)
+	cases := []struct {
+		name           string
+		rmName         string
+		rpName         string
+		err            error
+		expectedRMName string
+	}{
+		{"RM/RP undefined", "", "", nil, mockMultiRM.defaultRMName},
+		{"RM defined, RP undefined", "aws", "", nil, "aws"},
+		{"RM defined/doesn't exist, RP undefined", "aws123", "", ErrRMNotDefined("aws123"), "aws123"},
+		{"RM defined, RP defined", "aws", "aws1", nil, "aws"},
+		{"RM defined, RP defined/doesn't exist", "aws", "awsa", nil, "aws"},
+		{"RM undefined, RP defined", "", "aws1", nil, "aws"},
+		{"RM undefined, RP defined + conflict", "", "gcp2", ErrRMConflict([]string{"default", "gcp", "aws"}, "gcp2"), ""},
+		{"RM undefined, RP defined/doesn't exist", "", "gcp3", ErrRPNotDefined("gcp3"), ""},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			rmName, err := mockMultiRM.getRM(tt.rmName, tt.rpName)
+			require.Equal(t, tt.expectedRMName, rmName)
+			require.Equal(t, tt.err, err)
+		})
+	}
 }