sous-chefs / chef-splunk

Development repository for the chef-splunk cookbook
https://supermarket.chef.io/cookbooks/chef-splunk
Apache License 2.0
75 stars 122 forks source link

Fix interpolation issue in the splunk_login_successful method #205

Closed haidangwa closed 12 months ago

haidangwa commented 3 years ago

Description

Issues Resolved

204

Check List

ramereth commented 3 years ago

@haidangwa can you please rebase instead of doing a merge commit update to keep the history linear? Also please address the chefspec issues.

haidangwa commented 3 years ago

Working on it now -- was having issues with my testing environment, but that seems to be resolved now, ATM.

the-label-bot[bot] commented 3 years ago

Size Prediction: medium Confidence: 0.999

Nice! This should be quick to review

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

the-label-bot[bot] commented 3 years ago

Size Prediction: medium Confidence: 0.999

Nice! This should be quick to review

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

ramereth commented 3 years ago

@haidangwa looks like we need to deal with some Chef 17 issues before we can get this merged unfortunately.

haidangwa commented 3 years ago

@ramereth I'm running the latest chef-workstation on my laptop and the test for uninstall-forwarder passes in Test Kitchen with Chef 17. I'm not sure why this is failing in Github Actions.

ramereth commented 3 years ago

@haidangwa I can replicate the problem on my end with 17.1.35. I suspect it has something to do with the systemd? helper included the cookbook conflicting with something else. That really should use chef-utils helpers IMO, but that would increase the Chef Client requirement (which we're going to need to do anyway).

haidangwa commented 3 years ago

The issue is between Chef Infra Client v17.0.150 and 17.0.242.

All tests pass under v17.0.150, but the uninstall-forwarder tests fail under 17.0.242

@ramereth

the-label-bot[bot] commented 3 years ago

Kind Prediction: fix Confidence: 0.999

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

the-label-bot[bot] commented 3 years ago

Kind Prediction: fix Confidence: 0.999

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

the-label-bot[bot] commented 3 years ago

Kind Prediction: fix Confidence: 0.999

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

haidangwa commented 3 years ago

@ramereth I've narrowed this down even further. Using kitchen-dokken, the chef infra clients pass all the way through and including 17.0.194. It breaks between 17.0.194 and 17.0.199. That seems definitive that it is a Chef Infra Client bug.

17.0.194: PASS

CHEF_VERSION=17.0.194 KITCHEN_LOCAL_YAML=kitchen.dokken.yml kitchen converge uninstall-forwarder-amazonlinux-2

17.0.199: FAIL *uninstall-forwarder test suites

CHEF_VERSION=17.0.199 KITCHEN_LOCAL_YAML=kitchen.dokken.yml kitchen converge uninstall-forwarder-amazonlinux-2

based on tags from docker hub

haidangwa commented 3 years ago

looks like ruby 3.0.1 was added to the chef omnibus_overrides.rb between v17.0.197 and v17.0.198

diff --git a/omnibus_overrides.rb b/omnibus_overrides.rb
index d20d702d73..32c2cc6e69 100644
--- a/omnibus_overrides.rb
+++ b/omnibus_overrides.rb
@@ -16,7 +16,8 @@ override "ncurses", version: "5.9"
 override "nokogiri", version: "1.11.0"
 override "openssl", version: mac_os_x? ? "1.1.1k" : "1.0.2y"
 override "pkg-config-lite", version: "0.28-1"
-override "ruby", version: "2.7.2"
+override "bundler", version: "2.2.15"
+override "ruby", version: "3.0.1"
 override "ruby-windows-devkit-bash", version: "3.1.23-4-msys-1.0.18"
 override "util-macros", version: "1.19.0"
 override "xproto", version: "7.0.28"
haidangwa commented 3 years ago

I think getting this cookbook to be compatible with Chef Infra Client 17 is beyond the scope of this PR. Fixing the changes to be Chef 17 compatible should be in itself a separate Pull Request.

haidangwa commented 3 years ago

@ramereth OK. With all that said (above), I am thinking the problem is in the service provider/resource; perhaps the recent changes to the service resource in Chef 17. In my custom resource, splunk_installer, it calls two helper methods, and both helper methods are referencing the node data.

  service 'Splunk' do
    service_name server? ? 'Splunkd' : 'SplunkForwarder'
    action systemd? ? %i(stop disable) : :stop
  end

Note: server? and systemd? are helper methods declared in libraries/helpers.rb and both return true/false after making a comparison using a node attribute.

    def server?
      node['splunk']['is_server'] == true
    end

     def systemd?
       node['init_package'] == 'systemd'
     end

However, the error does not fail for the server? and fails for the systemd?. It is perhaps a bug in the service provider's action argument.

It's also not that the custom resource doesn't have access to the node object, as I confirmed this by adding a log resource right before my custom resource uses the service resource. My log resource did not fail in my test.

  log "init_package is #{node['init_package']}"

  service 'Splunk' do
    service_name server? ? 'Splunkd' : 'SplunkForwarder'
    action systemd? ? %i(stop disable) : :stop
  end

In fact, it worked fine, as demonstrated in my chef run output, below:

    * log[init_package is systemd] action write
haidangwa commented 3 years ago

With all the above being said, I have refactored the pull request changes. It is now compatible with Chef 17 (with its alleged bug in the service provider) and removed a few unneeded dependencies. I've updated the PR description to match my entries in CHANGELOG.md. I've also cleaned up the commit history in my branch.

the-label-bot[bot] commented 3 years ago

Size Prediction: medium Confidence: 0.999

Nice! This should be quick to review

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

the-label-bot[bot] commented 3 years ago

Size Prediction: large Confidence: 0.999

This might take someone longer to review. Suggestion: Breaking it up into smaller pieces might be helpful.

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

haidangwa commented 3 years ago

All tests pass under Chef 17. This awaits a review and merge.

the-label-bot[bot] commented 3 years ago

Kind Prediction: fix Confidence: 0.999

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

the-label-bot[bot] commented 3 years ago

Size Prediction: large Confidence: 0.999

This might take someone longer to review. Suggestion: Breaking it up into smaller pieces might be helpful.

Provide the bot with feedback using a :thumbsup: or :thumbsdown:!

ramereth commented 3 years ago

@haidangwa so it looks like some of the fixes you have in here were done in #215 and conflicts with this. Can you please rebase and resolve the conflicts? Thanks!

damacus commented 12 months ago

Hey @haidangwa can you reopen this PR when you have a clean branch. Thanks!

Looks like some great work here.