feat: Use verbosity-based logging in network role#868
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a new network_secure_logging variable (default true) to control no_log behavior for sensitive tasks, and switches facts-related tasks to verbosity-based logging while documenting the new variable. Flow diagram for no_log behavior with network_secure_logging and verbosityflowchart TD
A[Start_task] --> B[Is_task_sensitive_credentials_or_secrets]
B -->|Yes| C[Set_no_log_to_network_secure_logging]
B -->|No| D[Is_task_facts_module_service_facts_or_package_facts]
D -->|Yes| E[Evaluate_ansible_verbosity_less_than_2]
E -->|True| F[Set_no_log_true_hide_facts_output]
E -->|False| G[Set_no_log_false_show_facts_output]
D -->|No| H[Use_existing_no_log_behavior_or_default]
C --> I[Execute_task_with_configured_no_log]
F --> I
G --> I
H --> I
I --> J[End_task]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider wrapping the templated
no_logexpressions with| bool(e.g.no_log: "{{ network_secure_logging | bool }}") to ensure they are always interpreted as booleans even if overridden via inventory or extra vars. - Since the verbosity-based logging rule is used in multiple places, you might want to introduce a dedicated variable (e.g.
network_fact_logging_quiet: ansible_verbosity < 2) indefaults/main.ymland reference that forservice_factsandpackage_factsto make the intent clearer and easier to adjust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider wrapping the templated `no_log` expressions with `| bool` (e.g. `no_log: "{{ network_secure_logging | bool }}"`) to ensure they are always interpreted as booleans even if overridden via inventory or extra vars.
- Since the verbosity-based logging rule is used in multiple places, you might want to introduce a dedicated variable (e.g. `network_fact_logging_quiet: ansible_verbosity < 2`) in `defaults/main.yml` and reference that for `service_facts` and `package_facts` to make the intent clearer and easier to adjust.
## Individual Comments
### Comment 1
<location path="tasks/set_facts.yml" line_range="8" />
<code_context>
when:
- network_provider == "nm" or network_state != {}
- no_log: true
+ no_log: "{{ network_secure_logging }}"
# If any 802.1x connections are used, the wpa_supplicant
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider explicitly casting `network_secure_logging` to a boolean for robustness.
If `network_secure_logging` is set as a string in vars (e.g. `'false'`), Ansible may treat it as truthy, leading to unexpected logging or suppression. Using `no_log: "{{ network_secure_logging | bool }}"` (optionally with `| default(true)`) would make this robust to different input types and avoid surprises.
```suggestion
no_log: "{{ network_secure_logging | default(true) | bool }}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
==========================================
- Coverage 43.11% 42.52% -0.59%
==========================================
Files 12 13 +1
Lines 3124 3172 +48
==========================================
+ Hits 1347 1349 +2
- Misses 1777 1823 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| when: | ||
| - network_provider == "nm" or network_state != {} | ||
| no_log: true | ||
| no_log: "{{ network_secure_logging }}" |
There was a problem hiding this comment.
| no_log: "{{ network_secure_logging }}" | |
| no_log: "{{ ansible_verbosity < 2 }}" |
Adding no_log: true was just for verbosity, not security - same with other places
so I don't think we need network_secure_logging for now
074ba4e to
58797a6
Compare
network_secure_logging defaulting to true58797a6 to
9d5bcf6
Compare
|
@spetrosi you'll need to sign your commit for the |
- Replace no_log: true with no_log: "{{ ansible_verbosity < 3 }}"
in all tasks (setup, service_facts, package_facts, and service tasks)
- This allows output to be shown when running with -vvv or higher verbosity
- The no_log was used for verbosity control, not security purposes
In the network role, no_log: true was used to reduce verbose output,
not to protect sensitive information. Using ansible_verbosity allows
users to see the output when debugging with increased verbosity flags.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Sergei Petrosian <spetrosi@redhat.com>
9d5bcf6 to
e600e2f
Compare
This PR replaces hardcoded
no_log: truewith verbosity-based loggingno_log: "{{ ansible_verbosity < 3 }}"throughout the network role.Changes
no_log: truewithno_log: "{{ ansible_verbosity < 3 }}"in:tasks/set_facts.yml: setup, service_facts, package_facts taskstasks/main.yml: NetworkManager and initscripts service tasksRationale
In the network role,
no_log: truewas used for verbosity control to reduce verbose output from service and package fact gathering tasks, not to protect sensitive information.Using
ansible_verbosity < 3allows users to see the output when debugging with increased verbosity flags (-vvvor higher), while keeping output concise by default.This differs from roles where
no_logis used for security purposes (e.g., protecting credentials or secrets), where a*_secure_loggingparameter would be appropriate.