From eff5dd1812370fa13bffd6b41d6f8f5242e72467 Mon Sep 17 00:00:00 2001
From: Dan Upton <daniel@floppy.co>
Date: Fri, 28 Apr 2023 11:22:42 +0100
Subject: [PATCH] resource: owner references must include a uid (#17169)

---
 .../grpc-external/services/resource/write.go  | 25 +++++++++
 .../services/resource/write_test.go           | 51 +++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/agent/grpc-external/services/resource/write.go b/agent/grpc-external/services/resource/write.go
index 121c7ce39ccc..ad17e61c51d9 100644
--- a/agent/grpc-external/services/resource/write.go
+++ b/agent/grpc-external/services/resource/write.go
@@ -119,6 +119,31 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
 				return errUseWriteStatus
 			}
 
+			// Generally, we expect resources with owners to be created by controllers,
+			// and they should provide the Uid. In cases where no Uid is given (e.g. the
+			// owner is specified in the resource HCL) we'll look up whatever the current
+			// Uid is and use that.
+			//
+			// An important note on consistency:
+			//
+			// We read the owner with StrongConsistency here to reduce the likelihood of
+			// creating a resource pointing to the wrong "incarnation" of the owner in
+			// cases where the owner is deleted and re-created in quick succession.
+			//
+			// That said, there is still a chance that the owner has been deleted by the
+			// time we write this resource. This is not a relational database and we do
+			// not support ACID transactions or real foreign key constraints.
+			if input.Owner != nil && input.Owner.Uid == "" {
+				owner, err := s.Backend.Read(ctx, storage.StrongConsistency, input.Owner)
+				switch {
+				case errors.Is(err, storage.ErrNotFound):
+					return status.Error(codes.InvalidArgument, "resource.owner does not exist")
+				case err != nil:
+					return status.Errorf(codes.Internal, "failed to resolve owner: %v", err)
+				}
+				input.Owner = owner.Id
+			}
+
 			// TODO(spatel): Revisit owner<->resource tenancy rules post-1.16
 
 		// Update path.
diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go
index f8620f4a3431..b2960a9243d7 100644
--- a/agent/grpc-external/services/resource/write_test.go
+++ b/agent/grpc-external/services/resource/write_test.go
@@ -491,6 +491,57 @@ func TestWrite_Owner_Immutable(t *testing.T) {
 	require.ErrorContains(t, err, "owner cannot be changed")
 }
 
+func TestWrite_Owner_Uid(t *testing.T) {
+	server := testServer(t)
+	client := testClient(t, server)
+
+	demo.RegisterTypes(server.Registry)
+
+	t.Run("uid given", func(t *testing.T) {
+		artist, err := demo.GenerateV2Artist()
+		require.NoError(t, err)
+
+		album, err := demo.GenerateV2Album(artist.Id)
+		require.NoError(t, err)
+		album.Owner.Uid = ulid.Make().String()
+
+		_, err = client.Write(testContext(t), &pbresource.WriteRequest{Resource: album})
+		require.NoError(t, err)
+	})
+
+	t.Run("no uid - owner not found", func(t *testing.T) {
+		artist, err := demo.GenerateV2Artist()
+		require.NoError(t, err)
+
+		album, err := demo.GenerateV2Album(artist.Id)
+		require.NoError(t, err)
+
+		_, err = client.Write(testContext(t), &pbresource.WriteRequest{Resource: album})
+		require.Error(t, err)
+		require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String())
+	})
+
+	t.Run("no uid - automatically resolved", func(t *testing.T) {
+		artist, err := demo.GenerateV2Artist()
+		require.NoError(t, err)
+
+		rsp1, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: artist})
+		require.NoError(t, err)
+		artist = rsp1.Resource
+
+		album, err := demo.GenerateV2Album(clone(artist.Id))
+		require.NoError(t, err)
+
+		// Blank out the owner Uid to check it gets automatically filled in.
+		album.Owner.Uid = ""
+
+		rsp2, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: album})
+		require.NoError(t, err)
+		require.NotEmpty(t, rsp2.Resource.Owner.Uid)
+		require.Equal(t, artist.Id.Uid, rsp2.Resource.Owner.Uid)
+	})
+}
+
 type blockOnceBackend struct {
 	storage.Backend