[SSSD] [PATCHES][PRELIMINARY] Support the 'services' NSS map in SSSD

Stephen Gallagher sgallagh at redhat.com
Tue Jan 24 21:48:39 UTC 2012


On Tue, 2012-01-24 at 11:19 -0500, Stephen Gallagher wrote:
> On Tue, 2012-01-24 at 16:03 +0100, Jakub Hrozek wrote:
> > On Mon, Jan 23, 2012 at 10:12:15PM -0500, Stephen Gallagher wrote:
> > > On Sat, 2012-01-21 at 16:54 -0500, Stephen Gallagher wrote:
> > > > These patches add support for getservbyname() and getservbyport() to
> > > > the NSS responder. They do not yet support enumeration of services 
> > > > through setservent(), getservent() and endservent(). These patches also
> > > > include a reference implementation using the proxy ID provider (which I
> > > > used to test and validate the functionality).
> > > > 
> > > > Work still to come: enumeration support and LDAP provider
> > > > implementation.
> > > > 
> > > > Patch 0001: Make the sized_string structure available elsewhere in the
> > > > source.
> > > > 
> > > > Patch 0002: Add support for strtouint16 (needed for managing the port
> > > > number)
> > > > 
> > > > Patch 0003: Make add_string() and add_ulong() available to all sysdb
> > > > files.
> > > > 
> > > > Patch 0004: Add routines to save, delete and modify service entries in
> > > > the sysdb. (Also includes some very rudimentary unit tests)
> > > > 
> > > > Patch 0005: Add LDB indexes for the port and protocol arguments, since
> > > > we'll be searching on them.
> > > > 
> > > > Patch 0006: Add support to the NSS sss_client to look up getservbyname()
> > > > and getservbyport().
> > > > 
> > > > Patch 0007: Add support for services in dp requests
> > > > 
> > > > Patch 0008: Add routines to the negative cache to handle services
> > > > 
> > > > Patch 0009: The big one. This patch adds a new feature to the NSS
> > > > responder to handle looking up services by name and by port (optionally
> > > > filtering by protocol).
> > > > 
> > > > Important note: I did not use the existing check_cache() functionality
> > > > here. I changed the cache check and lookup logic to match that which was
> > > > proposed in https://fedorahosted.org/sssd/ticket/1126 (to deal with
> > > > multiple domains more efficiently). I would like this very carefully
> > > > reviewed (by two engineers) because I think our long-term plan is going
> > > > to be to migrate the existing NSS responders over to this style.
> > > > 
> > > > /* Provider Lookup Logic:
> > > >  * Iterate through the available caches. If the cached entry is
> > > >  * present and not expired, return it immediately(*). If it is
> > > >  * present and expired, add it to a list of domains eligible to
> > > >  * be checked. If it is in the negative cache, skip over it and
> > > >  * do not add it to the eligible domain list.
> > > >  *
> > > >  * Once we have searched all of the caches, if the entry has not
> > > >  * been determined to be available, search all domains in order
> > > >  * to see if any of them contain the requested entry.
> > > >  *
> > > >  * (*) Optionally perform a midpoint cache refresh if appropriate.
> > > >  */
> > > > 
> > > > See the functions getserv_send() and lookup_service_*() for more
> > > > details.
> > > > 
> > > > 
> > > > Patch 0010: This is the reference implementation I used for testing
> > > > purposes. I tested against proxy_lib_name = files (and changed the
> > > > 'services' line in my /etc/nsswitch.conf to contain only 'sss')
> > > 
> > > 
> > > 
> > > New round of patches. The first seven patches are unchanged from the
> > > first pass (though offset by three, since the first three patches have
> > > been pushed). The eighth patch is just Jakub's acked patch for
> > > refactoring the dp_req routines from "[PATCH] DP: Refactor
> > > responder_dp_req so it's reusable by other responders" in the proper
> > > order to ensure it doesn't get lost/put in the wrong place. (Though I'm
> > > pretty sure it will apply cleanly anywhere after 0006).
> > > 
> > > Patch 0009: Add support to sss_client to make enumeration requests for
> > > services
> > > 
> > > Patch 0010: Add sysdb routines for requesting the complete list of
> > > cached services.
> > > 
> > > Patch 0011: Another big one. Adds support for enumerating services to
> > > the NSS provider.
> > > 
> > > Patch 0012: The proxy provider implementation of the enumeration logic.
> > > Useful for testing purposes (I tested against the 'files' NSS library).
> > > 
> > > There is a known issue with the proxy provider support that I'm going to
> > > work on in a later patch. Specifically, when we get back the list of
> > > available services from the proxy provider, we get services that have
> > > multiple available protocols back as separate entries. This causes the
> > > second value to overwrite the primary value in sysdb_store_service().
> > > I'm going to add the ability to merge them, but since the LDAP provider
> > > (when finished) won't suffer this problem (since LDAP lookups will
> > > return all protocols in the same request), I'm not stressing over it
> > > right now. Once the code is in, I'll file a bug so we don't forget about
> > > it.
> > 
> > Patch 0001: SYSDB: Add sysdb routines for manipulating service entries
> > 
> > I agree with Simo that proto?sanitized_proto:"*" is confusing. I think
> > it would be more readable to set sanitized_proto to NULL on definition
> > and just use sanitized_proto in the ternary operator.
> > 
> 
> Whoops. I forgot to include Simo's review comment. Fixed.
> 
> > Why the cast of "int proto" in sysdb_getservbyport() to "unsigned int" ?
> > Would it make sense to add a check if proto >= 0 ? (I can't help
> > wondering why the glibc interface uses int and not unsigned int in the
> > first place..)
> > 
> 
> I assume you meant "int port". I cast it because (as you observe) glibc
> uses 'int' instead of 'uint16_t' which is what it SHOULD be doing.
> 
> I added a check.
> 
> > I noticed we use the same construct with sysdb_transaction_start() and
> > bool in_transaction all around the code. I think a macro that would wrap
> > the start, commit and cancel might be nicer and save us from some bugs in
> > the future. Not as part of this patch, of course.
> > 
> 
> We can think about it. Please file a ticket.
> 
> > Can you add a DEBUG() message fot the case sysdb_store_service() is
> > deleting a corrupt entry?
> > 
> 
> Yeah, good idea.
> 
> > The loop after sysdb_getservbyname() in sysdb_store_service() always
> > references res->msgs[0], shouldn't that be res->msgs[i] ?
> > 
> 
> Thanks, bad copy-paste
> 
> > In sysdb_svc_add() isn't it safer to use "if (aliases && aliases[0])"? I
> > think the caller might appreciate the simplicity of passing NULL for no
> 
> Good idea. Fixed
> 
> > aliases. The same applies for aliases and protocols in sysdb_svc_update().
> > 
> 
> For aliases, yes. There MUST be at least one protocol. I've added an
> EINVAL check at the start of the function.
> 
> > In sysdb_svc_delete() you forgot to set in_transaction to false after
> > sysdb_transaction_commit().
> > 
> 
> Good catch. That macro is sounding better :)
> 
> > 
> > Patch 0002: NSS: Add client support for services (non-enumeration)
> > Ack
> > 
> > Patch 0003: NSS: Add client support for services (non-enumeration)
> > Can you #define some SERVICES_METADATA_COUNT to avoid using the 
> > constant 8 in the functions?
> > Otherwise looks OK
> > 
> 
> Done. I also made the same change to patch 0009
> 
> > Patch 0004: DP: Add support for services in dp requests
> > Ack
> > 
> > Patch 0005: NSS: Add negative cache routines for services
> > Ack
> > 
> > The rest of the patches is TBR, it might take time to review the big
> > ones, so I decided to send the review in parts.
> 
> 
> Since the full patchset is enormous, I'm only attaching the current
> updates. Anyone who wants the full set of patches can get them from my
> personal git repo with the following commands:
> 
> git remote add sgallagh \
> git://fedorapeople.org/home/fedora/jhrozek/public_git/sssd.git
> 
> git remote update
> 
> git format-patch -M -C --patience --full-index \
> 6e4efa5a0960a83fef6a654152edfc618bcebece..HEAD
> 
> 
> I'll continue to update the format-patch command in subsequent
> revisions.

Just a heads-up. I'm reworking the "extra" field in the backend so that
it's parsed by be_get_account_info() instead of in the providers itself
(so they'll be able to get breq->extra_value instead of parsing
breq->filter_value).

This is going to require adjustment for the proxy provider.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120124/16a91bdb/attachment.sig>


More information about the sssd-devel mailing list