[SERVER-262] `puppetserver gem env` does not work, useful for troubleshooting Created: 2014/12/16  Updated: 2016/09/27  Resolved: 2015/01/29

Status: Closed
Project: Puppet Server
Component/s: Puppet Server
Affects Version/s: SERVER 1.0.0
Fix Version/s: SERVER 1.0.2

Type: Bug Priority: Normal
Reporter: Jeff McCune Assignee: Erik Dasher
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to SERVER-323 MRI/system ruby paths and gems appear... Closed
relates to SERVER-522 Temporarily undo `puppetserver gem en... Closed
relates to SERVER-263 puppetserver gem install fails in an ... Closed
relates to SERVER-68 CLI options passed to puppetserver ge... Closed
relates to SERVER-377 "puppetserver gem" command doesn't wo... Closed
relates to SERVER-254 support for pry-stack_explorer Closed
relates to SERVER-297 Consolidate environment variable hand... Closed
Template:
Epic Link: Green: 1.0.2/PE3.7.2
Sub-team: Emerald
Story Points: 1
Sprint: SERVER 2014/12/31, Server 2015-01-21, Server 2015-02-04
QA Contact: Erik Dasher

 Description   

Working with puppet server 1.0.0 from packages on EL 7 I noticed the following error:

[vagrant@localhost ~]$ puppetserver gem env
ERROR:  While executing gem ... (NoMethodError)
    undefined method `split' for nil:NilClass

This error also happens in my local development environment where:

lein gem --config ~/.puppet-server/puppet-server.conf env
ERROR:  While executing gem ... (NoMethodError)
    undefined method `split' for nil:NilClass
lein gem --config ~/.puppet-server/puppet-server.conf env  18.59s user 0.78s system 174% cpu 11.132 total

The use case is that I'm trying to debug some behaviors in server/master.rb and as part of that effort I'd like to insert a pry debugger into the first line. For pry to work, I need to make sure the gem is installed in a location available to puppet server, which led me to use puppetserver gem ... and the dev equivalent of lein gem --config ~/.puppet-server/puppet-server.conf ....

The gem command is a very helpful troubleshooting and debugging tool from an end-user perspective because it provides authoritative and comprehensive information about PATH, LOAD_PATH, GEM_PATH, GEM_HOME, etc...

For example, I'd expect something like this output:

gem env
RubyGems Environment:
  - RUBYGEMS VERSION: 2.2.2
  - RUBY VERSION: 2.1.4 (2014-10-27 patchlevel 265) [x86_64-darwin13.0]
  - INSTALLATION DIRECTORY: /opt/crossfader/versions/ruby/2.1.4/gemsets/clojure/ruby/2.1.0
  - RUBY EXECUTABLE: /opt/crossfader/versions/ruby/2.1.4/bin/ruby
  - EXECUTABLE DIRECTORY: /opt/crossfader/versions/ruby/2.1.4/gemsets/clojure/ruby/2.1.0/bin
  - SPEC CACHE DIRECTORY: /Users/jeff/.gem/specs
  - RUBYGEMS PLATFORMS:
    - ruby
    - x86_64-darwin-13
  - GEM PATHS:
     - /opt/crossfader/versions/ruby/2.1.4/gemsets/clojure/ruby/2.1.0
     - /opt/crossfader/versions/ruby/2.1.4/gemsets/global/ruby/2.1.0
     - /opt/crossfader/versions/ruby/2.1.4/lib/ruby/gems/2.1.0
     - /Users/jeff/.gem/ruby/2.1.0
  - GEM CONFIGURATION:
     - :update_sources => true
     - :verbose => true
     - :backtrace => false
     - :bulk_threshold => 1000
  - REMOTE SOURCES:
     - https://rubygems.org/
  - SHELL PATH:
     - /opt/crossfader/bin
     - /opt/crossfader/versions/ruby/2.1.4/gemsets/clojure/ruby/2.1.0/bin
     - /opt/crossfader/versions/ruby/2.1.4/gemsets/global/ruby/2.1.0/bin
     - /opt/crossfader/versions/ruby/2.1.4/bin
     - /opt/crossfader/versions/lein/2.5.0/bin
     - /opt/crossfader/versions/jdk/1.7.0_71/bin
     - /usr/local/bin
     - /usr/bin
     - /bin
     - /usr/sbin
     - /sbin

UPDATE: A fix for this was implemented for SERVER 1.0.2. The fix introduced a new behavior around the handling of the GEM_PATH environment variable which led to a separate PR being done - covered by SERVER-522 - to revert this fix as part of the SERVER 1.0.3 release. In short, the GEM_PATH environment variable had started allowing an external environment value to flow through to the JRuby environment in the "puppetserver gem" process. Concerns by some about the user experience with this led to the later revert. Work to restore the "puppetserver gem env" command to working order and clarifying the overall behavior with respect to environment variable handling should be done as part of SERVER-297.



 Comments   
Comment by Chris Price [ 2014/12/16 ]

do other commands like 'list' work?

Comment by Jeff McCune [ 2014/12/16 ]

Yes, list and which both work as expected.

I'll look into getting a backtrace.

Comment by Jeff McCune [ 2014/12/16 ]

Exception is thrown from here: https://github.com/jruby/jruby/blob/1.7.17/lib/ruby/shared/rubygems/commands/environment_command.rb#L148

gem env works upstream, my suspicion is that we're unsetting PATH and shouldn't be.

Comment by Jeremy Barlow [ 2014/12/17 ]

Yeah, that's a good hint. I recall running into something like that when I was working on the irb subcommand. I ended up doing this:

(let [jruby-config (RubyInstanceConfig.)
      gem-home     (get-in config [:jruby-puppet :gem-home])
      env          (doto (HashMap. (.getEnvironment jruby-config))
                           (.put "GEM_HOME" gem-home))]
  (.setEnvironment env))

The gem cli command currently just does this:

(doto (ScriptingContainer.)
  (.setEnvironment (hash-map "GEM_HOME"
                             (get-in config [:jruby-puppet :gem-home]))))

The ruby and irb commands use a JRuby Main instead of a ScriptingContainer, but the issue could be the same. I'm thinking we should do a surgical setting of GEM_HOME in the gem subcommand, preserving other environment variables, like we do in the ruby and irb commands.

Jeff McCune, can you try that out and see if it works?

Comment by Jeff McCune [ 2014/12/17 ]

> Jeff McCune, can you try that out and see if it works?

Sure thing.

Comment by Jeff McCune [ 2014/12/19 ]

Jeremy Barlow Thanks for the help, not merging into the existing environment was the exact problem.

Pull request: https://github.com/puppetlabs/puppet-server/pull/342

Comment by Jeff McCune [ 2014/12/31 ]

Erik Dasher In our stand up this morning, Kevin Corcoran mentioned that it'd be really great to get this into PE 3.7.2, and I agreed. Aaron asked us to make sure and get your sign-off that you'd be able to QA this change without much stress before he'll propose it as being included in PE 3.7.2.

It's a small change, but the QA burden might be high. Hopefully not, there's good unit test coverage and an automated acceptance test that runs the command and validates some basic behaviors from the end user perspective.

Please let us know if you're comfortable reviewing this. It's OK if not, but without this patch it's difficult for customers to install and manage gems for use with puppetserver with PE 3.7.2 since the PATH and other potentially important environment variables are discarded with the gem subcommand.

Comment by Erik Dasher [ 2015/01/05 ]

Jeff McCune, I propose that we validate with the below steps. If you think these are adequate, than we should move forward with this.

  • run puppetserver gem env and ensure it returns the environment and doesn't error.
  • run the gem env that belongs to the os and view the environment
  • modify the puppet server's ruby gem environment
  • run puppetserver gem env and ensure it returns the expected environment and doesn't error.
  • run the gem env that belongs to the os and view the environment
  • modify the OSes gem environment
  • run puppetserver gem env and ensure it returns the expected (NOT modified) environment and doesn't error.
Comment by Jeff McCune [ 2015/01/05 ]

Erik Dasher Agreed, here's the modified list that we talked about in person. I'm happy to add automated tests too:

  • run puppetserver gem env and ensure it returns the environment and doesn't error. (This is covered by the (SERVER-262) Verify the gem env command returns expected information step)
  • run the gem env that belongs to the os and view the environment (OS gem command testing is unnecessary)
  • modify the puppet server's ruby gem environment (We should validate that setting GEM_PATH with the intent of modifying the system gem executable behavior does not adversely affect the behavior of puppetserver gem)
  • run puppetserver gem env and ensure it returns the expected environment and doesn't error. (This is covered by the (SERVER-262) Verify the gem env command returns expected information step)
  • run the gem env that belongs to the os and view the environment (Not necessary, see above)
  • modify the OSes gem environment (Not necessary, see above about GEM_PATH)
  • run puppetserver gem env and ensure it returns the expected (NOT modified) environment and doesn't error. (Should test this, see comment above about GEM_PATH)

Erik Dasher Just to document what we talked about in person, I'll be available to help test all of these items and work with you and will also try and get these automated prior to the release.

Comment by Jeff McCune [ 2015/01/05 ]

Aaron Armstrong Please consider this ticket a candidate for PE 3.7.2 assuming Erik and Chun are OK with the whole body of work, not just this issue in isolation.

Comment by Erik Dasher [ 2015/01/05 ]

QA agrees. Include this ticket in PE 3.7.2.

Comment by Jeff McCune [ 2015/01/05 ]

Cherry-picked down from master into stable as https://github.com/puppetlabs/puppet-server/commit/0bc69c3

Resolved the merge conflict and code behavior conflict by removing "JARS_NO_REQUIRE" "true"

Comment by Erik Dasher [ 2015/01/28 ]

Validated that all the below commands work on RHEL 7 with 3.7.2-rc0-187-g8f7e0d5
puppetserver gem install pry --no-ri --no-rdoc
puppetserver gem list
puppetserver gem env

Validated that modifications to gem-home: setting in the jruby-puppet section of the pe-puppetserver.conf file show immediately without puppetserver restart in the puppetserver gem env output.

Modified system ruby by installing RVM and checking puppetserver gem env. Paths were modified in puppetserver gem env, but the GEM_HOME setting is preserved.

Comment by Erik Dasher [ 2015/01/28 ]

Fix Version should be set to SERVER 1.0.4

Comment by Erik Dasher [ 2015/01/28 ]

Discussed at length with Jeremy Barlow. Need input from Chris Price and or Jeff McCune.

Possible Issue: Because the GEM PATHs flow through from MRI/System ruby into JRuby/PuppetServer ruby, installing a gem in system ruby will cause it to show up in the puppetserver gem list command.

EXAMPLE/STEPS TO REPRODUCE:

  • run puppetserver gem list.
  • run gem install nokogiri
  • run puppetserver gem list

RESULTS:
Nokogiri shows up in the puppetserver gem list

EXPECTED RESULTS:
Nokogiri was installed in MRI/System ruby, and should not show up in puppetserver gem list.

Comment by Erik Dasher [ 2015/01/28 ]

Discussed with Chris Price and Jeremy Barlow. We already have issues with "puppetserver gem" coming in from support. Allowing the GEM PATHs to flow through from MRI System ruby into JRuby will increase confusion. I believe the consensus is that we need to fix this, either by reverting this change, or by looking at providing better separation between MRI ruby paths and puppetserver Jruby paths.

Ping Aaron Armstrong Chun Du John DuarteModifying this will cause a new puppetserver version, as well as new builds. I think we can still make the currently defined schedule, even with this fix.

Comment by Chun Du [ 2015/01/28 ]

Chris Price Erik Dasher Christopher Thorn What is the impact to upgrader? CC Rajasree Talla

Comment by Chris Price [ 2015/01/28 ]

It doesn't impact upgrades.

What it does is it makes the output of 'puppetserver gem list' show gems from the system ruby, as opposed to just the puppetserver jruby. It's supposed to only show gems from the latter. That is going to make it incredibly confusing to users if they need to debug anything relating to gems; if they install a gem in the system ruby, it will look like it's installed in puppetserver even though it's not.

Comment by Erik Dasher [ 2015/01/28 ]

Setting this to Failed Review. I think we are moving towards reverting...

Comment by Erik Dasher [ 2015/01/29 ]

puppetserver gem env works. The problem that it introduces shall be tracked in a separate ticket.

Comment by Jeff McCune [ 2015/01/29 ]

There seems to be a bit of confusion surrounding the behavior of the gem system, so this is an attempt to clarify things.

What it does is it makes the output of 'puppetserver gem list' show gems from the system ruby, as opposed to just the puppetserver jruby.

This isn't precisely true try because the system ruby does not set `GEM_PATH` by default. The puppetserver subcommands will only show additional gem libraries if the `GEM_PATH` environment variable is defined in the parent process that spawns `puppetserver`. Typically, `GEM_PATH` is only set when something above and beyond the default system behavior is being done. For example, initializing rvm or rbenv in the shell that also spawns `puppetserver` commands will set GEM_PATH so that multiple versions of Ruby can co-exist with different library loading path. The act of setting GEM_PATH is what causes puppetserver to look in additional directories for gem libraries.

The user has to take some willful action to initialize their shell process to get these libraries to be visible to puppetserver.

GEM_PATH will almost certainly not be defined from inside the service management framework so there is little chance that installing gems with rvm will affect a puppetserver process started by the service management framework.

It's supposed to only show gems from the latter.

It does show only gems from puppetserver, by default, unless the user has taken explicit steps in an interactive shell that affect the behavior of rubygems, e.g. setting GEM_PATH by initializing rvm.

I think the question we need to answer is, is puppetserver supposed to only have access to its own gems in all cases even if the end user sets GEM_PATH in their shell?

That is going to make it incredibly confusing to users if they need to debug anything relating to gems; if they install a gem in the system ruby, it will look like it's installed in puppetserver even though it's not.

This will only be confusing when they have GEM_PATH defined in the shell they're running the puppetserver command from. This is also how it's always been in Puppet Enterprise. Before puppetserver, Puppet Enterprise did not take extraordinary steps to defensively manage GEM_HOME and GEM_PATH.

In such a situation where there is confusion, commands like `gem env` and `gem which` are the primary tools to troubleshoot and disambiguate which library is loaded from where on the filesystem.

This behavior of allowing the user to define where to load libraries from is how Ruby itself, not just Puppet Enterprise, has always worked (RUBYLIB, GEM_PATH, GEM_HOME) and it's also how most systems tools work. For example, sysadmins are accustomed to having to deal with CLASSPATH and JAVA_HOME in the JVM. Python has PYTHONHOME and PYTHONPATH. Perl has PERLLIB and PERL5LIB, and the operating system itself has LD_LIBRARY_PATH to affect the behavior of the system dynamic linker.

All of these environment variables are intended to allow user customization of library loading behavior.

The above information is just for some background and FYI for those curious.

I think we have a decision of, "be conservative, do not allow end user customization of Gem library loading behavior under any circumstances" To that end, we could explicitly manage or unset GEM_HOME, GEM_PATH, RUBYLIB, RUBY_OPT and RUBYOPTS. We should not, however, unset all environment variables because those like PATH are essential to the behavior of many system tools like `gem env`.

Comment by Chris Price [ 2015/01/29 ]

I feel pretty strongly at this point that environment variables need to be whitelisted on an as-needed basis, and that we should not allow any environment variables other than the ones that we've explicitly whitelisted for a very well known reason to bleed through into the server environment.

Comment by Chris Price [ 2015/01/29 ]

I could see a world where we allowed that whitelist to be configured in our config files, but by default I don't think it should contain anything other than the ones we've explicitly whitelisted for our own reasons.

As for gem env, I'm open to discussion about whitelisting PATH in order to make that work, but it would take a lot of convincing at this point for me to back off of the stance that a whitelist is the right way to be handling this.

Comment by Erik Dasher [ 2015/01/29 ]

https://testrail.ops.puppetlabs.net/index.php?/cases/view/62455

Generated at Sat Aug 08 09:01:00 PDT 2020 using Jira 8.5.2#805002-sha1:a66f9354b9e12ac788984e5d84669c903a370049.