-
Notifications
You must be signed in to change notification settings - Fork 16
real report processor #69
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
base: master
Are you sure you want to change the base?
Conversation
# # Is customizing this client separately valuable? Do people actually /do/ it? | ||
# # puppet.conf already allows all this to be customized in a single place | ||
# # and specifying these means we create a new pool each time. | ||
# # | ||
# # Default retry limit is 1. | ||
# retry_limit = SETTINGS.fetch(:report_retry_limit, 1) | ||
# client = Puppet::HTTP::Client.new( | ||
# pool: Puppet::HTTP::Pool.new(SETTINGS[:report_timeout]), | ||
# retry_limit: retry_limit, | ||
# ssl_context: Puppet::SSL::SSLContext.new( | ||
# cacerts: SETTINGS[:ssl_ca], | ||
# client_cert: SETTINGS[:ssl_cert], | ||
# private_key: SETTINGS[:ssl_key], | ||
# )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bastelfreak thoughts on simplifying and removing all this?
it's annoying that the UI shows a delete and new file instead of a rename/edit. But it's recorded in history so it's only another click to get back to previous iterations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking of enhancing https://github.com/theforeman/foreman_puppet to provide an endpoint that processes the native Puppet report. Then I'd expect you can use the built in HTTP report processor, provided the certificates from Puppetserver are trusted on the Foreman side.
Similar, the ENC uploads facts but the native way for that is supposed to be a fact indirector. Can you use Puppet::Node::Facts::Rest to upload facts? Then the ENC could use Puppet::Node::Rest as well.
If we can make that work you don't need anything on an OpenVoxServer (other than some config keys) to integrate with Foreman, making this whole module obsolete.
Some thing to be aware of: you can only have 1 fact indirector so if also want to use OpenVoxDB you need to come up with something else. For example, like https://github.com/puppetlabs/puppetlabs-satellite_pe_tools/blob/main/lib/puppet/indirector/facts/satellite.rb that inherits from the DB class and uploads via HTTP before calling super()
.
I've been wanting to do that for a long time, but never found the time and focus to do it.
creates => "${puppet_basedir}/reports", | ||
} | ||
|
||
file { "${puppet_basedir}/reports/foreman.rb": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always liked that the report processor was available, regardless of the environment. Shipping a report processor in a module never made sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the standard way to do it. In all the docs, handled automatically for you. Doesn't require poking into directories managed by the gem/package. And it's very straightforward -- your server is going to be managed by an agent regardless so you just ensure that the module is in the environment your server will use. Put it in multiple environments if you want. Or put it in a basemodulepath
and sync it to all environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but that's the part I dislike: which environment is used? I think it's just confusing and will never like it. Determening which code (or which version) is executed by the server is rather important to me.
@bastelfreak I am very much in favor of this solution. It would mean that your customer from #39 would need to resolve their certificate jumble another way. Do you have thoughts on that? @ekohl imho we don't let the good get in the way of perfection. I would suggest that I simplify this report processor to make connections exactly like the built-in Then when we have time to do the new report endpoint, we version it and people can move at their leisure. This reduces the version coupling between the module and foreman itself. It's probable that we will have time to do both integrations (including a native classification indirector instead of shelling out to an ENC) sometime over the next year. |
Indon't like the standard place if I'm honest. Combining them into a single PR is also poor practice because I'd say that's a breaking change. Using the built in client is an enhancement |
None of the configuration in
foreman.yaml
is actually required (except for one pathological case that @bastelfreak mentioned.)The built-in http client already takes care of retry and timeout and SSL certs. In most cases, the Foreman server is the same as the OpenVox server and when it's not, there's a built-in setting called
reporturl
that thehttp
report processor uses.We can simplify this such that the only configuration required is to install the module and enable it with
reports=foreman
.