Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bundler: freeze lockfile on run, and "normalize" platform on plugin changes #13015

Merged
merged 29 commits into from
Aug 17, 2021

Conversation

robbavey
Copy link
Member

@robbavey robbavey commented Jun 21, 2021

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > 2.23 making runtime changes to Gemfile.lock (unless the lock file was frozen) based on the specific platform the application was being run on, overriding any platforms (including generic java platform) set during build time. This was in conflict with changes made in #12782, which prevented the logstash user writing to files in /usr/share/logstash.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic java platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, --enable-local-plugin-development. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'

to facilitate quick and simple plugin testing.

This PR also sets the silence_root_warning flag to avoid bundler printing out alarming looking warning messages when sudo is used. This warning message was concerning for users - it would be printed out during normal operation of bin/logstash-plugin install/update/remove when run under sudo, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

Release notes

  • Updated Bundler to latest version
  • Ensured that Gemfile.lock are appropriately frozen
  • Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

How to test this PR locally

  • Ensure that logstash runs after install, as stand-alone, deb, rpm
  • Ensure that plugin installation, upgrade, removal and offline-pack creation and installation works, and that logstash runs correctly after each of these operations

The vagrant tests have been updated to include these tests for debian and rpm across multiple flavours of those distributions:

https://logstash-ci.elastic.co/job/elastic+logstash+master+multijob-acceptance/278/

Related issues

Relates: #12958, #12910, #12818.

@robbavey robbavey force-pushed the bundler_update branch 7 times, most recently from c07d595 to ff036c6 Compare June 25, 2021 14:06
@robbavey robbavey force-pushed the bundler_update branch 2 times, most recently from bb6829c to b932c7b Compare July 1, 2021 21:09
@robbavey robbavey force-pushed the bundler_update branch 2 times, most recently from b4420b9 to 97063eb Compare July 26, 2021 14:16
Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass:

Two things aren't clear to me:

  • What is the new --dev flag that we are adding to the logstash process, and when does it need to be set? I see that it effectively causes bundler to not freeze the lockfile, but I don't understand when it needs to get set and why. I suspect that a more descriptive flag or an environment variable may be a more discoverable path forward, and that we should also document this as a part of the "how to develop Logstash" tutorials.
  • We moved around a lot of silence_root_warning bits, removing a default from our invoke!, explicitly passing it to invoke! in a lot of places, and using its effective value to set an ENV variable that presumably the underlying bundler uses. Can you explain why we did what we did?

Comment on lines 182 to 193
def specific_platform
::Gem.platforms.find {|plat| plat.is_a?(::Gem::Platform) && plat.os=='java' && !plat.cpu.nil?}
end

def genericize_platform
output = LogStash::Bundler.invoke!({:add_platform => 'java', :silence_root_warning => true})
remove_platform_options = {:remove_platform => specific_platform.to_s, :silence_root_warning => true} unless specific_platform.nil?
output << LogStash::Bundler.invoke!(remove_platform_options) unless remove_platform_options.nil?
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not possible directly with our tooling as-is, it is possible to end up with a Gemfile.lock with more than one specific platform. Since we effectively have a zero-or-more situation, I would prefer to iterate over an enumberable.

Suggested change
def specific_platform
::Gem.platforms.find {|plat| plat.is_a?(::Gem::Platform) && plat.os=='java' && !plat.cpu.nil?}
end
def genericize_platform
output = LogStash::Bundler.invoke!({:add_platform => 'java', :silence_root_warning => true})
remove_platform_options = {:remove_platform => specific_platform.to_s, :silence_root_warning => true} unless specific_platform.nil?
output << LogStash::Bundler.invoke!(remove_platform_options) unless remove_platform_options.nil?
end
def specific_platforms
::Gem.platforms.select {|plat| plat.is_a?(::Gem::Platform) && plat.os=='java' && !plat.cpu.nil?}
end
def genericize_platform
output = LogStash::Bundler.invoke!({:add_platform => 'java', :silence_root_warning => true})
specific_platform.each do |platform_to_remove|
output << LogStash::Bundler.invoke!({:remove_platform => platform_to_remove.to_s, :silence_root_warning => true})
end
end

lib/pluginmanager/bundler/logstash_injector.rb Outdated Show resolved Hide resolved
@robbavey
Copy link
Member Author

robbavey commented Jul 26, 2021

@yauuie Having the concept of a dev mode/thawed mode enables a logstash developer to make changes to the Gemfile and have them picked up at runtime. An example would be to use a development plugin - prior to this change it was possible to add , :path => '/path/to/plugin' and have it picked up automatically and used, which can be useful when testing plugins. Having the dev flag allowed this to still be possible. It also provides an "escape hatch" if a user somehow manages to find themselves broken and frozen.

I to'ed and fro'ed on the dev flag, what the right name for it was (thaw, unfreeze?), whether an ENV would make sense and whether it was worth including at all, and I'm happy to take suggestions - its one reason why I hadn't documented it yet...

The silence_root_warning is set to avoid this error, which is output during any interaction with bin/logstash-plugin when used with sudo, which is required in a deb/rpm environment.:

Don't run Bundler as root. Bundler can ask for sudo if it is needed, and
installing your bundle as root will break this application for all non-root
users on this machine.

As far as using the ENV, I could probably do with investing some more effort to see if that is avoidable - I'd run into somewhat of a brick wall trying to get the setting to take effect and that did the trick.

removing a default from our invoke!,
Which default did I remove?

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will give more thought to the --dev flag. I worry that a short flag that is very easy to type will lend itself to becoming overloaded or divorced from its original intent.

In my mind it is a rare feature that doesn't need to be frequently typed, and would benefit from having a longer, more-descriptive name. Maybe --enable-local-plugin-development? When this flag is not enabled, and a user tries to use a local path-defined plugin, can we reliably detect this and suggest adding the flag?

@@ -91,7 +91,7 @@ def setup!(options = {})
# @option options [Array] :without Exclude gems that are part of the specified named group (default: [:development])
# @return [String, Exception] the installation captured output and any raised exception or nil if none
def invoke!(options = {})
options = {:max_tries => 10, :clean => false, :install => false, :update => false, :local => false,
options = {:silence_root_warning => false, :max_tries => 10, :clean => false, :install => false, :update => false, :local => false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where your diff effectively removes the default value, since our options becomes the result of merging the given options onto the hard-coded hash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦🏼 this doesn't remove a default. How did I read it that way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the variable, so it is set across the board anyway - I don't think there is a place that seeing the bundler error would be useful or actionable for users, so I'm not sure what value not silencing it would bring

@robbavey robbavey force-pushed the bundler_update branch 3 times, most recently from 35958a2 to fd4d669 Compare July 27, 2021 21:12
@robbavey
Copy link
Member Author

jenkins test this please

@robbavey robbavey force-pushed the bundler_update branch 3 times, most recently from de845c2 to 8ac6138 Compare July 28, 2021 19:04
@robbavey robbavey marked this pull request as ready for review July 29, 2021 17:32
@robbavey robbavey changed the title [wip] Bundler: freeze lockfile on run, and "normalize" platform on pl… Bundler: freeze lockfile on run, and "normalize" platform on plugin changes Jul 29, 2021
@robbavey
Copy link
Member Author

@yaauie Ready for another round, I think

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundler changes look great.

I've left a couple comments about reducing repetition in specs by using a helper method, and noted a lack of clarity in changes to the QA matrix and supporting code.

# to update plugins when logstash is run as a service
# Note: Using an `ENV` here, as ::Bundler.settings.set_local(:silence_root_warning, true)
# does not work (set_global *does*, but that seems too drastic a change)
ENV["BUNDLE_SILENCE_ROOT_WARNING"] = "true"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truly a nitpick, since we exit the process before doing anything else that will be affected by this, but it may be good to clean up after ourselves so we don't pollute the ENV for longer than we need to.

def with_env(modifications)
  backup_env = ENV.to_hash
  ENV.replace(backup_env.merge(modifications))

  yield
ensure
  ENV.replace(backup_env)
end

Which would allow us to do:

  with_env("BUNDLE_SILENCE_ROOT_WARNING" => "true") do
    if !debug?
        # Will deal with transient network errors
        execute_bundler_with_retry(options)
    else
      options[:verbose] = true
      execute_bundler(options)
    end
  end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like that. Not a fan of unintended side effects. Even if it's not a problem now, its the kind of subtle thing that can bite later

@@ -93,6 +93,11 @@ def inject(gemfile_path, lockfile_path, dependencies)

builder.eval_gemfile("bundler file", gemfile.generate())
definition = builder.to_definition(lockfile_path, {})
# Remove the specific platform, and leave the generic platform
LogStash::Bundler.specific_platforms.each do |specific_platform|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 is there a reason we don't just invoke LogStash::Bundler::genericise_platform here? Is it possible that only removing the specific platforms and not adding a generic platform could leave us with no platforms?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular code path is only used during the offline pack installation, and doesn't call out to the command-line bundler and the invoke! method that the genericize_platform uses. However, I have made a change here to call the specific_platforms filter on the platforms returned from definition.platforms, rather than making a call to Gem::platforms

Comment on lines 273 to 275
Allow Gemfile to be manipulated directly to facilitate simpler local plugin development.
This is an advanced setting, intended only for use by logstash developers, and should
not be used in production.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is included in command-line --help output that doesn't play nicely with wrapping, we should reformat to keep the individual lines short (<40char).

Suggested change
Allow Gemfile to be manipulated directly to facilitate simpler local plugin development.
This is an advanced setting, intended only for use by logstash developers, and should
not be used in production.
Allow Gemfile to be manipulated directly
to facilitate simpler local plugin
development.
This is an advanced setting, intended
only for use by Logstash developers,
and should not be used in production.

Comment on lines +257 to +260
private static final String MUTATED_GEMFILE_ERROR = "Logstash was unable to start due to an unexpected Gemfile change.\n" +
"If you are a user, this is a bug.\n" +
"If you are a logstash developer, please try restarting logstash with the " +
"`--enable-local-plugin-development` flag set.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 for useful only-when-needed guidance

Comment on lines 48 to 52
logstash.start_service
Stud.try(40.times, RSpec::Expectations::ExpectationNotMetError) do
expect(logstash).to be_running
end
logstash.stop_service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we are adding a secondary validation that Logstash can start up after performing the primary task successfully.

Since we are doing it repeatedly, I think we should break this out into a helper method.

@@ -33,23 +33,39 @@

before do
logstash.run_command_in_path("bin/logstash-plugin install --no-verify --version #{previous_version} #{plugin_name}")
logstash.run_command_in_path("bin/logstash-plugin list")
# Logstash won't update when we have a pinned version in the gemfile so we remove them
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment appears to go with a line that was moved.

It is not clear to me why we need to do a gemfile replacement to un-pin, and whether that is supposed to affect the result of the have_installed validation that is now occurring before the un-pinning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the comment around.

It's a pretty hacky test. We install a named, older version of the test plugin, and do a have_installed check to ensure the pre-condition that the plugin has installs as expected. The sed ugliness that is done on the Gemfile is to remove the pin - bin/logstash-plugin update will not upgrade pinned plugins (in this case a plugin that has been installed at a specific version). Whether that is desired behaviour, or whether it is something we should allow to be bypassed via a --force option is a valid question

qa/rspec/commands.rb Show resolved Hide resolved
@@ -6,12 +6,8 @@
"centos-7": { "box": "elastic/centos-7-x86_64", "type": "redhat" },
"oel-6": { "box": "elastic/oraclelinux-6-x86_64", "type": "redhat" },
"oel-7": { "box": "elastic/oraclelinux-7-x86_64", "type": "redhat" },
"fedora-28": { "box": "elastic/fedora-28-x86_64", "type": "redhat", "experimental": true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes to drop old platforms in scope? If so, I'd like to know why. I would assume that we would merely include later versions, instead of dropping these entirely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not. I'll reinstate them, and bring the removal up in a separate PR, or at least spark a discussion as to whether we want to keep them - they're not run in CI right now, and I was trying to figure it if it was the cause behind test failures. (Actually hashicorp/vagrant#12265 is the cause)

@robbavey robbavey requested a review from karenzone July 30, 2021 17:40
@robbavey
Copy link
Member Author

Adding @karenzone for doc update review

@robbavey
Copy link
Member Author

@yaauie Ready for another round - the CI failures look unrelated, see: #13136

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm largely comfortable merging as-is. This is wide-ranging, but sensible and merely a side-effect of us hooking bundler in such a way.

I have left a few nitpicks and also have a question about needing to ensure that a generic java is in the platform list inside the "logstash injector" when we are removing the specific javas.

lib/pluginmanager/install.rb Outdated Show resolved Hide resolved
Comment on lines 63 to 65
Stud.try(40.times, RSpec::Expectations::ExpectationNotMetError) do
yield
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This certainly works for all of the specs you have introduced in this PR, but it may prove to be a mismatch when we introduce other specs that check other things with the running service.

It may be helpful to only yield once when we know that logstash is actually running. I know this double-checks in the cases of our current use, but it also ensures that we don't retry other expectations 40x in this helper.

Suggested change
Stud.try(40.times, RSpec::Expectations::ExpectationNotMetError) do
yield
end
Stud.try(40.times, RSpec::Expectations::ExpectationNotMetError) do
expect(logstash).to be_running
end
yield

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion didn't quite work - expect(logstash).to be_running doesn't work when a specific jdk_path is used, but I've added the equivalent to the helper method

qa/docker/spec/spec_helper.rb Outdated Show resolved Hide resolved
@robbavey
Copy link
Member Author

@yaauie Ready for another round, integration tests running at https://logstash-ci.elastic.co/job/elastic+logstash+master+multijob-acceptance/284/

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks for sticking through all the bonus review cycles ❤️

@robbavey robbavey merged commit 4707cbd into master Aug 17, 2021
robbavey added a commit that referenced this pull request Aug 17, 2021
…malize" platform on plugin changes

Backport PR #13015 to 7.15 branch. Original Message:

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > `2.23` making runtime changes to `Gemfile.lock` (unless the lock file was `frozen`) based on the specific platform the application was being run on, overriding any platforms (including generic `java` platform) set during build time. This was in conflict with changes made in #12782, which prevented the logstash user writing to files in `/usr/share/logstash`.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic `java` platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, `--enable-local-plugin-development`. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

```
gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'
```

to facilitate quick and simple plugin testing.

This PR also sets the `silence_root_warning` flag to avoid bundler printing out alarming looking warning messages when `sudo` is used. This warning message was concerning for users - it would be printed out during normal operation of `bin/logstash-plugin install/update/remove` when run under `sudo`, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

* Updated Bundler to latest version
* Ensured that `Gemfile.lock` are appropriately frozen
* Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

(cherry picked from commit 4707cb)
robbavey added a commit to robbavey/logstash that referenced this pull request Aug 17, 2021
…d "normalize" platform on plugin changes

Backport PR elastic#13015 to 7.x branch. Original Message:

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > `2.23` making runtime changes to `Gemfile.lock` (unless the lock file was `frozen`) based on the specific platform the application was being run on, overriding any platforms (including generic `java` platform) set during build time. This was in conflict with changes made in elastic#12782, which prevented the logstash user writing to files in `/usr/share/logstash`.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic `java` platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, `--enable-local-plugin-development`. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

```
gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'
```

to facilitate quick and simple plugin testing.

This PR also sets the `silence_root_warning` flag to avoid bundler printing out alarming looking warning messages when `sudo` is used. This warning message was concerning for users - it would be printed out during normal operation of `bin/logstash-plugin install/update/remove` when run under `sudo`, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

* Updated Bundler to latest version
* Ensured that `Gemfile.lock` are appropriately frozen
* Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

(cherry picked from commit 4707cb)
robbavey added a commit to robbavey/logstash that referenced this pull request Aug 17, 2021
…d "normalize" platform on plugin changes

Backport PR elastic#13015 to 7.x branch. Original Message:

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > `2.23` making runtime changes to `Gemfile.lock` (unless the lock file was `frozen`) based on the specific platform the application was being run on, overriding any platforms (including generic `java` platform) set during build time. This was in conflict with changes made in elastic#12782, which prevented the logstash user writing to files in `/usr/share/logstash`.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic `java` platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, `--enable-local-plugin-development`. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

```
gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'
```

to facilitate quick and simple plugin testing.

This PR also sets the `silence_root_warning` flag to avoid bundler printing out alarming looking warning messages when `sudo` is used. This warning message was concerning for users - it would be printed out during normal operation of `bin/logstash-plugin install/update/remove` when run under `sudo`, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

* Updated Bundler to latest version
* Ensured that `Gemfile.lock` are appropriately frozen
* Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

(cherry picked from commit 4707cb)
robbavey added a commit to robbavey/logstash that referenced this pull request Aug 17, 2021
…nd "normalize" platform on plugin changes

Backport PR elastic#13015 to 7.15 branch. Original Message:

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > `2.23` making runtime changes to `Gemfile.lock` (unless the lock file was `frozen`) based on the specific platform the application was being run on, overriding any platforms (including generic `java` platform) set during build time. This was in conflict with changes made in elastic#12782, which prevented the logstash user writing to files in `/usr/share/logstash`.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic `java` platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, `--enable-local-plugin-development`. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

```
gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'
```

to facilitate quick and simple plugin testing.

This PR also sets the `silence_root_warning` flag to avoid bundler printing out alarming looking warning messages when `sudo` is used. This warning message was concerning for users - it would be printed out during normal operation of `bin/logstash-plugin install/update/remove` when run under `sudo`, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

* Updated Bundler to latest version
* Ensured that `Gemfile.lock` are appropriately frozen
* Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

(cherry picked from commit 4707cb)
robbavey added a commit that referenced this pull request Aug 18, 2021
#13141)

* Backport PR #13015 to 7.15: Bundler: freeze lockfile on run, and "normalize" platform on plugin changes

Backport PR #13015 to 7.15 branch. Original Message:

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > `2.23` making runtime changes to `Gemfile.lock` (unless the lock file was `frozen`) based on the specific platform the application was being run on, overriding any platforms (including generic `java` platform) set during build time. This was in conflict with changes made in #12782, which prevented the logstash user writing to files in `/usr/share/logstash`.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic `java` platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, `--enable-local-plugin-development`. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

```
gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'
```

to facilitate quick and simple plugin testing.

This PR also sets the `silence_root_warning` flag to avoid bundler printing out alarming looking warning messages when `sudo` is used. This warning message was concerning for users - it would be printed out during normal operation of `bin/logstash-plugin install/update/remove` when run under `sudo`, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

* Updated Bundler to latest version
* Ensured that `Gemfile.lock` are appropriately frozen
* Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

(cherry picked from commit 4707cb)

* Remove code pinning bundler to `~> 1.17`

Also updates lockfile to update `BUNDLED WITH` to latest
robbavey added a commit that referenced this pull request Aug 18, 2021
#13140)

* Backport PR #13015 to 7.x: Bundler: freeze lockfile on run, and "normalize" platform on plugin changes

Backport PR #13015 to 7.x branch. Original Message:

This PR enables the upgrade of bundler to the latest version.

Prior to this PR, the ability to do so was blocked by bundler.setup in versions of bundler > `2.23` making runtime changes to `Gemfile.lock` (unless the lock file was `frozen`) based on the specific platform the application was being run on, overriding any platforms (including generic `java` platform) set during build time. This was in conflict with changes made in #12782, which prevented the logstash user writing to files in `/usr/share/logstash`.

This PR will freeze the lockfile when logstash is run, and unfreeze it when manipulating plugins (install, update, remove, install from offline pack) to allow new plugins to be added. While unfrozen, changes are also made to ensure that the platform list remains as the generic `java` platform, and not changed to the specific platform for the runtime JVM.

This PR also introduces a new runtime flag, `--enable-local-plugin-development`. This flag is intended for use by Logstash developers only, and enables a mode of operation where a Gemfile can be manipulated, eg

```
gem "logstash-integration-kafka", :path => '/users/developer/code/plugins/logstash-integration-kafka'
```

to facilitate quick and simple plugin testing.

This PR also sets the `silence_root_warning` flag to avoid bundler printing out alarming looking warning messages when `sudo` is used. This warning message was concerning for users - it would be printed out during normal operation of `bin/logstash-plugin install/update/remove` when run under `sudo`, which is the expected mode of operation when logstash is installed to run as a service via rpm/deb packages.

This PR also updates the vagrant based integration tests to ensure that Logstash still runs after plugin update/install/remove operations, fixes up some regular expressions that would cause test failures, and removes some dead code from tests.

* Updated Bundler to latest version
* Ensured that `Gemfile.lock` are appropriately frozen
* Added new developer-only flag to facilitate local plugin development to allow unfrozen lockfile in a development environment

(cherry picked from commit 4707cb)

* Remove code pinning bundler to ~> 1.17
@robbavey robbavey mentioned this pull request Aug 18, 2021
5 tasks
kares added a commit to kares/logstash that referenced this pull request Aug 23, 2021
* master:
  Update releases list (elastic#13149)
  Bundler: freeze lockfile on run, and "normalize" platform on plugin changes (elastic#13015)
  Doc: Move shared region tags for breaking changes to 8.0.0 content (elastic#13131)
  Update Snakeyaml version to 1.29 (elastic#13129)
  Doc: Add breaking changes to breaking changes page (elastic#13130)
  Added faraday-* and ruby2_keywords notices to licences list (elastic#13126)
  Release notes draft for 8.0.0-alpha1 (elastic#13098)
  valid variable expansion for the batch files (elastic#12912)
  Doc: Enhance and expand DLQ docs (elastic#12959)
  Update releases list with 7.13.4 (elastic#13088)
  Doc:Fix typo and adjust keystore text (elastic#12779)
  doc: add pipeline.ecs_compatibility docs (elastic#12421)
  ecs_compatibility: revert breaking change; keep `disabled` as default for 8.0.0 (elastic#13080)
  Doc: Add kafka schema registry setting to troubleshooting (elastic#13073)
  [Test] Fix Unix acceptance tests (elastic#13071)
  Move retrieval of Stack version from Gradle's configuration to execution (elastic#13042)
  Update releases list with 6.8.17 and 7.13.3 (elastic#13041)
  Docs: keep elastic_app_search output meta-data (elastic#13048)
  Fix LS benchmarking tool to work with releases >= 7.10.0 (elastic#13052)
  Revert "Change Gradle's :logstash-integration-tests:integrationTests task to depends on copyES (elastic#12847)" (elastic#13045)
@kares kares mentioned this pull request Sep 1, 2021
@kares kares added the v7.15.0 label Nov 25, 2021
@maltewhiite
Copy link

@MarcoMartins86
Copy link

@maltewhiite I've also encountered this problem. I'm using the logstash official docker and I was adding gems to the Gemfile manually which triggers the error. The tricky part was to run the embedded bundle to install the gems, not sure if there is an easier way to do it without having hardcoded paths that might change in the future. This solved for me
/usr/share/logstash/bin/ruby -S $(/usr/share/logstash/bin/ruby -S gem env gemdir 2>&1 | grep /usr/share/logstash/vendor/bundle/)/bin/bundle add redis, if you're not adding gems manually but still see the error maybe try to do a bundle lock before running logstash.

@maltewhiite
Copy link

@MarcoMartins86 - Thanks a lot!!

Could you explain what happens here?
$(/usr/share/logstash/bin/ruby -S gem env gemdir 2>&1 | grep /usr/share/logstash/vendor/bundle/)/bin/bundle

  • What is 2>&1?
  • What is $( ) ?

@MarcoMartins86
Copy link

@maltewhiite

  • $( ): basically evaluates the expression inside the parenthesis
  • 2>&1 | : redirects all output (stdout, stderror) from previous call /usr/share/logstash/bin/ruby -S gem env gemdir to grep

Finally, the whole thing is just to give you the root path for the gem dir /usr/share/logstash/vendor/bundle/jruby/2.5.0, so in the future, the version can change and I don't need to change it manually.

@jsvd jsvd deleted the bundler_update branch June 2, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants