[SERVER-297] Consolidate environment variable handling behaviors Created: 2015/01/12  Updated: 2015/06/17  Resolved: 2015/06/09

Status: Closed
Project: Puppet Server
Component/s: Puppet Server
Affects Version/s: None
Fix Version/s: SERVER 2.1.1

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

Issue Links:
Blocks
blocks SERVER-254 support for pry-stack_explorer Closed
Relates
relates to SERVER-262 `puppetserver gem env` does not work,... Closed
relates to SERVER-377 "puppetserver gem" command doesn't wo... Closed
relates to SERVER-538 Report Processo(specifically hipchat)... Closed
relates to SERVER-522 Temporarily undo `puppetserver gem en... Closed
relates to SERVER-584 Ability to specify arbitrary values f... Closed
relates to SERVER-721 Backport SERVER-297 to stable Closed
relates to SERVER-374 improve "puppetserver gem" handling o... Closed
Template:
Sub-team: emerald
Story Points: 3
Sprint: Server Emerald 2015-05-13, Server Emerald 2015-05-27, Server Emerald 2015-06-10
CS Priority: Minor
Release Notes: Bug Fix
QA Contact: Erik Dasher

 Description   

The way puppet server handles environment variables should be consolidated and normalized across the various use cases of the production service and all subcommand invocations.

This came up in the discussion on https://github.com/puppetlabs/puppet-server/pull/354

Also, interested in hashing out the whole discussion on clearing/overriding/preserving of different environment variables. You have some great points there. Wanted to get some other opinions to see where others are on that.

Yeah, maybe we should just pull that commit out of this PR and reconsider our environment support both for the CLI and the production scripting containers as a separate topic?

I'd be good with separating them, too, although I think the tests would need to be rewritten a bit to work for the code as currently implemented.

I think it also would be good to cover the environment rework stuff under a new JIRA ticket and expand the scope to include reworking the production ScriptingContainer code to be consistent with that of the subcommand tools.

Some work had been done in SERVER-262 – and committed for the SERVER 1.0.2 release – to fix the "puppetserver gem env" command. Prior to SERVER-262, running the "puppetserver gem env" command would result in an error:

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

SERVER-262 was later reverted in SERVER-522 – SERVER 1.0.3 release – because of some concerns about the new behavior which would have allowed the GEM_PATH variable to flow through from the user's environment into the JRuby environment in a "puppetserver gem" command execution. In addition to the overall work to consolidate environment handling behavior, this ticket should also cover restoring "puppetserver gem env" to working order.

UPDATE - One of the pieces we'll need to reconcile as part of this work is how the ruby-load-path gets set. In the irb.clj subcommand, the load path is front-loaded with some embedded JRuby lib paths as it is set onto the ScriptingContainer. See: https://github.com/puppetlabs/puppet-server/blob/puppet-server-1.0.8/src/clj/puppetlabs/puppetserver/cli/irb.clj#L20-L24. This was done in order to avoid some errors that were otherwise being produced by the system rubygems. For ScriptingContainer initialization in production puppetserver code, however, this front-loading was not being done. In SERVER-538, we found that the hipchat gem could not be loaded correctly unless the JRuby paths were front-loaded onto the load path on CentOS 6. The front-loading approach may, therefore, be something we should do for production code as well. We'd have to consider the best way to do that, though. For example, should the JRuby lib paths just be frontloaded onto the beginning of the list of load paths or could the "ruby-load-path" configuration variable provide a way for the user to designate a location in the list of paths into which the JRuby libs are expanded?

UPDATE (2) - To avoid the risk of loading too much work into this one ticket, I'd propose that we limit the scope of this ticket to just consolidating the ScriptingContainer initialization behaviors for now and defer related work out to the implementation for linked tickets:

  • SERVER-377 - proxy support for puppetserver gem
  • SERVER-156 - proxy support for http client in production puppetserver master code
  • SERVER-584 - ability to specify arbitrary values for environment variables

The work for SERVER-297, then, would be about ensuring that all of the puppetserver CLI tools and production puppetserver code stack use the same ScriptingContainer initialization logic. The work to be done for the proxy and arbitrary environment variable tickets may alter the original implementation but for now the implementation would just include the following with respect to the JRubyPuppet environment initialization:

1) Set GEM_HOME from gem-home in the jruby-puppet config as before.
2) Set JARS_NO_REQUIRE to true.
3) Set HOME and PATH from the corresponding environment variables in the shell. Production Puppet Server code whitelists these for Facter resolution. This is also needed to allow the irb subcommand to continue working and to restore puppetserver gem env to working order. Note that as we do SERVER-584 later that we may decide to derive this only from a configuration setting instead.
4) Set load path from the ruby-load-path setting in os-settings/jruby-puppet as appropriate per branch. For Server 1.X only, add in the "front-loading" of the load path with embedded JRuby path libs, as is done for the irb subcommand today - see https://github.com/puppetlabs/puppet-server/blob/puppet-server-1.0.8/src/clj/puppetlabs/puppetserver/cli/irb.clj#L20-L24. This is needed for the irb command to continue working without errors and to address SERVER-538. The use case outlined in SERVER-538 of being able to properly load the hipchat gem should be validated against this work on Server 1.x to confirm that the issue has been addressed as expected. Note that this is only strictly needed for Server 1.X and not Server 2.X because for Server 2.x, core Ruby Puppet code is loaded into a separate directory from the system one.

Note that I'm not entirely sure if the above changes will be sufficient for the "ruby" subcommand to continue working properly. It may have dependencies on flowing through other environment variables which would need to be discovered as this work is undertaken.



 Comments   
Comment by Jeff McCune [ 2015/01/12 ]

The specific discussion point is around how to be as robust as possible while also giving the end-user the environment variable knobs and dials they expect.

On the extreme conservative side, we've tried unsetting all environment variables except those necessary, e.g. GEM_HOME. This caused issues, e.g. `gem env` expects PATH to be set.

On the extreme lax side, we could do nothing and manage no environment variables.

There's a big middle ground, but something I specifically propose is to manage environment variables common to Ruby. These are GEM_HOME, GEM_PATH, RUBYLIB, RUBY_OPTS, and RUBYOPT. If puppetserver doesn't need one of these variables set, then it unsets it. For other environment variables, e.g. FOO_DEBUG=true and PATH, puppetserver could pass those variables through untouched.

Comment by Steve Barlow [ 2015/03/18 ]

Pulled out because of PE 3.8 dates.

Comment by Nan Liu [ 2015/04/14 ]

Please note discussion on puppet-dev:
https://groups.google.com/forum/#!topic/puppet-dev/Ac6gCTJXGnA

This has impact on function calls over http and honoring system http_proxy, no_proxy environment settings.

Comment by Geoff Williams [ 2015/04/14 ]

I saw the google groups discussion and to me the mapping idea is a bit complex and another thing for users to get wrong. How about a hybrid approach in the puppet server config file - with good defaults?:

  jruby-puppet {
    gem-home: /var/lib/puppet/jruby-gems,
    # add more explicit variables here if you like...
    environment-whitelist: {
      http_proxy,
      https_proxy,
      no_proxy,
    }
    ...
  }

features

  • users can allow the variables they want to pass through
  • explicit values can be set where needed
  • sensible defaults can be used
  • we steer customers towards a consistent set of environment variables

Additionally, it would be nice if the proxy variables could be parsed out of puppet.conf so that they will be used if set but can be overriden by explicit environment variables (if set).

Comment by Jeremy Barlow [ 2015/04/14 ]

There's been a fair amount of discussion here and in related tickets and puppet-dev about a desire to have an option for at least the proxy-related environment variables to be passed through from the shell environment to Ruby code in the Puppet Server process. Are there any concrete use cases for passing through other environment variables that users have?

Comment by Jeff McCune [ 2015/05/29 ]

TODO:

Jeremy explained that in Puppet Server 1.x we needed to prepend the 3 ruby lib
directories to the front of the LOAD_PATH because Puppet's code was located in
the /usr system Ruby load path and we were picking up the wrong version of
rubygems (or something similar resulting from cross-loading a library from a
different copy of Ruby than the JRuby in puppetserver) when trying to require
the hipchat gem in SERVER-538

In Puppet Server 2.x this should no longer be an issue because the Puppet code
is (finally) located outside of a core ruby library directory.

We need to validate this actually works if we remove the three elements
pre-pended on the front of the load path in 2.x. Jeremy mentioned he tried to
remove them in the master branch but that didn't work for some
as-of-yet-unknown reason.

Comment by Jeff McCune [ 2015/06/05 ]

Note: While this ticket will be resolved, all of the work described was not completed. The remaining work is captured in SERVER-721 which is back-port the change from master to stable.

Comment by Jeff McCune [ 2015/06/05 ]

PR for FOO_DEBUG smoke test failures is at: https://github.com/puppetlabs/puppet-server/pull/598

Comment by Jeff McCune [ 2015/06/08 ]

I was able to successfully use the hipchat report processor module with our hipchat ${PROJECT_ROOM} room from the master branch. The build I tested is //builds.puppetlabs.lan/puppetserver/2.1.1.master.SNAPSHOT.2015.06.08T1118/repo_configs/rpm/pl-puppetserver-2.1.1.master.SNAPSHOT.2015.06.08T1118-el-7-x86_64.repo with PC1

Comment by Erik Dasher [ 2015/06/09 ]

Jeff's test above on the hipchat gem is a regression check rather than a FR:

Previous behavior was that 'puppetserver gem env' would throw an uncaught exception trying to parse path.

Installed Red Hat Enterprise Linux Server release 6.5 (Santiago), Puppet v4.1.0 via puppet-agent-1.1.0-1.el6.x86_64.rpm, and puppetserver-2.1.1.master-0.1SNAPSHOT.2015.06.09T0120.el6.noarch.rpm.

puppetserver irb supplies a prompt, puts RUBY_VERSION returns 1.9.3.
puppetserver ruby -pe 'puts RUBY_VERSION' returns "1.9.3" after feeding in some returns.
puppetserver gem env returns expected output:
RubyGems Environment:

Then had some fun with pry...
puppetserver gem install pry
puppetserver irb
irb(main):001:0>
irb(main):002:0* require 'pry'
=> true
irb(main):003:0> binding.pry
[1] pry(main)> ls
IRB::ExtendCommandBundle#methods:
install_alias_method irb_context irb_fg irb_kill irb_push_workspace irb_workspaces
irb irb_current_working_workspace irb_help irb_load irb_require
irb_change_workspace irb_exit irb_jobs irb_pop_workspace irb_source
self.methods:
bindings cws inspect irb_current_working_binding irb_popb irb_pushb jobs pushws workspaces
cb cwws irb_bindings irb_cwb irb_popws irb_pushws kill pwws
chws exit irb_cb irb_cws irb_print_working_binding irb_pwb popb quit
conf fg irb_change_binding irb_cwws irb_print_working_workspace irb_pwws popws source
context help irb_chws irb_pop_binding irb_push_binding irb_quit pushb to_s
locals: _ __ dir ex file in out pry

puppetserver ruby -rpry -e 'binding.pry;' also works.

Generated at Tue Jan 28 08:28:22 PST 2020 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.