Add test for IoP services with alternate hostname fact#22081
Open
jnagare-redhat wants to merge 4 commits into
Open
Add test for IoP services with alternate hostname fact#22081jnagare-redhat wants to merge 4 commits into
jnagare-redhat wants to merge 4 commits into
Conversation
Test validates that when Satellite and Insights are configured to use the same alternate display name (via custom RHSM fact), the host registration, IoP services, and re-registration all function correctly without creating duplicate host records. This test covers: - Configuring Satellite to use network.hostname-override fact - Creating custom RHSM fact with alternate hostname using gen_string - Configuring Insights with same alternate display name - Verifying hostname resolution from Satellite - Host registration with global registration - IoP Vulnerability and Advisor services functionality - Force re-registration maintaining alternate hostname - Verification of no duplicate host records or stale IoP entries Implementation notes: - Uses lightweight recommendation (chmod 777) instead of package operations - Verifies natural vulnerabilities from base system packages - Avoids time-consuming dnf update and mariadb installation - Focus is on hostname functionality, not specific CVE testing Changes: - Added gen_string import for random hostname generation - Use meaningful prefix 'alt-insights-' with random suffix - Removed e2e marker as requested - Updated Verifies tag to SAT-44011 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The test was searching for hosts using the original hostname prefix, but since the host is registered with an alternate hostname (alt-insights-*), the search returned no results. Fixed by: - Search by domain name instead of hostname prefix - Filter results to match either original or alternate hostname - Verify only one host exists and it uses the alternate hostname This ensures we properly detect duplicates while accounting for the fact that the host is registered with a different name.
Contributor
Reviewer's GuideAdds a new end-to-end UI test that verifies IoP (Red Hat Lightspeed) Vulnerability and Advisor services continue to work correctly when a host is registered using an alternate hostname provided via a custom RHSM fact, including after forced re-registration, while avoiding heavy package operations and focusing on natural CVEs and recommendations. 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}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Changing permissions on
/etc/shadow(chmod 777 /etc/shadow) is extremely intrusive and may break or destabilize the test system; consider using a dedicated test file or a safer misconfiguration pattern that still triggers Advisor without touching core security-sensitive files. - The test relies on
ping -c 1 {alternate_hostname}to verify resolution, which can be flaky or blocked in some environments; usinggetent hostsornslookupfor name resolution validation would be more robust. - The test modifies global state on both Satellite and the client (
hammer settings set register_hostname_fact,/etc/hosts, RHSM facts, and insights-client.conf`) without any cleanup or restoration, so it would be safer to add teardown logic or fixtures that restore previous settings to avoid side effects on subsequent tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing permissions on `/etc/shadow` (`chmod 777 /etc/shadow`) is extremely intrusive and may break or destabilize the test system; consider using a dedicated test file or a safer misconfiguration pattern that still triggers Advisor without touching core security-sensitive files.
- The test relies on `ping -c 1 {alternate_hostname}` to verify resolution, which can be flaky or blocked in some environments; using `getent hosts` or `nslookup` for name resolution validation would be more robust.
- The test modifies global state on both Satellite and the client (`hammer settings set register_hostname_fact`, `/etc/hosts`, RHSM facts, and insights-client.conf`) without any cleanup or restoration, so it would be safer to add teardown logic or fixtures that restore previous settings to avoid side effects on subsequent tests.
## Individual Comments
### Comment 1
<location path="tests/foreman/ui/test_rhcloud_insights_vulnerability.py" line_range="1416-1423" />
<code_context>
+ # Step 2: Create custom RHSM fact on the client with alternate hostname
+ rhsm_fact_content = f'{{"network.hostname-override":"{alternate_hostname}"}}'
+ assert client.execute('mkdir -p /etc/rhsm/facts').status == 0
+ result = client.execute(
+ f'echo \'{rhsm_fact_content}\' > /etc/rhsm/facts/hostname.facts'
+ )
+ assert result.status == 0, f'Failed to create RHSM fact file: {result.stderr}'
+
+ # Step 3: Configure Insights to use the same alternate display name
+ result = client.execute(
+ f'echo "display_name={alternate_hostname}" >> /etc/insights-client/insights-client.conf'
+ )
+ assert result.status == 0, (
</code_context>
<issue_to_address>
**suggestion (testing):** Test permanently mutates system config files (`/etc/rhsm/facts` and Insights config) without cleanup, risking cross-test interference.
This test modifies `/etc/rhsm/facts/hostname.facts` and `/etc/insights-client/insights-client.conf` but never restores them, so subsequent tests may see non-default facts/config and fail unpredictably.
Please wrap these changes in setup/teardown (e.g., `try/finally`) that:
- Backs up and restores/removes `hostname.facts`, and
- Backs up `insights-client.conf` or removes the added `display_name=` line afterward.
That way the test remains isolated and doesn’t leak state into others.
Suggested implementation:
```python
# Step 2: Create custom RHSM fact on the client with alternate hostname
hostname_facts_path = '/etc/rhsm/facts/hostname.facts'
hostname_facts_backup = f'{hostname_facts_path}.bak'
# Backup existing RHSM hostname facts (if any) before overwriting
client.execute(
f'[ -f {hostname_facts_path} ] && cp {hostname_facts_path} {hostname_facts_backup} || true'
)
rhsm_fact_content = f'{{"network.hostname-override":"{alternate_hostname}"}}'
assert client.execute('mkdir -p /etc/rhsm/facts').status == 0
result = client.execute(
f'echo \'{rhsm_fact_content}\' > {hostname_facts_path}'
)
assert result.status == 0, f'Failed to create RHSM fact file: {result.stderr}'
```
To fully implement the isolation you requested, the following additional changes are needed elsewhere in the same test function:
1. **Wrap the mutation of `/etc/rhsm/facts/hostname.facts` and `/etc/insights-client/insights-client.conf` in a `try/finally` block:**
- Before modifying either file:
- Define paths and backup paths:
```python
hostname_facts_path = '/etc/rhsm/facts/hostname.facts'
hostname_facts_backup = f'{hostname_facts_path}.bak'
insights_conf_path = '/etc/insights-client/insights-client.conf'
insights_conf_backup = f'{insights_conf_path}.bak'
```
- Backup existing files (if present):
```python
client.execute(
f'[ -f {hostname_facts_path} ] && cp {hostname_facts_path} {hostname_facts_backup} || true'
)
client.execute(
f'[ -f {insights_conf_path} ] && cp {insights_conf_path} {insights_conf_backup} || true'
)
```
- Start a `try:` block immediately after the backups and keep **all code that relies on the modified hostname fact / display name inside this `try:` block**.
2. **Configure Insights with the alternate display name inside the `try:` block:**
- Right after creating `hostname.facts`, add:
```python
result = client.execute(
f'echo "display_name={alternate_hostname}" >> {insights_conf_path}'
)
assert result.status == 0, (
f'Failed to configure insights client display name: {result.stderr}'
)
```
3. **Restore/clean up in the `finally:` block (after the rest of the test logic):**
- Add a `finally:` that:
- Restores or removes `hostname.facts`:
```python
client.execute(
f'[ -f {hostname_facts_backup} ] '
f'&& mv {hostname_facts_backup} {hostname_facts_path} '
f'|| rm -f {hostname_facts_path}'
)
```
- Restores Insights config or removes the injected `display_name=` line:
```python
client.execute(
f'[ -f {insights_conf_backup} ] '
f'&& mv {insights_conf_backup} {insights_conf_path} '
f'|| ( [ -f {insights_conf_path} ] '
f'&& sed -i "/^display_name=/d" {insights_conf_path} || true )'
)
```
- Ensure the `finally:` comes *after* all assertions and operations that depend on the alternate hostname fact / Insights display name, so the cleanup only runs once the test completes.
These additional changes will ensure that both `/etc/rhsm/facts/hostname.facts` and `/etc/insights-client/insights-client.conf` are restored to their original state (or cleaned) after the test, preventing cross-test interference.
</issue_to_address>
### Comment 2
<location path="tests/foreman/ui/test_rhcloud_insights_vulnerability.py" line_range="1429-1441" />
<code_context>
+ # Step 4: Verify the alternate hostname is resolvable from Satellite
+ # Add entry to /etc/hosts for resolution
+ client_ip = client.execute('hostname -I | awk \'{print $1}\'').stdout.strip()
+ result = satellite.execute(
+ f'echo "{client_ip} {alternate_hostname}" >> /etc/hosts'
+ )
+ assert result.status == 0, f'Failed to add hosts entry on Satellite: {result.stderr}'
</code_context>
<issue_to_address>
**suggestion (testing):** Appending to Satellite `/etc/hosts` without cleanup can have side effects on other tests.
This test adds an entry for the alternate hostname to Satellite’s `/etc/hosts` but never removes it. That persistent change can bleed into other tests that reuse the same host/name or expect a clean hostname resolution state.
Please update the test to clean up after itself, e.g.:
- Use a `try/finally` to remove the added line, or
- Use a dedicated hosts file for this test, if supported.
Restoring `/etc/hosts` to its original state will keep tests isolated and avoid hard-to-debug interactions.
```suggestion
# Step 4: Verify the alternate hostname is resolvable from Satellite
# Add entry to /etc/hosts for resolution and ensure cleanup after the test
client_ip = client.execute('hostname -I | awk \'{print $1}\'').stdout.strip()
try:
result = satellite.execute(
f'echo "{client_ip} {alternate_hostname}" >> /etc/hosts'
)
assert result.status == 0, (
f'Failed to add hosts entry on Satellite: {result.stderr}'
)
# Verify resolution
result = satellite.execute(f'ping -c 1 {alternate_hostname}')
assert result.status == 0, (
f'Alternate hostname {alternate_hostname} is not resolvable from Satellite'
)
finally:
# Remove the hosts entry to avoid side effects on other tests
satellite.execute(
f"sed -i.bak '/{alternate_hostname}$/d' /etc/hosts"
)
```
</issue_to_address>
### Comment 3
<location path="tests/foreman/ui/test_rhcloud_insights_vulnerability.py" line_range="1475-1477" />
<code_context>
+ 'IoP registration failed - Red Hat Lightspeed status is not Reporting'
+ )
+
+ # Step 9: Create recommendations on the host (lightweight, no package operations)
+ # Add permission misconfiguration to trigger recommendation
+ assert client.execute('chmod 777 /etc/shadow').status == 0
+
+ # Run insights-client to report recommendations
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Changing permissions on `/etc/shadow` is risky even in tests and should be reverted or targeted at a safer path.
`chmod 777 /etc/shadow` will certainly fire the rule, but it’s an invasive and persistent system change with potential security and functional side effects, and the test doesn’t restore the original permissions.
Instead, either:
- Use a less sensitive file (e.g., create a test file with insecure perms in `/tmp` or another Advisor-recognized path), or
- At least record the original mode of `/etc/shadow` and restore it in a `finally` block.
This keeps the test safer and avoids polluting the environment for later steps or tests.
Suggested implementation:
```python
# Step 9: Create recommendations on the host (lightweight, no package operations)
# Add permission misconfiguration to trigger recommendation
# NOTE: Changing /etc/shadow is risky – record original mode and restore it after the test
orig_mode_result = client.execute("stat -c '%a' /etc/shadow")
assert orig_mode_result.status == 0, f"Failed to get /etc/shadow mode: {orig_mode_result.stdout}"
orig_mode = orig_mode_result.stdout.strip()
try:
assert client.execute('chmod 777 /etc/shadow').status == 0
# Run insights-client to report recommendations
result = client.execute('insights-client')
assert result.status == 0, f'insights-client failed: {result.stdout}'
finally:
# Best-effort restore of original permissions to avoid polluting the environment
client.execute(f'chmod {orig_mode} /etc/shadow')
```
If the Advisor rule you are targeting can be triggered by an insecure permission on a less sensitive file, consider further refactoring this step to:
1. Create a temporary file (e.g. in /tmp),
2. Set its permissions to a vulnerable mode (e.g. 777), and
3. Adjust the rule preconditions so it uses that file instead of /etc/shadow.
That would allow you to remove all interaction with /etc/shadow entirely while still validating the recommendation behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jnagare-redhat
added
CherryPick
Split compound assertions 'assert X and Y' into separate assertions to comply with PT018 linting rule. Changed: - assert search_result and len(search_result) > 0 → assert search_result → assert len(search_result) > 0 This provides clearer error messages and follows pytest best practices.
jnagare-redhat
force-pushed
the
alternate-hostname-iop-test
branch
from
62e57a9 to
b416ce6
Compare
Automatic formatting applied by ruff: - Collapsed multi-line assert statements to single lines where appropriate - Reformatted list comprehension for better readability - Code style improvements for consistency
jnagare-redhat
force-pushed
the
alternate-hostname-iop-test
branch
from
aff507e to
3451a4f
Compare
Contributor
Author
|
trigger: test-robottelo |
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a comprehensive test to validate that IoP Advisor and Vulnerability services work correctly when using alternate hostname facts.
Automaton of : https://redhat.atlassian.net/browse/SAT-44011
Problem Statement
When Satellite and Insights are configured to use alternate display names (via custom RHSM facts), we need to ensure that:
Solution
Added
test_positive_iop_services_with_alternate_hostname_factintests/foreman/ui/test_rhcloud_insights_vulnerability.pyTest Coverage (17 Steps)
network.hostname-overridefactsetup_insights=trueforce=trueImplementation Highlights
dnf updateand package installationsalt-insights-{random}Files Changed
tests/foreman/ui/test_rhcloud_insights_vulnerability.py(+265 lines)Testing
✅ Test executed successfully in CI environment
✅ All 17 test steps passed
✅ Verified duplicate host detection works correctly
✅ Confirmed no stale IoP entries after re-registration
Verifies
SAT-44011
Type of Change
Summary by Sourcery
Add a new end-to-end UI test validating IoP services behavior when using an alternate hostname fact for Satellite and Insights-registered hosts.
New Features:
Tests: