Namespaced actors (Multitenant placement service) #7745

elena-kolevska · 2024-05-17T13:58:50Z

Description

This PR implements support for namespaced actors (multi-tenant placement service).

Requirements

Multi-tenancy means that sidecars belonging to tenant A will not receive placement information for tenant B (As a consequence, multiple tenants can have actor types with the same name - a hotly requested feature).
When mTLS is enabled, the sidecar reported namespace should be verified through spiffe ID. When mTLS is not enabled, we accept the reported namespace as-is.

Some implementation details

The in-memory raft-based store was adapted to group placement data by namespace
Placement tables are kept per namespace
Sidecars in a namespace will only receive the placement table for their own namespace
A stream connection pool will be kept per namespace

Backwards compatibility scenarios:

The format of the placement table that the sidecars receive will not change, only its content will, making it fully backwards compatible.
Old sidecars that use TLS do report their namespace at the moment; we will default to that one, even if the sidecar didn’t report it explicitly (because it’s on an old version).
We have no way of knowing the namespace for old sidecars that do not use TLS. For these sidecars we will use a special “empty” namespace. When they connect to a new placement service, they will only get the actor types hosted on other old sidecars that are not on TLS (in the “empty namespace”).

TLS enabled

As soon as the placement server is updated, sidecars A, B and C will see each other’s actor types, and no others. The same is true for sidecars D, E and F.

TLS not enabled

Sidecars A and B will not have information about the actor types hosted on sidecar C.
Sidecar D will not have information about the actor types hosted on sidecar E and F.
Sidecars C, E and F will see each other’s actor-types, as before

For true backwards compatibility, we would keep sending actor types from all six sidecars to the old sidecars, but this is a security concern and breaks the first requirement for multi-tenancy above (”sidecars belonging to tenant A will not receive placement information for tenant B”). A malicious sidecar could connect with an older version on purpose and see actor types that belong to other namespaces.

This behaviour will be clearly documented (”If you’re not using TLS you need to have a uniform version per namespace, either upgrade all sidecars in a namespace or none.”).

Issue reference

#4711
#3167

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Unit tests passing
End-to-end tests passing
Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
Specification has been updated / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
Provided sample for the feature / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

Signed-off-by: Elena Kolevska <elena@kolevska.com>

elena-kolevska · 2024-05-20T13:23:53Z

pkg/placement/placement.go

-	grpcServer := grpc.NewServer(sec.GRPCServerOptionMTLS())
+
+	keepaliveParams := keepalive.ServerParameters{
+		Time:    1 * time.Second,


We can discuss these numbers

👍 1 second is fairly aggressive, but not insane. gRPC library uses 5 seconds by default https://github.com/grpc/grpc-go/blob/e22436abb80982dab3294250b7284eedd8f20768/examples/features/keepalive/server/main.go#L47

We've seen recently a few examples of managed K8s clusters running in what appeared to be sporadically unreliable networks. In these cases 1 second does seem a bit aggressive, however if we're confident that reconnections shouldn't cause any widespread issues then I guess its fine to keep

Time in this case is the period of inactivity after which the server will try to ping the client to check if the stream is still alive. Knowing that the sidecars pings every second, maybe we can relax this requirement, and make it 2 seconds.
Timeout represents the duration after the ping during which the server will wait for a response, after which the connection is closed. I think we can increase this to 3 seconds, which will match the old faulty host detection timeout of 3 seconds.

JoshVanL

Some comments from me 🙂

pkg/actors/placement/placement.go

pkg/placement/leadership.go

JoshVanL · 2024-05-20T13:03:20Z

pkg/placement/leadership.go

+		leaderLoopCancel()
+		<-loopNotRunning
+	}()
+


We have three wrapped contexts here in this function, as well as double selects, onces, defers etc. which can be tricky to follow. Would using a concurrency.RunnerManager per for loop help?

This is existing code I moved from membership.go to its own file. I would like to limit changes to that part, just so we can keep the scope of the PR smaller, but I agree that this is a good candidate for refactoring.

JoshVanL · 2024-05-20T13:05:23Z

pkg/placement/leadership.go

+
+		barrier := raft.Barrier(barrierWriteTimeout)
+		if err := barrier.Error(); err != nil {
+			log.Error("failed to wait for barrier", "error", err)


Is this a transient error we can ignore? Should we not return the error here and ultimately close placement- given this could mean we can no longer write to the db (?).

Yes, this looks safe, because the loop will just restart. If there's a real problem with the raft node, it will lose its leader status anyway, which will cancel the context and the loop.

JoshVanL · 2024-05-20T13:42:09Z

tests/integration/suite/placement/dissemination/namespaced_actors.go

+					require.Len(t, placementTables.GetEntries(), 6)
+					require.Contains(t, placementTables.GetEntries(), "actor2")
+					require.Contains(t, placementTables.GetEntries(), "actor3")
+					require.Contains(t, placementTables.GetEntries(), "actor4")
+					require.Contains(t, placementTables.GetEntries(), "actor5")
+					require.Contains(t, placementTables.GetEntries(), "actor6")
+					require.Contains(t, placementTables.GetEntries(), "actor10")


I think require will cause a panic when an assertion fails inside a EventuallyWithT loop. Consider using assert, with an if check on the assertion to gate further assertions to avoid a nil pointer panic .

dapr/tests/integration/suite/actors/reminders/rebalancing.go

Line 123 in 58edd57

if assert.NoError(c, rErr) {

The requires should be ok, because they're under an if condition, but in the eventual case of them failing in the future, it's better to have them as asserts.
I also added a check for the element's presence.

tests/integration/suite/placement/dissemination/namespaced_actors_tls.go

JoshVanL · 2024-05-20T13:43:25Z

tests/integration/suite/placement/quorum/insecure.go

@@ -124,7 +126,7 @@ func (i *insecure) Run(t *testing.T, ctx context.Context) {
 		if err != nil {
 			return false
 		}
-		err = stream.Send(&v1pb.Host{Id: "app-1"})
+		err = stream.Send(&v1pb.Host{Id: "app-1", Namespace: "default"})


Please add tests which have namespace as an empty string. We should also include empty string namespace tests elsewhere if we don't already.

Yeah, we do. We have one here specific to dissemination, and there two in an auth context here and here.
I don't know if it makes sense to do the same for the quorum tests, since the namespace doesn't influence the leader election and dapr finding the right leader.

tests/integration/suite/placement/dissemination/namespaced_actors_tls.go

Signed-off-by: Elena Kolevska <elena@kolevska.com>

…/dapr into feature/namespaced-actors

Signed-off-by: Elena Kolevska <elena@kolevska.com>

…ad of EOF Signed-off-by: Elena Kolevska <elena@kolevska.com>

Signed-off-by: Elena Kolevska <elena@kolevska.com>

pkg/placement/raft/state.go

pkg/placement/raft/fsm.go

pkg/placement/pool.go

Signed-off-by: Elena Kolevska <elena@kolevska.com>

JoshVanL · 2024-06-03T21:28:52Z

pkg/actors/placement/placement.go

@@ -128,6 +130,7 @@ func NewActorPlacement(opts internal.ActorsProviderOptions) internal.PlacementSe
 		closeCh:           make(chan struct{}),
 		resiliency:        opts.Resiliency,
 		virtualNodesCache: hashing.NewVirtualNodesCache(),
+		namespace:         security.CurrentNamespace(),


Please pass as option as this function uses env var which can be awkward for testing.

JoshVanL · 2024-06-03T21:32:01Z

pkg/placement/membership.go

-							p.memberUpdateCount.Add(1)
+						// ApplyCommand returns true only if the command changes the hashing table.
+						if updateCount {
+							c, _ := p.memberUpdateCount.GetOrSet(op.host.Namespace, &atomic.Uint32{})


Are we ignoring errors here?

No, this just returns a bool indicating if the value already existed.

tests/integration/suite/placement/ha/placementfailover.go

JoshVanL · 2024-06-03T21:35:26Z

tests/integration/suite/placement/dissemination/namespacedtls.go

+func getClient(t *testing.T, ctx context.Context, addr string) rtv1.DaprClient {
+	t.Helper()
+
+	conn, err := grpc.DialContext(ctx, addr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, conn.Close()) })
+	return rtv1.NewDaprClient(conn)
+}


Suggested change

func getClient(t *testing.T, ctx context.Context, addr string) rtv1.DaprClient {

t.Helper()

conn, err := grpc.DialContext(ctx, addr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())

require.NoError(t, err)

t.Cleanup(func() { require.NoError(t, conn.Close()) })

return rtv1.NewDaprClient(conn)

}

JoshVanL · 2024-06-03T21:35:49Z

tests/integration/suite/placement/dissemination/namespacedtls.go

+	n.daprd3.WaitUntilAppHealth(t, ctx)
+
+	t.Run("host1 can see actor 1 in ns1, but not actors 2 and 3 in ns2", func(t *testing.T) {
+		client := getClient(t, ctx, n.daprd1.GRPCAddress())


Suggested change

client := getClient(t, ctx, n.daprd1.GRPCAddress())

client := n.daprd1.GRPCClient()

JoshVanL · 2024-06-03T21:38:18Z

tests/integration/suite/placement/authz/mtls.go

+		Id:        "app-1",
+		Namespace: "",
+	})
+	require.NoError(t, err)


Please check there error type returned.

We shouldn't have an error here.

JoshVanL · 2024-06-03T21:39:08Z

tests/integration/suite/placement/authz/mtls.go

-		var err error
+	var err error
+
+	require.EventuallyWithT(t, func(c *assert.CollectT) {


This Eventually isn't doing anything.

We use it as a retry until placement is ready, and then check the error outside of the function.

The Eventually is only going to be run once as c is not asserted. It doesn't seem like we need this Eventually if the tests are passing?

Ah yes! Fixed.

JoshVanL · 2024-06-03T21:39:41Z

tests/integration/framework/process/placement/placement.go

@@ -252,7 +252,7 @@ func (p *Placement) RegisterHostWithMetadata(t *testing.T, parentCtx context.Con
 		for {
 			in, err := stream.Recv()
 			if err != nil {
-				if ctx.Err() != nil || errors.Is(err, context.Canceled) || errors.Is(err, io.EOF) || status.Code(err) == codes.Canceled {
+				if ctx.Err() != nil || errors.Is(err, context.Canceled) || errors.Is(err, io.EOF) || status.Code(err) == codes.Canceled || status.Code(err) == codes.FailedPrecondition || status.Code(err) == codes.Unavailable {


Why are we doing this?

It was an old code I added to, but it doesn't make sense as it is... I changed it to return on any error.

pkg/placement/pool.go

Signed-off-by: Elena Kolevska <elena@kolevska.com>

yaron2 · 2024-06-05T23:52:40Z

Incredibly happy to see this land.

elena-kolevska added 26 commits May 16, 2024 22:11

Streams WIP

0d3a7c5

Signed-off-by: Elena Kolevska <elena@kolevska.com>

wip

e056466

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Raft state update complete. test missing

fec6294

Signed-off-by: Elena Kolevska <elena@kolevska.com>

It builds

9f7640e

Signed-off-by: Elena Kolevska <elena@kolevska.com>

More wip. Raft package finished and tested.

b5eba9d

Signed-off-by: Elena Kolevska <elena@kolevska.com>

code works, but a few unit tests are not passing

86d0d17

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Refactors ReportDaprStatus. Updates tests.

d4ffaf5

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Fixes a few tests

9bc030c

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Fixes api_level integration tests

aeaae17

Signed-off-by: Elena Kolevska <elena@kolevska.com>

wip

9b681e7

Signed-off-by: Elena Kolevska <elena@kolevska.com>

handle removing last member in namespace

36a453c

Signed-off-by: Elena Kolevska <elena@kolevska.com>

More tests and cleanups

077576b

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Temporarily point to kit in fork

e706972

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Int. tests done

49c7864

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Fixes test

23f0d16

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Lint

8c49083

Signed-off-by: Elena Kolevska <elena@kolevska.com>

fix

be5a60c

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Cleanup

73fa2a8

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Removes faultyHost check, uses grpc keepalive instead

d31dedd

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Fixes test

bf9f686

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Adds namespace to “placement/state” endpoint

7e3387e

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Adds namespace to metrics. Cleanup

6b7707e

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Convert unit to nanoseconds

11a428b

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Metrics fix

2595617

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Replaces custom atomic map with haxmap

1ef40d1

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Cleanup

e5f9e97

Signed-off-by: Elena Kolevska <elena@kolevska.com>

elena-kolevska commented May 20, 2024

View reviewed changes

JoshVanL requested changes May 20, 2024

View reviewed changes

elena-kolevska added 2 commits May 20, 2024 15:10

More tests

4b40fae

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Small fixes

518fec0

Signed-off-by: Elena Kolevska <elena@kolevska.com>

elena-kolevska and others added 6 commits May 27, 2024 01:16

Pins kit version

cb0e9fb

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Linter

1a2c18c

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Removes extra line

59d7112

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Merge branch 'master' into feature/namespaced-actors

f4fda86

Pleases linter

7e457cd

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Merge branch 'feature/namespaced-actors' of github.com:elena-kolevska…

f1ffb26

…/dapr into feature/namespaced-actors

mikeee mentioned this pull request May 28, 2024

v1.14 Release Planning #7605

Open

46 tasks

elena-kolevska added 5 commits May 29, 2024 01:42

Adds test for placement failover

4469cbc

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Fixes linter

0189d4e

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Updates test

2f39275

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Fixes linter

2fe5c70

Signed-off-by: Elena Kolevska <elena@kolevska.com>

More linter stuff

46b1a8a

Signed-off-by: Elena Kolevska <elena@kolevska.com>

hhunter-ms mentioned this pull request May 30, 2024

[Actors] Namespaced actors dapr/docs#4118

Open

elena-kolevska and others added 6 commits May 31, 2024 10:26

Adding another status code, because windows reports Unavailable inste…

14bd9f2

…ad of EOF Signed-off-by: Elena Kolevska <elena@kolevska.com>

Increase test timeout

83ddf06

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Increase time for mac os tests in CI

2d6fd0f

Signed-off-by: Elena Kolevska <elena@kolevska.com>

More time for osx runners to complete test

94df798

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Increase timeout to 30sec

fd99813

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Merge branch 'master' into feature/namespaced-actors

2913a68

yaron2 requested changes Jun 3, 2024

View reviewed changes

pkg/placement/raft/state.go Outdated Show resolved Hide resolved

pkg/placement/raft/fsm.go Outdated Show resolved Hide resolved

pkg/placement/pool.go Outdated Show resolved Hide resolved

elena-kolevska and others added 2 commits June 3, 2024 13:33

Addresses review comments

3aadf8b

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Merge branch 'master' into feature/namespaced-actors

cfadf0d

JoshVanL requested changes Jun 3, 2024

View reviewed changes

elena-kolevska added 4 commits June 3, 2024 15:18

Fixes from review

86bcf40

Signed-off-by: Elena Kolevska <elena@kolevska.com>

More fixes

8143c4b

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Removes unneeded eventually

94d6567

Signed-off-by: Elena Kolevska <elena@kolevska.com>

small fix

db40a23

Signed-off-by: Elena Kolevska <elena@kolevska.com>

yaron2 approved these changes Jun 5, 2024

View reviewed changes

yaron2 merged commit 941178c into dapr:master Jun 5, 2024
18 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespaced actors (Multitenant placement service) #7745

Namespaced actors (Multitenant placement service) #7745

elena-kolevska commented May 17, 2024

elena-kolevska May 20, 2024

JoshVanL May 20, 2024 •

edited

yaron2 May 24, 2024

elena-kolevska May 28, 2024

JoshVanL left a comment

JoshVanL May 20, 2024

elena-kolevska May 20, 2024

JoshVanL May 20, 2024

elena-kolevska May 20, 2024

JoshVanL May 20, 2024

elena-kolevska May 22, 2024

JoshVanL May 20, 2024

elena-kolevska May 22, 2024

JoshVanL Jun 3, 2024

JoshVanL Jun 3, 2024

elena-kolevska Jun 3, 2024

JoshVanL Jun 3, 2024

JoshVanL Jun 3, 2024

JoshVanL Jun 3, 2024

elena-kolevska Jun 3, 2024

JoshVanL Jun 3, 2024

elena-kolevska Jun 3, 2024

JoshVanL Jun 5, 2024

elena-kolevska Jun 5, 2024

JoshVanL Jun 3, 2024

elena-kolevska Jun 3, 2024

yaron2 commented Jun 5, 2024

	client := getClient(t, ctx, n.daprd1.GRPCAddress())
	client := n.daprd1.GRPCClient()

Namespaced actors (Multitenant placement service) #7745

Namespaced actors (Multitenant placement service) #7745

Conversation

elena-kolevska commented May 17, 2024

Description

Issue reference

Checklist

Choose a reason for hiding this comment

JoshVanL May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshVanL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaron2 commented Jun 5, 2024

JoshVanL May 20, 2024 •

edited