-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Add support to configure parameters form insights-client.conf #177
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ptoscano Is this value built-in to the insights client? where does it come from?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does there need to be a check first for rhc_insights.file_redaction is defined
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICS
rhc_insights.file_redaction
is defined indefaults/main.yml
.
That unfortunately doesn't mean anything.
As
defaults/main.yml
as well astasks/insights-client.yml
are part of one role and shipped together I do not see a scenario whererhc_insights.file_redaction
would 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.yml
check 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 defined
checks where the var is defined indefaults/main.yml
as the check is not striclty necessary. But I might miss a good reason here.
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.
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_insights
at all. There is no mechanism in Ansible like "merge the default dict valued parameterrhc_insights
defined indefaults/main.yml
with therhc_insights
parameter given by the user". If the user definesrhc_insights
, then the value indefaults/main.yml
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.conf
configurable/manageable by this role.Reason: This enables users to configure the
insights-client
parameters at scale.Result: Currently only the parameters
autoupdate
,ansible_host
,display_name
,remediation
,tags
, andrhc_proxy
are supported by this role. This PR adds support for the following parameters:loglevel
auto_config
authmethod
username
password
base_url
obfuscate
obfuscate_hostname
redaction_file
content_redaction_file
tags_file
Issue Tracker Tickets (Jira or BZ if any):
https://issues.redhat.com/browse/RHEL-35245
Additional information: