dayglojesus / managedmac

Comprehensive Puppet module for OS X.
http://dayglojesus.github.io/managedmac/
Apache License 2.0
62 stars 21 forks source link

managedmac::activedirectory::provider: 'mobileconfig' should be deprecated #72

Closed dayglojesus closed 9 years ago

dayglojesus commented 9 years ago

The Dsconfigad provider should deprecate Mobileconfig as the default provider.

I am not sure who is currently using managedmac or this class, but this AD bind via profile methodology needs to be deprecated and eventually removed.

groob commented 9 years ago

Hi,

I'm using the mobileconfig provider for AD binding. Will switching from one to the other require any intervention?

I'll run some tests.

dayglojesus commented 9 years ago

Thanks for chiming in @groob. There is a feature branch named dsconfigad, but the feature is not done yet. When it is, you can expect no significant change to the class parameters/interface -- your present Hiera YAML should work the same way -- but there will be more options available, including a choice of provider.

I would recommend reviewing the new class documentation: https://github.com/dayglojesus/managedmac/blob/dsconfigad/manifests/activedirectory.pp#L3-L65

I am aiming to deprecate the default provider in 0.7.0 and remove it entirely at some point after that.

Please share your thoughts on the deprecation timeline and any comments on the basic strategy.

Cheers!

clburlison commented 9 years ago

@dayglojesus I am in favor of the new provider. The profile issues described in the comments of the activedirectory class are the reason I never started to bind via a profile a year ago. Once the feature branch is complete I will be sure to test the new provider.

Since, I am not currently using the activedirectory class your timeline does not affect my environment.

dayglojesus commented 9 years ago

Thanks @clburlison. Yeah, binding via the profile was an experiment. I really hoped it would work out, but it simply caused too many issues.

The new provider will be more extensible.

In testing, things we may want to watch out for are timeouts and hangs. Simulating a crappy network or one where the domain is unavailable in our different environments could produce some interesting results (read: edge cases).

Let me know what you find!

Cheers.

dayglojesus commented 9 years ago

I've created a milestone for this change: https://github.com/dayglojesus/managedmac/milestones/0.7.0

dayglojesus commented 9 years ago

@groob @clburlison This branch is ready for testing. In theory, if you are already using the Mobileconfig provider, setting managedmac::activedirectory::provider to dsconfigad should have no net effect.

That is to say, all of the class parameters and values should be idempotent regardless of type/provider.

However, there may be some slightly different behaviour where default values are concerned. You will notice that in the last few commits, I've made some changes that could affect your setup if you explicitly setting empty/nil/undef values.

In any case, I would really appreciate some feedback on this. Cheers.

dayglojesus commented 9 years ago

This feature branch has been merged into 0.6.0

clburlison commented 9 years ago

I was able to test the dsconfigad provider and I am receiving the following output:

Error: Could not autoload puppet/provider/dsconfigad/default: cannot load such file -- pry
Error: Could not autoload puppet/type/dsconfigad: Could not autoload puppet/provider/dsconfigad/default: cannot load such file -- pry
Error: Could not autoload puppet/type/dsconfigad: Could not autoload puppet/provider/dsconfigad/default: cannot load such file -- pry on node testvm.local
Error: Could not autoload puppet/type/dsconfigad: Could not autoload puppet/provider/dsconfigad/default: cannot load such file -- pry on node testvm.local

Any ideas what the pry error is pointing to? The virtual machine is able to bind via the old provider. I can provider more info if needed just not sure what details would help.

dayglojesus commented 9 years ago

I am the worst. I always forget to remove debugging. Should be fixed with...

https://github.com/dayglojesus/managedmac/commit/f1ac56b307a5d92dd3a450cf14393c2717490b48

Thanks @clburlison !

clburlison commented 9 years ago

New issue. The provider isn't finding the computer name. https://github.com/dayglojesus/managedmac/blob/0.6.0/lib/puppet/provider/dsconfigad/default.rb#L215-L223

Notice: Compiled catalog for testvm.local in environment production in 2.97 seconds
Info: Applying configuration version '1425656311'
Notice: /Stage[main]/Managedmac::Activedirectory/Dsconfigad[bisd.k12]/ensure: created
Notice: Dsconfigad[bisd.k12](provider=default): Binding to domain...
Error: /Stage[main]/Managedmac::Activedirectory/Dsconfigad[bisd.k12]: Could not evaluate: Missing required parameter: computer is invalid or empty
Notice: Finished catalog run in 13.66 seconds

Should the provider use the hostname fact instead?

011-adm-testvm:~ techsupport$ sudo facter -p computername

011-adm-testvm:~ techsupport$ sudo facter -p hostname
011-adm-testvm
011-adm-testvm:~ techsupport$ 

Edit: I now realize :computer is a variable that the provider isn't finding. The issue remains however my previous logic is wrong.

dayglojesus commented 9 years ago

@clburlison this is expected, but I realize now I forgot to document it! Sorry...

If you use the dsconfigad provider, you will need to supply: managedmac::activedirectory::computer. This is new behaviour, but it is related to the /usr/sbin/dsconfigad operates -- it does not require you to specify a computer value, but if you do not, it will automatically select one for you based on your current DNS name.

This is almost always a bad thing and something I went to great lengths to avoid in my own setup whilst using the Mobileconfig provider. I made it a required parameter in the managedmac::activedirectory class and the provider because I felt it might help prevent some people from binding to their AD with bad hostnames. However, I am open to discussing this. It is fairly atypical of my design decision making process as it pertains to types/providers where I usually try to express the original specifications of any utility I may be abstracting.

Let me know if you feel differently or if you have any suggestions on how to handle this.

In the meantime, I have created issue #73.

clburlison commented 9 years ago

@dayglojesus No problem. I am here to finding error, typos, and helping with documentation. I understand your logic for this move and fully support it. The DNS issue you refer to has happened in my environment as well.

Would you be apposed to making the default for the computer parameter the facter fact sp_local_host_name. If I am reading the docs correctly that will get the value from the system profile which should resolve the DNS issue with /usr/sbin/dsconfigad? And if I am reading your provider correctly, you already have the logic in place for when system profiler outputs a blank name.

If not we can always add this in our heira data (for reference purposes): managedmac::activedirectory::computer: "%{::sp_local_host_name}"

dayglojesus commented 9 years ago

@clburlison I think I am more in favour allowing the class to fail when computer isn't supplied. I kind of see the failure as a "feature". :smile: Adding a default value, while I understand the impulse, might mask a configuration problem (it's non-deterministic).

"And if I am reading your provider correctly, you already have the logic in place for when system profiler outputs a blank name."

Not sure about this... There are no calls to `system_profiler -- can you expand?

clburlison commented 9 years ago

I have no problem with making the computer parameter a requirement. I just think that is how most would logically bind to the their AD environment.

The facter sp_local_host_name fact does the call system_profiler. What I was trying to say is that using that fact should be a "sane" default and if system_profiler returned null value to facter your provider would simply give the error message Could not evaluate: Missing required parameter: computer is invalid or empty.

dayglojesus commented 9 years ago

@groob Have you had time to test this branch? Any findings to report?

clburlison commented 9 years ago

@dayglojesus Finished testing this provider. All issues I had have been addressed in the latest commits (mostly documentation related). I tried to produce network connectivity issues by playing with /etc/hosts and this provider did it's job. No more issues to report from me.

dayglojesus commented 9 years ago

Thanks @clburlison. FTR: I may retarget the deprecation to v1.0 and skip any interim releases.

dayglojesus commented 9 years ago

@clburlison @groob This is a heads-up that will be publishing 0.7.0 next week.

This release will deprecate the mobileconfig provider.

dayglojesus commented 9 years ago

Fixed in 0.7.0 and merged into master