From cdc389b6a5570a31f5d48f6bc7c95313c2fef8ba Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Wed, 29 May 2024 09:16:05 -0700 Subject: [PATCH 1/2] (maint) Allow not applying settings catalog on startup On initialization of a JRuby instance, Puppet will compile and apply a catalog containing resources for the configured directories specified in the appropriate settings sections. The goal of this is to ensure any directories Puppet requires exist. In practice this should be managed by our packages. Attempting to apply a catalog every time a JRuby instance is unnecessary, slow, and causes race conditions when attempting to instantiate JRuby instances in parallel. Puppet has a setting "settings_catalog" that controls this behavior. This PR addresses issues that arise when we set "settings_catalog" to `false`: 1. In code the process of compiling a settings catalog brings most of Puppet's ruby code into scope. When not compiling a settings catalog we must actually declare our dependencies. 2. The $yamldir/facts directory is not created by packages despite being used by default. --- .../ca_files_test/puppet.conf | 20 ---- dev/puppetserver.conf | 5 + project.clj | 3 +- .../puppet/jvm/logging_spec.rb | 8 +- .../puppet/server/log_collector.rb | 2 + .../puppetserver-lib/puppet/server/logger.rb | 2 + .../puppetserver-lib/puppet/server/logging.rb | 2 + .../puppet/server/puppet_config.rb | 9 ++ .../services/master/master_service_test.clj | 97 ++++++------------- 9 files changed, 58 insertions(+), 90 deletions(-) delete mode 100644 dev-resources/puppetlabs/services/master/master_service_test/ca_files_test/puppet.conf diff --git a/dev-resources/puppetlabs/services/master/master_service_test/ca_files_test/puppet.conf b/dev-resources/puppetlabs/services/master/master_service_test/ca_files_test/puppet.conf deleted file mode 100644 index 25ac97234..000000000 --- a/dev-resources/puppetlabs/services/master/master_service_test/ca_files_test/puppet.conf +++ /dev/null @@ -1,20 +0,0 @@ -[main] -certname = localhost - -cadir = target/master-service-test/ca-files-test/ca -vardir = target/master-service-test/ca-files-test/var -ssldir = target/master-service-test/ca-files-test/ssl - -capub = target/master-service-test/ca-files-test/ca/ca_pub.pem -cakey = target/master-service-test/ca-files-test/ca/ca_key.pem -cacert = target/master-service-test/ca-files-test/ca/ca_crt.pem -localcacert = target/master-service-test/ca-files-test/ca/ca.pem -cacrl = target/master-service-test/ca-files-test/ca/ca_crl.pem -hostcrl = target/master-service-test/ca-files-test/ca/crl.pem - -hostpubkey = target/master-service-test/ca-files-test/public_keys/localhost.pem -hostprivkey = target/master-service-test/ca-files-test/private_keys/localhost.pem -hostcert = target/master-service-test/ca-files-test/certs/localhost.pem - -serial = target/master-service-test/ca-files-test/certs/serial -cert_inventory = target/master-service-test/ca-files-test/inventory.txt diff --git a/dev/puppetserver.conf b/dev/puppetserver.conf index 0ff555c12..a6f6d0ed1 100644 --- a/dev/puppetserver.conf +++ b/dev/puppetserver.conf @@ -89,6 +89,11 @@ jruby-puppet: { # For testing running requests through a single JRuby instance. DO NOT ENABLE unless # explicitly testing this functionality. # multithreaded: true + + # (optional) When (re)filling a pool one instance will be initialized first, then + # the remaining instances will be initialized at the specified level of concurrency. + # Set to one for the previous serialized behavior. Default is three. + # instance-creation-concurrency: 1 } # Settings related to HTTP client requests made by Puppet Server. diff --git a/project.clj b/project.clj index 4d4743125..c17a59af2 100644 --- a/project.clj +++ b/project.clj @@ -100,7 +100,8 @@ :puppet-platform-version 8 :java-args ~(str "-Xms2g -Xmx2g " "-Djruby.logger.class=com.puppetlabs.jruby_utils.jruby.Slf4jLogger") - :create-dirs ["/opt/puppetlabs/server/data/puppetserver/jars"] + :create-dirs ["/opt/puppetlabs/server/data/puppetserver/jars" + "/opt/puppetlabs/server/data/puppetserver/yaml"] :repo-target "puppet8" :nonfinal-repo-target "puppet8-nightly" :bootstrap-source :services-d diff --git a/spec/puppet-server-lib/puppet/jvm/logging_spec.rb b/spec/puppet-server-lib/puppet/jvm/logging_spec.rb index 63f3915e6..ae2255287 100644 --- a/spec/puppet-server-lib/puppet/jvm/logging_spec.rb +++ b/spec/puppet-server-lib/puppet/jvm/logging_spec.rb @@ -5,8 +5,12 @@ describe Puppet::Server::Logging do context 'when setting the log level' do - before :each do - Puppet::Server::PuppetConfig.initialize_puppet(puppet_config: {}) + it 'flush logging queue' do + # The logger will queue any old log messages, creating a logging destination + # will force the pending queue to be flushed to this logger. We don't care + # about these messages so we discard the logger, but we do not want them to + # interfere with the next tests. + _, _ = Puppet::Server::Logging.capture_logs('debug') do; end end it 'correctly filters messages' do diff --git a/src/ruby/puppetserver-lib/puppet/server/log_collector.rb b/src/ruby/puppetserver-lib/puppet/server/log_collector.rb index 01ec5c18d..4fcf46742 100644 --- a/src/ruby/puppetserver-lib/puppet/server/log_collector.rb +++ b/src/ruby/puppetserver-lib/puppet/server/log_collector.rb @@ -1,3 +1,5 @@ +require 'puppet/util/log' + module Puppet module Server # Log to an array, just for testing. diff --git a/src/ruby/puppetserver-lib/puppet/server/logger.rb b/src/ruby/puppetserver-lib/puppet/server/logger.rb index 1ee1f02d1..db2b7194a 100644 --- a/src/ruby/puppetserver-lib/puppet/server/logger.rb +++ b/src/ruby/puppetserver-lib/puppet/server/logger.rb @@ -1,3 +1,5 @@ +require 'puppet' +require 'puppet/util' require 'puppet/server' require 'java' diff --git a/src/ruby/puppetserver-lib/puppet/server/logging.rb b/src/ruby/puppetserver-lib/puppet/server/logging.rb index 5d49059d2..de1abb8a0 100644 --- a/src/ruby/puppetserver-lib/puppet/server/logging.rb +++ b/src/ruby/puppetserver-lib/puppet/server/logging.rb @@ -1,3 +1,5 @@ +require 'puppet' +require 'puppet/util/log' require 'puppet/server/log_collector' module Puppet diff --git a/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb b/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb index afd1d8b90..b1ab0eb23 100644 --- a/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb +++ b/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb @@ -1,6 +1,13 @@ require 'puppet/server' require 'puppet/server/logger' require 'puppet/server/http_client' +require 'puppet/indirector/indirection' +require 'puppet/file_serving/content' +require 'puppet/file_serving/metadata' +require 'puppet/file_bucket/file' +require 'puppet/node' +require 'puppet/application_support' +require 'puppet/ssl/oids' class Puppet::Server::PuppetConfig @@ -88,6 +95,8 @@ def self.initialize_puppet(puppet_config:) Puppet.push_context(dummy_ssl_context) end + # We have no added support for setting "settings_catalog" to false in the puppet.conf. + # We should default to not applying the settings catalog and remove this line in Puppet 9. Puppet.settings.use :main, :server, :ssl, :metrics if Puppet::Indirector::Indirection.method_defined?(:set_global_setting) diff --git a/test/integration/puppetlabs/services/master/master_service_test.clj b/test/integration/puppetlabs/services/master/master_service_test.clj index c800ab8e9..6f526c3d9 100644 --- a/test/integration/puppetlabs/services/master/master_service_test.clj +++ b/test/integration/puppetlabs/services/master/master_service_test.clj @@ -316,45 +316,6 @@ (+ (:duration-millis requested-instance) (:time requested-instance)))))))))))))) -(deftest ^:integration ca-files-test - (testing "CA settings from puppet are honored and the CA - files are created when the service starts up" - (let [ca-files-test-runtime-dir (str master-service-test-runtime-dir - "/ca-files-test") - ca-files-test-puppet-conf (fs/file test-resources-path - "ca_files_test/puppet.conf")] - (fs/delete-dir ca-files-test-runtime-dir) - (testutils/with-puppet-conf-files - {"puppet.conf" ca-files-test-puppet-conf} - ca-files-test-runtime-dir - (logutils/with-test-logging - (bootstrap-testutils/with-puppetserver-running - app - {:jruby-puppet {:gem-path gem-path - :server-conf-dir ca-files-test-runtime-dir - :max-active-instances 1} - :webserver {:port 8081}} - (let [jruby-service (tk-app/get-service app :JRubyPuppetService)] - (jruby-service/with-jruby-puppet - jruby-puppet jruby-service :ca-files-test - (letfn [(test-path! - [setting expected-path] - (is (= (ks/absolute-path expected-path) - (.getSetting jruby-puppet setting))) - (is (fs/exists? (ks/absolute-path expected-path))))] - - (test-path! "capub" (str ca-files-test-runtime-dir "/ca/ca_pub.pem")) - (test-path! "cakey" (str ca-files-test-runtime-dir "/ca/ca_key.pem")) - (test-path! "cacert" (str ca-files-test-runtime-dir "/ca/ca_crt.pem")) - (test-path! "localcacert" (str ca-files-test-runtime-dir "/ca/ca.pem")) - (test-path! "cacrl" (str ca-files-test-runtime-dir "/ca/ca_crl.pem")) - (test-path! "hostcrl" (str ca-files-test-runtime-dir "/ca/crl.pem")) - (test-path! "hostpubkey" (str ca-files-test-runtime-dir "/public_keys/localhost.pem")) - (test-path! "hostprivkey" (str ca-files-test-runtime-dir "/private_keys/localhost.pem")) - (test-path! "hostcert" (str ca-files-test-runtime-dir "/certs/localhost.pem")) - (test-path! "serial" (str ca-files-test-runtime-dir "/certs/serial")) - (test-path! "cert_inventory" (str ca-files-test-runtime-dir "/inventory.txt"))))))))))) - (def graphite-enabled-config {:metrics {:server-id "localhost" :reporters {:graphite {:update-interval-seconds 5000 @@ -729,34 +690,36 @@ (is (= 404 (:status resp))))))) (deftest ^:integration facts-upload-api - (bootstrap-testutils/with-puppetserver-running - app - {:jruby-puppet {:gem-path gem-path - :max-active-instances 2 ; we need 2 jruby-instances since processing the upload uses an instance - :server-code-dir test-resources-code-dir - :server-conf-dir master-service-test-runtime-dir - :server-var-dir (fs/tmpdir)}} - (let [jruby-service (tk-app/get-service app :JRubyPuppetService) - jruby-instance (jruby-testutils/borrow-instance jruby-service :facts-upload-endpoint-test) - container (:scripting-container jruby-instance)] - (try - (let [facts (.runScriptlet container "facts = Puppet::Node::Facts.new('puppet.node.test') - facts.values['foo'] = 'bar' - facts.to_json") - response (http-put "/puppet/v3/facts/puppet.node.test?environment=production" facts)] - - (testing "Puppet Server responds to PUT requests for /puppet/v3/facts" - (is (= 200 (:status response)))) - - (testing "Puppet Server saves facts to the configured facts terminus" - ;; Ensure the test is configured properly - (is (= "yaml" (.runScriptlet container "Puppet::Node::Facts.indirection.terminus_class"))) - (let [stored-facts (-> (.runScriptlet container "facts = Puppet::Node::Facts.indirection.find('puppet.node.test') - (facts.nil? ? {} : facts).to_json") - (json/parse-string))] - (is (= "bar" (get-in stored-facts ["values" "foo"])))))) - (finally - (jruby-testutils/return-instance jruby-service jruby-instance :facts-upload-endpoint-test)))))) + (let [tmpdir (fs/tmpdir)] + (fs/mkdir (str tmpdir "/yaml")) + (bootstrap-testutils/with-puppetserver-running + app + {:jruby-puppet {:gem-path gem-path + :max-active-instances 2 ; we need 2 jruby-instances since processing the upload uses an instance + :server-code-dir test-resources-code-dir + :server-conf-dir master-service-test-runtime-dir + :server-var-dir (fs/tmpdir)}} + (let [jruby-service (tk-app/get-service app :JRubyPuppetService) + jruby-instance (jruby-testutils/borrow-instance jruby-service :facts-upload-endpoint-test) + container (:scripting-container jruby-instance)] + (try + (let [facts (.runScriptlet container "facts = Puppet::Node::Facts.new('puppet.node.test') + facts.values['foo'] = 'bar' + facts.to_json") + response (http-put "/puppet/v3/facts/puppet.node.test?environment=production" facts)] + + (testing "Puppet Server responds to PUT requests for /puppet/v3/facts" + (is (= 200 (:status response)))) + + (testing "Puppet Server saves facts to the configured facts terminus" + ;; Ensure the test is configured properly + (is (= "yaml" (.runScriptlet container "Puppet::Node::Facts.indirection.terminus_class"))) + (let [stored-facts (-> (.runScriptlet container "facts = Puppet::Node::Facts.indirection.find('puppet.node.test') + (facts.nil? ? {} : facts).to_json") + (json/parse-string))] + (is (= "bar" (get-in stored-facts ["values" "foo"])))))) + (finally + (jruby-testutils/return-instance jruby-service jruby-instance :facts-upload-endpoint-test))))))) (deftest ^:integration v4-queue-limit (bootstrap-testutils/with-puppetserver-running From b6815bf8c9537db76b7204fe6cd62205692b1e9a Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 4 Jun 2024 14:28:37 -0700 Subject: [PATCH 2/2] typo: no -> now --- src/ruby/puppetserver-lib/puppet/server/puppet_config.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb b/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb index b1ab0eb23..b9fde88aa 100644 --- a/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb +++ b/src/ruby/puppetserver-lib/puppet/server/puppet_config.rb @@ -95,7 +95,7 @@ def self.initialize_puppet(puppet_config:) Puppet.push_context(dummy_ssl_context) end - # We have no added support for setting "settings_catalog" to false in the puppet.conf. + # We have now added support for setting "settings_catalog" to false in the puppet.conf. # We should default to not applying the settings catalog and remove this line in Puppet 9. Puppet.settings.use :main, :server, :ssl, :metrics