coordinator: Reject routing mutators with different repo storages

As with the preceding commit, we have the same issue when routing
mutators where the first message contains two repositories with
different storages. The result is that we may end up rewriting the
repositories to a repository on the wrong storage.

As routing mutators also uses `rewrittenRepositoryMessage()` the bug
should already be fixed by the preceding commit. But we'd still try to
connect to the wrong storage node first and will then try to resolve the
additional repository, which is likely to fail. The result would be a
confusing error message that the repository wasn't found, where the real
issue is that we got a request with differing repository storages.

Add an explicit check to catch this error condition so that we can
return a proper error message to the client.

Changelog: fixed

2 files changed, 16 insertions, 17 deletions

diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index fb2c65eee..7214718ed 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -382,6 +382,21 @@ func (c *Coordinator) mutatorStreamParameters(ctx context.Context, call grpcCall
 	} else if err != nil {
 		return nil, structerr.NewInvalidArgument("%w", err)
 	} else {
+		// We do not support resolving multiple different repositories that reside on
+		// different virtual storages. This kind of makes sense from a technical point of
+		// view as Praefect cannot guarantee to resolve both virtual storages. So for the
+		// time being we accept this restriction and handle it explicitly.
+		//
+		// Note that this is the same condition as in `rewrittenRepositoryMessage()`. This
+		// is done so that we detect such erroneous requests before we try to connect to the
+		// target node, which allows us to return a proper error to the user that indicates
+		// the underlying issue instead of an unrelated symptom.
+		//
+		// This limitation may be lifted in the future.
+		if virtualStorage != additionalRepo.GetStorageName() {
+			return nil, structerr.NewInvalidArgument("resolving additional repository on different storage than target repository is not supported")
+		}
+
 		additionalRepoRelativePath = additionalRepo.GetRelativePath()
 	}
 
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index e2f1caf98..71a96ba53 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -365,23 +365,7 @@ func TestStreamDirectorMutator(t *testing.T) {
 							Repository: targetRepo,
 						},
 					},
-					// This is a bug: we try to resolve the additional
-					// repository as a member of the target repository's
-					// storage. In the best case this fails, which is a
-					// reasonable outcome. But in the worst case, if the
-					// additional repository's path exists on the target
-					// repository's storage, we could forward the request to the
-					// wrong node.
-					expectedErr: structerr.NewNotFound("%w", fmt.Errorf("mutator call: route repository mutator: %w",
-						fmt.Errorf("resolve additional replica path: %w",
-							fmt.Errorf("get additional repository id: %w",
-								// Note how the error message uses
-								// the target repo's storage name,
-								// but the additional repo's path.
-								commonerr.NewRepositoryNotFoundError(targetRepo.StorageName, additionalRepo.RelativePath),
-							),
-						),
-					)),
+					expectedErr: structerr.NewInvalidArgument("resolving additional repository on different storage than target repository is not supported"),
 				}
 			},
 		},