Skip to content

Conversation

aneta-petrova
Copy link
Member

@aneta-petrova aneta-petrova commented Sep 4, 2025

What changes are you introducing?

  • Moving the conceptual, referential information (to be used during the planning phase by the architect persona) from the Installation guide to the Planning guide. This includes:
    • Reviewing the contents of this new "networking considerations" section in the Planning guide
    • Moving security considerations to a different place in the Planning guide's table of contents
  • Reviewing the procedural information (to be used during the installation phase by the admin persona) in the Installation guide. This includes:
    • Merging existing separate procedures (one for server, one for smart proxy) and three firewalld-related snippets into a single procedure module.
  • Fixing various xrefs and links that got broken by all the renaming.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

This is part of preparing the installation prerequisites for the new installation guide in #4087

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

N/A

Contributor checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.16/Katello 4.18 (Satellite 6.18)
  • Foreman 3.15/Katello 4.17
  • Foreman 3.14/Katello 4.16 (Satellite 6.17; orcharhino 7.4)
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only; orcharhino 7.3)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • We do not accept PRs for Foreman older than 3.9.

Review checklists

Tech review (performed by an Engineer who did not author the PR; can be skipped if tech review is unnecessary):

  • The PR documents a recommended, user-friendly path.
  • The PR removes steps that have been made unnecessary or obsolete.
  • Any steps introduced or updated in the PR have been tested to confirm that they lead to the documented end result.

Style review (by a Technical Writer who did not author the PR):

  • The PR conforms with the team's style guidelines.
  • The PR introduces documentation that describes a user story rather than a product feature.

@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels Sep 4, 2025
@aneta-petrova aneta-petrova force-pushed the review-port-requirements branch from eb83efb to d1edc57 Compare September 4, 2025 07:20
@aneta-petrova aneta-petrova removed the Needs testing Requires functional testing label Sep 4, 2025
@aneta-petrova aneta-petrova force-pushed the review-port-requirements branch 6 times, most recently from b74d86b to 0690c02 Compare September 4, 2025 08:47
@aneta-petrova aneta-petrova marked this pull request as ready for review September 4, 2025 09:13
@aneta-petrova
Copy link
Member Author

Hi @ekohl, this is my idea for how to split and organize the docs about ports between Planning and Installation. Can you please take a look? Does this go in the direction that you had in mind?

@aneta-petrova aneta-petrova force-pushed the review-port-requirements branch from 41c3420 to 9d189e0 Compare September 4, 2025 09:21
Copy link
Member

Choose a reason for hiding this comment

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

For this chapter we run into the naming problem: Capsule in downstream is IMHO something quite different than a Smart Proxy.

My interpretation:

  • Smart Proxy: the generic concept that Foreman connects to
  • Foreman Proxy: the Ruby implementation of Smart Proxy
  • Capsule: The downstream naming for a server that has a few services: Foreman Proxy, Pulp, Apache (for the RHSM registration API). Upstream we sometimes refer to this as Content Proxies: a Proxy with a set of content features.

Now looking ahead we're going to make this a bigger problem. The Insights on Premise is also recognized as a Smart Proxy. Then with pulp_smart_proxy the Pulp server can identify as a Smart Proxy.

In other words: I think soon we need to have the discussion within Red Hat: where do we want to go with this. Do we stop branding Smart Proxy as Capsule?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is actionable right now but it's interesting information so I'll keep this thread open for awareness.

@aneta-petrova aneta-petrova force-pushed the review-port-requirements branch from 1b3c6c8 to 6772d24 Compare September 5, 2025 07:04
@aneta-petrova
Copy link
Member Author

aneta-petrova commented Sep 5, 2025

