Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mon: improvements and cleanup for AuthMonitor #52008

Merged
merged 6 commits into from Jun 29, 2023
Merged

mon: improvements and cleanup for AuthMonitor #52008

merged 6 commits into from Jun 29, 2023

Conversation

rishabh-d-dave
Copy link
Contributor

The commits originally were part of #41779.

This PR contains creates and uses helper functions to not only reduce code duplication but also improve readability. Helper functions have been added for encoding keyrings (fully as well as partially), updating caps for an entity, creating new entity and checking and encoding caps. Besides, it also replace vectors by maps for storing caps.

Contribution Guidelines

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)
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

@rishabh-d-dave rishabh-d-dave requested a review from a team as a code owner June 12, 2023 10:53
@github-actions github-actions bot added the core label Jun 12, 2023
@rishabh-d-dave rishabh-d-dave changed the title mon: improvements for AuthMonitor mon: improvements and cleanup for AuthMonitor Jun 12, 2023
@rishabh-d-dave rishabh-d-dave mentioned this pull request Jun 12, 2023
Merged
4 tasks
@rishabh-d-dave
Copy link
Contributor Author

jenkins retest this please

rzarzynski
rzarzynski requested changes Jun 13, 2023
View reviewed changes
src/auth/KeyRing.h Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
rzarzynski
rzarzynski reviewed Jun 13, 2023
View reviewed changes
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
@github-actions github-actions bot added the core label Jun 13, 2023
rzarzynski
rzarzynski requested changes Jun 14, 2023
View reviewed changes
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
@rishabh-d-dave rishabh-d-dave force-pushed the improvements-authmon branch 3 times, most recently from 19ee380 to 2b07176 Compare June 15, 2023 19:10
rzarzynski
rzarzynski reviewed Jun 15, 2023
View reviewed changes
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
rzarzynski
rzarzynski requested changes Jun 15, 2023
View reviewed changes
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
@rishabh-d-dave
AuthMonitor: add a method for encoding keyring
9df4eab 
Instead of repeating same lines of code to encode entity's auth keyring
(or just key) before printing, add a method for this and use it instead.
This commit is nothing more than refactoring to avoid duplication and
improve readability; it shouldn't make any functional changes.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
rzarzynski
rzarzynski approved these changes Jun 17, 2023
View reviewed changes
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Outdated Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
src/mon/AuthMonitor.cc Show resolved Hide resolved
@rishabh-d-dave
AuthMonitor: use map, not vector, for caps
7738a64 
Although the variable "caps_vec" is defined as a vector (with following
as its members: {"mon", <moncap>, "osd", <osdcap>, "mgr", <mgrcap>,
"mds", <mdscap>}) it actually is used and iterated as a map. Simplify
the code by using a map instead of vector for storing caps.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
AuthMonitor: add helper method _check_and_encode_caps()
020ecff 
Instead of rewriting, code to check if caps are valid & to encode caps,
let's create a new helper method and use it instead.

Also, the name "new_caps" is used to store encoded caps. Let's rename
such variables to "encoded_caps".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

Made few minor changes to some of the commit messages.

@rishabh-d-dave
AuthMonitor: make code for updating caps reusable
9d3dd63 
Move the code for "ceph auth caps" command to a separate function so
that the code can be reused to update caps.

Most of the code here is same as the code for creating new entity, so
let's modify this method to also create an entity. Also, create helper
methods to update and create an entity.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
AuthMonitor: refactor valid_caps()
8627af3 
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

Minor change in commit message.

@rishabh-d-dave
Copy link
Contributor Author

@ljflores Please pick this PR for testing with RADOS suite.

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check arm64

@rishabh-d-dave
Copy link
Contributor Author

I've talked with Radoslaw and Laura. I am getting started with testing on my own.

@rishabh-d-dave rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Jun 21, 2023
@vshankar
Copy link
Contributor

I've talked with Radoslaw and Laura. I am getting started with testing on my own.

Any reason @yuriw will not run this with main branch tests?

rishabh-d-dave
rishabh-d-dave commented Jun 23, 2023
View reviewed changes
Copy link
Contributor Author

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RADOS testing went fine - https://tracker.ceph.com/projects/rados/wiki/MAIN#2023-Jun-23.

Got it double checked with Laura. Thanks to @ljflores.

@ljflores
Copy link
Contributor

@vshankar Yuri could have picked it up but @rishabh-d-dave wanted to test it himself.

@vshankar
Copy link
Contributor

vshankar commented Jun 26, 2023 •
edited

RADOS testing went fine - https://tracker.ceph.com/projects/rados/wiki/MAIN#2023-Jun-23.

Got it double checked with Laura. Thanks to @ljflores.

Any other subsystem runs required for this change?

@rishabh-d-dave
Copy link
Contributor Author

RADOS testing went fine - https://tracker.ceph.com/projects/rados/wiki/MAIN#2023-Jun-23.
Got it double checked with Laura. Thanks to @ljflores.

Any other subsystem runs required for this change?

I've tested this PR with CephFS testsuite as well.

rishabh-d-dave
rishabh-d-dave commented Jun 27, 2023
View reviewed changes
Copy link
Contributor Author

@rishabh-d-dave rishabh-d-dave left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CephFS testsuite ran fine with against this PR. For results see - https://tracker.ceph.com/projects/cephfs/wiki/Main#27-Jun-2023

@cbodley
Copy link
Contributor

cbodley commented Jun 28, 2023

Any other subsystem runs required for this change?

no need to run rgw

@rzarzynski rzarzynski merged commit 5cd4a0e into ceph:main Jun 29, 2023
11 checks passed
@rishabh-d-dave rishabh-d-dave deleted the improvements-authmon branch June 29, 2023 07:41
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 29, 2023 •
edited

@rzarzynski Many thanks for so many and so quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rzarzynski rzarzynski rzarzynski approved these changes

@vshankar vshankar Awaiting requested review from vshankar

Assignees
No one assigned
Labels
cleanup core improvements mon wip-rishabh-testing Rishabh's testing label
Projects
None yet
Milestone
No milestone
5 participants
@rishabh-d-dave @vshankar @ljflores @cbodley @rzarzynski