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
