Add foremanctl tests for sos report - #22099
Conversation
shubhamsg199
added
the
No-CherryPick
shubhamsg199
added
the
Stream
Reviewer's GuideAdds end-to-end tests for the foremanctl sos plugin to validate data collection and credential scrubbing in sos reports on containerized Satellite, using the existing logging test module as the host for new tests and a new constant import. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
sosreport_extract, relying onls /var/tmp/sosreport-*.tar.xz | head -1makes the test dependent on filesystem ordering and can pick an older tarball; consider capturing the exact tarball path from thesos reportinvocation or using a more deterministic selection (e.g., by timestamp or by cleaning old tarballs before running). - The tests mix
FOREMANCTL_PARAMETERS_FILEwith a hard-coded/var/lib/foremanctl/parameters.yamlpath; it would be more robust to consistently use the constant (or a small helper) so that a path change only needs to be updated in one place. - In the password scrubbing test you define
SENSITIVE_KEYWORD = ('password',)but use a hard-codedgrep -i password; consider deriving the grep expression from the same keyword list so the detection of original sensitive values and the scrubbing assertions stay in sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `sosreport_extract`, relying on `ls /var/tmp/sosreport-*.tar.xz | head -1` makes the test dependent on filesystem ordering and can pick an older tarball; consider capturing the exact tarball path from the `sos report` invocation or using a more deterministic selection (e.g., by timestamp or by cleaning old tarballs before running).
- The tests mix `FOREMANCTL_PARAMETERS_FILE` with a hard-coded `/var/lib/foremanctl/parameters.yaml` path; it would be more robust to consistently use the constant (or a small helper) so that a path change only needs to be updated in one place.
- In the password scrubbing test you define `SENSITIVE_KEYWORD = ('password',)` but use a hard-coded `grep -i password`; consider deriving the grep expression from the same keyword list so the detection of original sensitive values and the scrubbing assertions stay in sync.
## Individual Comments
### Comment 1
<location path="tests/foreman/cli/test_logging.py" line_range="264-276" />
<code_context>
+ assert tarball, 'No sosreport tarball found'
+
+ module_target_sat.execute(f'mkdir -p {self.EXTRACT_DIR}')
+ module_target_sat.execute(f'tar xf {tarball} -C {self.EXTRACT_DIR}')
+
+ report_dir = module_target_sat.execute(
+ f'ls -d {self.EXTRACT_DIR}/sosreport-*'
+ ).stdout.strip()
+ yield report_dir
+ module_target_sat.execute(f'rm -rf /var/tmp/sosreport-* {self.EXTRACT_DIR}')
+
+ @pytest.mark.foremanctl
</code_context>
<issue_to_address>
**suggestion (testing):** Assert success of tar extraction and cleanup commands in the sosreport fixture
The fixture currently ignores the exit status of the `tar` extraction and cleanup commands, so failures (e.g., corrupt tar, permissions, disk full) would surface later as less clear test errors. Please capture the return values of these `execute` calls and assert `status == 0` so setup failures are detected and reported immediately.
```suggestion
tarball = module_target_sat.execute(
'ls /var/tmp/sosreport-*.tar.xz | head -1'
).stdout.strip()
assert tarball, 'No sosreport tarball found'
mkdir_result = module_target_sat.execute(f'mkdir -p {self.EXTRACT_DIR}')
assert mkdir_result.status == 0, (
f'Failed to create sosreport extract directory {self.EXTRACT_DIR}:\n'
f'{mkdir_result.stdout}\n{mkdir_result.stderr}'
)
tar_result = module_target_sat.execute(f'tar xf {tarball} -C {self.EXTRACT_DIR}')
assert tar_result.status == 0, (
f'Failed to extract sosreport tarball {tarball}:\n'
f'{tar_result.stdout}\n{tar_result.stderr}'
)
report_dir = module_target_sat.execute(
f'ls -d {self.EXTRACT_DIR}/sosreport-*'
).stdout.strip()
yield report_dir
cleanup_result = module_target_sat.execute(
f'rm -rf /var/tmp/sosreport-* {self.EXTRACT_DIR}'
)
assert cleanup_result.status == 0, (
f'Failed to cleanup sosreport artifacts:\n'
f'{cleanup_result.stdout}\n{cleanup_result.stderr}'
)
```
</issue_to_address>
### Comment 2
<location path="tests/foreman/cli/test_logging.py" line_range="338-347" />
<code_context>
+ SENSITIVE_KEYWORD = ('password', )
</code_context>
<issue_to_address>
**suggestion:** Sensitive keyword coverage is limited to 'password' while the description mentions secrets/tokens
The docstring states we should scrub `password/secret/token`, but this constant (and the corresponding test) only covers `password`. If other keys like `secret`, `token`, or `client_secret` are meant to be scrubbed, please either extend `SENSITIVE_KEYWORD` to include them (or derive it from the implementation/requirements) and assert that each is masked as `********` in `parameters.yaml`. Otherwise, regressions for those fields may go unnoticed.
Suggested implementation:
```python
SENSITIVE_KEYWORD = ('password', 'secret', 'token', 'client_secret')
```
```python
original = module_target_sat.execute(
f"grep -Ei '{'|'.join(SENSITIVE_KEYWORD)}' {FOREMANCTL_PARAMETERS_FILE}"
)
```
To fully implement your comment, the rest of this test should:
1. Iterate over `SENSITIVE_KEYWORD` and assert that each matching entry in the extracted `parameters.yaml` is masked as `SCRUB_MARKER` (`********`), not just `password`.
2. Ensure any existing hard-coded references to "password" in subsequent assertions or scrubbing checks are updated to use `SENSITIVE_KEYWORD`, so regressions for `secret`, `token`, and `client_secret` cannot slip by untested.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| tarball = module_target_sat.execute( | ||
| 'ls /var/tmp/sosreport-*.tar.xz | head -1' | ||
| ).stdout.strip() | ||
| assert tarball, 'No sosreport tarball found' | ||
|
|
||
| module_target_sat.execute(f'mkdir -p {self.EXTRACT_DIR}') | ||
| module_target_sat.execute(f'tar xf {tarball} -C {self.EXTRACT_DIR}') | ||
|
|
||
| report_dir = module_target_sat.execute( | ||
| f'ls -d {self.EXTRACT_DIR}/sosreport-*' | ||
| ).stdout.strip() | ||
| yield report_dir | ||
| module_target_sat.execute(f'rm -rf /var/tmp/sosreport-* {self.EXTRACT_DIR}') |
There was a problem hiding this comment.
suggestion (testing): Assert success of tar extraction and cleanup commands in the sosreport fixture
The fixture currently ignores the exit status of the tar extraction and cleanup commands, so failures (e.g., corrupt tar, permissions, disk full) would surface later as less clear test errors. Please capture the return values of these execute calls and assert status == 0 so setup failures are detected and reported immediately.
| tarball = module_target_sat.execute( | |
| 'ls /var/tmp/sosreport-*.tar.xz | head -1' | |
| ).stdout.strip() | |
| assert tarball, 'No sosreport tarball found' | |
| module_target_sat.execute(f'mkdir -p {self.EXTRACT_DIR}') | |
| module_target_sat.execute(f'tar xf {tarball} -C {self.EXTRACT_DIR}') | |
| report_dir = module_target_sat.execute( | |
| f'ls -d {self.EXTRACT_DIR}/sosreport-*' | |
| ).stdout.strip() | |
| yield report_dir | |
| module_target_sat.execute(f'rm -rf /var/tmp/sosreport-* {self.EXTRACT_DIR}') | |
| tarball = module_target_sat.execute( | |
| 'ls /var/tmp/sosreport-*.tar.xz | head -1' | |
| ).stdout.strip() | |
| assert tarball, 'No sosreport tarball found' | |
| mkdir_result = module_target_sat.execute(f'mkdir -p {self.EXTRACT_DIR}') | |
| assert mkdir_result.status == 0, ( | |
| f'Failed to create sosreport extract directory {self.EXTRACT_DIR}:\n' | |
| f'{mkdir_result.stdout}\n{mkdir_result.stderr}' | |
| ) | |
| tar_result = module_target_sat.execute(f'tar xf {tarball} -C {self.EXTRACT_DIR}') | |
| assert tar_result.status == 0, ( | |
| f'Failed to extract sosreport tarball {tarball}:\n' | |
| f'{tar_result.stdout}\n{tar_result.stderr}' | |
| ) | |
| report_dir = module_target_sat.execute( | |
| f'ls -d {self.EXTRACT_DIR}/sosreport-*' | |
| ).stdout.strip() | |
| yield report_dir | |
| cleanup_result = module_target_sat.execute( | |
| f'rm -rf /var/tmp/sosreport-* {self.EXTRACT_DIR}' | |
| ) | |
| assert cleanup_result.status == 0, ( | |
| f'Failed to cleanup sosreport artifacts:\n' | |
| f'{cleanup_result.stdout}\n{cleanup_result.stderr}' | |
| ) |
| SENSITIVE_KEYWORD = ('password', ) | ||
| SCRUB_MARKER = '********' | ||
|
|
||
| original = module_target_sat.execute( | ||
| f'grep -i password {FOREMANCTL_PARAMETERS_FILE}' | ||
| ) | ||
| assert original.stdout.strip(), ( | ||
| f'No password entries found in {FOREMANCTL_PARAMETERS_FILE}' | ||
| ) | ||
|
|
There was a problem hiding this comment.
suggestion: Sensitive keyword coverage is limited to 'password' while the description mentions secrets/tokens
The docstring states we should scrub password/secret/token, but this constant (and the corresponding test) only covers password. If other keys like secret, token, or client_secret are meant to be scrubbed, please either extend SENSITIVE_KEYWORD to include them (or derive it from the implementation/requirements) and assert that each is masked as ******** in parameters.yaml. Otherwise, regressions for those fields may go unnoticed.
Suggested implementation:
SENSITIVE_KEYWORD = ('password', 'secret', 'token', 'client_secret') original = module_target_sat.execute(
f"grep -Ei '{'|'.join(SENSITIVE_KEYWORD)}' {FOREMANCTL_PARAMETERS_FILE}"
)To fully implement your comment, the rest of this test should:
- Iterate over
SENSITIVE_KEYWORDand assert that each matching entry in the extractedparameters.yamlis masked asSCRUB_MARKER(********), not justpassword. - Ensure any existing hard-coded references to "password" in subsequent assertions or scrubbing checks are updated to use
SENSITIVE_KEYWORD, so regressions forsecret,token, andclient_secretcannot slip by untested.
|
trigger: test-robottelo |
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
|
|
PRT Result |
b4d381b to
ee69d88
Compare
|
|
|
PRT Result |
|
Looking at it the test failure is probably due to a version mismatch, test environment is running |
Yes, seems the patch wasn't applied on the checked out satellite |
|
PRT doesn't support the sosreport org, so the PR couldn't be patched. I've attached the results from the manual tests. |
I always wondered about this. Why do we have it setup like this, where we need enablement of each org explicitly, instead of saying something like: packit:
theforeman:
foreman: 123
sosreport:
sos: 333
Dynflow:
dynflow: 999 |
|
Okay so the automated failure is due to the current PRT limitation rather than the tests themselves. Should we track support for the sosreport org separately and accept the manual test results for this PR?
|
@archanaserver IMO this is the way. Please create a GH issue and link it here. |
Thanks @stejskalleos, I created issue #22274 to track PRT support for sosreport org. |
@stejskalleos Yes, I don't have any objections. @archanaserver Thanks for creating the issue. I'll work on adding the config for sosreport org later. |
|
@stejskalleos @evgeni any thoughts before we merge this? |
|
no objections |
| class TestSOSReportForemanctl: | ||
| """Tests for the foremanctl sos plugin on containerized Satellite.""" | ||
|
|
There was a problem hiding this comment.
Currently, this change in Foremanctl depends on the upstream SOS package, so while upstream PRT might pass, downstream tests will continue to fail until the package becomes available there.
To handle this in the interim, I think we should add a skip_if_open marker to skip these tests when running against downstream, we could try install from Packit COPR repos as a workaround until the downstream package catches up, wdyt?
There was a problem hiding this comment.
As a short-term workaround, we could add an autouse fixture to enable the COPR repo from the upstream SOS PR and get this merged sooner.
I think this gives us about a month before the Packit RPM expires. After that, we can switch to using a SOS upstream nightly RPM built from their main branch, that wasn't available today, so I've opened a PR upstream to add a nightly build similar to what we already do in @theforeman added by @evgeni
There was a problem hiding this comment.
Added is_open to enable COPR repo from the upstream SOS PR as a short-term workaround
790441b to
0d26954
Compare
|
Signed-off-by: Shubham Ganar <shubhamsg123m@gmail.com>
|
|
PRT Result |
Satellite-QE
added
PRT-Passed
7 participants
Adding foremanctl tests for sos report
ref: sosreport/sos#4376
Summary by Sourcery
Add end-to-end tests validating the foremanctl sosreport plugin behavior on containerized Satellite.
New Features:
Tests: