[Pulp-list] Fwd: CDS: gofer authentication

[Jeff Ortel]

Thanks for you comments, Jason.

On 03/18/2011 11:40 AM, Jay Dobies wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> One more thing:
>
>          try:
>              secret = self._cds_stub(cds).initialize()
>              return secret
>          except RequestTimeout, e:
>              raise CdsTimeoutException(e), None, sys.exc_info()[2]
>          except DispatchError, e:
>              raise CdsCommunicationsException(e), None, sys.exc_info()[2]
>          except NotAuthorized, e:
>              raise CdsAuthException(e), None, sys.exc_info()[2]
>          except Exception, e:
>              raise CdsMethodException(e), None, sys.exc_info()[2]
>
> NotAuthorized is a subclass of DispatchError, so that code block will
> never get executed. Need to reverse the order on those.

Excellent catch.

>
>
> On 03/18/2011 12:38 PM, Jay Dobies wrote:
>>> Jason,
>>
>>> I pushed the pulp ->  external CDS authentication feature.
>>
>>> Commit: cf449c22299e01e872dff9b92a3cff65ac39cc36
>>
>>> Mind doing a quick review?  I'll be making a pass though the unit tests
>>> today.
>>
>>> -jeff
>>
>>
>> Looks pretty solid. Most of my comments focus around things you'll run
>> into in one way or another when you do the unit tests, though you may
>> have approaches in mind and I'm just not thinking of.
>>
>>
>> dispatcher.py:
>> - init_cds():  Method now returns the secret, don't forget to update the
>> docstring to mention that and say if it's a string or something more
>> complex.

All of the method docstrings are missing parameter specifications.  I'll add them too.

>>
>>
>> gofer_cds_plugin.py:
>> - There aren't any tests for this (sorry about that, my bad) so you'll
>> have to add the test file itself.
>>
>> - How are you going to override the location of the secret file for unit
>> tests? The path can be specified but the object is constructed inside of
>> getsecret(). The default is to read from config, but that's loaded by
>> gofer (there may be some way of overriding config values loaded by
>> gofer, which is one reason I'm asking since I don't know).

Well, not sure how to unit test this anyway.  The plugin needs to run with gofer so it's 
not really a unit test.  Seems like it needs to be tested in system tests?  Does it make 
sense to require the CDS plugin be configured in gofer and require qpidd/goferd running 
for unit tests to pass? Let's discuss.

>>
>> - We'll need tests that run both with the secret file present (scenario
>> of normal usage) and not present (scenario of an initialize). When
>> cleaning up the test runs by deleting the secret file directory, won't
>> we run into an issue since Secret is a singleton, is already loaded, so
>> __mkdir won't get called again?

See [1] below.

>>
>> - The caching doesn't come into play if gofer is restarted. The secret
>> is cached on write, but not on read. So if the CDS is bounced after it's
>> initialized, the caching isn't used.

Crap, thought I updated the cache in read().  good catch.  Again, see [1] below.

>>
>> - In my experience, singletons have always been a headache in unit tests
>> since modules aren't unloaded between runs, so you never know exactly
>> which test is the one causing the instantiation. Is there a benefit to
>> this approach over simply having module methods read_secret,
>> write_secret, and delete_secret and caching in the module itself?

Well, since the modules are only loaded once, implementing as functions and module 
variables presents the same headaches for testing.  I view the "secret" as an persistent 
object with read/write/delete operations.  Making it a singleton was really an after 
thought for performance.

Making it an object was a matter of philosophy (nope, not going to open up this debate 
here).  Is there a benefit beyond those well-known about OO programming?  No.  If there 
needs to be, then 95% of our classes in pulp should not have been classes.  That said, I'd 
be glad to rewrite as functions and module variables (it would take about 10 min).

[1] Actually, the more I think about it, making it a singleton and caching the secret 
really wasn't warranted.  Plus, it would be nice to be able to reset the secret without 
bouncing gofer.

>>
>>
>
> _______________________________________________
> Pulp-list mailing list
> Pulp-list at redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-list
>
>
> - --
> Jay Dobies
> RHCE# 805008743336126
> Freenode: jdob
> http://pulpproject.org
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iQEcBAEBAgAGBQJNg4r7AAoJEOMmcTqOSQHC9pYH/1guNB+tn2x6Q7cznFLPxlbI
> IoYGWlireMUELCcCM3G+LN4XUJyU34SqibJy7KzONmNxx2BjpPzKaJAAiIAhcd1O
> yc9LmNOWTtcD5kxh/ohQZXTalBDy8cAtay8q5ELPW+/nE5PPMhckOkBFzlCfcvMo
> AUCiYVZbwq0qfq9hNjByuXUwSXPrVqzjubRzP+51pMo2C6sfUaHbM07RxsOxlAF7
> 4wGNbfyqTeIDW7vCNXJsDPzM4nCCHToSsMvwcFokAxwygMH/xumMr4x//Cssz9E2
> bV0V2kKvu5SmnT19yaHHB+sWl0bUaQkvqV0YCrUvB+3goBcXxUoutkaXDReAyEw=
> =u1a2
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Pulp-list mailing list
> Pulp-list at redhat.com
> https://www.redhat.com/mailman/listinfo/pulp-list