summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAmar Tumballi <amarts@gmail.com>2019-08-06 23:20:02 +0300
committerAmar Tumballi <amarts@gmail.com>2019-09-30 17:24:08 +0000
commitae729065c0d9cb5411e5c31231b5e293b560d76a (patch)
tree74450566bd20c49cdaff8aa1826737f2b2454421
parentc6df9e962483bac5bfcd8916318b19040387ce81 (diff)
protocol/handshake: pass volume-id for extra check
With added check of volume-id during handshake, we can be sure to not connect with a brick if this gets re-used in another volume. This prevents any accidental issues which can happen with a stale client process lurking along. Also added test case for testing same volume name which would fetch a different volfile (ie, different bricks, different type), and a different volume name, but same brick. For reference: Currently a client<->server handshake happens in glusterfs through protocol/client translator (setvolume) to protocol/server using a dictionary which containes many keys. Rejection happens in server side if some of the required keys are missing in handshake dictionary. Till now, there was no single unique identifier to validate for a client to tell server if it is actually talking to a corresponding server. All we look in protocol/client is a key called 'remote-subvolume', which should match with a subvolume name in server volume file, and for any volume with same brick name (can be present in same cluster due to recreate), it would be same. This could cause major issue, when a client was connected to a given brick, in one volume would be connected to another volume's brick if its re-created/re-used. To prevent this behavior, we are now passing along 'volume-id' in handshake, which would be preserved for the life of client process, which can prevent this accidental connections. NOTE: This behavior wouldn't be applicable for user-snapshot enabled volumes, as snapshotted volume's would have different volume-id. Fixes: bz#1620580 Change-Id: Ie98286e94ce95ae09c2135fd6ec7d7c2ca1e8095 Signed-off-by: Amar Tumballi <amarts@redhat.com>
-rw-r--r--libglusterfs/src/glusterfs/glusterfs.h3
-rw-r--r--tests/bugs/bug-1620580.t51
-rw-r--r--xlators/protocol/client/src/client-handshake.c50
-rw-r--r--xlators/protocol/server/src/server-handshake.c17
-rw-r--r--xlators/protocol/server/src/server.c2
-rw-r--r--xlators/protocol/server/src/server.h1
-rw-r--r--xlators/storage/posix/src/posix-common.c8
7 files changed, 131 insertions, 1 deletions
diff --git a/libglusterfs/src/glusterfs/glusterfs.h b/libglusterfs/src/glusterfs/glusterfs.h
index 155bf43..0b2d7b1 100644
--- a/libglusterfs/src/glusterfs/glusterfs.h
+++ b/libglusterfs/src/glusterfs/glusterfs.h
@@ -596,6 +596,7 @@ struct _glusterfs_graph {
pthread_cond_t child_down_cond; /* for broadcasting CHILD_DOWN */
int parent_down;
char graph_uuid[128];
+ char volume_id[GF_UUID_BUF_SIZE];
};
typedef struct _glusterfs_graph glusterfs_graph_t;
@@ -731,6 +732,8 @@ struct _glusterfs_ctx {
pthread_cond_t janitor_cond;
pthread_mutex_t janitor_lock;
pthread_t janitor;
+
+ char volume_id[GF_UUID_BUF_SIZE]; /* Used only in protocol/client */
};
typedef struct _glusterfs_ctx glusterfs_ctx_t;
diff --git a/tests/bugs/bug-1620580.t b/tests/bugs/bug-1620580.t
new file mode 100644
index 0000000..bc113d4
--- /dev/null
+++ b/tests/bugs/bug-1620580.t
@@ -0,0 +1,51 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+. $(dirname $0)/../volume.rc
+
+cleanup;
+
+## Start glusterd
+TEST glusterd;
+TEST pidof glusterd;
+TEST $CLI volume info;
+
+## Lets create volume
+TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2};
+
+## Verify volume is created
+EXPECT "$V0" volinfo_field $V0 'Volume Name';
+EXPECT 'Created' volinfo_field $V0 'Status';
+## Start volume and verify
+TEST $CLI volume start $V0;
+EXPECT 'Started' volinfo_field $V0 'Status';
+TEST glusterfs -s $H0 --volfile-id=$V0 $M0
+
+#do some operation on mount, so that kill_brick is guaranteed to be
+#done _after_ first lookup on root
+
+TEST ls $M0
+TEST touch $M0/file
+
+TEST $CLI volume stop $V0
+TEST $CLI volume delete $V0
+
+# Case of Same volume name, but different bricks
+TEST $CLI volume create $V0 replica 2 $H0:$B0/${V0}{3,4};
+TEST $CLI volume start $V0;
+
+# Give time for 'reconnect' to happen
+sleep 4
+
+TEST ! ls $M0
+
+# Case of Same brick, but different volume (ie, recreated).
+TEST $CLI volume create $V1 $H0:$B0/${V0}{1,2};
+TEST $CLI volume start $V1;
+
+# Give time for 'reconnect' to happen
+sleep 4
+TEST ! ls $M0
+TEST ! stat $M0/file
+
+cleanup
diff --git a/xlators/protocol/client/src/client-handshake.c b/xlators/protocol/client/src/client-handshake.c
index 337b2f8..d990855 100644
--- a/xlators/protocol/client/src/client-handshake.c
+++ b/xlators/protocol/client/src/client-handshake.c
@@ -730,6 +730,7 @@ client_setvolume_cbk(struct rpc_req *req, struct iovec *iov, int count,
clnt_conf_t *conf = this->private;
dict_t *reply = NULL;
char *process_uuid = NULL;
+ char *volume_id = NULL;
char *remote_error = NULL;
char *remote_subvol = NULL;
gf_setvolume_rsp rsp = {
@@ -831,6 +832,34 @@ client_setvolume_cbk(struct rpc_req *req, struct iovec *iov, int count,
goto out;
}
+ ret = dict_get_str_sizen(reply, "volume-id", &volume_id);
+ if (ret < 0) {
+ /* this can happen if the server is of old version, so treat it as
+ just debug message */
+ gf_msg_debug(this->name, EINVAL,
+ "failed to get 'volume-id' from reply dict");
+ } else if (ctx->master && strncmp("snapd", remote_subvol, 5)) {
+ /* TODO: if it is a fuse mount or a snapshot enabled client, don't
+ bother */
+ /* If any value is set, the first element will be non-0.
+ It would be '0', but not '\0' :-) */
+ if (ctx->volume_id[0]) {
+ if (strcmp(ctx->volume_id, volume_id)) {
+ /* Ideally it shouldn't even come here, as server itself
+ should fail the handshake in that case */
+ gf_msg(this->name, GF_LOG_ERROR, EINVAL, PC_MSG_DICT_GET_FAILED,
+ "'volume-id' changed, can't connect to server. "
+ "Needs remount (%s - %s)",
+ volume_id, ctx->volume_id);
+ op_ret = -1;
+ goto out;
+ }
+ } else {
+ strncpy(ctx->volume_id, volume_id,
+ min(strlen(volume_id), GF_UUID_BUF_SIZE));
+ }
+ }
+
uint32_t child_up_int;
ret = dict_get_uint32(reply, "child_up", &child_up_int);
if (ret) {
@@ -932,6 +961,7 @@ client_setvolume(xlator_t *this, struct rpc_clnt *rpc)
};
call_frame_t *fr = NULL;
char *process_uuid_xl = NULL;
+ char *remote_subvol = NULL;
clnt_conf_t *conf = this->private;
dict_t *options = this->options;
char counter_str[32] = {0};
@@ -1014,6 +1044,26 @@ client_setvolume(xlator_t *this, struct rpc_clnt *rpc)
PACKAGE_VERSION);
}
+ ret = dict_get_str_sizen(this->options, "remote-subvolume", &remote_subvol);
+ if (ret || !remote_subvol) {
+ gf_msg(this->name, GF_LOG_WARNING, 0, PC_MSG_DICT_GET_FAILED,
+ "failed to find key 'remote-subvolume' in the options");
+ goto fail;
+ }
+
+ /* volume-id to be sent only for regular volume, not snap volume */
+ if (strncmp("snapd", remote_subvol, 5)) {
+ /* If any value is set, the first element will be non-0.
+ It would be '0', but not '\0' :-) */
+ if (this->ctx->volume_id[0]) {
+ ret = dict_set_str(options, "volume-id", this->ctx->volume_id);
+ if (ret < 0) {
+ gf_msg(this->name, GF_LOG_INFO, 0, PC_MSG_DICT_SET_FAILED,
+ "failed to set volume-id in handshake msg");
+ }
+ }
+ }
+
if (this->ctx->cmd_args.volfile_server) {
if (this->ctx->cmd_args.volfile_id) {
ret = dict_set_str_sizen(options, "volfile-key",
diff --git a/xlators/protocol/server/src/server-handshake.c b/xlators/protocol/server/src/server-handshake.c
index 17371f4..66286f4 100644
--- a/xlators/protocol/server/src/server-handshake.c
+++ b/xlators/protocol/server/src/server-handshake.c
@@ -231,6 +231,7 @@ server_setvolume(rpcsvc_request_t *req)
dict_t *config_params = NULL;
dict_t *params = NULL;
char *name = NULL;
+ char *volume_id = NULL;
char *client_uid = NULL;
char *clnt_version = NULL;
xlator_t *xl = NULL;
@@ -408,6 +409,22 @@ server_setvolume(rpcsvc_request_t *req)
client_name = "unknown";
}
+ ret = dict_get_str_sizen(params, "volume-id", &volume_id);
+ if (!ret && strcmp(xl->graph->volume_id, volume_id)) {
+ ret = dict_set_str(reply, "ERROR",
+ "Volume-ID different, possible case "
+ "of same brick re-used in another volume");
+ if (ret < 0)
+ gf_msg_debug(this->name, 0, "failed to set error msg");
+
+ op_ret = -1;
+ op_errno = EINVAL;
+ goto fail;
+ }
+ ret = dict_set_str(reply, "volume-id", tmp->volume_id);
+ if (ret)
+ gf_msg_debug(this->name, 0, "failed to set 'volume-id'");
+
client = gf_client_get(this, &req->cred, client_uid, subdir_mount);
if (client == NULL) {
op_ret = -1;
diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c
index 74453ed..1b6f394 100644
--- a/xlators/protocol/server/src/server.c
+++ b/xlators/protocol/server/src/server.c
@@ -1494,6 +1494,8 @@ server_process_child_event(xlator_t *this, int32_t event, void *data,
INIT_LIST_HEAD(&tmp->status_list);
tmp->name = gf_strdup(victim->name);
tmp->child_up = _gf_true;
+ strncpy(tmp->volume_id, victim->graph->volume_id,
+ GF_UUID_BUF_SIZE);
list_add_tail(&tmp->status_list,
&conf->child_status->status_list);
}
diff --git a/xlators/protocol/server/src/server.h b/xlators/protocol/server/src/server.h
index ab05b4f..821290b6 100644
--- a/xlators/protocol/server/src/server.h
+++ b/xlators/protocol/server/src/server.h
@@ -50,6 +50,7 @@ struct _volfile_ctx {
struct _child_status {
struct list_head status_list;
char *name;
+ char volume_id[GF_UUID_BUF_SIZE];
gf_boolean_t child_up;
};
struct server_conf {
diff --git a/xlators/storage/posix/src/posix-common.c b/xlators/storage/posix/src/posix-common.c
index c8f9217..abe086d 100644
--- a/xlators/storage/posix/src/posix-common.c
+++ b/xlators/storage/posix/src/posix-common.c
@@ -144,7 +144,13 @@ posix_notify(xlator_t *this, int32_t event, void *data, ...)
switch (event) {
case GF_EVENT_PARENT_UP: {
- /* the parent that posix xlator is up */
+ /* Notify the parent that posix xlator is up */
+ char *volume_id;
+ int ret = dict_get_str(this->options, "volume-id", &volume_id);
+ if (!ret) {
+ strncpy(this->graph->volume_id, volume_id,
+ min(strlen(volume_id), GF_UUID_BUF_SIZE));
+ }
default_notify(this, GF_EVENT_CHILD_UP, data);
} break;