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

Broken samba config - correctly build smb.conf #1453

Merged
merged 11 commits into from Oct 2, 2016

Conversation

Projects
None yet
2 participants
@MFlyer
Member

MFlyer commented Sep 29, 2016

Opening PR to have a single space for code comments
Reference to #1404 #1407
To @schakrava : not ready to merge, will tell when ready :)

Roadmap:

  • Let our Rockstor smb builder know how to loop inside current smb file like on smb shares, better define AD section, default global section and a global custom section (only one to iterate on with custom configs) (1e5e6fd)
  • Ensure skipping AD username and password fields in smb config file ❗️ (1e5e6fd)
  • Nicely handle having custom global configs with AD ON too (currently not possible because of a check over workgroup name - not possible with AD ON, so not able to save custom config on smb global
  • Samba config page - Avoid having workgroup both in workgroup field and custom params field (42dc6f2)
  • If AD ON disable workgroup field on samba config page (not really required, workgroup value by server side checks, just for a better UX) (7daefb5 to 9919b42)
  • Code cleaning and indent 4 spaces (42dc6f2)

Testing branch:

  • Random turn off/on samba and AD
  • AD ON add custom samba params
  • AD ON change workgroup, expected to have AD workgroup
  • AD OFF change/set workgroup on samba conf page and have it
  • AD OFF add custom samba params
  • Add / remove new samba shares
  • Samba config page workgroup field switches enable/disabled on AD current status

MFlyer added some commits Sep 27, 2016

@MFlyer

This comment has been minimized.

Show comment
Hide comment
@MFlyer

MFlyer Sep 29, 2016

Member

Updates on last coding session:

Main issue was to handle workgroup to be choosen between AD workgroup or samba workgroup (manually added to samba conf page)

Current state : if AD is ON don't let custom samba config (https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/smart_manager/views/samba_service.py#L55-L61)

        if (command == 'config'):
            aso = Service.objects.get(name='active-directory')
            if (aso.config is not None):
                e_msg = ('Active Directory service is configured, so '
                         'Workgroup is automatically retrieved and cannot '
                         'be set manually')
                handle_exception(Exception(e_msg), request)

Needed new workgroup selection race conditions over samba config page:
If AD is configured and AD is ON (both required, because we can have an old AD config, but AD OFF) then use AD workgroup like samba workgroup, else use user added workgroup.
Main issue: define when AD is ON (not a simple systemctl service), so neded service_status from system/services

Member

MFlyer commented Sep 29, 2016

Updates on last coding session:

Main issue was to handle workgroup to be choosen between AD workgroup or samba workgroup (manually added to samba conf page)

Current state : if AD is ON don't let custom samba config (https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/smart_manager/views/samba_service.py#L55-L61)

        if (command == 'config'):
            aso = Service.objects.get(name='active-directory')
            if (aso.config is not None):
                e_msg = ('Active Directory service is configured, so '
                         'Workgroup is automatically retrieved and cannot '
                         'be set manually')
                handle_exception(Exception(e_msg), request)

Needed new workgroup selection race conditions over samba config page:
If AD is configured and AD is ON (both required, because we can have an old AD config, but AD OFF) then use AD workgroup like samba workgroup, else use user added workgroup.
Main issue: define when AD is ON (not a simple systemctl service), so neded service_status from system/services

MFlyer added some commits Sep 29, 2016

#1404 #1407 - Added new workgroup field to AD config, to be checked w…
…hile choosing simple samba or ad workgroup - field populated not on config, but on AD switched on
#1404 #1407 - Full working custom params over samba conf, either with…
… AD ON or OFF. Correctly select right workgroup
Over #1404 and #1407 modified services and configure service views
New view collect new param adStatus only over smb conf page and
over a new helper enable/disable workgroup field
Final commit, fixes #1404, fixes #1407 - Added info on smb config pag…
…e when workgroup field disabled - PR ready to merge
@MFlyer

This comment has been minimized.

Show comment
Hide comment
@MFlyer

MFlyer Sep 30, 2016

Member

Hi @schakrava , PR is ready to merge.

Finally no func moving from views to samba.py, had a tricky solution (AD status not so easy to collect because not a real 'service' like smb, nmb etc etc, so had to pass current ad services status between views over Backbone router - that was fan because of trying to use backbone router optional params, available from backbone 0.9.9, and i finally realized we're on 0.9.2 after 25-30 mins eheh)

Mirko

Member

MFlyer commented Sep 30, 2016

Hi @schakrava , PR is ready to merge.

Finally no func moving from views to samba.py, had a tricky solution (AD status not so easy to collect because not a real 'service' like smb, nmb etc etc, so had to pass current ad services status between views over Backbone router - that was fan because of trying to use backbone router optional params, available from backbone 0.9.9, and i finally realized we're on 0.9.2 after 25-30 mins eheh)

Mirko

Small addon over #1404 and #1407 - Added default params checker becau…
…se without it custom params lost on every samba config - no more adds required to PR
'cups options' : 'raw',
'printcap name' : '/dev/null',
'map to guest' : 'Bad User'
}

This comment has been minimized.

@schakrava

schakrava Oct 2, 2016

Member

Perhaps in a future PR, we can pull these from settings.py?

@schakrava

schakrava Oct 2, 2016

Member

Perhaps in a future PR, we can pull these from settings.py?

@schakrava

This comment has been minimized.

Show comment
Hide comment
@schakrava

schakrava Oct 2, 2016

Member

Nicely done, as always @MFlyer . Thank you!

I am not able to test all of AD right now, but things look good as far as my testing goes. Can you also describe your testing effort?

Member

schakrava commented Oct 2, 2016

Nicely done, as always @MFlyer . Thank you!

I am not able to test all of AD right now, but things look good as far as my testing goes. Can you also describe your testing effort?

@schakrava schakrava merged commit c1382c9 into rockstor:master Oct 2, 2016

@MFlyer

This comment has been minimized.

Show comment
Hide comment
@MFlyer

MFlyer Oct 2, 2016

Member

Hi @schakrava ,
checking on first PR message there's a task list related to PR testing:

First checks done switching AD to simple samba / simple samba to AD while having lot of shell cats over samba/smb.conf to check all was working, with a special care about removing USERNAME and PASSWORD vals used on AD from samba custom params
Second test group was on samba custom params and workgroup field:
before this PR Rockstor with AD active wasn't able to accept custom params because of workgroup field, so had to remove workgroup python checks and had some hacks over workgroup selection (if AD is on AD workgroup wins over samba config page workgroup)
Last part was about beautify and add another check over workgroup: Rockstor had workgroup repeated in samba custom params box, so had a new handlebars helper to avoid it, plus (not required, but it's a better way to inform users) workgroup field disable + info box if AD is on.

Huge part of this?? Define when AD is ON or OFF, because it's not a systemctl service and overlaps with simple samba too, so had a long testing and long coffees over it 😄

Member

MFlyer commented Oct 2, 2016

Hi @schakrava ,
checking on first PR message there's a task list related to PR testing:

First checks done switching AD to simple samba / simple samba to AD while having lot of shell cats over samba/smb.conf to check all was working, with a special care about removing USERNAME and PASSWORD vals used on AD from samba custom params
Second test group was on samba custom params and workgroup field:
before this PR Rockstor with AD active wasn't able to accept custom params because of workgroup field, so had to remove workgroup python checks and had some hacks over workgroup selection (if AD is on AD workgroup wins over samba config page workgroup)
Last part was about beautify and add another check over workgroup: Rockstor had workgroup repeated in samba custom params box, so had a new handlebars helper to avoid it, plus (not required, but it's a better way to inform users) workgroup field disable + info box if AD is on.

Huge part of this?? Define when AD is ON or OFF, because it's not a systemctl service and overlaps with simple samba too, so had a long testing and long coffees over it 😄

@MFlyer MFlyer deleted the MFlyer:issue#Broken_Samba_config branch Oct 2, 2016

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