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

Major Update to Work With 2.3.1 (latest) #172

Merged
merged 67 commits into from Aug 14, 2017

Conversation

nmaludy
Copy link
Member

@nmaludy nmaludy commented Aug 7, 2017

Intro

Lots of work has been done here and i'll try to summarize as best i can.

Overview

When starting work on this module it failed to install numerous components in the profiles directory. Most of this was due to failures in external packages (rabbitmq, mongodb, postgres, etc), dependency/ordering issues in the code (you'll see lots of updates to with -> ), and/or changes to the st2 code since the module was last updated.

manifests/auth/standalone.pp

  • D not have access to the ::st2 variables.
  • Had a dependency issue where (on some platforms) allowed the htpasswd file to be created after the st2 services were starting
  • Created an unnecessary "test user"

manifests/auth_user.pp

  • Dependency issues here where the htpasswd file was sometimes trying to be created before the /etc/st2 directory was created, and other times it was trying to be created after the st2 services had started.

manifests/init.pp

  • Needed extra variables for SSL setup in st2web.
  • Needed extra variables for proper database setup (mongodb and postgres)
  • Needed path to the st2auth logging config file
  • Needed variables about the datastore encryption keys

manifests/kv.pp

  • Some puppet lint problems (notice the whitespace fix and reordering of class params)
  • Dependency issues where the tag being used for the Service resource was incorrect
  • Dependency issues where sometimes st2 hadn't been reloaded so the k/v loads would fail

manifests/notices.pp

  • Puppet lint fixes for using double quotes without variable interpolation in the string.

manifests/pack.pp

  • Unit tests revealed that many of the dependencies of this resource were not declared (group and directories)
  • Pointing at old location for config directory
  • Needed lots of dependency work to ensure resources were created in the proper order

manifests/params.pp

  • Broke down the old st2_server_packages variable into various components to align more with what ansible-st2 and the "one liner" shell scripts do in their functions.
  • Removed some unused code in the "init provider" section
  • Broke down the old st2_services into its components similar to st2_server_packages. FYI: The mistral services are handled by the mistral install instead of being grouped together into st2 server.
  • Added lots of new parameters for services that were not configured in the past like (nginx, st2web, mongodb, rabbitmq)

manifests/profile/client.pp

  • Removed stale comment

manifests/profile/fullinstall.pp

  • Mainly dependency cleanup here.
  • Ensure that packages are installed in the correct order and that there are meaningful anchors in place in case others need to execute tasks at certain points during the install.

manifests/profile/mistral.pp

  • This was completely re-written
  • Previously it was performing a lot of tasks manually that i believe st2mistral package now handles for us

manifests/profile/mongodb.pp

  • Completely re-written
  • It now handles auth (did not previously)
  • It also deals with several deficiencies in the puppetlabs-mongodb module. This module has lots of annoying bugs. I'm not at the point where i want to code up a new module myself yet, but we do have to work around several quirks for this to even work (sorry!).

manifests/profile/nginx.pp

  • New profile that installs and configures nginx (does not setup st2web config, that is left to the st2web profile)
  • Utilizes the nginx puppet module to do all of the heavy lifting here

manifests/profile/nodejs.pp

  • Completely re-written
  • Utilizes the nodejs puppet module to do all of the heavy lifting instead of doing it ourselves
  • Works around a small quirk of the module on RedHat distributions

manifests/profile/postgresql.pp

  • Expanded this to properly configure postgres for listening according to the standard installs (shell scripts and ansible-st2)
  • Also ensured that 9.4 is installed on RHEL6

manifests/profile/rabbitmq.pp

  • Greatly simplified by allowing the rabbitmq module to do all of the heavy lifting for us

manifests/profile/repos.pp

  • Fixed a bug where we were pointing to an all lowercase URL which caused st2 package installs to fail

manifests/profile/selinux.pp

  • Added a class that configures SELinux on RHEL hosts

manifests/profile/server.pp

  • Small changes here related to adding database auth capability
  • Added stanley user creation
  • Added datastore crypto creation
  • Added additional dependency management

manifests/profile/web.pp

  • Completely re-written
  • I don't believe that st2web was complete when this module was last touched, so this class got a complete overhaul

manifests/rbac.pp

  • Fixed a few puppet lint errors
  • Fixed an error where the RBAC rules were executed every puppet run

manifests/server/datastore_keys.pp

  • New manifest that manages the datastore crypto keys

manifests/stanley.pp

  • Removed unnecessary warning about ssh keys

manifests/user.pp

  • Fixed a couple small bugs related to a legacy "robots" group.
  • This got a pretty big overhaul with regards to SSH key creation. Now, if SSH keys are not present new ones will be created (just like the shell scripts and ansible-st2)

metadata.json

  • Reformatted the whole file to standard JSON formatting scheme
  • Updated module dependencies (some were missing)
  • Added supported OS block
  • Added supported puppet versions block

spec/*

  • Lots of small fixes here related to running the tests on various versions of ruby.
  • Finally found a happy medium where all tests now pass

@nmaludy
Copy link
Member Author

nmaludy commented Aug 7, 2017

This fixes #159

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! A lot of sync-up & overhaul work here.

I left a couple of comments to address.
Additionally, I think bumping the version 0.14.1 -> 1.0.0 is a bit of optimistic, - this module probably needs more iterations and testing from the users in the battlefield before we can tag it stable 1.0.0. So I'd go more conservative path like 0.14.1 -> 0.2.0, with faster iterations & version increase in future.

@@ -35,7 +36,7 @@
'repos' => 'main',
'include_src' => false,
'key' => '8756C4F765C9AC3CB6B85D62379CE192D401AB61',
'key_source' => 'https://bintray.com/user/downloadSubjectPublicKey?username=bintray',
'key_source' => 'https://bintray.com/user/downloadSubjectPublicKey?username=bintray'
Copy link
Member

Choose a reason for hiding this comment

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

Bintray was an old try and not used anymore, - we use PackageCloud in favor of Bintray.
So all Bintray occurencies could be removed now.

@@ -12,7 +13,7 @@
'repos' => 'main',
'include_src' => false,
'key' => '1E26DCC8B9D4E6FCB65CC22E40A96AE06B8C7982',
'key_source' => 'https://downloads.stackstorm.net/deb/pubkey.gpg',
'key_source' => 'https://downloads.stackstorm.net/deb/pubkey.gpg'
Copy link
Member

Choose a reason for hiding this comment

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

Everything pointing to download.stackstorm.net could be removed as well.
It's old st2 =< 0.13 repo, not available anymore.

CHANGELOG.md Outdated
@@ -1,5 +1,137 @@
# Changelog

## 1.0.0 (Aug 6, 2017)
Copy link
Member

Choose a reason for hiding this comment

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

I think it's still a way big road and more work till we can version it as 1.0.0.

Could we tag it more conservatively, like 0.2.0?

CHANGELOG.md Outdated

#### manifests/auth_user.pp

- Dependency issues here where the `htpasswd` file was sometimes trying to be created before the `/etc/st2` directory was created, and other times it was trying to be created after the st2 services had started.
Copy link
Member

Choose a reason for hiding this comment

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

Can we include for every new change listed in CHANGELOG if it's a bug, feature, enhancement or a breaking change?

ensure => file,
mode => '0440',
mode => '0755',
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need 0755 here? As I understand these are permissions for /opt/stackstorm/configs/*.yml files.

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 was just going by what's present in my "one liner" install:

# ls -al /opt/stackstorm/configs/
total 16
drwxr-xr-x.  2 st2  root 109 Jul 27 11:27 .
drwxr-xr-x. 11 root root 204 Aug  2 21:46 ..
-rwxr-xr-x.  1 st2  root 148 Jun 30 10:42 activedirectory.yaml
-rwxr-xr-x.  1 st2  root  72 Jun 29 23:16 digitalocean.yaml
-rwxr-xr-x.  1 st2  root  17 Jun 13 09:40 testconfigdefault.yaml
-rwxr-xr-x.  1 st2  root 162 Jun  7 13:01 vsphere.yaml

Copy link
Member

@arm4b arm4b Aug 8, 2017

Choose a reason for hiding this comment

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

+x flag is not needed on those yaml files, so 0640 should be fine.

@@ -190,10 +197,7 @@
}
} elsif $::operatingsystem == 'Ubuntu' {
$init_type = $::operatingsystemmajrelease ? {
Copy link
Member

Choose a reason for hiding this comment

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

Do we use init_type somewhere when the init files are included in packaging?

Copy link
Member Author

Choose a reason for hiding this comment

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

No we don't, i'll removed this.

There are a few other variables in this params class that are unused. I'll try to clean them up the best i can.

@arm4b
Copy link
Member

arm4b commented Aug 8, 2017

@arm4b
Copy link
Member

arm4b commented Aug 8, 2017

For some reason it changing the permissions to 755 for all files in /opt/stackstorm/packs

Notice: /Stage[main]/St2::Profile::Fullinstall/St2::Pack[st2]/File[/opt/stackstorm/packs/core/actions/announcement.yaml]/mode: mode changed '0664' to '0775'
Notice: /Stage[main]/St2::Profile::Fullinstall/St2::Pack[st2]/File[/opt/stackstorm/packs/core/actions/remote.yaml]/mode: mode changed '0664' to '0775'
Notice: /Stage[main]/St2::Profile::Fullinstall/St2::Pack[st2]/File[/opt/stackstorm/packs/packs/actions/pack_mgmt/search.py]/mode: mode changed '0664' to '0775'
Notice: /Stage[main]/St2::Profile::Fullinstall/St2::Pack[st2]/File[/opt/stackstorm/packs/packs/actions/pack_mgmt/download.py]/mode: mode changed '0664' to '0775'

'priority' => '10',
'content' => "%${_robots_group_name} ALL=(ALL) NOPASSWD: SETENV: ALL",
# note: passes in $name variable into template
'content' => template('st2/etc/sudoers.d/user.erb'),
})
Copy link
Member

Choose a reason for hiding this comment

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

Under ubuntu xenial Vagrant, found that something breaks paswordless sudo for the default user after running the provisioner.
Will check later what's going on there.

Copy link
Member

Choose a reason for hiding this comment

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

So before running the Puppet-st2, I have the following:

ubuntu@ubuntu16:~$ sudo ls -la /etc/sudoers.d/
total 16
drwxr-x---  2 root root 4096 Aug  8 15:35 .
drwxr-xr-x 91 root root 4096 Aug  8 15:35 ..
-r--r-----  1 root root  123 Aug  8 15:35 90-cloud-init-users
-r--r-----  1 root root  958 Mar 30  2016 README

Running the Puppet provisioner removes that file, - that's why it breaks exsiting sudoers.

Copy link
Member

@arm4b arm4b Aug 9, 2017

Choose a reason for hiding this comment

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

Guessing if Puppet's sudo module has some default flag with removes previous sudo records.

@nmaludy
Copy link
Member Author

nmaludy commented Aug 8, 2017

@armab i still agree with your permissions decision, however i found the code in the base repo that sets these permissions to 775 https://github.com/StackStorm/st2/blob/master/contrib/packs/actions/pack_mgmt/download.py#L219-L238

For now i'll just ensure that the packs/ directory and all files under it are owned by group st2packs

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.9%) to 37.084% when pulling 944e63a on EncoreTechnologies:master into ca41317 on StackStorm:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage decreased (-1.9%) to 37.084% when pulling 944e63a on EncoreTechnologies:master into ca41317 on StackStorm:master.

@arm4b
Copy link
Member

arm4b commented Aug 9, 2017

@nmaludy Thanks for the pointer! It turned into internal discussion with StackStorm team and StackStorm/st2#3660 we think we should enhance that in st2 core.

@nmaludy
Copy link
Member Author

nmaludy commented Aug 11, 2017

@armab i fixed the problem with extra "unmanaged" /etc/sudoers.d/* files being purged during runtime.

This "purge prevention" will be the default behavior in this st2 module.

My change also tries to be smart and externally compatible for users that wish to configure the sudo package however they like (maybe they want the purge feature). In this case the code detects if the ::sudo class has been instantiated previously and allows for the previous instance to dictate the main behavior.

@coveralls
Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-1.9%) to 37.084% when pulling 271cfa7 on EncoreTechnologies:master into ca41317 on StackStorm:master.

@coveralls
Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-1.9%) to 37.084% when pulling d5788f9 on EncoreTechnologies:master into ca41317 on StackStorm:master.

@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage decreased (-1.9%) to 37.084% when pulling 9b8aa36 on EncoreTechnologies:master into ca41317 on StackStorm:master.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Great work 👍

As discussed before, we tag this version as 1.0.0-beta, which will allow us to release bugfix version like 1.0.0-rc (or 1.0.0-rc1, 1.0.0-rc2 if needed) before stable 1.0.0.

@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage decreased (-1.9%) to 37.084% when pulling 9b8aa36 on EncoreTechnologies:master into ca41317 on StackStorm:master.

@nmaludy nmaludy merged commit 689735a into StackStorm:master Aug 14, 2017
bishopbm1 pushed a commit to EncoreTechnologies/puppet-st2 that referenced this pull request May 20, 2022
Major Update to Work With 2.3.1 (latest)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants