Skip to content

17594 - PuppetDB - Add ability to set standard host listen address and open firewall to standard port#21

Closed
dblessing wants to merge 1 commit intopuppetlabs:masterfrom
dblessing:57445ef70fa52af3f06804320caba04f2a73f959
Closed

17594 - PuppetDB - Add ability to set standard host listen address and open firewall to standard port#21
dblessing wants to merge 1 commit intopuppetlabs:masterfrom
dblessing:57445ef70fa52af3f06804320caba04f2a73f959

Conversation

@dblessing
Copy link
Copy Markdown

I tested various combinations of the old manage_redhat_firewall param and the new open port params. The deprecation notices and fail messages work great.

I also adjusted the distributed example file to show how to open ports for postgres and puppetdb when necessary.

See notes on pull request code for more information.

…d open firewall to standard port

Prior to this commit the module did not provide a way to set a bind address for the HTTP port.  This
commit allows users to not only bind to an address and port other than localhost and 8080, but it also
opens the firewall if explicitly requested.
Comment thread manifests/init.pp
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added this parameter so users can run postgres on a separate server from puppetdb. Without this, it would not be possible because postgres listens on localhost only. They would also need to open the firewall port....

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So, the idea of the top-level class (init.pp) is to be a super-simple high-level abstraction around installing PuppetDB all on one machine. It's always going to install postgres on the same machine as PuppetDB, so I don't think we need this parameter or thedatabase_host parameter.

The idea is that if folks want something more complicated, they'd use the lower level ::server and ::database classes on different machines... does that make sense? I'm open to feedback about changing that but I think that seems reasonable. I also think that most of the rest of the parameters are probably OK to add in.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sounds good to me. I actually waffled about this when I did it. I'll take some of those out.

@dblessing
Copy link
Copy Markdown
Author

@cprice-puppet - I feel like I stayed with what we discussed but there ended up being a ton of changes. If this oversteps what you were envisioning I am completely open to changing it. Thanks for reviewing.

@cprice404
Copy link
Copy Markdown

@bke-drewb thanks! I was expecting it to be a non-trivial number of changes. Reviewing now!

Comment thread manifests/init.pp
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that we need for this to default to true, because otherwise the puppet master can't connect.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, yes. It will always been to be open unless puppetdb, postgres and puppet master run on the same box. So, will people more often run all 3 together, or split postgres/puppetdb from the puppet master?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good question. I think I've been presuming that the most common scenario would be that PuppetDB and postgres would be on the same machine, but that would be a separate machine from the puppet master. Not entirely sure if that assumption is valid but that was how I'd chosen the defaults before.

@cprice404
Copy link
Copy Markdown

Sorry for the herky-jerky responses to the pull req, I kept having to leave for meetings before I finished.

Overall this looks awesome, thanks so much for the submission. I think with the one fix re port for backward compatibility, removing some of the postgres params from init.pp, and the changes to the default values for the open_ssl and open_postgres params, I'll be ready to merge this.

Thanks!

@cprice404
Copy link
Copy Markdown

Hey @bke-drewb : thanks again for this submission. Are you still actively working on this? If not, I will try to find some time in the next week or two to follow up on the last few changes so that I can merge it.

@dblessing
Copy link
Copy Markdown
Author

Hi @cprice-puppet. Yeah I am. I got stuck on a few other things. I will have time in the next 2 days or so no problem. Thanks.

@cprice404
Copy link
Copy Markdown

awesome -- thanks!

@dblessing
Copy link
Copy Markdown
Author

I submitted this pull request from a commit instead of from my issue branch. I am closing this pull request and all cumulative changes are in pull request #22

@dblessing dblessing closed this Nov 29, 2012
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.

2 participants