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

Cleanup and bugfixes to ensure cookbook runs on modern installs. #125

Merged
merged 37 commits into from
Aug 20, 2021

Conversation

malcyon
Copy link
Contributor

@malcyon malcyon commented Aug 5, 2021

This is a collection of various fixes and cleanup to ensure the Graylog cookbook runs properly.

Goals

  • The information in the README should be up-to-date and accurate (Fix NAT Documentation is out of date #123).
  • The kitchen setup should run successfully.
  • The rspec tests should run successfully.
  • Ensure the CI runs the tests.
  • Ensure the cookbook installs successfully on Ubuntu 20.04 and Centos 8.
  • Update default version of Graylog that gets installed.
  • Update default version of Graylog Sidecar that gets installed (Fixes Unable to install latest sidecar #114)

Notes for Reviewers

  • The commit history must be preserved - please use the rebase-merge or standard merge option instead of squash-merge
  • Sync up with the author before merging

Remove oracle suite since java cookbook doesn't support it anymore.
@malcyon
Copy link
Contributor Author

malcyon commented Aug 5, 2021

Issues with the java cookbook dependency:

  • It insists on Chef 15. Fixed the kitchen.yml to specify that version.
  • It no longer supports oracle installs. I removed that section of the kitchen.yml.
  • It now demands that it be called as a custom resource. If you try to call the default recipe, it throws an error. I had to create a java recipe just for kitchen in order to call the custom recipe.

@malcyon
Copy link
Contributor Author

malcyon commented Aug 6, 2021

Graylog now successfully starts with kitchen converge openjdk-ubuntu-2004.

Edit: It starts up with Centos and Debian as well.

@malcyon
Copy link
Contributor Author

malcyon commented Aug 9, 2021

@bernd @mariussturm
This is not ready for merge yet, but I would appreciate a quick review just to make sure I'm not going totally in the wrong direction on anything. I have not used Chef before, so I might have overlooked something. I'm mostly just trying to make sure everything is up to date.

@mariussturm
Copy link
Contributor

@juju2112 give me a moment, I can review this in the next days!

Copy link
Contributor

@mariussturm mariussturm left a comment

Choose a reason for hiding this comment

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

Looks very good in general! Just some smaller issues around a kitchen test run (see inline comments).

The Sidecar is not starting with: No API token was configured. - Not sure if we want to automate this.
If yes, we also need to rename the Sidecar tests in https://github.com/Graylog2/graylog2-cookbook/tree/master/spec/unit/recipes

collector-sidecar -> graylog-sidecar

.kitchen.yml Show resolved Hide resolved
Gemfile.lock Show resolved Hide resolved
Vagrantfile.erb Outdated Show resolved Hide resolved
Fix some failing rspec tests.
@malcyon
Copy link
Contributor Author

malcyon commented Aug 17, 2021

@mariussturm Since I'm renaming the sidecar recipe, should I also bump the package version up to 4.0.0? Just to show there might be breaking changes.

@mariussturm
Copy link
Contributor

@juju2112 yes please, good idea!

Donald Morton added 2 commits August 17, 2021 10:13
Update recipe version to 4.0.0.
@malcyon
Copy link
Contributor Author

malcyon commented Aug 17, 2021

@mariussturm There are two issues left with the rspec tests:

  1. It throws this error 10 times when it runs:

ERROR: Failed to load data bag item: "secrets" "graylog"

Based on reading the rspec docs, it seems like we are meant to stub out the data bag stuff in the rspec file. But I am not sure what the syntax is supposed to look like. There is a section on stubbing on the ChefSpec github README, but it does not look anything like our code.

https://github.com/chefspec/chefspec#stubbing

These errors don't seem to cause the rspec to fail, though.

  1. The test to make sure the exception is thrown when the user has an old version of java is never thrown.
Failures:

  1) graylog2::default when the Java installation is not compatible raise an error and inform the user about the wrong version
     Failure/Error: expect { chef_run }.to raise_error("Java version needs to be >= 8 and <= 11")
       expected Exception with "Java version needs to be >= 8 and <= 11" but nothing was raised
     # ./spec/unit/recipes/default_spec.rb:50:in `block (3 levels) in <top (required)>'

Finished in 1 minute 34.36 seconds (files took 0.77435 seconds to load)
19 examples, 1 failure

Failed examples:

rspec ./spec/unit/recipes/default_spec.rb:49 # graylog2::default when the Java installation is not compatible raise an error and inform the user about the wrong version

So far, I am unable to get this error to change in any way. I've tried changing the java_version variable to '7', hardcoding java 7 into the java_test recipe, commenting out the openjdk_install line so that it installs nothing and making it look for the "java is not installed" exception. Same result each time.

Rspec really needs a debug mode.

@malcyon
Copy link
Contributor Author

malcyon commented Aug 17, 2021

The Sidecar is not starting with: No API token was configured. - Not sure if we want to automate this.

I think I'd rather leave automating that as a separate task. I could add the server_api_token to the template and comment out the "sidecar service is running" test for now.

The sidecar recipe would need to hit the server api and create the token. It would need the credentials for the server, too. So, the recipe would need to pull the creds from the data bag. But if someone wanted to specify the token themselves, then it would need to use that instead and not go out and try to create it.

@mariussturm
Copy link
Contributor

Looked into the rspec errors, I think there is no way of testing the actual execution of a ruby_block. By default the block is not executed, if I force rspec to do it the note context is missing. So the best we can do is to check if there is a ruby_block at all and is it called in the recipe. The result can not be verified afaik.

@malcyon
Copy link
Contributor Author

malcyon commented Aug 18, 2021

Okay, that makes sense. I'll update the rspec test.

@malcyon
Copy link
Contributor Author

malcyon commented Aug 18, 2021

Oh, I see you already did that. Thanks!

@malcyon
Copy link
Contributor Author

malcyon commented Aug 18, 2021

The Travis CI job seems to be broken. It's attached to travis-ci.org and would need to be migrated to travis-ci.com. But I don't have access to do that.

I am looking at moving it it to either Github Actions or, failing that, our internal Jenkins. Perhaps it can run chef exec rspec and kitchen verify.

@mariussturm
Copy link
Contributor

Especially since the integration tests are pretty heavy, maybe our Jenkins is a good choice.

@malcyon
Copy link
Contributor Author

malcyon commented Aug 19, 2021

Okay, the Jenkins CI tests centos, debian, and ubuntu, and runs the rspec tests. Because it does it all in parallel, it only takes 5.5 minutes to run.

image

@mariussturm
Copy link
Contributor

This looks really great now, good work @juju2112!!! If there is nothing more open I would merge this now, any objections?

@malcyon
Copy link
Contributor Author

malcyon commented Aug 20, 2021

This looks really great now, good work @juju2112!!! If there is nothing more open I would merge this now, any objections?

Yep, we're good merge now. Thanks for your help!

@mariussturm mariussturm merged commit aec4679 into master Aug 20, 2021
@mariussturm mariussturm deleted the various-fixes branch August 20, 2021 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NAT Documentation is out of date Unable to install latest sidecar
2 participants