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

Add test coverage for BZ:2192939 #14025

Merged
merged 2 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions robottelo/hosts.py
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ def update_known_hosts(self, ssh_key_name, host, user=None):
f'Failed to put hostname in ssh known_hosts files:\n{result.stderr}'
)

def configure_puppet(self, proxy_hostname=None):
def configure_puppet(self, proxy_hostname=None, run_puppet_agent=True):
"""Configures puppet on the virtual machine/Host.
:param proxy_hostname: external capsule hostname
:return: None.
Expand Down Expand Up @@ -1023,12 +1023,14 @@ def configure_puppet(self, proxy_hostname=None):
self.execute('/opt/puppetlabs/bin/puppet agent -t')
Copy link
Contributor

Choose a reason for hiding this comment

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

Nowadays (since Puppet 6) if you want to bootstrap you use puppet ssl bootstrap. Then you can sign it. You may want to pass --waitforcert 0 as well to prevent it waiting for the server to sign it.

proxy_host = Host(hostname=proxy_hostname)
proxy_host.execute(f'puppetserver ca sign --certname {cert_name}')
# This particular puppet run would create the host entity under
# 'All Hosts' and let's redirect stderr to /dev/null as errors at
# this stage can be ignored.
result = self.execute('/opt/puppetlabs/bin/puppet agent -t 2> /dev/null')
if result.status:
raise ContentHostError('Failed to configure puppet on the content host')

if run_puppet_agent:
# This particular puppet run would create the host entity under
# 'All Hosts' and let's redirect stderr to /dev/null as errors at
# this stage can be ignored.
result = self.execute('/opt/puppetlabs/bin/puppet agent -t 2> /dev/null')
Copy link
Contributor

Choose a reason for hiding this comment

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

Using -t isn't quite a good test. While testing the original BZ I noticed it is NOT a reproducer for the issue.

-t (or --test) is a short hand for multiple options. Quoting the man page:

* --test:
  Enable the most common options used for testing. These are 'onetime',
  'verbose', 'no-daemonize', 'no-usecacheonfailure', 'detailed-exitcodes',
  'no-splay', and 'show_diff'.

The --verbose part always creates entries in the report. If you only want to run puppet it should be sufficient to use puppet agent --onetime --no-daemonize --no-splay.

if result.status:
raise ContentHostError('Failed to configure puppet on the content host')

def execute_foreman_scap_client(self, policy_id=None):
"""Executes foreman_scap_client on the vm to create security audit report.
Expand Down
61 changes: 60 additions & 1 deletion tests/foreman/cli/test_report.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import random

import pytest
from wait_for import wait_for

from robottelo.exceptions import CLIReturnCodeError

Expand Down Expand Up @@ -82,11 +83,69 @@ def test_positive_install_configure_host(
for client, puppet_proxy in zip(content_hosts, puppet_infra_host, strict=True):
client.configure_puppet(proxy_hostname=puppet_proxy.hostname)
report = session_puppet_enabled_sat.cli.ConfigReport.list(
{'search': f'host~{client.hostname},origin=Puppet'}
{'search': f'host~{client.hostname}'}
Gauravtalreja1 marked this conversation as resolved.
Show resolved Hide resolved
)
assert len(report)
assert report[0]['origin'] == 'Puppet'
facts = session_puppet_enabled_sat.cli.Fact.list({'search': f'host~{client.hostname}'})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not search specifically for the puppetmaster_fqdn fact?

assert len(facts)
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 a rather useless assertion. There's no guarantee that the puppetmaster_fqdn fact is present.

assert [f for f in facts if f['fact'] == 'puppetmaster_fqdn'][0][
'value'
] == puppet_proxy.hostname
Comment on lines +92 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd write this as:

assert [f['value'] for f in facts if f['fact'] == 'puppetmaster_fqdn'] == [puppet_proxy.hostname]

This won't fail with an IndexError if the puppetmaster_fqdn fact isn't present.

session_puppet_enabled_sat.cli.ConfigReport.delete({'id': report[0]['id']})
with pytest.raises(CLIReturnCodeError):
session_puppet_enabled_sat.cli.ConfigReport.info({'id': report[0]['id']})


@pytest.mark.e2e
@pytest.mark.rhel_ver_match('[^6]')
def test_positive_run_puppet_agent_generate_report_when_no_message(
session_puppet_enabled_sat, rhel_contenthost
):
"""Verify that puppet-agent can be installed from the sat-client repo
and configured to report back to the Satellite, and contains the origin
:id: 07777fbb-4f2e-4fab-ba5a-2b698f9b9f39
:setup:
1. Satellite and Capsule with enabled puppet plugin.
2. Blank RHEL content host
:steps:
1. Configure puppet on the content host. This creates sat-client repository,
installs puppet-agent, configures it, runs it to create the puppet cert,
signs in on the Satellite side and reruns it.
2. Assert that Config report created in the Satellite for the content host,
and has Puppet origin
3. Assert that Facts were reported for the content host.
:expectedresults:
1. Configuration passes without errors.
2. Config report is created and contains Puppet origin
3. Facts are acquired.
:customerscenario: true
:BZ: 2192939, 2257327, 2257314
:parametrized: yes
"""
Gauravtalreja1 marked this conversation as resolved.
Show resolved Hide resolved
sat = session_puppet_enabled_sat
client = rhel_contenthost
client.configure_puppet(proxy_hostname=sat.hostname, run_puppet_agent=False)
# Run either 'puppet agent --detailed-exitcodes' or 'systemctl restart puppet'
# to generate Puppet config report for host without any messages
assert client.execute('/opt/puppetlabs/bin/puppet agent --detailed-exitcodes').status == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This starts a daemon. You want to use --onetime --no-daemonize --no-splay as arguments to prevent that.

wait_for(
lambda: sat.cli.ConfigReport.list({'search': f'host~{client.hostname}'}) != [],
timeout=300,
delay=30,
)
report = sat.cli.ConfigReport.list({'search': f'host~{client.hostname}'})
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 a list of reports while the name implies it is a singular entity. I'd name it reports

assert len(report)
assert report[0]['origin'] == 'Puppet'
Copy link
Contributor

Choose a reason for hiding this comment

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

This will pass if there a report with the origin puppet. How about asserting every report has an origin instead?

assert None not in [report['origin'] for report in reports]

facts = sat.cli.Fact.list({'search': f'host~{client.hostname}'})
assert len(facts)
assert [f for f in facts if f['fact'] == 'puppetmaster_fqdn'][0]['value'] == sat.hostname
sat.cli.ConfigReport.delete({'id': report[0]['id']})
with pytest.raises(CLIReturnCodeError):
sat.cli.ConfigReport.info({'id': report[0]['id']})
Loading