Skip to content

router: protect routeall() and others from modification #502

@Serpentian

Description

@Serpentian

The function vshard.router.routeall is not marked as internal API (like it's done for vshard.router.bucket_discovery e.g.), but it returns router.replicasets table, which is used by the router itlself. If user modifies the returned from routeall table (e.g. deletes some replicaset just because he wants to skip it during request), then router is broken and cannot continue to work properly after that. Moreover, these types of errors are hard to find.

The same applies to vshard.router.route and vshard.router.bucket_discovery (internal one) functions.

Proposed solution

Create a special table of replicasets during router configuration, so that not to recreate it every time when we need to return replicasets to user. This table doesn't allow inserting of a new value or deleting of the existing one (__newindex), as otherwise the change of that table will be preserved between routeall.

Every replicaset is wrapped (it may be returned as value in routeall table or from route): replicaset has only one real field: _replicaset, which is the one we return now. It has all of the supported replicaset functions in the metatable.

This way user won't be able to modify the fields of returned tables (if he won't access rs._replicaset, which he should not if he doesn't understand, what he's doing). But the table is RO, which may not be very convenient (see alternative).

Considered alternatives

  1. table.deepcopy

table.copy or table.deepcopy may seem like a simple solution, but it's not. Connections cannot be copied. User will have to reconnect for every request after every routeall in such case.

  1. Allow to modify returning tables

The alternative solution is to create the wrapper on every routeall or route call and to wrap the replicaset on every route call. This way, user will be able to modify the returned by routeall or route table as he likes, we won't forbid adding new fields or deleting the old ones. It makes user experience less painful but requires creating it on every call (is it that costly? @Gerold103).

  1. Allow to access individual fields without modification of the router and replicas

However, user usually needs to access fields of the replicaset (e.g. bucket_count) and we don't want user to end up accessing _replicaset field. For that we modify the __index of the replicaset wrapper, allowing to access safe fields: all fields, except master_cond, on_master_required (they can be overwritten). It's forbidden to add new fields (or change _replicaset) to replicaset wrapper.

In that wrapper user still needs to access replica objects. So, it must also be wrapped. Still, all functions of the replica's metatable; only _replica real field, all internal fields can be accessed via __index, except conn. Link in replica next_by_priority points to the wrapper. master and replica in replicaset wrapper point to replica wrappers. priority_list and replicas in replicaset wrapper are the tables of wrappers, it's forbidden to modify them. It's forbidden to add new fields to replica wrapper.

It was decided to allow smth only if smb asks!

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingrouter

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions