Re: [libvirt] [PATCH v2 10/10] nss: Introduce libvirt-guest module

On Mon, Dec 05, 2016 at 11:31:56AM +0100, Michal Privoznik wrote: > So far the NSS module looks up only hostnames as provided by > guests themselves. However, there are some cases where this is > not enough: e.g. when there's a fresh new guest being installed > (with some generic hostname) say from a live ISO image; or some > (older) systems don't advertise their hostname in DHCP > transactions at all. > In cases like that it would be helpful if we translate domain > name as seen by libvirt too so that users can: > > # virsh start $dom && ssh $dom > > In order to achieve that new libvirt-guest module is introduced, > while older libvirt module maintains its current behaviour (that > is translating guest provided names into IP addresses). > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > docs/news.html.in | 4 ++ > docs/nss.html.in | 58 ++++++++++++++++++++++--- > src/Makefile.am | 8 ++++ > tests/Makefile.am | 22 +++++++++- > tests/nssdata/virbr0.macs | 23 ++++++++++ > tests/nssdata/virbr0.status | 5 +++ > tests/nssdata/virbr1.macs | 21 +++++++++ > tests/nssdata/virbr1.status | 5 +++ > tests/nssmock.c | 21 +++++++++ > tests/nsstest.c | 5 +++ > tools/Makefile.am | 46 ++++++++++++++++++-- > tools/nss/libvirt_guest_nss.syms | 12 ++++++ > tools/nss/libvirt_nss.c | 92 > ++++++++++++++++++++++++++++++++++------ > tools/nss/libvirt_nss.h | 6 ++- > 14 files changed, 305 insertions(+), 23 deletions(-) > create mode 100644 tests/nssdata/virbr0.macs > create mode 100644 tests/nssdata/virbr1.macs > create mode 100644 tools/nss/libvirt_guest_nss.syms > > diff --git a/docs/news.html.in b/docs/news.html.in > index 12dcb0141..2ea409894 100644 > --- a/docs/news.html.in > +++ b/docs/news.html.in > @@ -15,6 +15,10 @@ > <h3>v3.0.0 (<i>unreleased</i>)</h3> > <ul> > <li><strong>New features</strong> > + <ul> > + <li>New <code>libvirt-guest</code> nss module that > translates libvirt > + guest names into IP addresses</li> It would be nice to have the "summary<br/>description" here. Also you're missing newline before </li>. > + </ul> > </li> > <li><strong>Improvements</strong> > </li> > diff --git a/docs/nss.html.in b/docs/nss.html.in > index 84ea8df6d..ee18c9b78 100644 > --- a/docs/nss.html.in > +++ b/docs/nss.html.in > @@ -62,6 +62,48 @@ hosts: files libvirt dns > lookup given host name. > </p> > > + <h2><a name="Sources">Sources of information</a></h2> > + > + <p> > + As of <code>v3.0.0</code> release, libvirt offers two NSS modules > + implementing two different methods of hostname translation. The > first and > + older method is implemented by <code>libvirt</code> plugin and > + basically looks up the hostname to IP address translation in DHCP > server > + records. Therefore this is dependent on hostname provided by > guests. Thing > + is, not all the guests out there provide one in DHCP > transactions, nor I am being said (often) that "nor" can only be combined with "neither" and not even always (because I do that mistake often as well) and that "or" should be used in this context instead. Native speaker should probably correct me or you. > + every sysadmin out there believes all the guests. Hence libvirt > implements > + second method in <code>libvirt-guest</code> module which does > libvirt guest > + name to IP address translation (regardless of hostname set in the > guest). > + </p> > + > + <p> > + To enable either of the modules put their name into the > + <code>nsswitch.conf</code> file. For instance, to enable > + <code>libvirt-guest</code> module: > + </p> > + > +<pre> > +$ cat /etc/nsswitch.conf > +# /etc/nsswitch.conf: > +hosts: files libvirt-guest dns > +# ... > +</pre> > + <p>Or users can enable both at the same time:</p> > +<pre> > +$ cat /etc/nsswitch.conf > +# /etc/nsswitch.conf: > +hosts: files libvirt libvirt-guest dns > +# ... > +</pre> > + > + <p> > + This configuration will mean that if hostname is not found by the > + <code>libvirt</code> module (e.g. because a guest did not sent > hostname > + during DHCP transaction), the <code>libvirt-guest</code> module is > + consulted (and if the hostname matches libvirt guest name it will be > + resolved). > + </p> > + > <h2><a name="Internals">How does it work?</a></h2> > > <p> > @@ -100,15 +142,18 @@ hosts: files libvirt dns > <h2><a name="Limitations">Limitations</a></h2> > > <ol> > - <li>The libvirt NSS module matches only hostnames provided by > guest. If > - the libvirt name and one advertised by guest differs, the > latter is > - matched.</li> > + <li><del>The libvirt NSS module matches only hostnames provided > by guest. > + If the libvirt name and one advertised by guest differs, the > latter is > + matched.</del> As of <code>v3.0.0</code> there are two > libvirt NSS modules > + translating both hostnames provided by guest and libvirt > guest names.</li> Is <del/> just strike-through text? What's the point in that? Just make the documentation reflect the newest, current, state. > <li>The module works only in that cases where IP addresses are > assigned by > dnsmasq spawned by libvirt. Libvirt NATed networks are typical > example.</li> > </ol> > > <p> > + <i>The following paragraph describes implementation limitation of > the > + <code>libvirt</code> NSS module.</i> > These limitation are result of libvirt's internal implementation. > While > libvirt can report IP addresses regardless of their origin, a > public API > must be used to obtain those. However, for the API a connection > object is > @@ -134,8 +179,11 @@ virsh domifaddr --source lease $domain > </pre> > > <p> > - If there's no record for either of the aforementioned commands, > it's very > - likely that NSS module won't find anything and vice versa. > + <del>If there's no record for either of the aforementioned > commands, it's > + very likely that NSS module won't find anything and vice > versa.</del> > + As of <code>v3.0.0</code> libvirt provides > <code>libvirt-guest</code> NSS > + module that doesn't have this limitation. However, the statement > is still > + true for the <code>libvirt</code> NSS module. Same here. > diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c > index 979bf81e1..6cadf8c43 100644 > --- a/tools/nss/libvirt_nss.c > +++ b/tools/nss/libvirt_nss.c > @@ -267,6 +284,54 @@ findLease(const char *name, > goto cleanup; > } > > +#else /* defined(LIBVIRT_NSS_GUEST) */ > + > + for (i = 0; i < nMacmaps; i++) { > + const char **macs = (const char **) > virMACMapMgrLookup(macmaps[i], name); > + size_t j; > + > + if (!macs) > + continue; > + > + for (j = 0; j < nleases; j++) { > + virJSONValuePtr lease = > virJSONValueArrayGet(leases_array, j); > + const char *macAddr; > + > + if (!lease) { > + /* This should never happen (TM) */ > + ERROR("Unable to get element %zd of %zd", j, nleases); > + goto cleanup; > + } > + > + macAddr = virJSONValueObjectGetString(lease, "mac-address"); > + if (!macAddr) > + continue; > + > + if (!virStringListHasString(macs, macAddr)) > + continue; > + > + if (virJSONValueObjectGetNumberLong(lease, "expiry-time", > &expirytime) < 0) { > + /* A lease cannot be present without expiry-time */ > + ERROR("expiry-time field missing for %s", name); > + goto cleanup; > + } > + > + /* Do not report expired lease */ > + if (expirytime < (long long) currtime) { > + DEBUG("Skipping expired lease for %s", name); > + continue; > + } > + > + DEBUG("Found record for %s", name); > + *found = true; > + > + if (appendAddr(&tmpAddress, &ntmpAddress, lease, af) < 0) > + goto cleanup; > + } > + } > + > +#endif /* defined(LIBVIRT_NSS_GUEST) */ > + I feel like lot of this code is duplicated, but that can be fixed later. Also it might look more complicated and this is the best way to keep it. Did you try any other approach by any chance (just out of curiosity).