A tech review should focus on these two things:

  1. Sections "Opening required ports" in the server installation guide (https://theforeman-foreman-documentation-preview-pr-4210.surge.sh/nightly/Installing_Server/index-katello.html#opening-required-ports) and the proxy installation guide (https://theforeman-foreman-documentation-preview-pr-4210.surge.sh/nightly/Installing_Proxy/index-katello.html#opening-required-ports). The questions to ask are:
  1. Chapter "Networking considerations in Foreman" in the Planning guide (https://theforeman-foreman-documentation-preview-pr-4210.surge.sh/nightly/Planning_for_Project/index-katello.html#networking-considerations-in-foreman). There is no need to review the tables with ports thoroughly (we have been keeping them up-to-date), which means a reviewer should be fine just reading the accompanying text.

@aneta-petrova aneta-petrova force-pushed the review-port-requirements branch from 26efb19 to 84004cc Compare September 10, 2025 13:51
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

is there a technical reason to avoid macros in code blocks?

@aneta-petrova
Copy link
Member Author

is there a technical reason to avoid macros in code blocks?

There isn't but I'm trying to get an idea of the required lists of services first. Once I have that, I'll figure out how to include them more efficiently.

@aneta-petrova
Copy link
Member Author

A tech review should focus on these two things:

  1. Sections "Opening required ports" in the server installation guide (https://theforeman-foreman-documentation-preview-pr-4210.surge.sh/nightly/Installing_Server/index-katello.html#opening-required-ports) and the proxy installation guide (https://theforeman-foreman-documentation-preview-pr-4210.surge.sh/nightly/Installing_Proxy/index-katello.html#opening-required-ports). The questions to ask are:

@evgeni After agreeing to resolve the question of which ports need to be opened later (tracked in #4247), do you think the procedure as it is now is okay?

  1. Chapter "Networking considerations in Foreman" in the Planning guide (https://theforeman-foreman-documentation-preview-pr-4210.surge.sh/nightly/Planning_for_Project/index-katello.html#networking-considerations-in-foreman). There is no need to review the tables with ports thoroughly (we have been keeping them up-to-date), which means a reviewer should be fine just reading the accompanying text.

@ekohl @evgeni Have either of you had the chance to look at the changes in the Planning guide?

@maximiliankolb maximiliankolb added the Waiting on contributor Requires an action from the author label Sep 16, 2025
Copy link
Member Author

@aneta-petrova aneta-petrova left a comment

Choose a reason for hiding this comment

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

All those comments about which ports don't need to be open by default are great, thanks a lot, Evgeni and Ewoud! I created #4247 a few days ago so I added them there.

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Copy link
Member Author

@aneta-petrova aneta-petrova left a comment

Choose a reason for hiding this comment

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

I created a separate issue to track a team review of the Planning guide.

@aneta-petrova
Copy link
Member Author

two comments, but not blocking, LGTM

Thanks!

@ekohl @maximiliankolb Do you have any further comments?

Comment on lines 7 to 11
[NOTE]
====
Some outgoing traffic returns to {Project} to enable internal communication and security operations.
====

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand this & it seems rather confusing. "some" sounds bad and unspecific from a security perspective. Does this admonition add any value to users? I am leaning towards no.

Suggested change
[NOTE]
====
Some outgoing traffic returns to {Project} to enable internal communication and security operations.
====

Was this part of the docs before? If so, feel free to ignore my comment/move this to an GH issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to resolve this here so that we're sure the reviewed requirements are okay. @evgeni can you shed some light on this? Can we perhaps specify the note? Or is it safe to drop it?

Copy link
Member

Choose a reason for hiding this comment

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

Well, it was added in #2451 and is frankly a tad confusing.
So strap on your best glasses, we're using a Tardis!

The bug that #2451 was trying to fix (https://bugzilla.redhat.com/show_bug.cgi?id=2233266) was reported against Satellite 6.13, and we look at https://docs.redhat.com/en/documentation/red_hat_satellite/6.13/html/installing_satellite_server_in_a_connected_network_environment/preparing_your_environment_for_installation_satellite#Ports_and_Firewalls_Requirements_satellite we see the following entries with "Satellite" as the destination:

Destination Port Protocol Service Destination Required For Description
443 TCP HTTPS Satellite Capsule Capsule, Configuration management, Template retrieval, OpenSCAP, Remote Execution result upload
5646 TCP AMQP Satellite Server Katello agent Forward message to Qpid dispatch router on Capsule (optional)
5671 Satellite Server Remote install for Katello agent Send install command to client
5671 Satellite Server Remote install for Katello agent Forward message to dispatch router on Satellite

(Yes, protocol and service are missing for the last two lines, yes "Satellite" vs "Satellite Server" is inconsistent, have a 🍷 for me please)

Now, it is true that the way Katello agent communication was set up, the Germans would call "from behind, through the chest, into the eye" (Sorry Anet, if you don't have a visual picture of the pain, I either owe you one, or you can count yourself lucky, or both).

Luckily for us, today, stepped out of the Tardis again: Katello agent is a sin of the past and we don't have to care about it anymore and can solely focus on the one remaining entry in todays docs:

Destination Port Protocol Service Destination Required For Description
443 TCP HTTPS Satellite Capsule Capsule, Configuration management, Template retrieval, OpenSCAP, Remote Execution result upload

And the great thing? It's nonsense! I mean, yeah, the external Capsule needs to be able to talk to the Satellite on port 443 (and others!) but that should be covered by "{Project} incoming traffic" and "{SmartProxy} outgoing traffic" and never be part of "{Project} outgoing traffic"

So, my verdict is:

  • drop that note
  • drop line 65-73 of guides/common/modules/ref_project-server-port-and-firewall-requirements.adoc (| 443 | TCP | HTTPS | {Project} | {SmartProxy} | {SmartProxy} … )

Copy link
Member Author

Choose a reason for hiding this comment

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

That was quite the suspenseful read. I wish more comments were like this.

Also, it really helps that you summarized it at the end to tell me what it is that I'm supposed to do 😆 So I removed both the note and the row.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Sep 18, 2025
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Sep 18, 2025
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

I like the latest changes to guides/common/modules/con_smart-proxy-networking.adoc 👍

With the note getting dropped based on Evgeni's comment, IMO we're good to go!

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Thanks Anet, LGTM style-wise. Handing over to Evgeni for final tech ACK.

@maximiliankolb maximiliankolb added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective labels Sep 18, 2025
@maximiliankolb maximiliankolb added tech review done No issues from the technical perspective and removed Needs tech review Requires a review from the technical perspective labels Sep 18, 2025
@aneta-petrova aneta-petrova merged commit e561a50 into theforeman:master Sep 18, 2025
9 of 10 checks passed
@aneta-petrova aneta-petrova deleted the review-port-requirements branch September 18, 2025 09:56
aneta-petrova added a commit that referenced this pull request Sep 18, 2025
* Move port & firewall to Planning and review it
* Review port procedure
* Update installing postgresql to work without firewall snippets
* Drop firewall-cmd snippet from provisioning
* Merge modules on opening ports
* Drop integrated/external proxy definitions

Integrated/external proxy was reported as flawed concept that we should
get rid of.

* Drop obsolete information on outgoing traffic

---------

Co-authored-by: Maximilian Kolb <[email protected]>
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit e561a50)
@aneta-petrova
Copy link
Member Author

Merged to "master" and cherry-picked:

69993b2..56f3457 3.16 -> 3.16

Thanks everyone! This one makes me really happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants