[PATCH] anaconda realm command

[Stef Walter]

This patch adds the realm command to anaconda. It depends on the
pykickstart changes I added earlier (hmmm, how should I reflect this
dependency?).

Again, the big use case is automatic AD domain joins by adding a
line to a kickstart file. The line in the kickstart file looks like:

realm join domain.example.com

or:

realm join --one-time-password=mypassword domain.example.com

Attached are two patches. The first one sets the kernel hostname during
the post install, so it can be used by stuff running in the chroot.
realmd needs the hostname to be appropriately set.

The second patch adds the realm command implementation. It does this:

* Early in the install the realm is discovered. This tells us what
   kind of realm (such as AD domain vs. IPA domain etc.) and what
   kind of packages need to be installed to support it. The required
   packages are added to the install.

* After the install and configuration, we join the realm using the
   realmd. realmd has support for running within a chroot, and without
   a dbus system bus etc. So we use this to run within /mnt/sysimage.

Happy to adapt this as necessary. Looking forward to review.

Cheers,

Stef

[Chris Lumens]

I'll do real review later, but fow now I can answer the one easy
question...
> This patch adds the realm command to anaconda. It depends on the
> pykickstart changes I added earlier (hmmm, how should I reflect this
> dependency?).

We do this by changing this line in anaconda.spec.in:

%define pykickstartver 1.99.22

Of course, you don't know what version's going to be required yet and
neither do I since this is bound for F19.  We'll just have to make sure
to remember to change this at merge time.

- Chris

[Stef Walter]

On 11/21/2012 05:26 PM, Chris Lumens wrote:
> I'll do real review later, but fow now I can answer the one easy
> question...
> > This patch adds the realm command to anaconda. It depends on the
> > pykickstart changes I added earlier (hmmm, how should I reflect this
> > dependency?).
>
> We do this by changing this line in anaconda.spec.in:
>
> %define pykickstartver 1.99.22
>
> Of course, you don't know what version's going to be required yet and
> neither do I since this is bound for F19.  We'll just have to make sure
> to remember to change this at merge time.

Makes sense.

Anything I can do to help get these reviewed? Or did I just post them at
a bad, busy time?

Cheers,

Stef

[Chris Lumens]

> Anything I can do to help get these reviewed? Or did I just post them at
> a bad, busy time?

It's just a bad time.  Once we have a separate f18-final-branch and
master is tracking rawhide again, we can definitely get this in.  I'll
leave it marked as unread in my mail so I will know to go back and take
a look.

- Chris

[Vratislav Podzimek]

On Wed, 2012-11-28 at 12:01 -0500, [e-mail] wrote:
> > Anything I can do to help get these reviewed? Or did I just post them at
> > a bad, busy time?
>
> It's just a bad time.  Once we have a separate f18-final-branch and
> master is tracking rawhide again, we can definitely get this in.  I'll
> leave it marked as unread in my mail so I will know to go back and take
> a look.

I'm personally interested in these things, so I'll look at the patches.
I expect you'll want to see it anyway, but at least as a first round.

[Stef Walter]

On 11/28/2012 06:25 PM, Vratislav Podzimek wrote:
> On Wed, 2012-11-28 at 12:01 -0500, [e-mail] wrote:
> > > Anything I can do to help get these reviewed? Or did I just post them at
> > > a bad, busy time?
> >
> > It's just a bad time.  Once we have a separate f18-final-branch and
> > master is tracking rawhide again, we can definitely get this in.  I'll
> > leave it marked as unread in my mail so I will know to go back and take
> > a look.

Thanks Chris.
> I'm personally interested in these things, so I'll look at the patches.
> I expect you'll want to see it anyway, but at least as a first round.

Wonderful. Thank you.

Stef

[Vratislav Podzimek]

> From 5a18b92ad5e4e99c447edd29440bb0fb687b70e4 Mon Sep 17 00:00:00 2001
> From: Stef Walter 
> Date: Fri, 16 Nov 2012 11:28:43 +0100
> Subject: [PATCH 2/2] Add support for the realm kickstart command
>
> This command uses realmd to discover information about the domain
> to join, which adds in the right packages, and then joins the domain
> post install.
>
> anaconda.spec.in         |  1 +
>  pyanaconda/install.py    | 11 +++++++-
>  pyanaconda/kickstart.py  | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>  pyanaconda/yuminstall.py |  2 +-
>  4 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/anaconda.spec.in b/anaconda.spec.in
> index 0b5ef39..c2d8333 100644
> --- a/anaconda.spec.in
> +++ b/anaconda.spec.in
> @@ -122,6 +122,7 @@ Requires: python-pyblock >= %{pythonpyblockver}
>  Requires: libuser-python
>  Requires: newt-python
>  Requires: authconfig
> +Requires: realmd
>  Requires: firewalld >= %{firewalldver}
>  Requires: cryptsetup-luks
>  Requires: python-cryptsetup >= %{pythoncryptsetupver}
> diff --git a/pyanaconda/install.py b/pyanaconda/install.py
> index b6fc3e8..bdd7090 100644
> --- a/pyanaconda/install.py
> +++ b/pyanaconda/install.py
> @@ -78,6 +78,10 @@ def doConfiguration(storage, payload, ksdata, instClass):
>          ksdata.group.execute(storage, ksdata, instClass, u)
>          ksdata.user.execute(storage, ksdata, instClass, u)
> if ksdata.realm.discovered:
>    with progress_report(_("Joining realm: %s" % ksdata.realm.discovered)):
>        ksdata.realm.execute(storage, ksdata, instClass)

You need to increment the number in progress.send_init() at the
begining of doConfiguration if there is one more step. Otherwise the
progress bar will be one step ahead.
>  with progress_report(_("Running post install scripts")):
>      runPostScripts(ksdata.scripts)
>
> @@ -116,10 +120,15 @@ def doInstall(storage, payload, ksdata, instClass):
>  # Do packaging.
>
> # Discover information about realms to join, to determine additional packages
> if ksdata.realm.join_realm:
>    with progress_report(_("Discovering realm to join")):
>        ksdata.realm.discover()

The same goes also for progress.send_init() in doInstall.
>  # anaconda requires storage packages in order to make sure the target
>  # system is bootable and configurable, and some other packages in order
>  # to finish setting up the system.
> packages = storage.packages + ["authconfig", "firewalld"]
> packages = storage.packages + ["authconfig", "firewalld"] + ksdata.realm.packages
> payload.preInstall(packages=packages, groups=payload.languageGroups(ksdata.lang.lang))
> payload.install()
>
> diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py
> index 2600c65..e501947 100644
> --- a/pyanaconda/kickstart.py
> +++ b/pyanaconda/kickstart.py
> @@ -1262,6 +1262,75 @@ class RaidData(commands.raid.F18_RaidData):
>                                       parents=request)
>              storage.createDevice(luksdev)
>
> +class Realm(commands.realm.F19_Realm):
> def __init__(self, *args):
>    commands.realm.F19_Realm.__init__(self, *args)
>    self.packages = []
>    self.discovered = ""
> def discover(self):
>    if not self.join_realm:
>        return
>    try:
>        args = ["discover", "--verbose", "--install", "/"] + \
>               self.discover_options + [self.join_realm]
>        output = iutil.execWithCapture("/usr/sbin/realm", args, stderr="/dev/tty5", fatal=True)

Could you please split this line to two? The same goes for some others
longer than let's say 90 characters (80 ideally).
>    except RuntimeError as msg:
>        log.error("Error running /usr/sbin/realm %s: %s", args, msg)
>        return
>    except OSError as msg:
>        # TODO: This is a lousy way of propagating what will usually be 'no such realm'
>        log.error("Error running /usr/sbin/realm %s: %s", args, msg)
>        return
>    # Now parse the output for the required software. First line is the
>    # realm name, and following lines are information as "name: value"
>    self.packages = ["realmd"]
>    self.discovered = ""
>    for line in output.split("\n"):
>        if not self.discovered:
>            self.discovered = line.strip()
>        else:
>            parts = line.split (":", 1)
>            if len(parts) == 2 and parts[0].strip() == "required-package":
>                self.packages.append(parts[1].strip())

Are you expecting some empty lines at the begining of the output?
Because otherwise I believe something like:

lines = output.split("\n")
        if not lines:
            return
        	self.discovered = lines.pop(0).strip()

for line in lines:
            parts = line.split(":", 1)
            if len(parts) == 2 and parts[0].strip() == "required-package":
                self.packagse.append(parts[1].strip())

would do the same and could be better readable. This is probably just a
matter of opinion. If you are going to use your version, please remove
the additional space in 'parts = line.split (":", 1)'
>    log.info("Realm %s needs packages %s" % (self.discovered, ", ".join(self.packages)))
> def execute(self, *args):
>    if not self.discovered:
>        return
>    pw_args = ["--no-password"]
>    for arg in self.join_args:
>        if arg.startswith("--no-password") or arg.startswith("--one-time-password"):
>            pw_args = []

'break' should go here? Or, if you want, you could use:

if any(arg.startswith("--no-password")
               or arg.startswith("--one-time-password")
               for arg in self.join_args):
             pw_args = []
        else:
             pw_args = ["--no-password"]
>    args = ["join", "--install", ROOT_PATH, "--verbose"] + pw_args + self.join_args
>    try:
>        rc = iutil.execWithRedirect("/usr/sbin/realm", args,
>                                    stdout="/dev/tty5", stderr="/dev/tty5")
>    except RuntimeError as msg:
>        log.error("Error running /usr/sbin/realm %s: %s", args, msg)
>    if rc != 0:
>        log.error("Command failure: /usr/sbin/realm %s: %d", args, rc)
>        return
>    log.info("Joined realm %s", self.join_realm)
>    for (command, options) in self.after:
>        args = [command, "--install", ROOT_PATH, "--verbose"] + options
>        rc = iutil.execWithRedirect("/usr/sbin/realm", args,
>                                    stdout="/dev/tty5", stderr="/dev/tty5")
>        if rc != 0:
>            log.error("Command failure: /usr/sbin/realm %s: %d", args, rc)
>        log.info("Ran /usr/sbin/realm %s", args)
>
> class RootPw(commands.rootpw.F18_RootPw):
>      def execute(self, storage, ksdata, instClass, users):
>          algo = getPassAlgo(ksdata.authconfig.authconfig)
> @@ -1443,6 +1512,7 @@ commandMap = {
>          "part": Partition,
>          "partition": Partition,
>          "raid": Raid,
>    "realm": Realm,
>    "rootpw": RootPw,
>    "selinux": SELinux,
>    "services": Services,
>
> diff --git a/pyanaconda/yuminstall.py b/pyanaconda/yuminstall.py
> index aef7e8f..f8eb609 100644
> --- a/pyanaconda/yuminstall.py
> +++ b/pyanaconda/yuminstall.py
> @@ -1488,7 +1488,7 @@ reposdir=/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anacon
>      # installed (they could have been removed in kickstart).  So we'll force
>      # it.
>      def selectAnacondaNeeds(self):
>    for pkg in ['authconfig', 'chkconfig', 'system-config-firewall-base']:
>    for pkg in ['authconfig', 'chkconfig', 'realmd', 'system-config-firewall-base']:

Do we really want 'realmd' installed on every system?

Otherwise it looks good to me but as I've already mentioned in the
reply for the pykickstart patches I believe this should be a separate
addon and not part of the Anaconda itself.

[Stef Walter]

Thanks for the review. Made the changes noted to the attached patches.

On 12/03/2012 06:42 PM, Vratislav Podzimek wrote:
> Are you expecting some empty lines at the begining of the output?

Nope. Your code is more readable so I've put it in the patch. Thanks.
> >    log.info("Realm %s needs packages %s" % (self.discovered, ", ".join(self.packages)))
> > def execute(self, *args):
> >    if not self.discovered:
> >        return
> >    pw_args = ["--no-password"]
> >    for arg in self.join_args:
> >        if arg.startswith("--no-password") or arg.startswith("--one-time-password"):
> >            pw_args = []
>
> 'break' should go here?

Doesn't really matter, but is more readable, so added.
> >  def selectAnacondaNeeds(self):
> >    for pkg in ['authconfig', 'chkconfig', 'system-config-firewall-base']:
> >    for pkg in ['authconfig', 'chkconfig', 'realmd', 'system-config-firewall-base']:
>
> Do we really want 'realmd' installed on every system?

No. We do however need it in the installer. Once it's there, then the
anaconda realm command installs it (if necessary) on the system.

Should I remove it from the line above? What does the above reflect?
Because Anaconda does need it in this case. LMK.

Cheers,

Stef

[David Lehman]

On Wed, 2012-11-21 at 09:55 +0100, Stef Walter wrote:
> This patch adds the realm command to anaconda. It depends on the
> pykickstart changes I added earlier (hmmm, how should I reflect this
> dependency?).

yuminstall.py is not in use on master or f18-*branch anymore. I don't
think you want to keep the changes in there anyway, so you can probably
just drop that part of the patch.

David

[Stef Walter]

On 12/04/2012 04:39 PM, David Lehman wrote:
> On Wed, 2012-11-21 at 09:55 +0100, Stef Walter wrote:
> > This patch adds the realm command to anaconda. It depends on the
> > pykickstart changes I added earlier (hmmm, how should I reflect this
> > dependency?).
>
> yuminstall.py is not in use on master or f18-*branch anymore. I don't
> think you want to keep the changes in there anyway, so you can probably
> just drop that part of the patch.

Thanks for the heads up. Vratislav suggested the same, and I posted an
updated patch without it.

Cheers,

Stef