On Thu, Nov 08, 2012 at 08:20:04AM +0100, Martin Kosek wrote:
> On 11/07/2012 05:48 PM, Sumit Bose wrote:
> > On Wed, Nov 07, 2012 at 07:23:52PM +0530, Steeve Goveas wrote:
> >> On 11/07/2012 06:33 PM, Martin Kosek wrote:
> >>> On 11/07/2012 01:54 PM, Sumit Bose wrote:
> >>>> On Mon, Nov 05, 2012 at 01:18:49PM +0100, Martin Kosek wrote:
> >>>>> On 11/02/2012 09:50 PM, Sumit Bose wrote:
> >>>>>> On Fri, Nov 02, 2012 at 02:54:32PM +0100, Martin Kosek wrote:
> >>>>>>> On 11/02/2012 12:54 PM, Sumit Bose wrote:
> >>>>>>>> On Wed, Oct 31, 2012 at 04:03:14PM +0100, Martin Kosek wrote:
> >>>>>>>>> On 10/30/2012 12:16 PM, Sumit Bose wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> this patch allows ipa-adtrust-install to reset the NetBIOS domain 
> >>>>>>>>>> name
> >>>>>>>>>> and fixes https://fedorahosted.org/freeipa/ticket/3192 .
> >>>>>>>>>>
> >>>>>>>>>> bye,
> >>>>>>>>>> Sumit
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Hello Sumit,
> >>>>>>>>>
> >>>>>>>>> I found few issues with your patch:
> >>>>>>>> Thank you for the review.
> >>>>>>>>
> >>>>>>>>> 1) It requires admin to be kinited ("conn.do_sasl_gssapi_bind()") I 
> >>>>>>>>> do not
> >>>>>>>>> think this is necessary, ipa-adtrust-install already requires admin 
> >>>>>>>>> password to
> >>>>>>>>> be passed and it already connects to LDAP with these credentials:
> >>>>>>>>>
> >>>>>>>>> api.Backend.ldap2.connect(ccache.name)
> >>>>>>>>>
> >>>>>>>>> You could use ipa.Backend.ldap2 object to do entry retrieval
> >>>>>>>>> (ipa.Backend.ldap2.get_entry) without a need to init IPAdmin at all.
> >>>>>>>> fixed
> >>>>>>>>
> >>>>>>>>> 2) When doing try..except statement, rule of thumb says that it 
> >>>>>>>>> should be as
> >>>>>>>>> short as possible, so that it does not hide other potential errors 
> >>>>>>>>> and makes
> >>>>>>>>> clear what function raises the catched exception.
> >>>>>>>>>
> >>>>>>>>> In your case:
> >>>>>>>>>
> >>>>>>>>> try:
> >>>>>>>>>     entry = api.Backend.ldap2.get_entry(DN(('cn', api.env.domain),
> >>>>>>>>>                                         
> >>>>>>>>> api.env.container_cifsdomains,
> >>>>>>>>>                                         self.api.env.basedn),
> >>>>>>>>>                                        ['ipantflatname'])
> >>>>>>>>> except errors.NotFound:
> >>>>>>>>>     reset_netbios_name = False
> >>>>>>>>> else:
> >>>>>>>>>     # process entry
> >>>>>>>>>
> >>>>>>>>> Should be a pattern that you want.
> >>>>>>>> fixed
> >>>>>>>>
> >>>>>>>> I also move all the NetBIOS name related code into a separate 
> >>>>>>>> function.
> >>>>>>>>> 3) I think this line is redundant:
> >>>>>>>>> +                    print "Say 'yes' if the NetBIOS shall be 
> >>>>>>>>> changed and " \
> >>>>>>>>> +                          "'no' if the old one shall be kept."
> >>>>>>>>>
> >>>>>>>>> IMO, the question:
> >>>>>>>>>
> >>>>>>>>> reset_netbios_name = ipautil.user_input( 'Reset NetBIOS domain 
> >>>>>>>>> name?',  default
> >>>>>>>>> = False, allow_empty = False)
> >>>>>>>>>
> >>>>>>>>> and the information printed before is enough.
> >>>>>>>> I would prefer to keep it this way to make clear that
> >>>>>>>> ipa-adtrust-install will continue processing, but the old name if 
> >>>>>>>> kept
> >>>>>>>> even if a new name was given with --netbios-name on the command line.
> >>>>>>>>
> >>>>>>>> New version attached.
> >>>>>>>>
> >>>>>>>> bye,
> >>>>>>>> Sumit
> >>>>>>>>
> >>>>>>>>> Martin
> >>>>>>>
> >>>>>>> The new approach looks much better. Sending issues I found with the 
> >>>>>>> new patch:
> >>>>>>>
> >>>>>>> 1) When I run ipa-adtrust-install on a clean IPA, I can no longer 
> >>>>>>> enter NetBIOS
> >>>>>>> name interactively. I can only change it via script option...
> >>>>>>>
> >>>>>> fixed
> >>>>>>
> >>>>>>> 2) I saw few typos:
> >>>>>>>
> >>>>>>> +        print "Current NetBIOS domain name is %s new name is %s.\n" 
> >>>>>>> % \
> >>>>>>> should be:
> >>>>>>> +        print "Current NetBIOS domain name is %s, new name is %s.\n" 
> >>>>>>> % \
> >>>>>>>
> >>>>>>> +            print "NetBIOS domain name will be changes to %s.\n" % \
> >>>>>>> should be:
> >>>>>>> +            print "NetBIOS domain name will be changed to %s.\n" % \
> >>>>>>>
> >>>>>>>
> >>>>>> fixed
> >>>>>>
> >>>>>> new version attached.
> >>>>>>
> >>>>>> bye,
> >>>>>> Sumit
> >>>>>>> Martin
> >>>>> NetBIOS name is now asked when first installing ipa-adtrust-install.
> >>>>>
> >>>>> But I see that NetBIOS name is still not queried when I run re-install 
> >>>>> of
> >>>>> ADTRUST, I can only change current name via option. Is this is an 
> >>>>> intended
> >>>>> behavior so that people cannot mess it with by mistake?
> >>>> Yes. The old code didn't check if the NetBIOS name was already set in
> >>>> LDAP or not, hence it always asked the user if the generated NetBIOS
> >>>> name is the one the user expected.
> >>>>
> >>>> The new version looks up the NetBIOS name in the LDAP server and if set
> >>>> and no new name is given on the command line assumes that the admin
> >>>> does not want to change the NetBIOS name and skips the dialog.
> >>>>
> >>>> I'll add the QE team to hear what they prefer.
> >>>>
> >>>> Jenny, Scott, Steeve, assume ipa-adtrust-install is run after trust was
> >>>> already configured and no --netbios-name option is given on the command
> >>>> line. Shall the following dialog be shown:
> >>>>
> >>>> .....
> >>>> Enter the NetBIOS name for the IPA domain.
> >>>> Only up to 15 uppercase ASCII letters and digits are allowed.
> >>>> Example: EXAMPLE.
> >>>>
> >>>>
> >>>> NetBIOS domain name [IPA17]:
> >>>> ....
> >>>>
> >>>> The admin then has to press enter to confirm the current NetBIOS name or
> >>>> can enter a new one. Or shall the dialog be skipped which means that the
> >>>> NetBIOS can only be resetted if a new one is given at the command line
> >>>> with the --netbios-name option?
> >>> Ok, thanks from explanation. But from my devel perspective, since a 
> >>> change of
> >>> NetBIOS domain name can break existing trusts I would rather not offer a 
> >>> change
> >>> of NetBIOS name during interactive prompt. A mere stating of a current 
> >>> value
> >>> with asking to user to change it should be enough.
> >>>
> >>> QE input is welcome, yes.
> >>>
> >>> Martin
> >>>
> >> If changing the netbios name post creating trust would break
> >> existing trust, then I dont think that changing the netbios named
> >> should be allowed. A clear message to drop the trust and re-add it
> >> would be better.
> > 
> > With the current patch you already get a warning when your are about to
> > change the NetBIOS name:
> > 
> > ....
> > Current NetBIOS domain name is IPA17, new name is ABC.
> > 
> > Please note that changing the NetBIOS name might break existing trust
> > relationships.
> > Say 'yes' if the NetBIOS shall be changed and 'no' if the old one shall
> > be kept.
> > Do you want to reset the NetBIOS domain name? [no]: 
> > ....
> > 
> > So the admin can still change his mind. I think a description of what
> > you should considered when you really want to change the NetBIOS domain
> > name should go into the docs and not printed by ipa-adtrust-install. As
> > mentioned earlier changing the NetBIOS domain name should be seen as the
> > last resort and should be done with great care.
> > 
> > bye,
> > Sumit
> 
> I think this is fine.
> 
> ACK. Pushed to master, ipa-3-0.

Thank you.

bye,
Sumit
> 
> Martin
> 

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to