foremanctl deploy --certificate-cname by stejskalleos · Pull Request #21309 · SatelliteQE/robottelo

stejskalleos · 2026-04-14T08:57:52Z

Problem Statement

Test for foremanctl deploy --certificate-cname

Solution

write the test 🤔 ?

Related Issues

theforeman/foremanctl#441

Summary by Sourcery

Tests:

Add an installer test that verifies foremanctl deploy --certificate-cname persists the CNAME, updates server certificate SANs, and allows HTTPS access via the new name.

sourcery-ai · 2026-04-14T08:57:58Z

Reviewer's Guide

Adds an integration test that validates foremanctl deploy --certificate-cname correctly persists the CNAME in the parameters file, updates the server certificate SAN, and allows HTTPS access via the CNAME using the generated CA.

File-Level Changes

Change	Details	Files
Add an end-to-end installer test for `foremanctl deploy --certificate-cname` covering parameter persistence, certificate SAN contents, host resolution, and HTTPS connectivity.	Introduce `test_foremanctl_deploy_certificate_cname` parametrized on the default Satellite-on-RHEL fixture. Run `foremanctl deploy --certificate-cname <cname>` and assert successful completion. Load the remote foremanctl parameters YAML and assert the generated CNAME is present in `certificates_cnames`. Use `openssl x509` to inspect the server certificate and assert the CNAME appears in the Subject Alternative Name DNS entries. Ensure the CNAME resolves locally by conditionally appending an `/etc/hosts` entry based on the primary host IP. Perform an HTTPS request to the Satellite login URL using `curl` with the self-signed CA certificate and assert HTTP 200 is returned.	`tests/foreman/installer/test_install_foremanctl.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey - I've reviewed your changes and they look great!

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

ehelms · 2026-04-16T13:59:58Z

+
+
+@pytest.mark.parametrize('module_sat_ready_rhel', ['default'], indirect=True)
+def test_foremanctl_deploy_certificate_cname(module_sat_ready_rhel):


Could this test live in foremanctl ?

Yop, I'm planning to add some more coverage there. Here, I'm planning to extend the workflow to cover the whole e2e test:

Install satellite with cname

Add another cname

--reset-cname

In the foremanctl repo, I'll test it as three separate cases.

stejskalleos · 2026-04-22T11:43:37Z

trigger: test-robottelo
pytest: tests/foreman/installer/test_install_foremanctl.py

sourcery-ai

Hey - I've found 2 issues, and left some high level feedback:

The test modifies /etc/hosts but never restores it, which can leak state into subsequent tests; consider using a context manager or fixture to add and then remove the CNAME entry after the test completes.
The hardcoded certificate paths (/root/certificates/...) and curl URL path (/users/login) might change across environments; if possible, reuse existing helpers or configuration constants to avoid brittle assumptions in this integration test.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The test modifies /etc/hosts but never restores it, which can leak state into subsequent tests; consider using a context manager or fixture to add and then remove the CNAME entry after the test completes.
- The hardcoded certificate paths (/root/certificates/...) and curl URL path (/users/login) might change across environments; if possible, reuse existing helpers or configuration constants to avoid brittle assumptions in this integration test.

## Individual Comments

### Comment 1
<location path="tests/foreman/installer/test_install_foremanctl.py" line_range="192-201" />
<code_context>
+    satellite = module_sat_ready_rhel
+    cname = f'cname.{satellite.hostname}'
+
+    result = satellite.execute(
+        f'foremanctl deploy --certificate-cname {cname}',
+        timeout='10m',
+    )
+    assert result.status == 0, (
+        f'foremanctl deploy with --certificate-cname failed:\n{result.stderr}'
+    )
+
+    parameters_file = satellite.load_remote_yaml_file(FOREMANCTL_PARAMETERS_FILE)
+    assert cname in parameters_file.certificates_cnames
+
+    result = satellite.execute(
+        'openssl x509 -in /root/certificates/certs/$(hostname -f).crt -noout -ext subjectAltName'
+    )
+    assert result.status == 0, f'Failed to read server certificate:\n{result.stderr}'
+    assert f'DNS:{cname}' in result.stdout, (
+        f'CNAME {cname} not found in server certificate SANs:\n{result.stdout}'
+    )
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting that the SAN output contains all mandatory names, not just the new CNAME.

Right now the test only asserts `DNS:{cname}` is present. To also guard against regressions where existing SAN entries are dropped (e.g. the original FQDN), consider asserting that the canonical hostname (e.g. `DNS:$(hostname -f)`) is still present in the SAN output as well.

Suggested implementation:

```python
        3. The server certificate SAN includes DNS:<cname> and the canonical hostname (DNS:<hostname -f>)

```

```python
    result = satellite.execute(
        'openssl x509 -in /root/certificates/certs/$(hostname -f).crt -noout -ext subjectAltName'
    )
    assert result.status == 0, f'Failed to read server certificate:\n{result.stderr}'

    san_output = result.stdout
    assert f'DNS:{cname}' in san_output, (
        f'CNAME {cname} not found in server certificate SANs:\n{san_output}'
    )
    assert f'DNS:{satellite.hostname}' in san_output, (
        f'Canonical hostname {satellite.hostname} not found in server certificate SANs:\n{san_output}'
    )

```
</issue_to_address>

### Comment 2
<location path="tests/foreman/installer/test_install_foremanctl.py" line_range="207-220" />
<code_context>
+        f'CNAME {cname} not found in server certificate SANs:\n{result.stdout}'
+    )
+
+    satellite.execute(
+        f'grep -q {cname} /etc/hosts '
+        f'|| echo "$(hostname -I | awk \'{{print $1}}\') {cname}" >> /etc/hosts'
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid leaving persistent `/etc/hosts` modifications that may affect subsequent tests.

This test appends an entry to `/etc/hosts` without ever removing it, making the environment stateful across runs and potentially affecting other tests that depend on name resolution. Please either clean up the entry after the test (e.g., via `try/finally` or a fixture) or use a more isolated approach such as `curl --resolve` or a temporary hosts file. This will keep the test hermetic and avoid cross-test interference.

```suggestion
    assert f'DNS:{cname}' in result.stdout, (
        f'CNAME {cname} not found in server certificate SANs:\n{result.stdout}'
    )

    result = satellite.execute(
        f'ip=$(hostname -I | awk \'{{print $1}}\'); '
        f'curl -s -o /dev/null -w "%{{http_code}}" '
        f'--resolve "{cname}:443:$ip" '
        f'--cacert /root/certificates/certs/ca.crt '
        f'https://{cname}/users/login'
    )
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

Satellite-QE · 2026-04-22T12:37:11Z

PRT Result

Build Number: 15218
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/installer/test_install_foremanctl.py --external-logging
Test Result : ================= 16 passed, 65 warnings in 2485.12s (0:41:25) =================

stejskalleos · 2026-04-27T06:34:01Z

trigger: test-robottelo
pytest: tests/foreman/installer/test_install_foremanctl.py

Satellite-QE · 2026-04-27T07:04:16Z

PRT Result

Build Number: 15238
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/installer/test_install_foremanctl.py --external-logging
Test Result : ================= 30 warnings, 16 errors in 1143.76s (0:19:03) =================

Co-authored-by: Evgeni Golov <evgeni@golov.de>

stejskalleos · 2026-04-27T08:00:18Z

trigger: test-robottelo
pytest: tests/foreman/installer/test_install_foremanctl.py

Satellite-QE · 2026-04-27T08:51:18Z

PRT Result

Build Number: 15240
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/installer/test_install_foremanctl.py --external-logging
Test Result : ================= 16 passed, 66 warnings in 2476.29s (0:41:16) =================

nacoool

LGTM.

* foremanctl deploy --certificate-cname --------- Co-authored-by: Evgeni Golov <evgeni@golov.de>

github-actions Bot assigned stejskalleos Apr 14, 2026

stejskalleos mentioned this pull request Apr 14, 2026

Use community.crypto for the certificates role theforeman/foremanctl#441

Merged

sourcery-ai Bot reviewed Apr 14, 2026

View reviewed changes

Gauravtalreja1 added the No-CherryPick PR doesnt need CherryPick to previous branches label Apr 14, 2026

ehelms reviewed Apr 16, 2026

View reviewed changes

stejskalleos marked this pull request as draft April 17, 2026 05:54

stejskalleos force-pushed the ls/SAT-43440 branch 2 times, most recently from ed4b864 to c6cbe94 Compare April 22, 2026 11:42

stejskalleos marked this pull request as ready for review April 22, 2026 11:42

sourcery-ai Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 22, 2026

evgeni reviewed Apr 24, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

evgeni reviewed Apr 24, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

evgeni reviewed Apr 24, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

evgeni reviewed Apr 24, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

evgeni reviewed Apr 24, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py

evgeni requested changes Apr 24, 2026

View reviewed changes

foremanctl deploy --certificate-cname

bdb6de9

stejskalleos force-pushed the ls/SAT-43440 branch from c6cbe94 to bdb6de9 Compare April 27, 2026 06:32

Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 27, 2026

Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 27, 2026

evgeni reviewed Apr 27, 2026

View reviewed changes

Comment thread tests/foreman/installer/test_install_foremanctl.py Outdated

evgeni approved these changes Apr 27, 2026

View reviewed changes

Update tests/foreman/installer/test_install_foremanctl.py

ae6eb3b

Co-authored-by: Evgeni Golov <evgeni@golov.de>

stejskalleos force-pushed the ls/SAT-43440 branch from fe77274 to ae6eb3b Compare April 27, 2026 07:55

Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Apr 27, 2026

nacoool approved these changes Apr 27, 2026

View reviewed changes

evgeni merged commit 5bfc2ee into SatelliteQE:master Apr 27, 2026
12 checks passed

amolpati30 pushed a commit to amolpati30/robottelo that referenced this pull request Apr 27, 2026

foremanctl deploy --certificate-cname (SatelliteQE#21309)

843e201

* foremanctl deploy --certificate-cname --------- Co-authored-by: Evgeni Golov <evgeni@golov.de>



		@pytest.mark.parametrize('module_sat_ready_rhel', ['default'], indirect=True)
		def test_foremanctl_deploy_certificate_cname(module_sat_ready_rhel):

Uh oh!

Conversation

stejskalleos commented Apr 14, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem Statement

Solution

Related Issues

Summary by Sourcery

Uh oh!

sourcery-ai Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ehelms Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

stejskalleos Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

stejskalleos commented Apr 22, 2026

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Satellite-QE commented Apr 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stejskalleos commented Apr 27, 2026

Uh oh!

Satellite-QE commented Apr 27, 2026

Uh oh!

Uh oh!

stejskalleos commented Apr 27, 2026

Uh oh!

Satellite-QE commented Apr 27, 2026

Uh oh!

nacoool left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stejskalleos commented Apr 14, 2026 •

edited by sourcery-ai Bot

Loading

sourcery-ai Bot commented Apr 14, 2026 •

edited

Loading