>> * The expiration of a cached certificate is the earlier of the >> envelope >> expiration and the certificate's expiration, as opposed to just >> overriding >> the cache value > > Have you tested this in the real world? That is, default expiration > is usually something like 30 minutes; is this value stored in the cert > somehow such that a cert is considered expired even though it's valid > for five years?
The "expiration" getter is only ever called by the caching stuff, at least as far as I can find after doing an extensive code search. Yes, it's possible that somebody computes :expression somehow and sends it to a certificate, but that would be aberrant behaviour and ought to be corrected. For that matter, even confounding the cache's expiration attribute with the certificate's "content.not_after" value could be considered a bug (an analog to the use of "alias" to mean both host alias and resource alias). >> * Telling the cache to expire an item now removes it from the cache if >> possible, rather than just setting an expiration date in the past and >> hoping that somebody notices. > > I'm confused as to why these are relevant. It sounds like you're > triggering expiration; couldn't you just as easily trigger removal via > 'destroy'? You can't directly access the cache from outside, so calling destroy directly on the cache isn't an option, and it doesn't seem like it's worth puncturing the abstraction for. Calling while Certificate.destroy() while you're trying to get the certificate causes stack overflow, and attempting to untangle that got messier than I felt was worth it. > Once again, this is also at least two significantly different changes > - you've done a significant refactor and changed behaviour, and I > can't at all see which is which. I enumerated the behaviour changes in the commit message. The refactoring was all small scale (mostly replacing control structures with boolean & list operators to clear out the clutter). Excluding the tests, the changes break down roughly as: Soft --> Hard cache expiration 4 lines Expire certs with the wrong key 5 lines Removing previous signed keys 4 lines Message enhancements 3 lines Comment changes 4 lines Removing stray whitespace, dead vars, 3 lines Code smell reduction 41 lines removes, 11 added, 8 changed The code smell reductions are all semantically neutral, and are for the most part locally verifiable; while I can see from a minimal patch prospective that they aren't core to making the patch, we've had an intermittent policy of "fix things when you see them" and it seems like the most efficient way to get them fixed. If you insist, I can split them out, but it seems pointless. -- Markus -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
