[Devel] [PATCH] dump/restore: Support ipsets
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Tue Feb 11 17:07:21 MSK 2020
note: cover-letter is only required for series with multiple patches
On 2/3/20 7:27 PM, Valeriy Vdovin wrote:
> https://jira.sw.ru/browse/PSBM-100083
>
> Added ipset dump/restore functionality. At dump operation it calls
> 'ipset save' and stores result into raw text image. At restore
> it restores ipset by calling 'ipset restore'. This is done prior
> to restoring iptables.
>
> Signed-off-by: Valeriy Vdovin <valeriy.vdovin at virtuozzo.com>
> ---vz7-u13
> criu/image-desc.c | 1 +
> criu/include/image-desc.h | 1 +
> criu/include/magic.h | 1 +
> criu/net.c | 51 +++++++++++++++++++++++++++++++++++
Please split c/r and zdtm parts to two different patches, that is a
common style in criu patches.
> test/zdtm/static/Makefile | 1 +
> test/zdtm/static/netns-ipset.c | 56 +++++++++++++++++++++++++++++++++++++++
> test/zdtm/static/netns-ipset.desc | 14 ++++++++++
> 7 files changed, 125 insertions(+)
> create mode 100644 test/zdtm/static/netns-ipset.c
> create mode 100644 test/zdtm/static/netns-ipset.desc
>
> diff --git a/criu/image-desc.c b/criu/image-desc.c
> index 04e827d..475d176 100644
> --- a/criu/image-desc.c
> +++ b/criu/image-desc.c
> @@ -74,6 +74,7 @@ struct cr_fd_desc_tmpl imgset_template[CR_FD_MAX] = {
> FD_ENTRY_F(ROUTE, "route-%u", O_NOBUF),
> FD_ENTRY_F(ROUTE6, "route6-%u", O_NOBUF),
> FD_ENTRY_F(RULE, "rule-%u", O_NOBUF),
> + FD_ENTRY_F(IPSET, "ipset-%u", O_NOBUF),
> FD_ENTRY_F(IPTABLES, "iptables-%u", O_NOBUF),
> FD_ENTRY_F(IP6TABLES, "ip6tables-%u", O_NOBUF),
> FD_ENTRY_F(NFTABLES, "nftables-%u", O_NOBUF),
> diff --git a/criu/include/image-desc.h b/criu/include/image-desc.h
> index 1015191..d875722 100644
> --- a/criu/include/image-desc.h
> +++ b/criu/include/image-desc.h
> @@ -40,6 +40,7 @@ enum {
> CR_FD_ROUTE,
> CR_FD_ROUTE6,
> CR_FD_RULE,
> + CR_FD_IPSET,
> CR_FD_IPTABLES,
> CR_FD_IP6TABLES,
> CR_FD_NFTABLES,
> diff --git a/criu/include/magic.h b/criu/include/magic.h
> index 1a583f4..32421ed 100644
> --- a/criu/include/magic.h
> +++ b/criu/include/magic.h
> @@ -101,6 +101,7 @@
> #define RULE_MAGIC RAW_IMAGE_MAGIC
> #define TMPFS_IMG_MAGIC RAW_IMAGE_MAGIC
> #define TMPFS_DEV_MAGIC RAW_IMAGE_MAGIC
> +#define IPSET_MAGIC RAW_IMAGE_MAGIC
> #define IPTABLES_MAGIC RAW_IMAGE_MAGIC
> #define IP6TABLES_MAGIC RAW_IMAGE_MAGIC
> #define NFTABLES_MAGIC RAW_IMAGE_MAGIC
> diff --git a/criu/net.c b/criu/net.c
> index 8ee560c..7f57509 100644
> --- a/criu/net.c
> +++ b/criu/net.c
> @@ -1775,6 +1775,26 @@ static int restore_links()
> return 0;
> }
>
> +static int run_ipset_tool(char *arg1, char *arg2, char *arg3, int fdin, int fdout)
Do we actually need arg2 and arg3 in this function? Please remove.
> +{
> + char *cmd;
> + int ret;
> +
> + pr_debug("\tRunning ipset %s %s %s \n", arg1, arg2, arg3 ? : "");
> +
> + cmd = getenv("CR_IPSET_TOOL");
> + if (!cmd)
> + cmd = "ipset";
> +
> + ret = cr_system(fdin, fdout, -1, cmd,
> + (char *[]) { cmd, arg1, arg2, arg3, NULL }, 0);
> + if (ret) {
> + pr_err("ipset tool failed on %s %s %s \n", arg1, arg2, arg3 ? : "");
> + return -1;
> + }
> +
> + return 0;
> +}
>
> static int run_ip_tool(char *arg1, char *arg2, char *arg3, char *arg4, int fdin, int fdout, unsigned flags)
> {
> @@ -1891,6 +1911,16 @@ static inline int dump_rule(struct cr_imgset *fds)
> return 0;
> }
>
> +static inline int dump_ipset(struct cr_imgset *fds)
> +{
> + int ret;
> + struct cr_img *img;
> + img = img_from_set(fds, CR_FD_IPSET);
> + ret = run_ipset_tool("save", 0, 0, -1, img_raw_fd(img));
> + close_image(img);
Here we should not close image.
> + return ret;
> +}
> +
> static inline int dump_iptables(struct cr_imgset *fds)
> {
> struct cr_img *img;
> @@ -2129,6 +2159,23 @@ static int prepare_xtable_lock()
> return 0;
> }
>
> +static inline int restore_ipset(int pid)
> +{
> + int ret;
> + struct cr_img *img;
> + img = open_image(CR_FD_IPSET, O_RSTR, pid);
> + if (img == NULL)
> + return -1;
> + if (empty_image(img)) {
> + ret = 0;
> + goto out;
> + }
> + ret = run_ipset_tool("restore", 0, 0, img_raw_fd(img), -1);
> +out:
> + close_image(img);
> + return ret;
> +}
> +
> static inline int restore_iptables(int pid)
> {
> int ret = -1;
> @@ -2446,6 +2493,8 @@ int dump_net_ns(struct ns_id *ns)
> if (!ret)
> ret = dump_rule(fds);
> if (!ret)
> + ret = dump_ipset(fds);
> + if (!ret)
> ret = dump_iptables(fds);
> if (!ret)
> ret = dump_nftables(fds);
> @@ -2541,6 +2590,8 @@ static int prepare_net_ns_second_stage(struct ns_id *ns)
> if (!ret)
> ret = restore_rule(nsid);
> if (!ret)
> + ret = restore_ipset(nsid);
> + if (!ret)
> ret = restore_iptables(nsid);
> if (!ret)
> ret = restore_nftables(nsid);
> diff --git a/test/zdtm/static/Makefile b/test/zdtm/static/Makefile
> index 28717b1..bdef4d0 100644
> --- a/test/zdtm/static/Makefile
> +++ b/test/zdtm/static/Makefile
> @@ -143,6 +143,7 @@ TST_NOFILE := \
> poll \
> mountpoints \
> netns \
> + netns-ipset \
> netns-dev \
> session01 \
> session02 \
> diff --git a/test/zdtm/static/netns-ipset.c b/test/zdtm/static/netns-ipset.c
> new file mode 100644
> index 0000000..301f0d9
> --- /dev/null
> +++ b/test/zdtm/static/netns-ipset.c
> @@ -0,0 +1,56 @@
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "zdtmtst.h"
> +
> +const char *test_doc = "Check that ipset are dumped and restored correctly";
Please add an author line:
const char *test_author = ""
> +
> +#define RUN_OR_ERR(cmd, failmsg) if (system(cmd)) { pr_perror(failmsg); return -1; }
> +#define RUN_OR_FAIL(cmd, failmsg) if (system(cmd)) { fail(failmsg); return -1; }
> +
> +int main(int argc, char **argv)
> +{
> + char dump_ipset_old[] = "ipset save > /tmp/ipset.old";
Lets better put these files relative to the cwd. See
test/zdtm/static/netns-nf.c
> + char dump_ipset_new[] = "ipset save > /tmp/ipset.new";
> + char dump_iptables_old[] = "iptables -L INPUT 1 > /tmp/iptables.old";
> + char dump_iptables_new[] = "iptables -L INPUT 1 > /tmp/iptables.new";
> + char cmp_ipset[] = "diff /tmp/ipset.old /tmp/ipset.new";
> + char cmp_iptables[] = "diff /tmp/iptables.old /tmp/iptables.new";
> + char rm_ipset_files[] = "rm -fv /tmp/ipset.old /tmp/ipset.new";
> + char rm_iptables_files[] = "rm -fv /tmp/iptables.old /tmp/iptables.new";
> +
> + test_init(argc, argv);
> +
> + /* create ipset group and add some ip addresses to it */
> + RUN_OR_ERR("ipset create testgroup nethash", "Can't create test ipset");
Please rename testgroup to something matching the test name e.g.:
zdtm-netns-ipset or netns-ipset.
> + RUN_OR_ERR("ipset add testgroup 127.0.0.1/8", "Can't add ip addresses to ipset group");
> +
> + /* Use testgroup in iptables rule */
> + RUN_OR_ERR("iptables -I INPUT 1 -p tcp -m set --match-set testgroup src,dst -j ACCEPT",
> + "Failed to setup iptables rule with ipset group");
> +
> + /* dump ipset and iptables states to text files */
> + RUN_OR_ERR(dump_iptables_old, "Can't save iptables rules.");
> + RUN_OR_ERR(dump_ipset_old , "Can't save ipset list.");
> +
> + test_daemon();
> + test_waitsig();
> +
> + /* again dump ipset and iptables states to other text files */
> + RUN_OR_ERR(dump_iptables_new, "Can't dump restored iptables rules.");
> + RUN_OR_ERR(dump_ipset_new , "Can't save restored ipset list to file.");
> +
> + /* compare original and restored iptables rules */
> + RUN_OR_FAIL(cmp_iptables, "iptables rules differ");
> +
> + /* compare original and restored ipset rules */
> + RUN_OR_FAIL(cmp_ipset, "ipset lists differ");
> +
> + RUN_OR_ERR(rm_ipset_files, "Can't remove ipset files");
> + RUN_OR_ERR(rm_iptables_files, "Can't remove iptables files");
> +
> + pass();
> + return 0;
> +}
> diff --git a/test/zdtm/static/netns-ipset.desc b/test/zdtm/static/netns-ipset.desc
> new file mode 100644
> index 0000000..d6eefa3
> --- /dev/null
> +++ b/test/zdtm/static/netns-ipset.desc
> @@ -0,0 +1,14 @@
> +{
> + 'flavor': 'h ns uns',
> + 'flags': 'suid',
> + 'deps': [
> + '/usr/bin/rm',
> + '/usr/bin/sh',
> + '/usr/bin/diff',
> + '/usr/sbin/ipset',
> + '/usr/sbin/iptables',
> + '/lib64/libipset.so.13',
> + '/usr/lib64/xtables/libxt_set.so',
> + '/usr/lib64/xtables/libxt_standard.so|/lib/xtables/libxt_standard.so|/usr/lib/powerpc64le-linux-gnu/xtables/libxt_standard.so|/usr/lib/x86_64-linux-gnu/xtables/libxt_standard.so|/usr/lib/xtables/libxt_standard.so'
Hm, what if libraries will change path due to version change?
> + ]
> +}
>
--
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.
More information about the Devel
mailing list