feat: Add support to configure parameters form insights-client.conf #177
Conversation
- Added vars and tasks to configure the parameters in /etc/insights-client/insights-client.conf - Set the values of the default vars to the default values from /etc/insights-client/insights-client.conf - 'http_timeout' and 'cmd_timeout' are still missing Signed-off-by: Joerg Kastning <[email protected]>
- Fixed: Typo in defaults/main.yml - Added: Documentation for the variables added to defaults/main.yml Signed-off-by: Joerg Kastning <[email protected]>
Signed-off-by: Joerg Kastning <[email protected]>
The variables related to the insights-client basic authentication mechanism are being moved from the dict 'rhc_insights' to 'rhc_insights_auth'. - Using 'no_log: true' for authentication related variables - No need to use 'no_log: true' for all other variables under 'rhc_insights'
I have removed the previously added functionality to configure the path for the 'redaction_file' and the 'content_redaction_file' as I misunderstood their concept. - These files do not exist in a default installation of inisghts client - They can be created and used to omit commands and patterns from collection - See https://access.redhat.com/articles/4511681 for more details on this - There should be options to manage the contents of these files but not their path - This might be addressed in a seperate PR to keep the changes small Signed-off-by: Joerg Kastning <[email protected]>
- Option to configure tags is already present in this role - I misunderstood this config parameter previously - Therefore I remove it as obsolete
- The insights-client.conf allows the specification of deny lists - See YAML-style denylist configuration for Red Hat Insights Client - URL: https://access.redhat.com/articles/4511681 for details The dictionaries rhc_insights.file_redaction and rhc_insights.file_content_redaction can be used to specify lists for the commands, files, components, keywords, and patterns to omit from output when gathering data for Red Hat Insights. Signed-off-by: Joerg Kastning <[email protected]>
README.md
Outdated
| ``` | ||
|
|
||
| Configures the Base URL for the Insights API. | ||
| Defaults to "cert-api.access.redhat.com:443/r/insights". |
There was a problem hiding this comment.
@ptoscano Is this value built-in to the insights client? where does it come from?
There was a problem hiding this comment.
It's the default value from /etc/insights-client/insights-client.conf:
# Base URL for the Insights API
#base_url=cert-api.access.redhat.com:443/r/insights
Signed-off-by: Joerg Kastning <[email protected]>
- Set 'baseurl: null' in defaults/main.yml - This ensures the default value of the insights-client is used whatever it might be - Maintainers does not have to keep a default value from defaults/main.yml in sync with the default from the insights-client this way - If there is a need to specify the baseurl it can be done with this role still Signed-off-by: Joerg Kastning <[email protected]>
Signed-off-by: Joerg Kastning <[email protected]>
|
|
||
| - name: Configure username for authmethod BASIC | ||
| when: | ||
| - rhc_insights_auth.authmethod = "BASIC" |
There was a problem hiding this comment.
| - rhc_insights_auth.authmethod = "BASIC" | |
| - rhc_insights_auth.authmethod is defined | |
| - rhc_insights_auth.authmethod = "BASIC" |
|
|
||
| - name: Configure password for authmethod BASIC | ||
| when: | ||
| - rhc_insights_auth.authmethod == "BASIC" |
There was a problem hiding this comment.
| - rhc_insights_auth.authmethod == "BASIC" | |
| - rhc_insights_auth.authmethod is defined | |
| - rhc_insights_auth.authmethod == "BASIC" |
| @@ -133,6 +223,46 @@ | |||
| or "Registered" in __rhc_insights_status.stdout | |||
| changed_when: true | |||
|
|
|||
| - name: Configure file redaction | |||
| when: (rhc_insights.file_redaction.commands is defined) or | |||
There was a problem hiding this comment.
Does there need to be a check first for rhc_insights.file_redaction is defined?
There was a problem hiding this comment.
AFAICS rhc_insights.file_redaction is defined in defaults/main.yml. As defaults/main.yml as well as tasks/insights-client.yml are part of one role and shipped together I do not see a scenario where rhc_insights.file_redaction would not be defined. So with my current knowledge and understanding I would say there is no need to check first for rhc_insights.file_redaction is defined.
I see that most if not all tasks in tasks/insights-client.yml check whether variables are defined although all of these variables are defined in defaults/main.yml. TBH I do not understand why these checks are necessary. Are they in place for "just in case" scenarios or is there some reason I do not see?
Following my own argument I would suggest removing all the var is defined checks where the var is defined in defaults/main.yml as the check is not striclty necessary. But I might miss a good reason here.
There was a problem hiding this comment.
AFAICS
rhc_insights.file_redactionis defined indefaults/main.yml.
That unfortunately doesn't mean anything.
As
defaults/main.ymlas well astasks/insights-client.ymlare part of one role and shipped together I do not see a scenario whererhc_insights.file_redactionwould not be defined.
What if I run the role like this?
inventory:....
rhc_insights:
foo: bar
Defining a host variable takes precedence over settings in defaults/main.yml. The same applies to setting the variable at the play level or as one of the vars in the include_role call. Setting rhc_insights in this way completely replaces the existing contents defined in defaults/main.yml. https://docs.ansible.com/ansible/latest/reference_appendices/general_precedence.html#variables
So - while "top level" variables like rhc_insights will always be defined, their "sub-parameters" may or may not be defined - that is - rhc_insights is defined will always return true, but rhc_insights.file_redaction is defined can return true or false.
If you don't believe me, try running the role with different values and see what happens.
So with my current knowledge and understanding I would say there is no need to check first for
rhc_insights.file_redaction is defined.I see that most if not all tasks in
tasks/insights-client.ymlcheck whether variables are defined although all of these variables are defined indefaults/main.yml. TBH I do not understand why these checks are necessary. Are they in place for "just in case" scenarios or is there some reason I do not see?
Yes, and if some checks are missing, they should be added in another PR.
TBH, I'm really not sure why "sub-parameter" keys and values are defined in defaults/main.yml, except possibly as a convenience when the user does not provide a value for rhc_insights at all. There is no mechanism in Ansible like "merge the default dict valued parameter rhc_insights defined in defaults/main.yml with the rhc_insights parameter given by the user". If the user defines rhc_insights, then the value in defaults/main.yml is completely replaced.
Following my own argument I would suggest removing all the
var is definedchecks where the var is defined indefaults/main.ymlas the check is not striclty necessary. But I might miss a good reason here.
There was a problem hiding this comment.
What if I run the role like this?
inventory:.... rhc_insights: foo: bar
…
If you don't believe me, try running the role with different values and see what happens.
Thank you for reminding me about variable precedence and giving me a good example when things will break. I didn't find this as my own tests are still flawed and incomplete.
I cannot thank you enough for bearing with me and share your thoughts and insights on this as it is a great learning opportunity for myself.
TBH, I'm really not sure why "sub-parameter" keys and values are defined in
defaults/main.yml, except possibly as a convenience when the user does not provide a value forrhc_insightsat all. There is no mechanism in Ansible like "merge the default dict valued parameterrhc_insightsdefined indefaults/main.ymlwith therhc_insightsparameter given by the user". If the user definesrhc_insights, then the value indefaults/main.ymlis completely replaced.
Well, putting all variables, parameters and arguments accepted by the role into defaults/main.yml gives users a single place to look what input parameters are accepted. That's in alignment with this section of the automation good practices. And I understood that every condition needs to check whether a variable is defined or not as the whole dict defined defaults/main.yml is replaced when users specify this elsewhere according to variable precedence.
I'm going to check the conditionals in the tasks I've added so far. Please allow me some time to go through them.
| line: redaction_file=/etc/insights-client/file-redaction.yaml | ||
|
|
||
| - name: Configure file content redaction | ||
| when: (rhc_insights.file_content_redaction.keywords is defined) or |
There was a problem hiding this comment.
Does there need to be a check first for rhc_insights.file_content_redaction is defined?
| {% if rhc_insights.file_content_redaction.keywords %} | ||
| keywords: | ||
| {% for keyword in rhc_insights.file_content_redaction.keywords %} | ||
| - {{ keyword }} |
There was a problem hiding this comment.
Is it possible that this or any of the below values will need to be quoted? will it be possible that the values will contain yaml metacharacters?
There was a problem hiding this comment.
For patterns (with regex) that would be possible as the examples in YAML-style denylist configuration for Red Hat Insights Client show characters that need to be escaped.
For keywords, I tend to escape them just to be sure. Do you agree?
There was a problem hiding this comment.
This is tricky - afaik there is no "regex quote" or "regex escape" filter in Jinja or Ansible (like there is for shell metacharacters and the quote filter). If you use - "{{ pattern }}" I think this will cause problems if pattern contains " - same with '{{ pattern }}' and ' - however, this might be safe:
- >-
{{ pattern }}
|
How can we test this? |
|
Sorry for the late answer on this. While I appreciate the efforts on this, I have to raise my concerns on this implementation. The general "style" of the What I'm missing here are the actual use cases & stories behind the addition of those options. Sometimes configuring an option may be the right solution, and sometimes a problem can be solved in a different way. Unfortunately, without knowing them, it is hard to determine whether adding the options is the correct possibility, and even testing whether the use cases were actually solved. Also, new options mean additional maintenance cost, which thus needs to be "justified". In addition to that, I find some options problematic to add:
Regarding the other two "blocks" of options, i.e. redaction and obfuscation: these big features definitely require use cases and discussions on what is expected, what is needed, and so forth, and what would be the proper API to support them. Because of what I said, my recommendation for this PR would be to hold it, and move the discussion as internal with us, and determine the various use cases, etc. |
Enhancement: Make most of the parameters in
/etc/insights-client/insights-client.confconfigurable/manageable by this role.Reason: This enables users to configure the
insights-clientparameters at scale.Result: Currently only the parameters
autoupdate,ansible_host,display_name,remediation,tags, andrhc_proxyare supported by this role. This PR adds support for the following parameters:loglevelauto_configauthmethodusernamepasswordbase_urlobfuscateobfuscate_hostnameredaction_filecontent_redaction_filetags_fileIssue Tracker Tickets (Jira or BZ if any):
https://issues.redhat.com/browse/RHEL-35245
Additional information: