-
Notifications
You must be signed in to change notification settings - Fork 138
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.
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
Implement automatic Samba workgroup configuration from AD server (#2241) #2273
Implement automatic Samba workgroup configuration from AD server (#2241) #2273
Conversation
…ver (rockstor#2241). Add tests for system_directory_services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroggyFlox Thanks for an excellent pr and reporting your testing; and the added unit tests are reassuring. I've only had a look over the code and confirmed compile and unit test results:
...
test_domain_workgroup (system.tests.test_directory_services.SystemDirectoryServicesTests) ... ok
test_domain_workgroup_invalid (system.tests.test_directory_services.SystemDirectoryServicesTests) ... ok
test_domain_workgroup_missing (system.tests.test_directory_services.SystemDirectoryServicesTests) ... ok
----------------------------------------------------------------------
Ran 216 tests in 38.969s
OK
But we are still on a testing release so it should get a good test before we release to Stable anyway.
""" | ||
domain = "bogusad.bogusdomain.com" | ||
self.mock_run_command.side_effect = CommandException( | ||
err=["Didn't find the cldap server!", ""], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroggyFlox I know the following text is arbitrary for this unit test:
"Didn't find the cldap server!"
but I think it would be nice if we later edited this to be more like our actual custom exception msg which looks to be:
"Domain/Realm(bogusad.bogusdomain.com) could not be resolved. Check your DNS configuration and try again. Lower level error: [Errno -5] No address associated with hostname"
(Digression: we look to be missing a space before the bracket)
Not doing a suggestion as I'm about to merge this and it's unimportant. But it's preferred, in my opinion, for our mock output to stick as closely to the actual output as possible. This also help folk who want to extend the unit tests as they will have example output already there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, @phillxnet.
I believe the difference in error message is related to the fact that the unit test I wrote is centered on system.directory_services.domain_workgroup()
which, by itself returns that error.
If one tries to replicate the error message when configuring (and starting?) the Active Directory service with a bogus ad server, then you get the error you saw sent back from a previous step part of the general active_directory view (gethostbyname()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroggyFlox OK, I see that now. Silly me. Thanks for the schooling and the excellent addition here. We are definitely getting there now.
@FroggyFlox I'm moving towards creating a testing branch for our pending larger changes so I wanted to get this in first. Thanks for seeing to yet another rough edge and sorting yet another of our decreasing list of milestone issues. Much appreciated. This is a significantly improved user experience and thanks for going the extra mile on this one. I was anticipating something a lot more basic. |
Fixes #2241
@phillxnet, ready for review
We currently are surfacing a rather cryptic error to the user when starting the Active Directory service without having manually pre-configured the Samba service. As detailed in #2241 (comment), we can actually automatically do this configuration of the Samba service in some circumstances. This pull request (PR) thus proposes to implement such automatic configuration when possible, while surfacing a user-friendly error to the user when not possible, with instructions on the procedure.
Logic and implementation
Currently, the error occurs when trying to get the Samba service configuration from the database: if it hasn't been configured before, it naturally errors out. This PR thus simply proposes to include a
try/except
block to:"workgroup"
of the samba configuration as the AD server workgroup and saveNote that we need to take precaution in not silently overwriting the samba configuration without the user's knowledge. If the current workgroup value is already correct, everything is fine; if it differs, we raise a user-friendly error to let the user know that the Samba service configuration is currently incompatible. The entire logic is thus:
a. if the samba workgroup is the same as the AD workgroup, nothing to update
b. if the samba workgroup differs from the AD workgroup, then raise an error to the user
"workgroup"
of the samba configuration as the AD server workgroup and saveDemonstration
Configure NTP and AD services, but leave Samba service unconfigured.
Start AD service: completes successfully, and the Samba service has been automatically configured for the user:
Turn OFF the AD and Samba services.
Configure the Samba service to use a different workgroup:
MYGROUP
Start AD service ON: a user-friendly error is surfaced:
Switch the Samba service Workgroup to the correct one (
SAMDOM
) and try again to start the AD service: completes successfully.Testing
Three new unit tests were introduced, aimed at testing
system.directory_services.domain_workgroup()
as this was a simple wrapper to fetch and parse the "workgroup" information from the AD server:The full suite of tests yields:
Leap 15.2 (ISO install):
Full Testing Outputs
Leap15.2 (ISO install)