mon/MonClient: handle ms_handle_fast_authentication return by batrick · Pull Request #52939 · ceph/ceph

batrick · 2023-08-10T20:26:29Z

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

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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

github-actions · 2023-08-28T13:43:03Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

rzarzynski · 2023-08-28T17:46:19Z

@batrick: #52292 is merged.

batrick · 2023-08-30T13:39:26Z

I think to test this would require having caps which parse correctly for the mon's AuthMonitor but are too new to parse for an older OSD/mgr/mds. Would you agree @idryomov @rzarzynski ?

src/mon/MonClient.cc

src/mon/Monitor.cc

src/msg/Dispatcher.h

idryomov · 2023-08-30T20:04:04Z

I think to test this would require having caps which parse correctly for the mon's AuthMonitor but are too new to parse for an older OSD/mgr/mds.

Most likely, given that the only thing these handlers do is parse the caps string.

batrick · 2023-10-09T15:50:08Z

I think to test this would require having caps which parse correctly for the mon's AuthMonitor but are too new to parse for an older OSD/mgr/mds.

Most likely, given that the only thing these handlers do is parse the caps string.

Okay, I'll have to think of an easy way to test this. Alternatively we can just skip tests as risk is low, IMO.

batrick · 2023-10-20T14:46:51Z

src/msg/Dispatcher.h

-  virtual int ms_handle_fast_authentication(Connection *con) {
-    return 0;
+  virtual bool ms_handle_fast_authentication(Connection *con) {
+    return false;


@idryomov actually this should probably be true, yes? Otherwise every dispatcher that does not implement this method will fail here. I'll make this change.

I think it's dangerous to provide a default "yup, authenticated" implementation. I would rather make this a pure virtual.

So turning this into a pure virtual broke the build and required every dispatcher to be modified, even in places where it makes no sense, e.g. MonClient. I decided it's better to leave it as a regular virtual method which returns true.

batrick · 2023-10-20T14:48:06Z

I think to test this would require having caps which parse correctly for the mon's AuthMonitor but are too new to parse for an older OSD/mgr/mds.

Most likely, given that the only thing these handlers do is parse the caps string.

Given how brittle the test would be, I think we'll just take this as-is. Please review at your convenience @idryomov .

idryomov

I see two ms_handle_fast_authentication() calls in Monitor::handle_auth_request() where the return value is not checked. Is that intentional?

src/mon/Monitor.cc

src/osd/OSD.cc

idryomov · 2023-10-20T18:08:59Z

src/msg/Dispatcher.h

-  virtual int ms_handle_fast_authentication(Connection *con) {
-    return 0;
+  virtual bool ms_handle_fast_authentication(Connection *con) {
+    return false;


I think it's dangerous to provide a default "yup, authenticated" implementation. I would rather make this a pure virtual.

idryomov · 2023-10-20T18:11:01Z

Given how brittle the test would be, I think we'll just take this as-is.

This is up to @rzarzynski.

batrick · 2023-10-23T20:03:53Z

I see two ms_handle_fast_authentication() calls in Monitor::handle_auth_request() where the return value is not checked. Is that intentional?

~~Taking a look at this now.~~ Done.

rzarzynski · 2023-11-09T14:45:46Z

src/osd/OSD.cc

  AuthCapsInfo &caps_info = con->get_peer_caps_info();
  if (caps_info.allow_all) {
    s->caps.set_allow_all();
+    return true;


rzarzynski · 2023-11-09T14:46:45Z

src/osd/OSD.cc

-		 << " failed to parse caps '" << str << "'" << dendl;
-	ret = -EACCES;
-      }
+      return false;


rzarzynski · 2023-11-09T14:46:55Z

src/osd/OSD.cc

+      dout(10) << __func__ << " session " << s
+               << " " << s->entity_name
+               << " has caps " << s->caps << " '" << str << "'" << dendl;
+      return true;


rzarzynski · 2023-11-09T14:47:03Z

src/osd/OSD.cc

+    } else {
+      dout(10) << __func__ << " session " << s << " " << s->entity_name
+               << " failed to parse caps '" << str << "'" << dendl;
+      return false;


rzarzynski · 2023-11-09T14:51:16Z

src/osd/OSD.cc

+      return false;
+    }
+  } else {
+    return false;


This handles the original ret eq 0 case (neither allow_all nor caps_info.caps.length() > 0). This blurs the information with EACCESS but I think it's fine.

rzarzynski · 2023-11-09T14:57:06Z

src/mon/Monitor.cc

    s->caps.set_allow_all();
    s->authenticated = true;
-    ret = 1;
+    return true;


rzarzynski · 2023-11-09T14:57:23Z

src/mon/Monitor.cc

 	   << " in auth db" << dendl;
-      str.clear();
-      ret = -EACCES;
+      return false;


rzarzynski · 2023-11-09T14:57:39Z

src/mon/Monitor.cc

-      }
+    if (s->caps.parse(str, NULL)) {
+      s->authenticated = true;
+      return true;


rzarzynski · 2023-11-09T14:57:52Z

src/mon/Monitor.cc

+    } else {
+      derr << __func__ << " unparseable caps '" << str << "' for "
+           << con->get_peer_entity_name() << dendl;
+      return false;


rzarzynski · 2023-11-09T15:02:54Z

src/msg/Dispatcher.h

-  virtual int ms_handle_fast_authentication(Connection *con) {
-    return 0;
+  virtual bool ms_handle_fast_authentication(Connection *con) {
+    return true;


Any reason for not going pure virtual?

Dispatchers that don't handle authentication would need to have the method which seemed unnecessarily obtrusive.

I share the @idryomov's concern.

How about introducing an intermediary stage (with the default implementation) in the inheritance hierarchy to address those auth-uninterested dispatchers explicitly?

$ grep -r ms_handle_fast_authentication ./ ./mon/MonClient.cc: handle_authentication_dispatcher->ms_handle_fast_authentication(con); ./mon/MonClient.cc: handle_authentication_dispatcher->ms_handle_fast_authentication(con); ./mon/Monitor.h: int ms_handle_fast_authentication(Connection *con) override; ./mon/AuthMonitor.cc: mon.ms_handle_fast_authentication(s->con.get()) > 0) { ./mon/Monitor.cc: ms_handle_fast_authentication(con); ./mon/Monitor.cc: ms_handle_fast_authentication(con); ./mon/Monitor.cc:int Monitor::ms_handle_fast_authentication(Connection *con) ./msg/Dispatcher.h: virtual int ms_handle_fast_authentication(Connection *con) { ./mds/MDSDaemon.h: int ms_handle_fast_authentication(Connection *con) override; ./mds/MDSDaemon.cc:int MDSDaemon::ms_handle_fast_authentication(Connection *con) ./mgr/DaemonServer.h: int ms_handle_fast_authentication(Connection *con) override; ./mgr/DaemonServer.cc:int DaemonServer::ms_handle_fast_authentication(Connection *con) ./osd/OSD.h: int ms_handle_fast_authentication(Connection *con) override { ./osd/OSD.h: int ms_handle_fast_authentication(Connection *con) override; ./osd/OSD.cc:int OSD::ms_handle_fast_authentication(Connection *con) ./test/fio/fio_ceph_messenger.cc: int ms_handle_fast_authentication(Connection *con) override { ./test/msgr/perf_msgr_client.cc: int ms_handle_fast_authentication(Connection *con) override { ./test/msgr/test_msgr.cc: int ms_handle_fast_authentication(Connection *con) override { ./test/msgr/test_msgr.cc: int ms_handle_fast_authentication(Connection *con) override { ./test/msgr/test_msgr.cc: int ms_handle_fast_authentication(Connection *con) override { ./test/msgr/perf_msgr_server.cc: int ms_handle_fast_authentication(Connection *con) override {

$ grep -r public\ Dispatcher ./ ./mon/Monitor.h:class Monitor : public Dispatcher, ./mon/MonClient.h:struct MonClientPinger : public Dispatcher, ./mon/MonClient.h:class MonClient : public Dispatcher, ./neorados/RADOSImpl.h:class RADOS : public Dispatcher ./tools/cephfs_mirror/ClusterWatcher.h:class ClusterWatcher : public Dispatcher { ./tools/cephfs/MDSUtility.h:class MDSUtility : public Dispatcher { ./librados/RadosClient.h:class librados::RadosClient : public Dispatcher, ./librbd/io/ImageDispatcher.h:class ImageDispatcher : public Dispatcher<ImageCtxT, ImageDispatcherInterface> { ./librbd/io/ObjectDispatcherInterface.h: : public DispatcherInterface<ObjectDispatchInterface> { ./librbd/io/ObjectDispatcher.h: : public Dispatcher<ImageCtxT, ObjectDispatcherInterface> { ./librbd/io/ImageDispatcherInterface.h: : public DispatcherInterface<ImageDispatchInterface> { ./osdc/Objecter.h:class Objecter : public md_config_obs_t, public Dispatcher { ./mds/MDSDaemon.h:class MDSDaemon : public Dispatcher { ./mds/Beacon.h:class Beacon : public Dispatcher ./mds/MetricsHandler.h:class MetricsHandler : public Dispatcher { ./mds/MetricAggregator.h:class MetricAggregator : public Dispatcher { ./mgr/MgrStandby.h:class MgrStandby : public Dispatcher, ./mgr/DaemonServer.h:class DaemonServer : public Dispatcher, public md_config_obs_t ./mgr/MgrClient.h:class MgrClient : public Dispatcher ./osd/OSD.h:class OSD : public Dispatcher, ./osd/OSD.h: struct HeartbeatDispatcher : public Dispatcher { ./test/crimson/test_messenger.cc:class FailoverSuite : public Dispatcher { ./test/crimson/test_messenger.cc:class FailoverTest : public Dispatcher { ./test/crimson/test_messenger.cc:class FailoverSuitePeer : public Dispatcher { ./test/crimson/test_messenger.cc:class FailoverTestPeer : public Dispatcher { ./test/crimson/test_messenger_peer.cc:class FailoverSuitePeer : public Dispatcher { ./test/crimson/test_messenger_peer.cc:class FailoverTestPeer : public Dispatcher { ./test/fio/fio_ceph_messenger.cc:class FioDispatcher : public Dispatcher { ./test/mon/test_mon_workloadgen.cc:class TestStub : public Dispatcher ./test/mon/test-mon-msg.cc:class MonClientHelper : public Dispatcher ./test/direct_messenger/test_direct_messenger.cc:class MockDispatcher : public Dispatcher { ./test/testmsgr.cc:class Admin : public Dispatcher { ./test/msgr/perf_msgr_client.cc: class ClientDispatcher : public Dispatcher { ./test/msgr/test_msgr.cc:class FakeDispatcher : public Dispatcher { ./test/msgr/test_msgr.cc:class SyntheticDispatcher : public Dispatcher { ./test/msgr/test_msgr.cc:class MarkdownDispatcher : public Dispatcher { ./test/msgr/perf_msgr_server.cc:class ServerDispatcher : public Dispatcher { ./client/Client.h:class Client : public Dispatcher, public md_config_obs_t {

For many of them the burden could as low as running sed.

I think my concern in

#52939 (comment)

was not actually an issue. So I've changed the default return to false. That should render this conversation moot.

batrick · 2024-06-04T20:58:32Z

trivial rebase

batrick · 2024-06-10T20:27:58Z

jenkins test windows

batrick · 2024-06-11T17:04:31Z

This PR is under test in https://tracker.ceph.com/issues/66433.

batrick · 2024-06-13T01:49:44Z

This PR is under test in https://tracker.ceph.com/issues/66462.

* refs/pull/52939/head: mon/MonClient: handle ms_handle_fast_authentication return

batrick · 2024-06-22T14:50:29Z

This PR is under test in https://tracker.ceph.com/issues/66609.

batrick · 2024-06-23T18:25:38Z

@idryomov @rzarzynski this is ready for merge; just blocked on your review please.

src/mgr/DaemonServer.h

batrick · 2024-06-24T15:48:37Z

jenkins test make check arm64

batrick · 2024-06-28T15:47:36Z

jenkins test make check

Fixes: https://tracker.ceph.com/issues/62382 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick · 2024-07-02T14:46:09Z

jenkins test make check

batrick · 2024-07-02T14:46:14Z

jenkins test make check arm64

batrick · 2024-07-02T14:46:20Z

jenkins test api

batrick · 2024-07-03T14:25:13Z

jenkins test make check arm64

batrick · 2024-07-03T14:30:23Z

This PR is under test in https://tracker.ceph.com/issues/66822.

batrick · 2024-07-23T19:52:14Z

jenkins test make check arm64

batrick · 2024-07-25T17:39:19Z

jenkins test make check arm64

batrick · 2024-07-26T20:27:01Z

This PR is under test in https://tracker.ceph.com/issues/67214.

batrick added the core label Aug 10, 2023

github-actions bot added cephfs Ceph File System mgr mon labels Aug 10, 2023

github-actions bot added the needs-rebase label Aug 28, 2023

batrick force-pushed the i62382 branch from 51a9e97 to a064fb3 Compare August 30, 2023 12:53

github-actions bot removed the needs-rebase label Aug 30, 2023

idryomov reviewed Aug 30, 2023

View reviewed changes

src/mon/MonClient.cc Outdated Show resolved Hide resolved

src/mon/Monitor.cc Show resolved Hide resolved

src/mon/Monitor.cc Show resolved Hide resolved

src/msg/Dispatcher.h Outdated Show resolved Hide resolved

batrick force-pushed the i62382 branch from a064fb3 to ea9eb0e Compare October 9, 2023 15:48

batrick force-pushed the i62382 branch from ea9eb0e to 1dca85d Compare October 20, 2023 14:43

batrick commented Oct 20, 2023

View reviewed changes

batrick force-pushed the i62382 branch from 1dca85d to 0b70dae Compare October 20, 2023 14:46

batrick marked this pull request as ready for review October 20, 2023 14:47

batrick requested a review from a team as a code owner October 20, 2023 14:47

idryomov reviewed Oct 20, 2023

View reviewed changes

batrick force-pushed the i62382 branch from 0b70dae to 56e57ee Compare October 23, 2023 16:23

github-actions bot added the tests label Oct 23, 2023

batrick force-pushed the i62382 branch from 56e57ee to 8d0835b Compare October 23, 2023 18:10

batrick force-pushed the i62382 branch from 8d0835b to f0cd8f5 Compare October 24, 2023 00:47

batrick requested a review from rzarzynski October 24, 2023 12:13

batrick added the needs-review label Oct 24, 2023

rzarzynski reviewed Nov 9, 2023

View reviewed changes

ceph deleted a comment from github-actions bot Jun 4, 2024

batrick requested a review from idryomov June 10, 2024 20:28

batrick added the wip-pdonnell-testing label Jun 11, 2024

batrick added a commit to batrick/ceph that referenced this pull request Jun 13, 2024

Merge PR ceph#52939 into wip-pdonnell-testing-20240613.014923-debug

119deb3

* refs/pull/52939/head: mon/MonClient: handle ms_handle_fast_authentication return

rzarzynski approved these changes Jun 23, 2024

View reviewed changes

src/mgr/DaemonServer.h Show resolved Hide resolved

batrick force-pushed the i62382 branch from b105043 to 29791a0 Compare June 24, 2024 13:08

batrick removed the needs-review label Jun 24, 2024

mon/MonClient: handle ms_handle_fast_authentication return

bfbfbbf

Fixes: https://tracker.ceph.com/issues/62382 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick force-pushed the i62382 branch from 29791a0 to bfbfbbf Compare June 28, 2024 20:47

batrick merged commit 3888ac8 into ceph:main Aug 6, 2024

batrick deleted the i62382 branch August 6, 2024 21:06

This was referenced Aug 19, 2024

squid: mon/MonClient: handle ms_handle_fast_authentication return #59306

Merged

reef: mon/MonClient: handle ms_handle_fast_authentication return #59307

Merged

quincy: mon/MonClient: handle ms_handle_fast_authentication return #59308

Merged

Conversation

batrick commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

github-actions bot commented Aug 28, 2023

Uh oh!

rzarzynski commented Aug 28, 2023

Uh oh!

batrick commented Aug 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

idryomov commented Aug 30, 2023

Uh oh!

batrick commented Oct 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

batrick commented Oct 20, 2023

Uh oh!

idryomov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idryomov commented Oct 20, 2023

Uh oh!

batrick commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

batrick commented Jun 4, 2024

Uh oh!

batrick commented Jun 10, 2024

Uh oh!

batrick commented Jun 11, 2024

Uh oh!

batrick commented Jun 13, 2024

Uh oh!

batrick commented Jun 22, 2024

Uh oh!

batrick commented Jun 23, 2024

Uh oh!

Uh oh!

batrick commented Jun 24, 2024

batrick commented Aug 10, 2023 •

edited

Loading

batrick commented Oct 23, 2023 •

edited

Loading