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

smb: improve smb module test coverage #57394

Draft
wants to merge 50 commits into
base: main
Choose a base branch
from
Draft

smb: improve smb module test coverage #57394

wants to merge 50 commits into from

Conversation

phlogistonjohn
Copy link
Contributor

@phlogistonjohn phlogistonjohn commented May 10, 2024 •
edited

Depends on #57339

After adding a bunch of features and changing this we lacked test coverage in some areas. While it's not anywhere near 100% a bunch more things, esp with regards to error handling, are now covered.

In addition, I moved some stuff into a utils.py because it was easier to add/move tests for these utils to a new file. Stuff like proto.py and enums.py do not need test functions otherwise (they're largely mypy or import-time things).

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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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
  • jenkins test rook e2e

@phlogistonjohn
mgr/smb: make handler's resource ordering a top-level function
813927a 
There's no need for order_resources to be a method. Make order_resources
a proper function.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: move handler's resource check logic into functions
2050af4 
There's no need for these to be methods as they're largely
self-contained. Functions should be easier to unit-test independently in
the future too.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: split ConfigStore protocol listing methods out
13867d3 
Split the ConfigStore protocol into a new base protocol that covers the
methods needed to list keys in a store, called ConfigStoreListing.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: extend key fetching support in internal store helper module
fbd2f9c 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add staging area class to handler
1eaaeb4 
This in-memory staging area for new resources being applied to the
config helps simplify validation.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add linked_to_cluster fields for JoinAuth and UsersAndGroups
5c7ae87 
The linked_to_cluster field will be used for resources that must be
life-cycled along side the cluster using them. This is mainly intended
for use with `cluster create` command line commands.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: update handler to make use of linked_to_cluster fields
d36755a 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add rand_name function
a4bad24 
This will be used later to create resources that are not directly named
by a user.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add handler tests for new linked_to_cluster field
7605397 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add squash method to ResultGroup
e05121f 
The new squash method works similarly to `one` but permits multiple
resources to be reported on by moving them into the report for the
"parent" resource. This is meant for upcoming changes to `cluster
create` when it starts using linked join auths and linked
users-and-groups.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: convert smb module cluster create to use linked resources
00f2f71 
Convert the smb mgr module cli to use linked join auth and
users-and-groups resources when created on the `cluster create` command
line. These linked resources with have semi-ranodmized names and be
linked to the cluster that we're creating on the CLI.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: remove embedded join source and users and groups types
c6b0377 
Drop the embedded versions of the join source and usrers and groups
configs from the cluster resource type. This means that potentially
sensitive information is *always* stored in a separate resource
from the less-sensitive general cluster config. Future changes can
then make security related choices about those specific resources
without worrying about having embedded variants inside the cluster.

For convenience I added a new "emtpy" users and groups source type that
can be used for testing. It defines *no* users and groups (an empty set
basically). This wouldn't be useful in a production scenario today as it
would deploy an smbd that you couldn't log into. But for unit testing
it's extremely handy, and maybe it can be used for integration testing
too.

This is a large change because lots of unit tests relied on embedded
join auths or users-and-groups. I tried to handle all the breaking
changes in one go for bisect-ability.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: filter out python version blocks from coverage
63be58e 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
doc/mgr/smb: remove embedded join auth and users/groups source types
9255f19 
Update the documentation to match the code, removing references to
source_types 'inline' and 'password' and uris. Adding documentation
of the new linked_to_cluster field.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: cache new and stored keys separately in the staging area
6b5eef9 
We would like a quick way of determining what is new. Add methods to the
staging area class to make it possible to do so using (cached) keys.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add create_only arg for handler apply function
a0555a3 
Add a create_only argument to the handler class apply function. This
flag is used to prevent modification of existing resources. This flag
will be use by 'cluster create' and 'share create' commands to make
them true to their names and not sneaky modify-or-create commands.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add a unit test to cover apply func create_only argument
065700d 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: share and cluster create commands only create resources
dcfe898 
Prior to this change the create commands could be used counter to the
term 'create' as a create-or-update command. IMO this violates the
principle of least surprise so make them create-only.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add error handling & conversion hook to resourcelib
60b1581 
Add a method to supply the Resource instances with a callback that
can handle errors that occur during object construction from simplified
data. If the callback is not set, the exceptions are handled as usual.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add resource construction error handling exception
0cc7a5f 
Use error hook function to wrap plain ValueError instances to an
InvalidResourcError. This error retains the simplified data being
(re)constructed into an object and thus will be used later to generate
proper error results.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add result type for reporting resource validation errors
08b985d 
This error type can not take a real resource object because the resource
object could not be constructed from the data. Use the raw data for
reporting the error result.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: allow ResultGroup to take an initial list of results
0bdaa3d 
Can save a line of code later.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: use correct module when referring to result classes
8c7984e 
The result classes are not based in handler.py (any more) but the
module.py code was not updated to reflect that. Update it now.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: convert failures to create a valid resource into error results
5d6c518 
Convert failures to create a valid resource into error results that can
be reported back to the caller like the other error result types
generated by actually attempting to apply the resources.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add validation funcs for custom parameter dictionaries
ee636d6 
Custom parameter dictionaries will be used to pass options to samba
config without much filtering and control by the smb mgr module. Because
the risks that it entails the user must "agree" that using these options
can break their setup with a "magic" key-value pair.
This pair will be filtered out of the eventual data passed to samba.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add custom config options to share and cluster resources
1ee6d35 
Allow devs/testers/experimenters to add custom smb config params to our
managed shares or clusters. Use at your own risk.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: pass custom options to samba(cc) configuration
c7cc8d4 
Pass the custom share and/or global option key-value pairs to the
generated configurations.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
doc: add subsections on custom share and custom global options
20b2f9e 
Document the options, the risk of using it, and the "magic key-value"
pair needed to enable it.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add unit tests for custom_options funcs
a0b3545 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add enums for managing share login access control
e026757 
Add enums that will be used to create a share login control attribute.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add validation function for share login names
483aa97 
Add a validation function that will be used for a future share login
control attribute.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add login_control and restrict_access attributes to share
2d73d78 
The login_control attribute contains a list of LoginAccessEntry objects.
These will be mapped to samba share access parameters.
The restrict_access attribute is a boolean that makes access to the
share contingent on being listed in the login_control list, as
either a listed user or member of a listed group, for any access
level other than 'none' (as that always denies access).

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: include login_control content when generating share config
1337f90 
The login_control list (modified by restrict_access) defines the
smb.conf params 'read list', 'write list', 'admin users', 'invalid
users', and 'valid users'.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add validation unit tests for login names
cbebe8d 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add unit tests for share login_control attribute
f98e211 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add handler unit tests for login_control in share generation
9ca66ba 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
doc: document smb share login_control and restrict_access options
1adf94c 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: move a handful of small functions to utils.py
242ae29 
Includes updating the files that were using them and even
a few test functions.
Amusingly, the `one` function was being tested as part of
test_handler.py demonstrating the confusion that not having
a utils.py and corresponding test file brought.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add test coverage for all utils.py functions
88d07fb 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add test coverage for expanding login access enum
0fab2b4 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: fix some incorrect f-strings when raising validation error
8604911 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add more test cases for resource validation errors
46f3467 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: fix and test load_text function
b5b6b8d 
The load_text function was a bit buggy but mainly worked until I started
to try and write a proper test for it. The error handling here is a bit
of a hack, but we can refine it later if people notice/complain.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: fix an incorrect comment
0d94895 
At one point the comment was true but I ended doing a workaround
enabling a wrapper in proto.py of typing_extensions.Self and using it
here turning the comment untrue.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: extra check that we don't iterate over non-lists
98c7180 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: make InvalidResourceError based on ErrorResponseBase
b868d87 
Make InvalidResourceError based on ErrorResponseBase as well as
ValueError for consistency with the InvalidInputError class and in the
case it ever fails to be wrapped by a Response type.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: improve error case test coverage for handler
4f4efbd 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add heavily mocked unit test for fs.py module
7874c08 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add test coverage for creating users with cluster
3674379 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
mgr/smb: add test coverage for show command
634d110 
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@github-actions GitHub Actions
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
documentation mgr needs-rebase pybind
Projects
None yet
Milestone
No milestone
1 participant
@phlogistonjohn