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

feat: Add support to configure parameters form insights-client.conf #177

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Tronde
Copy link

@Tronde Tronde commented May 3, 2024

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, and rhc_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:

  • As I'm not a software developer and this is my first PR for this role, please bear with me if I have missed something or did something wrong
  • I followed the contribution guide to my best judgement
  • To add the new features I copied, pasted, and modified existing tasks so my contribution should align well with the existing code
  • Advice on how to improve are appreciated

Tronde added 3 commits May 2, 2024 20:23
- 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]>
@Tronde Tronde requested review from richm and ptoscano as code owners May 3, 2024 10:19
README.md Outdated Show resolved Hide resolved
Tronde and others added 4 commits May 10, 2024 11:24
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]>
@Tronde
Copy link
Author

Tronde commented May 21, 2024

With commit e2395bc I implemented the configuration to create file-redaction.yaml and file-content-redaction.yaml.

I guess that's it. I wait for the review by @ptoscano. :-)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
```

Configures the Base URL for the Insights API.
Defaults to "cert-api.access.redhat.com:443/r/insights".
Copy link
Contributor

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?

Copy link
Author

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

defaults/main.yml Outdated Show resolved Hide resolved
Tronde added 3 commits May 30, 2024 07:16
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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

That unfortunately doesn't mean anything.

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.

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 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?

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 in defaults/main.yml as the check is not striclty necessary. But I might miss a good reason here.

Copy link
Author

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 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.

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
Copy link
Contributor

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 }}
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Contributor

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 }}

@richm
Copy link
Contributor

richm commented May 31, 2024

How can we test this?

@ptoscano
Copy link
Collaborator

ptoscano commented Jun 4, 2024

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 rhc role is to work on use cases & functionalities related to the connection of a system to Red Hat / Satellite, and functionalities related to maintaining such. This means for example ensuring a system is connected & registered, it can get content, and there are certain Insights features available. Because of this, the role does not expose configuration bits of the underlying tools simply because they exist, and only in case they are needed, sometimes with different semantics than the underlying tools (e.g. rhc_insights.tags), or even that do not map at all (e.g. rhc_repositories, rhc_insights.remediation).

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:

  • rhc_insights_auth: the authentication was explicitly designed to be the same as used for registering a system, as a separate authentication for Insights is an extreme corner case; in addition to that, basic authentication for Insight is officially deprecated, and will be disabled at the end of this year (2024) [1]
  • rhc_insights.autoconfig & rhc_insights.baseurl: these two option are used to manually configure where insights-client connects to, without detecting that from the way the system is registered with subscription-manager; this is yet another corner case, mostly to support insights-client registered using basic authentication (and thus not able to automatically detect from subscription-manager)
  • rhc_insights.logevel: the default behaviour of insights-client is to even log debug messages to its log file (/var/log/insights-client/insights-client.log); it is not clear to me why this needs to be configurable via the rhc role

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.

[1] https://docs.redhat.com/en/documentation/red_hat_insights/1-latest/html/release_notes/april-2024#remote_host_configuration_rhc_and_the_insights_client

@richm richm marked this pull request as draft August 1, 2024 22:35
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.

3 participants