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

Add configuration options to the terminus to filter facts out #2634

Open
wants to merge 1 commit into
base: 6.x
Choose a base branch
from

Conversation

nbarrientos
Copy link

This changeset adds two configuration options to be consumed by the facts PuppetDB indirector:

  • fact_names_blacklist
  • fact_names_blacklist_regex

They can be used to configure a list of fact names that will never be sent to PuppetDB, based on exact fact names or regular expressions. I'm aware that PuppetDB supports black listing facts but over here we do prefer to filter them in the origin to avoid sending bits over the wire that will be discarded by the other end.

I'm happy to further develop the change request adding tests and documentation if you're interested.

This changeset adds two configuration options used by the facts PuppetDB
indirector:

  * fact_names_blacklist
  * fact_names_blacklist_regex

They can be used to configure a list of fact names that will never be
sent to PuppetDB, based on exact fact names or regular expressions.
@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

5 similar comments
@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@puppetcla
Copy link

Waiting for CLA signature by @nbarrientos

@nbarrientos - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html

@nbarrientos
Copy link
Author

The CLA is already signed by my employer.

@Zak-Kent
Copy link
Contributor

Zak-Kent commented Oct 5, 2018

@nbarrientos thanks for the PR! We may be interested in this change, but I'll need a bit more time to get back to you. The code you're changing here ends up getting packaged with a different project (puppetserver) so it would require changes from that team to validate any user supplied regex patterns in the blacklist option when they're starting up. Also, need to talk with the people I work with and see what their opinions are with regards to overall performance, etc. I'm on leave next week, but will try to give a more detailed response when I return. Thank you again for your interest!

@Zak-Kent
Copy link
Contributor

Zak-Kent commented Nov 2, 2018

@nbarrientos sorry it's taken a little while to get back to you. I asked around and someone suggested using the facter blocklist option as a way to block facts before they're collected. This could be used to filter out troublesome facts before they even make it to the indirector. Would this satisfy your use case, or is having regex support for blacklisting facts in the indirector important to you?

One reason we need to have the existing blacklist option in PDB is that the ingest api is public and in theory people may be submitting data to PDB outside of the indirector. The only way to make sure a blacklisted fact doesn't end up in PDB is to have the filtering done on the ingestion side.

@nbarrientos
Copy link
Author

Hi,

Thanks for getting back to me. In our case that approach does not suffice because we actually need those facts in the masters to make configuration decisions based on their values. What we don't want is to store them in PDB :)

@nbarrientos
Copy link
Author

Friendly ping :)

@Zak-Kent
Copy link
Contributor

Zak-Kent commented Dec 6, 2018

Hi @nbarrientos thanks for the ping and sorry for the delay getting back to you again. I spoke with the puppetserver team where the terminus code is run. They're ok with the proposed changes of adding a blacklist option in the terminus before sending facts along to pdb. If you would like to work on this and add testing and documentation like you suggested we'll gladly take a look. With the approaching holidays and an upcoming scheduled release we may be a little slow to review, but we'll get to it as soon as possible. If you're busy this may be something we'd want to add ourselves, but it would have to wait on a few other things we have in progress.

One outstanding thing we'd need to figure out is the best way to validate any user supplied regex and how that validation would fit into puppetserver. I can help look into this once I clear a few other things off my todo list. We may also need to think about what we'd want to call the terminus blacklist vs. the one that's already in puppetdb to avoid confusion.

Thanks again for the pr!

@nbarrientos
Copy link
Author

Hi @Zak-Kent!

Thanks for coming back to us.

Ok, perfect. I will create a task on our side to improve the patch to add some extra tests and all the necessary docs. We expect to work on it early next year. As I mentioned, this is code that we have already running since a few months in production in our deployment.

We can work out later together the naming of the configuration parameters etc.

@Zak-Kent
Copy link
Contributor

Zak-Kent commented Dec 7, 2018

Thanks @nbarrientos! that sounds great

Base automatically changed from master to 6.x February 2, 2021 22:08
@austb austb requested a review from a team as a code owner February 2, 2021 22:08
@CLAassistant
Copy link

CLAassistant commented Feb 24, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

5 participants