mds,client: validate pool against pool ids as well as pool names #45329

wxypro · 2022-03-10T06:08:41Z

Problem:
If the pool is a numeric string (eg. 23134), the current validation assumes
that it is a pool id and looks up the list of pool ids and fails validation
if the pool is not found amongst the list of pool ids.

Solution:
Since a string of digits can also be used as a pool name, the pool is validated
against list of pool names as well if it doesn't match any of the pool ids.

Fixes: https://tracker.ceph.com/issues/55165

Signed-off-by: wangxinyu wangxinyu@inspur.com

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

lxbsz · 2022-03-10T06:33:58Z

src/client/Client.cc

+    }
+    pool = osdmap->lookup_pg_pool_name(tmp);
+    if (pool < 0) {
+        return -CEPHFS_ENOENT;


Could you explain why the old code didn't work ? Did you hit any issue with that ?

If pool name is a positive integer and can be cast to unsigned. layout.pool is set to this pool name.
And then, use this name as id to ckeck the pool in osdmap.
If there is no pool with this id, it will return -ENOENT and no longer check as a pool name.

If the layout.pool is set to pool name, then the pool = boost::lexical_cast<unsigned>(tmp); should throw an exception and will be catched in catch (boost::bad_lexical_cast const&), then in the catch code it was checking with the pool name.

Didn't that work for you ?

If pool name is a positive integer，the name can be cast to unsigned. no exception will be thrown.

Do you mean the pool name will be something like "23134", which only includes digit numbers, but this is not a pool id ?

Okay. Make sense. Please explain this in the commit comment.

I'm being paranoid, but what if there are 2 pools, one with the pool name as "12345" and another pool with id as 12345.

src/client/Client.cc

lxbsz

LGTM.

vshankar · 2022-03-17T07:01:00Z

@wxypro could you please create a tracker for this?

mchangir

I'd like to suggest the following change to the commit message:

client: validate pool against pool ids as well as pool names

Problem:
If the pool is a numeric string (eg. 23134), the current validation assumes
that it is a pool id and looks up the list of pool ids and fails validation
if the pool is not found amongst the list of pool ids.

Solution:
Since a string of digits can also be used as a pool name, the pool is validated
against list of pool names as well if it doesn't match any of the pool ids.

gregsfortytwo · 2022-03-21T14:19:15Z

Jenkins test make check

vshankar · 2022-03-28T09:11:20Z

@wxypro ping?

Problem: If the pool is a numeric string (eg. 23134), the current validation assumes that it is a pool id and looks up the list of pool ids and fails validation if the pool is not found amongst the list of pool ids. Solution: Since a string of digits can also be used as a pool name, the pool is validated against list of pool names as well if it doesn't match any of the pool ids. Fixes: https://tracker.ceph.com/issues/55165 Signed-off-by: wangxinyu <wangxinyu@inspur.com>

dparmar18 · 2022-04-05T08:07:13Z

@wxypro Could you please go through the checklist and mark the boxes that are relevant?

lxbsz · 2022-04-06T01:29:03Z

src/mds/Server.cc

      try {
-	layout->pool_id = boost::lexical_cast<unsigned>(value);
+        pool = boost::lexical_cast<unsigned>(value);
+        if (!osdmap->have_pg_pool(pool)) {


s/osdmap->have_pg_pool(pool)/osdmap.have_pg_pool(pool)/

nmshelke · 2022-04-06T18:12:29Z

src/mds/Server.cc

-	layout->pool_id = pool;
+          // ignore
+      }
+      if (pool == -1) {


Can we directly move code under if block to line 5605 ? (as we are setting pool = -1 at line 5605 and checking again here)

In this case, we might not need the int64_t pool = -1; initialization on L5601

vshankar · 2022-06-01T12:20:51Z

@wxypro were you able to look at the comments?

vshankar · 2022-06-14T02:15:07Z

@wxypro ping?

github-actions · 2023-01-27T19:01:31Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

vshankar · 2023-02-02T04:56:02Z

@wxypro pinging again on this?

github-actions · 2023-04-09T13:02:10Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2023-05-09T13:18:36Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

github-actions bot added the cephfs Ceph File System label Mar 10, 2022

lxbsz reviewed Mar 10, 2022

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

wxypro force-pushed the cephfs_pool_check branch from 3392eef to 10292f6 Compare March 10, 2022 07:38

lxbsz requested a review from a team March 10, 2022 08:06

lxbsz approved these changes Mar 10, 2022

View reviewed changes

vshankar requested a review from a team March 17, 2022 06:43

mchangir reviewed Mar 21, 2022

View reviewed changes

wxypro force-pushed the cephfs_pool_check branch from 10292f6 to 40bc3d6 Compare March 23, 2022 03:37

wxypro changed the title ~~client: check as name too when client check data pool~~ mds,client: validate pool against pool ids as well as pool names Mar 23, 2022

wxypro force-pushed the cephfs_pool_check branch from 40bc3d6 to 1a94f74 Compare April 2, 2022 01:45

vshankar requested a review from dparmar18 April 4, 2022 12:46

lxbsz requested changes Apr 6, 2022

View reviewed changes

nmshelke reviewed Apr 6, 2022

View reviewed changes

djgalloway changed the base branch from master to main May 25, 2022 20:02

github-actions bot added the stale label Jan 27, 2023

github-actions bot removed the stale label Feb 2, 2023

github-actions bot added the stale label Apr 9, 2023

github-actions bot closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mds,client: validate pool against pool ids as well as pool names #45329

mds,client: validate pool against pool ids as well as pool names #45329

wxypro commented Mar 10, 2022 •

edited

lxbsz Mar 10, 2022 •

edited

wxypro Mar 10, 2022

lxbsz Mar 10, 2022 •

edited

wxypro Mar 10, 2022

lxbsz Mar 10, 2022 •

edited

wxypro Mar 10, 2022

lxbsz Mar 10, 2022

mchangir Feb 8, 2023

lxbsz left a comment

vshankar commented Mar 17, 2022

mchangir left a comment

gregsfortytwo commented Mar 21, 2022

vshankar commented Mar 28, 2022

dparmar18 commented Apr 5, 2022

lxbsz Apr 6, 2022

nmshelke Apr 6, 2022

dparmar18 Apr 6, 2022

vshankar commented Jun 1, 2022

vshankar commented Jun 14, 2022

github-actions bot commented Jan 27, 2023

vshankar commented Feb 2, 2023

github-actions bot commented Apr 9, 2023

github-actions bot commented May 9, 2023

mds,client: validate pool against pool ids as well as pool names #45329

mds,client: validate pool against pool ids as well as pool names #45329

Conversation

wxypro commented Mar 10, 2022 • edited

Checklist

lxbsz Mar 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz Mar 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz Mar 10, 2022 • edited

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

lxbsz left a comment

Choose a reason for hiding this comment

vshankar commented Mar 17, 2022

mchangir left a comment

Choose a reason for hiding this comment

gregsfortytwo commented Mar 21, 2022

vshankar commented Mar 28, 2022

dparmar18 commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vshankar commented Jun 1, 2022

vshankar commented Jun 14, 2022

github-actions bot commented Jan 27, 2023

vshankar commented Feb 2, 2023

github-actions bot commented Apr 9, 2023

github-actions bot commented May 9, 2023

wxypro commented Mar 10, 2022 •

edited

lxbsz Mar 10, 2022 •

edited

lxbsz Mar 10, 2022 •

edited

lxbsz Mar 10, 2022 •

edited