Puppet Pull Request Community Triage 2014-10-29

[Rob Reynolds]

Halloween edition. Feel free to wear a costume today.

Starts in about 20 minutes http://links.puppetlabs.com/pr-triage

--

Rob Reynolds

Developer, Puppet Labs

Join us at PuppetConf 2015, October 5-9 in Portland, OR - http://2015.puppetconf.com/

Register early to save 40%!

[Rob Reynolds]

Thanks all for attending. Best costume award goes to Henrik for Pinhead! 

Puppet PR Triage Notes 2014-10-29

Recording: http://youtu.be/qYRcJ2nip7M

Attending

• Adrien Thebo (adrienthebo)
• Andrew Parker (zaphod42)
• Aaron Stone (sodabrew)
• Britt Gresham (demophoon)
• Felix Frank (ffrank)
• Henrik Lindberg (hlindberg)
• Michael Smith (mikaelsmith)
• Rob Reynolds (ferventcoder)

Summary

At the start of the triage Puppet had 62 open PRs Facter had 4 open PRs, and Hiera had 1 open PR.

Note

All comments paraphrased...may not be exactly what was said.

New

Puppet

3247 - PUP-2310 Treat a CRL as expired if it's past next_update

• zaphod42: Can you trust the CRL? You [the agent] chose to trust the master
• zaphod42: you have a chain of trust from the agent to the CA authority, you can't ask someone in that chain if your trust has been violated.
• ffrank: since you don't normally contact the CA, the request is not appropriate.
• zaphod42: the master gets updated since it maintains it.
• ffrank: it would be really helpful if agents would keep up to date. Have created services that authenticate based on the CA.
• hlindberg: what happens if this is not done? what would you otherwise do?
• zaphod42: you would need to send it through another channel
• hlindberg: how is this different than the first time?
• zaphod42: it's not really, except the first time it's told what CA it should trust, which is also kind of a horrible thing really
• hlindberg: does this make us any worse off b/c of doing the same thing it does at first? Who do we have that can evaluate this?
• ferventcoder pinged joshcooper/adrienthebo about this
• joshcooper: this only addresses part of the problem. It doesn't actually fix the master getting the crl update.
• joshcooper: as long as it is using the CA to verify, then it is okay.
• zaphod42: Essentially the sense I'm getting is my concern may not be valid?
• ffrank: +1
• joshcooper: there needs to be more intelligence on when the agent should redownload a new CRL. This would address when CRL expires in 5 years, it doesn't address a revoked and updated CRL.
• zaphod42: maybe we just make getting CRL part of pluginsync
• joshcooper: pluginsync is two things now, modules and external facts
• zaphod42: might be critical to do CRL update first as a separate catalog application - agent should validate the CRL before it does anything else.
• adrienthebo: it turns out certificate status is kind of hard. Splitting out could create additional issues
• zaphod42: we had a PR for OCSP
• adrienthebo: it rotted, it was a large change. OCSP could have better results. Something something fundamentally flawed. If you are MITMed, then ...
• zaphod42: bringing the issue back to just this PR, it seems this is not going to address the case since it wouldn't expire for 5 years.
• adrienthebo: we should set aside some time to address the CRL issue at hand. Should we take the time to design a correct solution in the foreseeable future? I would like to.
• Action: 10/29 - joshcooper/adrienthebo to talk about some possible options for moving forward this week.

3238 - PUP-3547 File Server Mounts Should Support Directory Environments

• this adds the ability to have different mount points for every environment
• 10/29 - ffrank asked contributor whether there was a workaround for pseudo-modules?
• ferventcoder asked if this opens a possible security issue due to %e opening up possible elevation. zaphod42 mentions this doesn't open any issues that were not already there.
• zaphod42 states we already have a way - you create a module with your environment files. ffrank mentioned this earlier as well
• ferventcoder mentions possible big files and git, even though user is not restricted to using git for a repo
• zaphod42 states that this could be done now with just putting your files in a data module. And modules is where we should be going with this.
• ffrank mentions this could be done as a sort of enhancement for r10k (like symlinks to local directories posing as modules)
• ferventcoder pinged adrienthebo on r10k capabilities (it does support svn)
• Action: 10/29 - ffrank commented on the ticket and we are waiting to see what need the contributor has for this.

Held Over

Facter

778 - FACT-698 Add fips_enabled fact

• Josh adding notes to this PR and related PUP-3114 PR at 3044
• Uncertain of the value of the fact if we don't intend to merge PUP-3114
• Pinged Nick F on PUP-3114 for addressing this in docs
• 10/01 - still believe this is a documentation issue for explicit. If we fix the doc in Puppet, not sure if this is something to move forward on with facter.
• 10/07 - NO REVIEW
• 10/15 - ferventcoder pinged NickF/Joshcooper on the ticket about whether a decision has been made

Hiera

209 - (maint) use a more recent version of beaker

• when ready to merge, we'll want to watch the tests knowing this just went in
• we may want to prefer an even more recent version of beaker
• Action: 10/01 - holding on this for now, will evaluate again later
• 10/07 - NO REVIEW

Puppet

3189 - PUP-3057 ensure acceptance tests run on sol10

• 10/22 - comment that commit should be updated
• 10/22 - waiting for author, there are mentions of ubuntu that are surprising
• 10/29 - ferventcoder posed a question.
• Action: 10/29 - waiting on contributor

3202 - PUP-3479 Update Windows ffi dependency to new ffi

• 10/22 - ticket was logged, fixes made
• Action: 10/22 - ferventcoder to merge
• 10/29 - NO REVIEW, holding while we work out a couple of issues

3219 - PUP-3219 A more forgiving regexp for parse_selinux_context()

• 10/22, change relaxes the rule for matching quite extensively - what are the rules for the format?
• Action: 10/22 - kylog - ask if there is a specification.
• 10/29 - NO REVIEW

3165 - PUP-1077 Remove Nagios from core

• daenney was curious where this was and if there was anything to do
• kylog said we are about a week or two out on this - working on logistics
• Action: 10/22 - holding while we work out logistics
• 10/29 - NO REVIEW

3124 - PUP-3346 Don't allow insecure SSL connections w/curl

• ffrank noted that the -k is there from the beginning of time when this provider was added
• Action: 10/15 - kylog left a note on ticket asking if this would be an issue specifically with OS X as it appears this is what this addresses
• 10/29 - NO REVIEW

3145: PUP-3341 - Puppet apply breaks when an ENC returns an environment

• This is similar to 3144 (includes commits from 3144), so could come after
• ffrank: This is a bit more invasive, could override what ENC would tell a node to do
• joshcooper: https://projects.puppetlabs.com/issues/3910 is the behavior we added in 3.x
• ffrank: Shouldn't puppet apply exclusively look local?
• should apply option have option to override environments?
• zaphod42: we've been using the general rule that if it is specified on command line, it should take precedence.
• kylog: what if they disagree (both manifest and an ENC)?
• zaphod42: command line versus config can be discerned, just not yet sure if it can be discerned at the right point
• Holding on this for now while we evaluate more of the implications - Let's start a puppet-dev thread
• 10/05 - https://groups.google.com/forum/#!msg/puppet-dev/l5CnnSvOpW0/3cl-kzaacJQJ (@ffrank to start thread)
• 10/07 - Closed by ffrank to retarget to master
• 10/07 - ffrank retargeting against master
• 10/15 - discussion is still pending
• 10/29 - ffrank mentioned it doesn't seem like we are coming to a decision.
• Action: 10/29 - ffrank to give a suggested way forward and if no veto, we move forward with that

3107 - PUP-3319 - Retry send report if connection to master fails

• sets a limit of 4 tries, but adds a retries parameter
• zaphod42: this may catch system exit as well. That can be fixed.
• zaphod42: it would be good to have a test, so if report does fail
• much discussion and looking at removed retryaction.rb.
• we'd want to clean this up and bring it back in as retry_action.rb
• add blocks that could be passed to retry_action.
• Action: 10/01 - We are going to fix up retryaction and then reevaluate this PR.
• 10/07 - NO REVEW - Holding on this until we have retryaction updated.
• 10/29 - NO REVIEW

3027: PUP-3232 Prevent group duplication when group list created from multiple sources

• Need JIRA ticket, and some sort of tests
• As of 9/10, Kylo to notify contributor
• Specs and ticket added, but specs failing
• 10/01 - followup next meeting (skipped)
• 10/07 - kylog to take this and fixup
• 10/15 - kylog to look at this week
• 10/29 - discussion on whether this is valid or would break other things if fixed. ferventcoder asked if you could have multiple groups with the same name. ffrank not sure if this is necessary.
• Action: 10/29 - Waiting on more feedback

2649: Add slot support for portage package provider.

• Duplicates GH-2779, decision is to ping both contributors to see if they can coalesce their individual PRs into a single branch. Finch commenting this week.
• GH-2779 hasn't seen updates, closing that one in favor of this one. There's some concern about possibly using : as a special value in package resource titles but we don't want to go down that route.
• Pinged Adrien for a final call on this now that the specs are updated
• Ethan to look at next week at some point
• As of 9/3, Kylo asking contributor for a ticket, will revisit next week
• As of 9/10, Kylo following up on following up
• 9/30 - Kylo pinged contributor for ticket.
• Action: 10/01 - waiting on contributor
• 10/07 - NO REVIEW
• 10/15 - NO REVIEW
• 10/29 - ferventcoder pinged contributor with last warning about closing.

2770: PUP-1537 Allow version and held states of a package to specified as separate properties

• This is a very big and sticky issue, and might be worth promoting into the platform team because of the wide ranging ramifications
• Plan is to discuss how we promote issues like this, and respond to the contributor with our next steps forward
• Pending contributor response, willing to wait on this since we're asking a lot of the contributor
• Will need to talk with Adrien tomorrow and figure out how to hand this off.
• Didn't have time to look at on 8/27 -- major PR that has breaking changes and could require work. Would need to be targeted at Puppet 4 at the very least, but this one is tricky. Will try to investigate at next PR meeting.
• As of 9/3, punting this one a week, since it's a BIG change
• As of 9/10, probably punting to post-PuppetConf - too big for now
• Action: 10/01 - This needs a ticket number and should probably be one of the workshopped PRs kylog had mentioned in his email on triages. That way we could concentrate on this one. The commits would need fixed up to include ticket number
• 10/07 - ferventcoder pinged contributor about failing specs

Merged or Closed Prior to Triage

Facter

798 - FACT-719 Fix invalid argument warning when reading dmi data

• 10/07 - kylog waiting on contributor and will move forward
• 10/15 - kylog to explore this (may have exposed a GCE-related fix up)
• 10/27 - kylog merged

786 - FACT-680

• this matches what wikipedia mentions is how to detect link local
• mikaelsmith adding a cfact ticket to follow up
• This looks pretty good
• 10/01 - waiting on contributor for new tests versus changing existing tests.
• 10/07 - ferventcoder pinged contributor, contributor is on vacation for a couple of weeks (noted in JIRA ticket)
• 10/27 - kylog merged

Puppet

3175 - PUP-3462 Hiera scope: add the key 'calling_class_path'

• this is with windows not being able to use ':' as part of the filename
• 10/15 - kylog retargeting to 4.0
• 10/15 - ferventcoder to take for followup
• 10/22 - ferventcoder is +1 for Windows
• 10/22 - comments on PR, waiting for author
• 10/28 - ferventcoder merged

3005: PUP-2289 Fix host type and provider

• 10/15 - kylog mentioned operators need cleaned up
• 10/15 - joshcooper mentioned types of errors you should expect
• would this be able to handle other issues? lmello - it handles incomplete entries and new line in resource attributes. If it doesn't understand a line it will treat it as text.
• lmello - should send warnings when it finds incomplete lines? ferventcoder leans towards debug messages, kylog, hlindberg lean 50/50 on this - it's a bit of a gray area - it depends on if Puppet handles it gracefully
• 10/15 - lmello to fix up
• 10/24 - kylog fixed up whitespace and merge

3218 - (maint) Fixed debug and error output for Solaris zones

• 10/22 - notes made about fixing commit message regarding maint tag
• 10/22 - comments made, waiting on author
• 10/27 - kylog fixed up and submitted 3244, which was merged

3183 - PUP-3446 Self-sign REST interface test x509 certificate

• 10/22 - duplicate of PR-3150 ? (note says they both fix the same ticket)
• 10/22 - finch pinged since he authored PR 3150 and commented on PR 3183
• 10/27 - contributor closed this as a duplicate

3192 - PUP-3457 don't switch uid/gid if already the same

• 10/22 - updated by igalic, merged by ferventcoder during the triage

[Erik Dalén]

At Spotify we have solved this by having the Apache serve the CRL directly instead of doing it through the Puppet indirector. Then things like If-Modified-Since headers are supported.

Here's the apache config for this: https://gist.github.com/dalen/8419913  (the mode changing things aren't needed anymore as they are merged into puppet now).

Then we basically have curl as a prerun command with the -z option on the local CRL to fetch the CRL from the server if it has been updated later than the local one.

But OCSP would be a even better solution as the CRL file can grow pretty big after a while (more than 1MB) so having all agents download it on each update can be several GBs of extra traffic.

 

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CAMJiBK40tFgOw0AS3YTiLpFJQEd8LXGE_SJYejhxO3uLhCY6ig%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
Erik Dalén

[Trevor Vaughan]

+1 for an OCSP responder.

OpenSSL has a simple one built in but I'm assuming that you would want something embedded in Puppet itself since you have your own CA stack already.

R509 appears to be the most popular option but comes with a lot of baggage https://github.com/r509/r509-ocsp-responder. You may be able to easily stuff this behind your existing CA implementation.

It does make fore more chatter but, in a cloud environment where servers are popping in and out of existence (and/or compromised) your CRL could get HUGE over time.

Trevor

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CAAAzDLc8gMBYh7VEbLCPpr4ooAo_SrAkSMM-w0NP6p8zxGUDqA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Trevor Vaughan
Vice President, Onyx Point, Inc
(410) 541-6699
tvau...@onyxpoint.com

-- This account not approved for unencrypted proprietary information --